Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
328843005:
Consolidated 'git' refish parsing into a class (Closed)
|
Created:
6 years, 6 months ago by dnj Modified:
6 years, 5 months ago CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, iannucci+depot_tools_chromium.org, ilevy-cc_chromium.org Visibility:
Public. |
DescriptionConsolidated 'git' refish parsing into a class
Created the 'GitRefish' class to centralize 'git' refish parsing and consistent
usage by 'gclient' 'git' code.
BUG=373504
TEST=localtest
R=agable@chromium.org, iannucci@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=281553
Patch Set 1 #
Total comments: 21
Patch Set 2 : Updated from maurel's comments!] #
Total comments: 12
Patch Set 3 : Updated w/ comments #
Total comments: 12
Patch Set 4 : Updated w/ nits, added new case(s) to refish parsing #
Total comments: 2
Patch Set 5 : Updated comment formatting #Patch Set 6 : Removed legacy code; must have been merged back in during a rebase. #
Messages
Total messages: 25 (0 generated)
PTAL Depends on: https://codereview.chromium.org/327803006/
ping (+mmoss, +nodir)
+maruel, since he knows 'gclient'.
PING (maruel@, I'd like you to be primary reviewer since the others don't have time right now. Please let me know if you don't either!)
FTR, I'm not working on depot_tools anymore so I'm looking for someone else to become the official owner. https://codereview.chromium.org/328843005/diff/1/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode164 gclient_scm.py:164: # Disable 'unused argument: options' warning | pylint: disable=W0613 Can you remove it instead? https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode235 gclient_scm.py:235: # The initial string that the parsed Refish came from +4 https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode251 gclient_scm.py:251: 2 lines https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode254 gclient_scm.py:254: """Class to implement refish parsing and properties. No need to state a class is a class, so use something like: """Implements refish parsing and properties. https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode270 gclient_scm.py:270: """Parses a ref into a 'GitRefish' instance. """Parses a ref into a 'GitRefish' instance.""" https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode274 gclient_scm.py:274: remote = cls._DEFAULT_REMOTE shorthand is: remote = remote or cls._DEFAULT_REMOTE https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode276 gclient_scm.py:276: if other_remotes is not None: if other_remotes: https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode455 gclient_scm.py:455: default_rev = 'refs/remotes/%s/master' % (self.remote,) Not needed. https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode951 gclient_scm.py:951: Checked out %s to a detached HEAD. Before making any commits It really breaks vim collapsing when using indent mode, so that's why I generally recommend against doing that. https://codereview.chromium.org/328843005/diff/1/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): https://codereview.chromium.org/328843005/diff/1/tests/gclient_scm_test.py#ne... tests/gclient_scm_test.py:1585: self.assertEquals( Use assertEqual instead This test would maybe benefit from being rewritten as: data = { 'refs/heads/master': gclient_scm.GitRefish( ...), ... } for ref, expected = data.iteritems(): self.assertEqual(expected, self.parse(ref))
Updated, PTAL! https://codereview.chromium.org/328843005/diff/1/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode235 gclient_scm.py:235: # The initial string that the parsed Refish came from On 2014/06/24 21:18:11, M-A Ruel wrote: > +4 Done. https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode251 gclient_scm.py:251: On 2014/06/24 21:18:11, M-A Ruel wrote: > 2 lines Done. https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode254 gclient_scm.py:254: """Class to implement refish parsing and properties. On 2014/06/24 21:18:11, M-A Ruel wrote: > No need to state a class is a class, so use something like: > """Implements refish parsing and properties. Done. https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode270 gclient_scm.py:270: """Parses a ref into a 'GitRefish' instance. On 2014/06/24 21:18:11, M-A Ruel wrote: > """Parses a ref into a 'GitRefish' instance.""" Done. https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode274 gclient_scm.py:274: remote = cls._DEFAULT_REMOTE On 2014/06/24 21:18:11, M-A Ruel wrote: > shorthand is: > remote = remote or cls._DEFAULT_REMOTE Done. https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode276 gclient_scm.py:276: if other_remotes is not None: On 2014/06/24 21:18:11, M-A Ruel wrote: > if other_remotes: Done. https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode455 gclient_scm.py:455: default_rev = 'refs/remotes/%s/master' % (self.remote,) On 2014/06/24 21:18:11, M-A Ruel wrote: > Not needed. I think it looks cleaner to be explicit in formatting strings. I'll revert this line, though, if you disagree. https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode951 gclient_scm.py:951: Checked out %s to a detached HEAD. Before making any commits On 2014/06/24 21:18:11, M-A Ruel wrote: > It really breaks vim collapsing when using indent mode, so that's why I > generally recommend against doing that. Done.
https://codereview.chromium.org/328843005/diff/1/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode455 gclient_scm.py:455: default_rev = 'refs/remotes/%s/master' % (self.remote,) On 2014/06/24 22:10:50, dnj wrote: > On 2014/06/24 21:18:11, M-A Ruel wrote: > > Not needed. > > I think it looks cleaner to be explicit in formatting strings. I'll revert this > line, though, if you disagree. Please ask around to others if anyone else ever used this format. If you can find me someone that can vouch for this style, as I don't recall anyone else doing this, I'll let you do use these 3 extra chars. :) https://codereview.chromium.org/328843005/diff/20001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/328843005/diff/20001/gclient_scm.py#newcode281 gclient_scm.py:281: if ( Make this one line. https://codereview.chromium.org/328843005/diff/20001/gclient_scm.py#newcode284 gclient_scm.py:284: if (len(ref_split) >= 3) and (ref_split[1] == 'heads'): Remove extra parenthesis, less characters is cleaner code. https://codereview.chromium.org/328843005/diff/20001/gclient_scm.py#newcode343 gclient_scm.py:343: if ref_tuple is not None: return '/'.join(ref_tuple) if ref_tuple else ref_tuple https://codereview.chromium.org/328843005/diff/20001/gclient_scm.py#newcode347 gclient_scm.py:347: # Instantiate This comment doesn't add much value, remove. https://codereview.chromium.org/328843005/diff/20001/gclient_scm.py#newcode981 gclient_scm.py:981: self.Print("""\ Why both """ and " ? https://codereview.chromium.org/328843005/diff/20001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): https://codereview.chromium.org/328843005/diff/20001/tests/gclient_scm_test.p... tests/gclient_scm_test.py:1593: self.assertEquals( assertEqual. Likely loop based.
https://codereview.chromium.org/328843005/diff/1/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode164 gclient_scm.py:164: # Disable 'unused argument: options' warning | pylint: disable=W0613 On 2014/06/24 21:18:11, M-A Ruel wrote: > Can you remove it instead? Done. https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode455 gclient_scm.py:455: default_rev = 'refs/remotes/%s/master' % (self.remote,) On 2014/06/25 01:24:58, M-A Ruel wrote: > On 2014/06/24 22:10:50, dnj wrote: > > On 2014/06/24 21:18:11, M-A Ruel wrote: > > > Not needed. > > > > I think it looks cleaner to be explicit in formatting strings. I'll revert > this > > line, though, if you disagree. > > Please ask around to others if anyone else ever used this format. If you can > find me someone that can vouch for this style, as I don't recall anyone else > doing this, I'll let you do use these 3 extra chars. :) I actually picked up this habit from iannucci@, who has just confirmed that he prefers this.
On 2014/06/25 01:55:06, dnj wrote: > https://codereview.chromium.org/328843005/diff/1/gclient_scm.py > File gclient_scm.py (right): > > https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode164 > gclient_scm.py:164: # Disable 'unused argument: options' warning | pylint: > disable=W0613 > On 2014/06/24 21:18:11, M-A Ruel wrote: > > Can you remove it instead? > > Done. > > https://codereview.chromium.org/328843005/diff/1/gclient_scm.py#newcode455 > gclient_scm.py:455: default_rev = 'refs/remotes/%s/master' % (self.remote,) > On 2014/06/25 01:24:58, M-A Ruel wrote: > > On 2014/06/24 22:10:50, dnj wrote: > > > On 2014/06/24 21:18:11, M-A Ruel wrote: > > > > Not needed. > > > > > > I think it looks cleaner to be explicit in formatting strings. I'll revert > > this > > > line, though, if you disagree. > > > > Please ask around to others if anyone else ever used this format. If you can > > find me someone that can vouch for this style, as I don't recall anyone else > > doing this, I'll let you do use these 3 extra chars. :) > > I actually picked up this habit from iannucci@, who has just confirmed that he > prefers this. And I picked it up when I had my innocent string-format turn into an application-halting assertion when the formatted object turned into a tuple :) In general, we should probably have a discussion about moving to str.format, which doesn't have this problem (and also has some nice additional things).
On 2014/06/25 01:58:17, iannucci wrote: > On 2014/06/25 01:55:06, dnj wrote: > > I actually picked up this habit from iannucci@, who has just confirmed that he > > prefers this. > > And I picked it up when I had my innocent string-format turn into an > application-halting assertion when the formatted object turned into a tuple :) > > In general, we should probably have a discussion about moving to str.format, > which doesn't have this problem (and also has some nice additional things). Ok, fine with me. And I don't have an opinion on str.format.
Awesome. Would you say it LGTY? :)
On 2014/06/25 19:24:24, dnj wrote: > Awesome. Would you say it LGTY? :) I was waiting for your next patchset.
Sorry about that - I missed round 2. Updated, and thanks again! https://codereview.chromium.org/328843005/diff/20001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/328843005/diff/20001/gclient_scm.py#newcode281 gclient_scm.py:281: if ( On 2014/06/25 01:24:58, M-A Ruel wrote: > Make this one line. Done. https://codereview.chromium.org/328843005/diff/20001/gclient_scm.py#newcode284 gclient_scm.py:284: if (len(ref_split) >= 3) and (ref_split[1] == 'heads'): On 2014/06/25 01:24:58, M-A Ruel wrote: > Remove extra parenthesis, less characters is cleaner code. Done. https://codereview.chromium.org/328843005/diff/20001/gclient_scm.py#newcode343 gclient_scm.py:343: if ref_tuple is not None: On 2014/06/25 01:24:58, M-A Ruel wrote: > return '/'.join(ref_tuple) if ref_tuple else ref_tuple Done. https://codereview.chromium.org/328843005/diff/20001/gclient_scm.py#newcode347 gclient_scm.py:347: # Instantiate On 2014/06/25 01:24:58, M-A Ruel wrote: > This comment doesn't add much value, remove. Done. https://codereview.chromium.org/328843005/diff/20001/gclient_scm.py#newcode981 gclient_scm.py:981: self.Print("""\ On 2014/06/25 01:24:58, M-A Ruel wrote: > Why both """ and " ? Ack sorry, that was a mistake. https://codereview.chromium.org/328843005/diff/20001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): https://codereview.chromium.org/328843005/diff/20001/tests/gclient_scm_test.p... tests/gclient_scm_test.py:1593: self.assertEquals( On 2014/06/25 01:24:58, M-A Ruel wrote: > assertEqual. Likely loop based. Done.
lgtm with a few nits. https://codereview.chromium.org/328843005/diff/40001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/328843005/diff/40001/gclient_scm.py#newcode270 gclient_scm.py:270: """Parses a ref into a 'GitRefish' instance.""" I'd like a small description of each argument, nothing huge, just clarify the types. In particular "remote" and "other_remotes", one is a string, the other an iterable but it's not totally obvious. https://codereview.chromium.org/328843005/diff/40001/gclient_scm.py#newcode277 gclient_scm.py:277: ref = ref.strip() optional: Another option is to assert ref == ref.strip(), ref to ensure callers are cleaning up their inputs. As you prefer. https://codereview.chromium.org/328843005/diff/40001/gclient_scm.py#newcode297 gclient_scm.py:297: remote_ref = ref_split[3:] Why not? remote_ref = '/'.join(ref_split[3:]) https://codereview.chromium.org/328843005/diff/40001/gclient_scm.py#newcode317 gclient_scm.py:317: is_branch = False What about? git checkout -b foo/bar origin/master https://codereview.chromium.org/328843005/diff/40001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): https://codereview.chromium.org/328843005/diff/40001/tests/gclient_scm_test.p... tests/gclient_scm_test.py:1593: remove https://codereview.chromium.org/328843005/diff/40001/tests/gclient_scm_test.p... tests/gclient_scm_test.py:1609: I don't see value in the whitespaces, it's already obvious. You can also align at +2 datastructures instead of +4.
Updated w/ suggestion and implementation of further parsing. PTAL. https://codereview.chromium.org/328843005/diff/40001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/328843005/diff/40001/gclient_scm.py#newcode270 gclient_scm.py:270: """Parses a ref into a 'GitRefish' instance.""" On 2014/06/25 20:54:57, M-A Ruel wrote: > I'd like a small description of each argument, nothing huge, just clarify the > types. In particular "remote" and "other_remotes", one is a string, the other an > iterable but it's not totally obvious. Done. https://codereview.chromium.org/328843005/diff/40001/gclient_scm.py#newcode277 gclient_scm.py:277: ref = ref.strip() On 2014/06/25 20:54:57, M-A Ruel wrote: > optional: Another option is to assert ref == ref.strip(), ref > to ensure callers are cleaning up their inputs. As you prefer. I like this better. Done. https://codereview.chromium.org/328843005/diff/40001/gclient_scm.py#newcode297 gclient_scm.py:297: remote_ref = ref_split[3:] On 2014/06/25 20:54:57, M-A Ruel wrote: > Why not? remote_ref = '/'.join(ref_split[3:]) I'm keeping the refs as tuples for easier handling until the end, when I convert them back into 'git' values via 'compose'. https://codereview.chromium.org/328843005/diff/40001/gclient_scm.py#newcode317 gclient_scm.py:317: is_branch = False On 2014/06/25 20:54:57, M-A Ruel wrote: > What about? > git checkout -b foo/bar origin/master The current implementation intended to reflect the 'gclient' logic which, I believe, would not make this inference. There isn't enough information in a refish string ("foo/bar") to reconstruct that entire checkout, so the checkout scenario is not supported by 'gclient'. I'll upload another patch set that will infer "foo/bar => origin/foo/bar". https://codereview.chromium.org/328843005/diff/40001/tests/gclient_scm_test.py File tests/gclient_scm_test.py (right): https://codereview.chromium.org/328843005/diff/40001/tests/gclient_scm_test.p... tests/gclient_scm_test.py:1593: On 2014/06/25 20:54:57, M-A Ruel wrote: > remove Done. https://codereview.chromium.org/328843005/diff/40001/tests/gclient_scm_test.p... tests/gclient_scm_test.py:1609: On 2014/06/25 20:54:57, M-A Ruel wrote: > I don't see value in the whitespaces, it's already obvious. You can also align > at +2 datastructures instead of +4. The whitespace is useful to me b/c it lets vim traverse each test case via { and }. I'll dedent though, will look much better.
Ping - I know I have LGTM but I added some extra logic that I'd someone to proof before I fire da missilez.
lgtm https://codereview.chromium.org/328843005/diff/60001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/328843005/diff/60001/gclient_scm.py#newcode7 gclient_scm.py:7: from __future__ import print_function We ought to remove them eventually. https://codereview.chromium.org/328843005/diff/60001/gclient_scm.py#newcode270 gclient_scm.py:270: """ """Parses ...
The CQ bit was checked by dnj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dnj@chromium.org/328843005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 328843005-80001 failed and returned exit status 1. Running presubmit commit checks ... Checking out rietveld... Running save-description-on-failure.sh Running push-basic.sh Running upstream.sh Running submit-from-new-dir.sh Running abandon.sh Running submodule-merge-test.sh Running upload-local-tracking-branch.sh Running hooks.sh Running post-dcommit-hook-test.sh Running upload-stale.sh Running patch.sh Running basic.sh ** Presubmit ERRORS ** Pylint (97 files) (35.52s) failed ************* Module /b/infra_internal/commit_queue/workdir/tools/depot_tools/gclient_scm.py W0612:507,6:GitWrapper.update: Unused variable 'rev_type' Presubmit checks took 114.7s to calculate.
The CQ bit was checked by dnj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dnj@chromium.org/328843005/100001
Message was sent while issue was closed.
Change committed as 281553
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/370393002/ by smut@google.com. The reason for reverting is: https://code.google.com/p/chromium/issues/detail?id=391871. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
