Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2432113002:
[Findit] Add local_git_repository (Closed)
|
Created:
4 years, 2 months ago by Sharu Jiang Modified:
4 years, 1 month ago CC:
chromium-reviews, infra-reviews+infra_chromium.org, inferno, Martin Barbella Target Ref:
refs/heads/master Project:
infra Visibility:
Public. |
Description[Findit] Add local_git_parsers, local_git_repository
BUG=605369
Committed: https://chromium.googlesource.com/infra/infra/+/bc02c3473ee6c1a08f4ef97ef66f0486f870ec2a
Patch Set 1 #Patch Set 2 : Fix nits. #Patch Set 3 : Split adding local_git_parsers part to another cl. #Patch Set 4 : Fix nits. #Patch Set 5 : . #
Total comments: 4
Patch Set 6 : Fix nits. #
Total comments: 23
Patch Set 7 : Address comments. #Patch Set 8 : change to incremental #Patch Set 9 : Rebase. #
Total comments: 19
Patch Set 10 : Address comments #Patch Set 11 : Add tests and comments. #Patch Set 12 : Rebase. #
Total comments: 39
Patch Set 13 : Rebase. #Messages
Total messages: 44 (23 generated)
katesonia@chromium.org changed reviewers: + stgao@chromium.org, wrengr@chromium.org
PTAL :)
Description was changed from ========== [Findit] Add local_git_parsers, local_git_repository BUG= ========== to ========== [Findit] Add local_git_parsers, local_git_repository BUG= ==========
Since this cl is too big, split the adding local_git_parsers part to another cl: https://codereview.chromium.org/2435863003/
Description was changed from ========== [Findit] Add local_git_parsers, local_git_repository BUG= ========== to ========== [Findit] Add local_git_parsers, local_git_repository BUG=605369 ==========
katesonia@chromium.org changed reviewers: + chanli@chromium.org, lijeffrey@chromium.org
https://codereview.chromium.org/2432113002/diff/80001/appengine/findit/common... File appengine/findit/common/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/80001/appengine/findit/common... appengine/findit/common/local_git_repository.py:53: if not repo_url or (hasattr(self, '_repo_url') and Why hasattr is needed? Isn't self._repo_url already added in the constructor? https://codereview.chromium.org/2432113002/diff/80001/appengine/findit/common... File appengine/findit/common/repo_util.py (left): https://codereview.chromium.org/2432113002/diff/80001/appengine/findit/common... appengine/findit/common/repo_util.py:77: def GetRepoToCloneUrlDict(host_url): If this is not needed, we should delete in the other CL.
Patchset #7 (id:120001) has been deleted
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:140001) has been deleted
https://codereview.chromium.org/2432113002/diff/80001/appengine/findit/common... File appengine/findit/common/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/80001/appengine/findit/common... appengine/findit/common/local_git_repository.py:53: if not repo_url or (hasattr(self, '_repo_url') and On 2016/10/22 00:43:35, stgao wrote: > Why hasattr is needed? Isn't self._repo_url already added in the constructor? Oops, should have deleted. https://codereview.chromium.org/2432113002/diff/80001/appengine/findit/common... File appengine/findit/common/repo_util.py (left): https://codereview.chromium.org/2432113002/diff/80001/appengine/findit/common... appengine/findit/common/repo_util.py:77: def GetRepoToCloneUrlDict(host_url): On 2016/10/22 00:43:35, stgao wrote: > If this is not needed, we should delete in the other CL. Done.
https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... File appengine/findit/common/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:36: updated_repos = set() I'd think this should be private/protected. Would code using this module ever have a legitimate reason to directly write to this variable? If so, why? If not, then make it private. Would code using this module ever need to read this variable? If so, (a) why? (b) use a @classmethod or @staticmethod to provide read-only access to the public. If not, great; keeping it private helps improve modularity by hiding this implementation detail from other code. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:41: self.repo_url = repo_url What's the difference between self._repo_url vs self.repo_url? Since it's not part of the usual @property pattern, the names are confusing. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:48: def real_repo_path(self): Needs docstring explaining what "real" means here. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:52: def repo_url(self): Needs docstring explaining how this differs from the "real" one, and why we make the distinction. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:61: url_parts = repo_url.split('https://', 1)[1].split('/') This regex seems very fragile. Don't we have a library function somewhere for parsing URIs into their component parts? https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:64: self._repo_path = '/'.join(url_parts[1:]) Calling join after split is expensive. If sticking with the split approach, you should pass 1 as the second argument to split. That way you get the host and repo_path as the two things split returns; no post-processing necessary. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:76: subprocess.call(['git', 'clone', self.repo_url, real_repo_path]) We should check the return value to make sure things worked. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:81: subprocess.check_call( Ditto. Especially given as we're silencing stderr. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:93: return 'cd %s; %s' % (self.real_repo_path, command) Should be 'cd %s && %s'. If the cd fails for some reason, we don't want to silently run the command in the wrong place. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:120: """Returns blame of the file at ``path`` of the given revision.""" Is our style to use double backticks when referring to code, or using pipes? When I asked mbarbella, he said pipes (so that's what I've been using). Whichever we use, we should be consistent.
https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... File appengine/findit/common/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:91: command = 'TZ=UTC %s --date=format-local:"%s"' % ( Should TZ=UTC be set through env? Does this work across platform? https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:120: """Returns blame of the file at ``path`` of the given revision.""" On 2016/10/25 18:12:18, wrengr wrote: > Is our style to use double backticks when referring to code, or using pipes? > When I asked mbarbella, he said pipes (so that's what I've been using). > Whichever we use, we should be consistent. In infra, double backticks is the way to go.
https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... File appengine/findit/common/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:36: updated_repos = set() On 2016/10/25 18:12:18, wrengr wrote: > I'd think this should be private/protected. Would code using this module ever > have a legitimate reason to directly write to this variable? If so, why? If not, > then make it private. Would code using this module ever need to read this > variable? If so, (a) why? (b) use a @classmethod or @staticmethod to provide > read-only access to the public. If not, great; keeping it private helps improve > modularity by hiding this implementation detail from other code. no, it is neither written or read by other code. Done. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:41: self.repo_url = repo_url On 2016/10/25 18:12:18, wrengr wrote: > What's the difference between self._repo_url vs self.repo_url? Since it's not > part of the usual @property pattern, the names are confusing. the self._repo_url set the initial default value, self.repo_url call the setter to do some other stuff besides setting self._repo_url. But yes moving this into repo_url setter should be cleaner. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:48: def real_repo_path(self): On 2016/10/25 18:12:18, wrengr wrote: > Needs docstring explaining what "real" means here. Done. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:52: def repo_url(self): On 2016/10/25 18:12:18, wrengr wrote: > Needs docstring explaining how this differs from the "real" one, and why we make > the distinction. Done. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:61: url_parts = repo_url.split('https://', 1)[1].split('/') On 2016/10/25 18:12:18, wrengr wrote: > This regex seems very fragile. Don't we have a library function somewhere for > parsing URIs into their component parts? Done. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:64: self._repo_path = '/'.join(url_parts[1:]) On 2016/10/25 18:12:18, wrengr wrote: > Calling join after split is expensive. If sticking with the split approach, you > should pass 1 as the second argument to split. That way you get the host and > repo_path as the two things split returns; no post-processing necessary. Done. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:76: subprocess.call(['git', 'clone', self.repo_url, real_repo_path]) On 2016/10/25 18:12:18, wrengr wrote: > We should check the return value to make sure things worked. Done. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:81: subprocess.check_call( On 2016/10/25 18:12:18, wrengr wrote: > Ditto. Especially given as we're silencing stderr. Done. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:91: command = 'TZ=UTC %s --date=format-local:"%s"' % ( On 2016/10/25 23:04:23, stgao wrote: > Should TZ=UTC be set through env? Does this work across platform? Do we want to use UTC everywhere? I think it may be safer to restrict the use of UTC just here in local_git_repository. I cannot find anywhere in the git doc https://git-scm.com/docs/git-log saying it doesn't work across platform, I think as long as git works fine for different platform, this argument should work fine. https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:93: return 'cd %s; %s' % (self.real_repo_path, command) On 2016/10/25 18:12:18, wrengr wrote: > Should be 'cd %s && %s'. If the cd fails for some reason, we don't want to > silently run the command in the wrong place. Done. Good to know this usage :) https://codereview.chromium.org/2432113002/diff/160001/appengine/findit/commo... appengine/findit/common/local_git_repository.py:120: """Returns blame of the file at ``path`` of the given revision.""" On 2016/10/25 23:04:23, stgao wrote: > On 2016/10/25 18:12:18, wrengr wrote: > > Is our style to use double backticks when referring to code, or using pipes? > > When I asked mbarbella, he said pipes (so that's what I've been using). > > Whichever we use, we should be consistent. > > In infra, double backticks is the way to go. Acknowledged.
Patchset #8 (id:200001) has been deleted
Patchset #8 (id:220001) has been deleted
Patchset #9 (id:260001) has been deleted
Patchset #9 (id:280001) has been deleted
https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_repository.py:43: self.repo_url = repo_url I still think you should rename the repo_url and _repo_url fields to make clear what the difference is. Moreover, the repo_url property and the repo_url field clash with one another. Since the _repo_url field is associated with the repo_url property (as it should be), the repo_url field is the one that needs to be renamed/removed. https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_repository.py:82: if not os.path.exists(self.real_repo_path): In general it's bad to check whether a file/directory exists and then mess with that file based on the result, since this involves race conditions. The race-free approach is to assume it already exists, and catch the error to do whatever else when it doesn't. https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_repository.py:123: start_revision = 'HEAD' if start_revision == 'master' else start_revision Since this conditional rewrite is used in a bunch of places, it'd be cleaner to factor it out as a standalone function. https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_repository.py:132: return local_git_parsers.GitChangeLogsParser()(output, self.repo_url) Presumably you want to save the result of local_git_parsers.GitChangeLogsParser() so that you can reuse the same parser, rather than constructing a new one every time GetChangeLogs is called. Ditto for the other parsers. Also, it'd clean up the code here if the parsers return None whenever ``output`` is falsy, rather than doing that check here. Especially given as the parsers will already need to check for falsy arguments before trying to parse them. https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/repo_util.py (right): https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/repo_util.py:94: return None You probably want to throw an exception packaging up the ``p.returncode``, ``command``, and ``stderrdata``; that way callers can decide whether to eat the error and return None, vs do something else instead (retry, recover some other way, crash the program). https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/test/local_git_repository_test.py (right): https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/test/local_git_repository_test.py:30: def testCloneOrUpdateRepoIfNeeded(self): This should be broken out as three different tests https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/test/local_git_repository_test.py:221: self.assertTrue(region.ToDict(), expected_region.ToDict()) Why compare the results of ToDict rather than comparing the Region objects themselves? (defining the Region.__eq__ method as necessary) https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/test/repo_util_test.py (right): https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/test/repo_util_test.py:147: def returncode(self): -> ``return 1 if self.command == 'dummy' else 0`` https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/test/repo_util_test.py:156: self.mock(subprocess, 'Popen', _MockPopen) -> ``self.mock(subprocess, 'Popen', lambda command, **_: _MockProcess(command))``
Patchset #10 (id:320001) has been deleted
https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_repository.py:43: self.repo_url = repo_url On 2016/11/01 20:26:52, wrengr wrote: > I still think you should rename the repo_url and _repo_url fields to make clear > what the difference is. Moreover, the repo_url property and the repo_url field > clash with one another. Since the _repo_url field is associated with the > repo_url property (as it should be), the repo_url field is the one that needs to > be renamed/removed. Done. https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_repository.py:82: if not os.path.exists(self.real_repo_path): On 2016/11/01 20:26:53, wrengr wrote: > In general it's bad to check whether a file/directory exists and then mess with > that file based on the result, since this involves race conditions. The > race-free approach is to assume it already exists, and catch the error to do > whatever else when it doesn't. what if one process is git cloning the repo, and another process find out the path exists and do a git pull? Would there be race condition? https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_repository.py:123: start_revision = 'HEAD' if start_revision == 'master' else start_revision On 2016/11/01 20:26:53, wrengr wrote: > Since this conditional rewrite is used in a bunch of places, it'd be cleaner to > factor it out as a standalone function. Done. https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_repository.py:132: return local_git_parsers.GitChangeLogsParser()(output, self.repo_url) On 2016/11/01 20:26:52, wrengr wrote: > Presumably you want to save the result of > local_git_parsers.GitChangeLogsParser() so that you can reuse the same parser, > rather than constructing a new one every time GetChangeLogs is called. Ditto for > the other parsers. > > Also, it'd clean up the code here if the parsers return None whenever ``output`` > is falsy, rather than doing that check here. Especially given as the parsers > will already need to check for falsy arguments before trying to parse them. Acknowledged. https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/repo_util.py (right): https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/repo_util.py:94: return None On 2016/11/01 20:26:53, wrengr wrote: > You probably want to throw an exception packaging up the ``p.returncode``, > ``command``, and ``stderrdata``; that way callers can decide whether to eat the > error and return None, vs do something else instead (retry, recover some other > way, crash the program). Crashing the program working for local testing, but if this is in the production code, I think we'd better use logging.error. Let's decide the location of those local_* files first. https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/test/local_git_repository_test.py (right): https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/test/local_git_repository_test.py:30: def testCloneOrUpdateRepoIfNeeded(self): On 2016/11/01 20:26:53, wrengr wrote: > This should be broken out as three different tests Done. https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/test/local_git_repository_test.py:221: self.assertTrue(region.ToDict(), expected_region.ToDict()) On 2016/11/01 20:26:53, wrengr wrote: > Why compare the results of ToDict rather than comparing the Region objects > themselves? (defining the Region.__eq__ method as necessary Added a TODO. https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/test/repo_util_test.py (right): https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/test/repo_util_test.py:147: def returncode(self): On 2016/11/01 20:26:53, wrengr wrote: > -> ``return 1 if self.command == 'dummy' else 0`` Done. https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/test/repo_util_test.py:156: self.mock(subprocess, 'Popen', _MockPopen) On 2016/11/01 20:26:53, wrengr wrote: > -> ``self.mock(subprocess, 'Popen', > lambda command, **_: _MockProcess(command))`` Done.
Patchset #11 (id:360001) has been deleted
Patchset #11 (id:380001) has been deleted
Patchset #11 (id:400001) has been deleted
Patchset #11 (id:420001) has been deleted
Patchset #12 (id:460001) has been deleted
Patchset #12 (id:480001) has been deleted
Ping :)
https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/commit_util.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/lib/g... appengine/findit/lib/gitiles/commit_util.py:9: import urllib2 You have these imports but you don't use them? https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/local_git_parsers.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_parsers.py:1: #Copyright 2016 The Chromium Authors. All rights reserved. Nit: a space before "Copyright" https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. Nit: 2014 -> 2016. Same for other added files https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/test/script_util_test.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/test/script_util_test.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. Nit: 2016
lgtm with nits (including chanli's) https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/300001/appengine/findit/lib/g... appengine/findit/lib/gitiles/local_git_repository.py:82: if not os.path.exists(self.real_repo_path): On 2016/11/05 01:18:15, Sharu Jiang wrote: > On 2016/11/01 20:26:53, wrengr wrote: > > In general it's bad to check whether a file/directory exists and then mess > with > > that file based on the result, since this involves race conditions. The > > race-free approach is to assume it already exists, and catch the error to do > > whatever else when it doesn't. > > what if one process is git cloning the repo, and another process find out the > path exists and do a git pull? Would there be race condition? Probably. Depends on whether git is smart enough to lock things while it's working. I think it is? but not sure. Needs some research. If it turns out that git isn't smart enough to handle it, we can always generate our own lockfile just before doing the clone and then remove it once the clone is finished. Feel free to deal with this in a separate cl; just be sure to make a ticket and reference it here https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/test/commit_util_test.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/lib/g... appengine/findit/lib/gitiles/test/commit_util_test.py:7: import subprocess ditto about unused import https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:73: def _SetFieldsFromRepoUrl(self, repo_url): Why is this factored out as a separate method? why not just inline it into the repo_url setter? https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/test/local_git_repository_test.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_repository_test.py:29: repo = local_git_repository.LocalGitRepository('httt://repo1') Just to be sure, is that supposed to be "httt" rather than "http"/"https"? If so, then probably should have a comment saying so (and why) https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_repository_test.py:36: repo = local_git_repository.LocalGitRepository('httt://repo2') ditto https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_repository_test.py:145: change_log.ChangeLog('author1', personally I'd linewrap after the opening parens, so that you're not forced to wrap the message fields https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_repository_test.py:170: self.assertEqual(changelog.ToDict(), expected_changelog.ToDict()) Should compare the CL objects directly via __eq__. Feel free to add a todo and make that change in a separate cl https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_repository_test.py:245: if self.command == 'dummy': Can be made a ``return 1 if ... else 0`` oneliner, like in util_scripts/test/script_util_test.py
lgtm with nits. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/test/commit_util_test.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/lib/g... appengine/findit/lib/gitiles/test/commit_util_test.py:6: import StringIO same here. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:37: follow the instructions in ('https://g3doc.corp.google.com/company/teams/ Could we use a go link instead? https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:44: def __init__(self, repo_url=None): empty line above. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:120: command = 'TZ=UTC %s --date=format-local:"%s"' % ( So it works again? https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:160: if not os.path.isfile(os.path.join(self.real_repo_path, path)): What if the file got deleted after some revision? Is this expected?
https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:37: follow the instructions in ('https://g3doc.corp.google.com/company/teams/ On 2016/11/08 21:56:36, stgao (slow on Monday) wrote: > Could we use a go link instead? I like the idea, but iirc policy is that go links shouldn't be shared to anywhere outside of google. Thus, since the infra repo is publicly available, it shouldn't have go links. Perhaps we could use http://g.co/ instead?
https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/commit_util.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/lib/g... appengine/findit/lib/gitiles/commit_util.py:9: import urllib2 On 2016/11/08 01:56:20, chanli wrote: > You have these imports but you don't use them? Oops, cleaned up. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/lib/g... File appengine/findit/lib/gitiles/test/commit_util_test.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/lib/g... appengine/findit/lib/gitiles/test/commit_util_test.py:6: import StringIO On 2016/11/08 21:56:36, stgao (slow on Monday) wrote: > same here. Oops, got messed up with too many moving around. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/lib/g... appengine/findit/lib/gitiles/test/commit_util_test.py:7: import subprocess On 2016/11/08 19:32:37, wrengr wrote: > ditto about unused import Done. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/local_git_parsers.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_parsers.py:1: #Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/08 01:56:20, chanli wrote: > Nit: a space before "Copyright" Done. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2016/11/08 01:56:20, chanli wrote: > Nit: 2014 -> 2016. Same for other added files Done. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:37: follow the instructions in ('https://g3doc.corp.google.com/company/teams/ On 2016/11/10 21:27:04, wrengr wrote: > On 2016/11/08 21:56:36, stgao (slow on Monday) wrote: > > Could we use a go link instead? > > I like the idea, but iirc policy is that go links shouldn't be shared to > anywhere outside of google. Thus, since the infra repo is publicly available, it > shouldn't have go links. Perhaps we could use http://g.co/ instead? we needs to submit a proposal and get approved to create new shorturl in http://g.co/ ... https://support.google.com/oh/answer/6210028?hl=en And for the public shortening tool like https://goo.gl/ The internal url cannot be shortened. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:44: def __init__(self, repo_url=None): On 2016/11/08 21:56:36, stgao (slow on Monday) wrote: > empty line above. Done. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:73: def _SetFieldsFromRepoUrl(self, repo_url): On 2016/11/08 19:32:37, wrengr wrote: > Why is this factored out as a separate method? why not just inline it into the > repo_url setter? Because this needs to be called in __init__, and having self.repo_url = repo_url in __init__ is confusing as you said in previous comments. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:120: command = 'TZ=UTC %s --date=format-local:"%s"' % ( On 2016/11/08 21:56:36, stgao (slow on Monday) wrote: > So it works again? The only case it doesn't work is 'git blame' + '--porcelain/incremental', so it works for the rest. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:160: if not os.path.isfile(os.path.join(self.real_repo_path, path)): On 2016/11/08 21:56:36, stgao (slow on Monday) wrote: > What if the file got deleted after some revision? Is this expected? good catch, this should be removed since if the file doesn't exist, the command output should be None. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/test/local_git_repository_test.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_repository_test.py:29: repo = local_git_repository.LocalGitRepository('httt://repo1') On 2016/11/08 19:32:37, wrengr wrote: > Just to be sure, is that supposed to be "httt" rather than "http"/"https"? If > so, then probably should have a comment saying so (and why) Sorry, this is typo... https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_repository_test.py:36: repo = local_git_repository.LocalGitRepository('httt://repo2') On 2016/11/08 19:32:37, wrengr wrote: > ditto Done. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_repository_test.py:145: change_log.ChangeLog('author1', On 2016/11/08 19:32:37, wrengr wrote: > personally I'd linewrap after the opening parens, so that you're not forced to > wrap the message fields Done. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_repository_test.py:170: self.assertEqual(changelog.ToDict(), expected_changelog.ToDict()) On 2016/11/08 19:32:37, wrengr wrote: > Should compare the CL objects directly via __eq__. Feel free to add a todo and > make that change in a separate cl Done. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/test/local_git_repository_test.py:245: if self.command == 'dummy': On 2016/11/08 19:32:37, wrengr wrote: > Can be made a ``return 1 if ... else 0`` oneliner, like in > util_scripts/test/script_util_test.py Done. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/test/script_util_test.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/test/script_util_test.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2016/11/08 01:56:20, chanli wrote: > Nit: 2016 Done.
https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:37: follow the instructions in ('https://g3doc.corp.google.com/company/teams/ On 2016/11/11 00:29:06, Sharu Jiang wrote: > On 2016/11/10 21:27:04, wrengr wrote: > > On 2016/11/08 21:56:36, stgao (slow on Monday) wrote: > > > Could we use a go link instead? > > > > I like the idea, but iirc policy is that go links shouldn't be shared to > > anywhere outside of google. Thus, since the infra repo is publicly available, > it > > shouldn't have go links. Perhaps we could use http://g.co/ instead? > > we needs to submit a proposal and get approved to create new shorturl in > http://g.co/ ... > https://support.google.com/oh/answer/6210028?hl=en > > And for the public shortening tool like https://goo.gl/ > The internal url cannot be shortened. go link seems fine as it is used a lot in the infra repo like https://cs.chromium.org/chromium/infra/navbar.md https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:160: if not os.path.isfile(os.path.join(self.real_repo_path, path)): On 2016/11/11 00:29:06, Sharu Jiang wrote: > On 2016/11/08 21:56:36, stgao (slow on Monday) wrote: > > What if the file got deleted after some revision? Is this expected? > > good catch, this should be removed since if the file doesn't exist, the command > output should be None. This seems unexpected compared to the Gitiles. So it is impossible to get the content of a deleted file which existed in a previous revision?
https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:73: def _SetFieldsFromRepoUrl(self, repo_url): On 2016/11/11 00:29:05, Sharu Jiang wrote: > On 2016/11/08 19:32:37, wrengr wrote: > > Why is this factored out as a separate method? why not just inline it into the > > repo_url setter? > > Because this needs to be called in __init__, and having self.repo_url = repo_url > in __init__ is confusing as you said in previous comments. Oh! That's what was up with things before! If that's the only reason for factoring things out, then feel-free to use repo_url.setter directly within __init__ ---just be sure to add a comment clarifying that it's actually a call to the setter for the property. (Or, perhaps make things more explicit by directly calling the assignment method on the property object backing ``self.repo_url``.) The biggest reason why it was so confusing before was because of the lack of documentation indicating what was really going on
Patchset #13 (id:520001) has been deleted
https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:37: follow the instructions in ('https://g3doc.corp.google.com/company/teams/ On 2016/11/11 01:07:59, stgao (slow on Monday) wrote: > On 2016/11/11 00:29:06, Sharu Jiang wrote: > > On 2016/11/10 21:27:04, wrengr wrote: > > > On 2016/11/08 21:56:36, stgao (slow on Monday) wrote: > > > > Could we use a go link instead? > > > > > > I like the idea, but iirc policy is that go links shouldn't be shared to > > > anywhere outside of google. Thus, since the infra repo is publicly > available, > > it > > > shouldn't have go links. Perhaps we could use http://g.co/ instead? > > > > we needs to submit a proposal and get approved to create new shorturl in > > http://g.co/ ... > > https://support.google.com/oh/answer/6210028?hl=en > > > > And for the public shortening tool like https://goo.gl/ > > The internal url cannot be shortened. > > go link seems fine as it is used a lot in the infra repo like > https://cs.chromium.org/chromium/infra/navbar.md Done. https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:73: def _SetFieldsFromRepoUrl(self, repo_url): On 2016/11/11 18:38:48, wrengr wrote: > On 2016/11/11 00:29:05, Sharu Jiang wrote: > > On 2016/11/08 19:32:37, wrengr wrote: > > > Why is this factored out as a separate method? why not just inline it into > the > > > repo_url setter? > > > > Because this needs to be called in __init__, and having self.repo_url = > repo_url > > in __init__ is confusing as you said in previous comments. > > Oh! That's what was up with things before! If that's the only reason for > factoring things out, then feel-free to use repo_url.setter directly within > __init__ ---just be sure to add a comment clarifying that it's actually a call > to the setter for the property. (Or, perhaps make things more explicit by > directly calling the assignment method on the property object backing > ``self.repo_url``.) The biggest reason why it was so confusing before was > because of the lack of documentation indicating what was really going on According to http://stackoverflow.com/questions/3941919/how-to-set-a-python-property-in-init The previous implementation looks fine, I changed back and add comments to avoid confusion :)
https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... File appengine/findit/util_scripts/git_checkout/local_git_repository.py (right): https://codereview.chromium.org/2432113002/diff/500001/appengine/findit/util_... appengine/findit/util_scripts/git_checkout/local_git_repository.py:160: if not os.path.isfile(os.path.join(self.real_repo_path, path)): On 2016/11/11 01:07:59, stgao (slow on Monday) wrote: > On 2016/11/11 00:29:06, Sharu Jiang wrote: > > On 2016/11/08 21:56:36, stgao (slow on Monday) wrote: > > > What if the file got deleted after some revision? Is this expected? > > > > good catch, this should be removed since if the file doesn't exist, the > command > > output should be None. > > This seems unexpected compared to the Gitiles. > So it is impossible to get the content of a deleted file which existed in a > previous revision? It is possible to get the content of a deleted file which existed in a previous revision. I meant if the file indeed does not exist, the command will return None.
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/2432113002/#ps540001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Findit] Add local_git_parsers, local_git_repository BUG=605369 ========== to ========== [Findit] Add local_git_parsers, local_git_repository BUG=605369 Committed: https://chromium.googlesource.com/infra/infra/+/bc02c3473ee6c1a08f4ef97ef66f0... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:540001) as https://chromium.googlesource.com/infra/infra/+/bc02c3473ee6c1a08f4ef97ef66f0...
Message was sent while issue was closed.
Patchset #13 (id:540001) has been deleted |
