Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
509843002:
Give git map-branches extra information behind -v and -vv flags. (Closed)
|
Created:
6 years, 3 months ago by calamity Modified:
6 years, 3 months ago CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, iannucci+depot_tools_chromium.org, ilevy-cc_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Project:
tools Visibility:
Public. |
DescriptionGive git map-branches extra information behind -v and -vv flags.
This CL adds information to the git map-branches command. Invoking
it map-branches with -v, will show tracking status of branches and
invoking with -vv will additionally show the Rietveld URL and the
branch hash.
BUG=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=291776
Patch Set 1 : #Patch Set 2 : handle deleted upstreams more informatively #
Total comments: 43
Patch Set 3 : address comments #Patch Set 4 : address more comments #Patch Set 5 : make main loop nicer, privatize tags #
Total comments: 5
Patch Set 6 : rebase, address comments, make tests work on git < 1.9 #
Total comments: 2
Patch Set 7 : add # pragma: no cover to compensate for git < 1.9 #
Messages
Total messages: 20 (4 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
calamity@chromium.org changed reviewers: + mgiuca@chromium.org
iannucci@chromium.org changed reviewers: + iannucci@chromium.org
We'll also want to update the manpage examples for this. it's in man/src: * on a no-changes branch, run man/src/make_docs.sh * do a `git checkout --force` to clean up the miscellaneous changes (dates, etc) * apply your patch * edit man/src/git-map-branches.demo.1.sh to show off -v and -vv * run man/src/make_docs.sh again to regenerate the man pages * take a look at `git help map-branches` and man/html/git-map-branches.html https://codereview.chromium.org/509843002/diff/60001/git_common.py File git_common.py (right): https://codereview.chromium.org/509843002/diff/60001/git_common.py#newcode725 git_common.py:725: def normalized_version(): add a docstring, it's not obvious that this is getting the version of the git client. https://codereview.chromium.org/509843002/diff/60001/git_common.py#newcode742 git_common.py:742: (branch, branch_hash, upstream_branch, tracking_status) = line.split(':') won't this be wrong if your git version is < 1.9? We could fill this info in for < 1.9 by running upstream() on each branch though https://codereview.chromium.org/509843002/diff/60001/git_common.py#newcode745 git_common.py:745: ahead = ahead_match.group() if ahead_match else '' let's cast these to int() or None for this function. it makes more sense than returning a string https://codereview.chromium.org/509843002/diff/60001/git_common.py#newcode755 git_common.py:755: } let's use a namedtuple for this, instead of a freeform dict https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py File git_map_branches.py (right): https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:239: parser.add_argument('-v', action='store_true', let's use the action 'count' for this, instead of having -v and -vv https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:240: help='Display upstream tracking status') should we just do this by default? is there any reason not to? https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:254: verbosity = 2 then you don't need this https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:260: print mapper.output not sure if this is clearer than just returning the output from start()
https://codereview.chromium.org/509843002/diff/60001/git_common.py File git_common.py (right): https://codereview.chromium.org/509843002/diff/60001/git_common.py#newcode94 git_common.py:94: # Version which for-each-ref format string first supported upstream:track. This is hard to read and I don't know what it's saying. Can you rewrite it? https://codereview.chromium.org/509843002/diff/60001/git_common.py#newcode725 git_common.py:725: def normalized_version(): Can you rename it to "get_git_version". Maybe then it won't need a docstring. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py File git_map_branches.py (right): https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:46: aligned columns.""" The first sentence is supposed to fit on one line. If you need more detail, have a blank line and then another paragraph (like a git commit). https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:66: # Update maximum column lengths Full stop. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:70: def __str__(self): I wouldn't use __str__ for this. __str__ should be short and not have newlines (so that it is appropriate for putting in a print with "%s"). Call this method as_string or something. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:77: separators.""" Same. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:105: """A class which constructs output representing the tree's branch structure""" nit: Need a full stop at the end. Also it needs to fit on one line: have fun! I think you need documentation about what the major attributes of this class represent. In particular, give some details about |tracking_info| -- what its keys and values are. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:115: for branch in self.tracking_info.keys(): nit: Don't need .keys() (just "for branch in self.tracking_info"). Alternatively, do: for branch, branch_info in self.tracking_info.iteritems() and then |branch_info| is equal to self.tracking_info[branch]. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:135: for parent in sorted(self.parent_map.keys()): Don't need .keys(). (Note that iterating over a dict produces its keys, so .keys() is never necessary unless you actually directly want a list of keys.) https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:141: else hash_one(parent, short=True)} Super hard to read. Break this expression out and indent it properly. Also I can't find a citation for this, but I believe the correct Python style is to have the "normal" case first and the "error" case second. So invert the logic here. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:179: suffix = ' *' optional (might look weird): Combine this into one expression: if branch == self.current_branch or (self.current_branch == 'HEAD' and branch_hash == self.current_hash): https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:216: # The rietveld issue associated with the branch. Capital R.
https://codereview.chromium.org/509843002/diff/60001/git_common.py File git_common.py (right): https://codereview.chromium.org/509843002/diff/60001/git_common.py#newcode94 git_common.py:94: # Version which for-each-ref format string first supported upstream:track. On 2014/09/01 05:12:50, Matt Giuca wrote: > This is hard to read and I don't know what it's saying. Can you rewrite it? Done. https://codereview.chromium.org/509843002/diff/60001/git_common.py#newcode725 git_common.py:725: def normalized_version(): On 2014/08/29 18:41:47, iannucci wrote: > add a docstring, it's not obvious that this is getting the version of the git > client. Done. https://codereview.chromium.org/509843002/diff/60001/git_common.py#newcode725 git_common.py:725: def normalized_version(): On 2014/09/01 05:12:50, Matt Giuca wrote: > Can you rename it to "get_git_version". Maybe then it won't need a docstring. Done. Left the docstring in anyway. https://codereview.chromium.org/509843002/diff/60001/git_common.py#newcode742 git_common.py:742: (branch, branch_hash, upstream_branch, tracking_status) = line.split(':') On 2014/08/29 18:41:47, iannucci wrote: > won't this be wrong if your git version is < 1.9? > > We could fill this info in for < 1.9 by running upstream() on each branch though It makes tracking_status an empty string which then makes ahead and behind None. upstream() won't work for tracking_status since tracking_status is the [ x ahead, x behind ] information. To get this in git < 1.9, we'd need to run git branch -vv and parse out the information there. That's probably do-able but a touch less safe than this. Unless you meant wrong in some other way? https://codereview.chromium.org/509843002/diff/60001/git_common.py#newcode745 git_common.py:745: ahead = ahead_match.group() if ahead_match else '' On 2014/08/29 18:41:47, iannucci wrote: > let's cast these to int() or None for this function. it makes more sense than > returning a string Done. https://codereview.chromium.org/509843002/diff/60001/git_common.py#newcode755 git_common.py:755: } On 2014/08/29 18:41:47, iannucci wrote: > let's use a namedtuple for this, instead of a freeform dict Oh, cool! https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py File git_map_branches.py (right): https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:46: aligned columns.""" On 2014/09/01 05:12:50, Matt Giuca wrote: > The first sentence is supposed to fit on one line. If you need more detail, have > a blank line and then another paragraph (like a git commit). Done. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:66: # Update maximum column lengths On 2014/09/01 05:12:50, Matt Giuca wrote: > Full stop. Done. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:70: def __str__(self): On 2014/09/01 05:12:51, Matt Giuca wrote: > I wouldn't use __str__ for this. __str__ should be short and not have newlines > (so that it is appropriate for putting in a print with "%s"). Call this method > as_string or something. Done. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:77: separators.""" On 2014/09/01 05:12:50, Matt Giuca wrote: > Same. Done. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:105: """A class which constructs output representing the tree's branch structure""" On 2014/09/01 05:12:51, Matt Giuca wrote: > nit: Need a full stop at the end. > > Also it needs to fit on one line: have fun! > > I think you need documentation about what the major attributes of this class > represent. In particular, give some details about |tracking_info| -- what its > keys and values are. Done. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:115: for branch in self.tracking_info.keys(): On 2014/09/01 05:12:51, Matt Giuca wrote: > nit: Don't need .keys() (just "for branch in self.tracking_info"). > > Alternatively, do: > for branch, branch_info in self.tracking_info.iteritems() > > and then |branch_info| is equal to self.tracking_info[branch]. Done. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:135: for parent in sorted(self.parent_map.keys()): On 2014/09/01 05:12:50, Matt Giuca wrote: > Don't need .keys(). > > (Note that iterating over a dict produces its keys, so .keys() is never > necessary unless you actually directly want a list of keys.) Done. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:141: else hash_one(parent, short=True)} On 2014/09/01 05:12:50, Matt Giuca wrote: > Super hard to read. Break this expression out and indent it properly. Also I > can't find a citation for this, but I believe the correct Python style is to > have the "normal" case first and the "error" case second. So invert the logic > here. N/A. But I fixed this to be a lot nicer =D https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:179: suffix = ' *' On 2014/09/01 05:12:50, Matt Giuca wrote: > optional (might look weird): Combine this into one expression: > if branch == self.current_branch or (self.current_branch == 'HEAD' and > branch_hash == self.current_hash): Done. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:216: # The rietveld issue associated with the branch. On 2014/09/01 05:12:51, Matt Giuca wrote: > Capital R. Done. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:239: parser.add_argument('-v', action='store_true', On 2014/08/29 18:41:47, iannucci wrote: > let's use the action 'count' for this, instead of having -v and -vv Done. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:240: help='Display upstream tracking status') On 2014/08/29 18:41:47, iannucci wrote: > should we just do this by default? is there any reason not to? Eh. Not really. I just didn't want to modify the original default functionality. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:254: verbosity = 2 On 2014/08/29 18:41:47, iannucci wrote: > then you don't need this Done. https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:260: print mapper.output On 2014/08/29 18:41:47, iannucci wrote: > not sure if this is clearer than just returning the output from start() I think this is useful in case BranchMapper gets used for something else. start() would be the data processing call after which clients could ask for information.
lgtm, can't wait to try it out :) https://codereview.chromium.org/509843002/diff/60001/git_common.py File git_common.py (right): https://codereview.chromium.org/509843002/diff/60001/git_common.py#newcode742 git_common.py:742: (branch, branch_hash, upstream_branch, tracking_status) = line.split(':') On 2014/09/01 06:53:42, calamity wrote: > On 2014/08/29 18:41:47, iannucci wrote: > > won't this be wrong if your git version is < 1.9? > > > > We could fill this info in for < 1.9 by running upstream() on each branch > though > > It makes tracking_status an empty string which then makes ahead and behind None. > > upstream() won't work for tracking_status since tracking_status is the [ x > ahead, x behind ] information. To get this in git < 1.9, we'd need to run git > branch -vv and parse out the information there. > > That's probably do-able but a touch less safe than this. > > Unless you meant wrong in some other way? I think the .split(':') will explode on the assignment (not enough values to unpack into tuple)... but now I see that you're adding the extra ':' for both versions :) https://codereview.chromium.org/509843002/diff/140001/git_common.py File git_common.py (right): https://codereview.chromium.org/509843002/diff/140001/git_common.py#newcode765 git_common.py:765: missing_upstreams[info.upstream] = empty_info Maybe this is more convenient in the user-code (I haven't scrolled down yet :)), but it might make more sense for this to just be None https://codereview.chromium.org/509843002/diff/140001/git_map_branches.py File git_map_branches.py (right): https://codereview.chromium.org/509843002/diff/140001/git_map_branches.py#new... git_map_branches.py:204: behind_string = 'behind ' + str(behind) I'd use formatting for this 'behind %d' % behind
lgtm https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py File git_map_branches.py (right): https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:240: help='Display upstream tracking status') Seems consistent with git branch's behaviour of not being very helpful by default :) Seriously, I'd say leave it off by default for now, send out a PSA on chromium-dev after it lands and see what people think, if they find issues with it, etc. Then if enough people like it and after any bugs are fixed, turn it on by default. https://codereview.chromium.org/509843002/diff/140001/git_map_branches.py File git_map_branches.py (right): https://codereview.chromium.org/509843002/diff/140001/git_map_branches.py#new... git_map_branches.py:204: behind_string = 'behind ' + str(behind) +1. In general, prefer formatting over +.
https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py File git_map_branches.py (right): https://codereview.chromium.org/509843002/diff/60001/git_map_branches.py#newc... git_map_branches.py:240: help='Display upstream tracking status') On 2014/09/02 23:43:33, Matt Giuca wrote: > Seems consistent with git branch's behaviour of not being very helpful by > default :) > > Seriously, I'd say leave it off by default for now, send out a PSA on > chromium-dev after it lands and see what people think, if they find issues with > it, etc. Then if enough people like it and after any bugs are fixed, turn it on > by default. Done. https://codereview.chromium.org/509843002/diff/140001/git_common.py File git_common.py (right): https://codereview.chromium.org/509843002/diff/140001/git_common.py#newcode765 git_common.py:765: missing_upstreams[info.upstream] = empty_info On 2014/09/02 22:31:27, iannucci wrote: > Maybe this is more convenient in the user-code (I haven't scrolled down yet :)), > but it might make more sense for this to just be None Done. https://codereview.chromium.org/509843002/diff/140001/git_map_branches.py File git_map_branches.py (right): https://codereview.chromium.org/509843002/diff/140001/git_map_branches.py#new... git_map_branches.py:204: behind_string = 'behind ' + str(behind) On 2014/09/02 22:31:27, iannucci wrote: > I'd use formatting for this > > 'behind %d' % behind Done.
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/509843002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 509843002-160001 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 ** tests/git_common_test.py (5.80s) failed ......................u....... ---------------------------------------------------------------------- Ran 30 tests in 5.521s OK (unexpected successes=1) Name Stmts Miss Cover Missing ------------------------------------------ git_common 375 1 99% 740 FATAL: not at 100% coverage. Presubmit checks took 101.8s to calculate.
https://codereview.chromium.org/509843002/diff/160001/git_common.py File git_common.py (right): https://codereview.chromium.org/509843002/diff/160001/git_common.py#newcode739 git_common.py:739: if get_git_version() >= MIN_UPSTREAM_TRACK_GIT_VERSION: I guess we know what version the CQ's git is... you can add `# pragma: no cover` on this line with a comment saying that the depot_tools CQ currently has 1.8.
https://codereview.chromium.org/509843002/diff/160001/git_common.py File git_common.py (right): https://codereview.chromium.org/509843002/diff/160001/git_common.py#newcode739 git_common.py:739: if get_git_version() >= MIN_UPSTREAM_TRACK_GIT_VERSION: On 2014/09/03 01:48:48, iannucci wrote: > I guess we know what version the CQ's git is... > > you can add `# pragma: no cover` on this line with a comment saying that the > depot_tools CQ currently has 1.8. Cool. Done.
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/509843002/180001
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as 291776 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
