Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1835963003:
Gerrit git cl: stop creating a shadow branch. (Closed)
|
Created:
4 years, 8 months ago by tandrii(chromium) Modified:
4 years, 8 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@H150 Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionGerrit git cl: stop creating a shadow branch.
R=bauerb@chromium.org,ukai@chromium.org,iannucci@chromium.org
BUG=579175, 580136
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299587
Patch Set 1 #
Total comments: 10
Patch Set 2 : +review #Patch Set 3 : +fix #
Total comments: 4
Patch Set 4 : +review2 #Patch Set 5 : With tested recovery #
Total comments: 6
Patch Set 6 : review #Messages
Total messages: 26 (7 generated)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835963003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1835963003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
Description was changed from ========== Gerrit git cl: stop creating a shadown branch. R=bauerb@chromium.org,ukai@chromium.org,iannucci@chromium.org BUG=579175 ========== to ========== Gerrit git cl: stop creating a shadow branch. R=bauerb@chromium.org,ukai@chromium.org,iannucci@chromium.org BUG=579175 ==========
FTR: the proof the initial upload + re-upload still works with this CL is here: https://codereview.chromium.org/1835813002/#ps60001
lgtm https://codereview.chromium.org/1835963003/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1835963003/diff/1/git_cl.py#newcode2593 git_cl.py:2593: print "\nDescription is empty. Aborting..." nit: DieWithError? what is difference between DieWithError and return 1? https://codereview.chromium.org/1835963003/diff/1/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1835963003/diff/1/tests/git_cl_test.py#newcod... tests/git_cl_test.py:679: 'abcdef0123456789'],), ''), ref_to_push ?
Looks mostly good! https://codereview.chromium.org/1835963003/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1835963003/diff/1/git_cl.py#newcode2539 git_cl.py:2539: # Issues already on Gerrit really must have exactly 1 Change-Id. In practice this could fail (e.g. an old CL that was pushed without a Change-Id in the description). I'm not really sure how to handle that case though... Maybe just treat it as if there was no descripion? https://codereview.chromium.org/1835963003/diff/1/git_cl.py#newcode2580 git_cl.py:2580: 'Note: maybe you\'ve uploaded it with --no-squash or with old\n' Nit: "[...] with an old version [...]".
https://codereview.chromium.org/1835963003/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1835963003/diff/1/git_cl.py#newcode2539 git_cl.py:2539: # Issues already on Gerrit really must have exactly 1 Change-Id. On 2016/03/29 08:22:49, Bernhard Bauer wrote: > In practice this could fail (e.g. an old CL that was pushed without a Change-Id > in the description). I'm not really sure how to handle that case though... Maybe > just treat it as if there was no descripion? Didn't know it was possible, but let's assume so. Then how do you indicate to Gerrit that you are uploading a new patchset for *specific* issue? My take so far is that if this assert fails, a bug to infra will be filed, and we'll deal with it once it happens. https://codereview.chromium.org/1835963003/diff/1/git_cl.py#newcode2593 git_cl.py:2593: print "\nDescription is empty. Aborting..." On 2016/03/29 02:16:16, ukai wrote: > nit: DieWithError? > > what is difference between DieWithError and return 1? Same thing, afaiu. This was copy-paste from above, but will change here too.
https://codereview.chromium.org/1835963003/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1835963003/diff/1/git_cl.py#newcode2539 git_cl.py:2539: # Issues already on Gerrit really must have exactly 1 Change-Id. On 2016/03/29 08:37:41, tandrii(chromium)-OOO wrote: > On 2016/03/29 08:22:49, Bernhard Bauer wrote: > > In practice this could fail (e.g. an old CL that was pushed without a > Change-Id > > in the description). I'm not really sure how to handle that case though... > Maybe > > just treat it as if there was no descripion? > > Didn't know it was possible, but let's assume so. Then how do you indicate to > Gerrit that you are uploading a new patchset for *specific* issue? If a commit is pushed that doesn't have a Change-Id (or it's not in the footer, etc.), Gerrit internally assigns one, but it doesn't change the CL description. You can see an example of that at https://chromium-review.googlesource.com/#/c/329268; Gerrit shows the internal Change-Id below the description text field. If you want to upload a new patch set to that CL, it needs to have a commit message that uses that internal Change-Id in its footer. > My take so far is that if this assert fails, a bug to infra will be filed, and > we'll deal with it once it happens. Hm.... I'm not super happy with putting additional work (even just filing a bug) on the shoulders of a random developer who is just trying to upload a CL. Can we at least expand the error message to say something like "Contact chrome-infra@"? :)
https://codereview.chromium.org/1835963003/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1835963003/diff/1/git_cl.py#newcode2539 git_cl.py:2539: # Issues already on Gerrit really must have exactly 1 Change-Id. On 2016/03/29 09:03:21, Bernhard Bauer wrote: > On 2016/03/29 08:37:41, tandrii(chromium)-OOO wrote: > > On 2016/03/29 08:22:49, Bernhard Bauer wrote: > > > In practice this could fail (e.g. an old CL that was pushed without a > > Change-Id > > > in the description). I'm not really sure how to handle that case though... > > Maybe > > > just treat it as if there was no descripion? > > > > Didn't know it was possible, but let's assume so. Then how do you indicate to > > Gerrit that you are uploading a new patchset for *specific* issue? > > If a commit is pushed that doesn't have a Change-Id (or it's not in the footer, > etc.), Gerrit internally assigns one, but it doesn't change the CL description. > You can see an example of that at > https://chromium-review.googlesource.com/#/c/329268; Gerrit shows the internal > Change-Id below the description text field. If you want to upload a new patch > set to that CL, it needs to have a commit message that uses that internal > Change-Id in its footer. Ah, i see. > > > My take so far is that if this assert fails, a bug to infra will be filed, and > > we'll deal with it once it happens. > > Hm.... I'm not super happy with putting additional work (even just filing a bug) > on the shoulders of a random developer who is just trying to upload a CL. Can we > at least expand the error message to say something like "Contact chrome-infra@"? > :) Ah, given that this is likely to happen, I agree, too much burden. So how do you like the new version? https://codereview.chromium.org/1835963003/diff/1/git_cl.py#newcode2580 git_cl.py:2580: 'Note: maybe you\'ve uploaded it with --no-squash or with old\n' On 2016/03/29 08:22:49, Bernhard Bauer wrote: > Nit: "[...] with an old version [...]". Done. https://codereview.chromium.org/1835963003/diff/1/git_cl.py#newcode2593 git_cl.py:2593: print "\nDescription is empty. Aborting..." On 2016/03/29 08:37:41, tandrii(chromium)-OOO wrote: > On 2016/03/29 02:16:16, ukai wrote: > > nit: DieWithError? > > > > what is difference between DieWithError and return 1? > > Same thing, afaiu. This was copy-paste from above, but will change here too. Done.
https://codereview.chromium.org/1835963003/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1835963003/diff/40001/git_cl.py#newcode2563 git_cl.py:2563: if options.force: Why do we do this only if force is _set_? https://codereview.chromium.org/1835963003/diff/40001/git_cl.py#newcode2572 git_cl.py:2572: continue This is the last statement in the body of a while loop, so it doesn't actually do anything, no? I think we could simplify the control flow in here a bit. How about this: change_id = cl._codereview_impl._GetChangeDetail([])['change_id'] while True: footer_change_ids = git_footers.get_footer_change_id(message) if not footer_change_ids: message = git_footers.add_footer_change_id(message, change_id) print('WARNING: appended missing Change-Id to issue description') continue if footer_change_ids == [change_id]: break # There is already a valid footer [...] message = '%s\n\nChange-Id: %s' % (message, change_id) # ... # Sanity check [...] assert [change_id] == git_footers.get_footer_change_id(message)
Description was changed from ========== Gerrit git cl: stop creating a shadow branch. R=bauerb@chromium.org,ukai@chromium.org,iannucci@chromium.org BUG=579175 ========== to ========== Gerrit git cl: stop creating a shadow branch. R=bauerb@chromium.org,ukai@chromium.org,iannucci@chromium.org BUG=579175,580136 ==========
https://codereview.chromium.org/1835963003/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1835963003/diff/40001/git_cl.py#newcode2563 git_cl.py:2563: if options.force: On 2016/03/29 13:27:38, Bernhard Bauer wrote: > Why do we do this only if force is _set_? good catch, missing "not". https://codereview.chromium.org/1835963003/diff/40001/git_cl.py#newcode2572 git_cl.py:2572: continue On 2016/03/29 13:27:38, Bernhard Bauer wrote: > This is the last statement in the body of a while loop, so it doesn't actually > do anything, no? Yep, but I thought reading is easier that way. > > I think we could simplify the control flow in here a bit. How about this: > > change_id = cl._codereview_impl._GetChangeDetail([])['change_id'] > while True: > footer_change_ids = git_footers.get_footer_change_id(message) > if not footer_change_ids: > message = git_footers.add_footer_change_id(message, change_id) > print('WARNING: appended missing Change-Id to issue description') > continue > > if footer_change_ids == [change_id]: > break > > # There is already a valid footer [...] > message = '%s\n\nChange-Id: %s' % (message, change_id) > # ... > > # Sanity check [...] > assert [change_id] == git_footers.get_footer_change_id(message) Hey, I like that. Fixed.
Thanks, LGTM!
I've just realized that I need backwards compatible mode which will fetch change-id from deprecated git_cl_upload/xxx branch. Will upload a new patchset with that.
PTAL - i've added the case to fetch issue id based on changed id in shadow branch.
lgtm
Still LGTM, just some comment nits: https://codereview.chromium.org/1835963003/diff/80001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1835963003/diff/80001/git_cl.py#newcode2538 git_cl.py:2538: # TODO(tandrii): deperecate this after 2016Q2. Backwards compatability Nit: "compatibility" https://codereview.chromium.org/1835963003/diff/80001/git_cl.py#newcode2540 git_cl.py:2540: # using which we can fetch actual issue number and set is as the property Nit: "set _it_ as the property" https://codereview.chromium.org/1835963003/diff/80001/git_cl.py#newcode2585 git_cl.py:2585: # Doing this automatically is non-trivial as we don't want to loose Nit: "lose"
Gentlemen, I appreciate your prompt reviews. And I'm landing this. With luck, it'll stick. https://codereview.chromium.org/1835963003/diff/80001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1835963003/diff/80001/git_cl.py#newcode2538 git_cl.py:2538: # TODO(tandrii): deperecate this after 2016Q2. Backwards compatability On 2016/03/31 10:11:54, Bernhard Bauer wrote: > Nit: "compatibility" Done. https://codereview.chromium.org/1835963003/diff/80001/git_cl.py#newcode2540 git_cl.py:2540: # using which we can fetch actual issue number and set is as the property On 2016/03/31 10:11:54, Bernhard Bauer wrote: > Nit: "set _it_ as the property" Done. https://codereview.chromium.org/1835963003/diff/80001/git_cl.py#newcode2585 git_cl.py:2585: # Doing this automatically is non-trivial as we don't want to loose On 2016/03/31 10:11:54, Bernhard Bauer wrote: > Nit: "lose" Done.
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, ukai@chromium.org Link to the patchset: https://codereview.chromium.org/1835963003/#ps100001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835963003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1835963003/100001
Message was sent while issue was closed.
Description was changed from ========== Gerrit git cl: stop creating a shadow branch. R=bauerb@chromium.org,ukai@chromium.org,iannucci@chromium.org BUG=579175,580136 ========== to ========== Gerrit git cl: stop creating a shadow branch. R=bauerb@chromium.org,ukai@chromium.org,iannucci@chromium.org BUG=579175,580136 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299587 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=299587 |
