Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2038673002:
git cl description: avoid appending BUG= after Change-Id. (Closed)
|
Created:
4 years, 6 months ago by tandrii(chromium) Modified:
4 years, 6 months ago CC:
chromium-reviews, tandrii+omg_git_cl_chromium.org, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@bug-footer-fix Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
Descriptiongit cl description: avoid appending BUG= after Change-Id.
R=yyanagisawa@chromium.org,sergiyb@chromium.org
BUG=614587
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300692
Patch Set 1 #Patch Set 2 : 80chars #
Total comments: 13
Patch Set 3 : awesome review #Patch Set 4 : Updated patchset dependency #
Total comments: 2
Patch Set 5 : nit #Messages
Total messages: 29 (13 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/2038673002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2038673002/1
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/2038673002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2038673002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2038673002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2038673002/diff/20001/git_cl.py#newcode2711 git_cl.py:2711: if not self._description_lines: Why are you accessing self._description_lines here and not using self.description and self.set_description instead? Please add a comment with an explanation. https://codereview.chromium.org/2038673002/diff/20001/git_cl.py#newcode2717 git_cl.py:2717: assert top_lines[-1] == '' and len(top_lines) >= 2 Please mention that this is also guaranteed in the git_footers.split_footers documentation. When reading this code, first thing that came to my mind was that CL description is specified during upload in which case this assumption may be violated by our users. And I needed to read the entire split_footers function to see that indeed this assumption holds. Also after further reading, I've noticed that top_lines may be shorter than 2. For example, for description description = '\nMy-Footer: foo" the function git.footers.split_footers(description) will return: ([''], ['My-Footer: foo'], [('My-Footer', 'foo')]) https://codereview.chromium.org/2038673002/diff/20001/git_cl.py#newcode2720 git_cl.py:2720: insert_index = len(top_lines) Shouldn't you also check and if necessary add an empty line here to guarantee that footers are separated from the actual descriptions? The whole insert_index logic is quite confusing... please add some comments or even better - try to rewrite in a more elegant way. It seems to me that logic in this function is deeply intervened and depends on undocumented logic in gerrit_footers.split_footers and that makes it really confusing. https://codereview.chromium.org/2038673002/diff/20001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/2038673002/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1031: ('foo\n\nChange-Id: Ixx', 'R=one', 'foo\n\nR=one\n\nChange-Id: Ixx'), Also test adding a new footer to an existing one (first 3 tests) and adding Gerrit footer to simple description (last test): ('foo\n\nR=one\n\nChange-Id: Ixx', 'TBR=two', 'foo\n\nR=one\nTBR=two\n\nChange-Id: Ixx'), ('foo\n\nR=one\n\nChange-Id: Ixx', 'Foo-Bar: baz', 'foo\n\nR=one\n\nChange-Id: Ixx\nFoo-Bar: baz'), ('foo\n\nChange-Id: Ixx', 'Foo-Bar: baz', 'foo\n\nChange-Id: Ixx\nFoo-Bar: baz'), ('foo', 'Change-Id: Ixx', 'foo\n\nChange-Id: Ixx'),
lgtm
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/2038673002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2038673002/40001
Thanks for reviews! PTAL https://codereview.chromium.org/2038673002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2038673002/diff/20001/git_cl.py#newcode2600 git_cl.py:2600: self._description_lines = (description or '').strip().splitlines() see this? https://codereview.chromium.org/2038673002/diff/20001/git_cl.py#newcode2711 git_cl.py:2711: if not self._description_lines: On 2016/06/02 22:59:53, Sergiy Byelozyorov wrote: > Why are you accessing self._description_lines here and not using > self.description and self.set_description instead? Please add a comment with an > explanation. because _description_lines is actual primary storage of information. Why does that need a comment? https://codereview.chromium.org/2038673002/diff/20001/git_cl.py#newcode2717 git_cl.py:2717: assert top_lines[-1] == '' and len(top_lines) >= 2 On 2016/06/02 22:59:53, Sergiy Byelozyorov wrote: > Please mention that this is also guaranteed in the git_footers.split_footers > documentation. When reading this code, first thing that came to my mind was that > CL description is specified during upload in which case this assumption may be > violated by our users. And I needed to read the entire split_footers function to > see that indeed this assumption holds. Added comment. > Also after further reading, I've noticed that top_lines may be shorter than 2. > For example, for description > description = '\nMy-Footer: foo" > the function git.footers.split_footers(description) will return: > ([''], ['My-Footer: foo'], [('My-Footer', 'foo')]) So, previously that wasn't the case - top_lines was always at least 2. But after I've modified the previous CL for split_footers implementation, yes, you are right. refactored. https://codereview.chromium.org/2038673002/diff/20001/git_cl.py#newcode2720 git_cl.py:2720: insert_index = len(top_lines) On 2016/06/02 22:59:53, Sergiy Byelozyorov wrote: > Shouldn't you also check and if necessary add an empty line here to guarantee > that footers are separated from the actual descriptions? The whole insert_index > logic is quite confusing... please add some comments or even better - try to > rewrite in a more elegant way. It seems to me that logic in this function is > deeply intervened and depends on undocumented logic in > gerrit_footers.split_footers and that makes it really confusing. OK, fair. Rewrote it. WDYT? https://codereview.chromium.org/2038673002/diff/20001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/2038673002/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1031: ('foo\n\nChange-Id: Ixx', 'R=one', 'foo\n\nR=one\n\nChange-Id: Ixx'), On 2016/06/02 22:59:53, Sergiy Byelozyorov wrote: > Also test adding a new footer to an existing one (first 3 tests) and adding > Gerrit footer to simple description (last test): > > ('foo\n\nR=one\n\nChange-Id: Ixx', 'TBR=two', > 'foo\n\nR=one\nTBR=two\n\nChange-Id: Ixx'), > ('foo\n\nR=one\n\nChange-Id: Ixx', 'Foo-Bar: baz', 'foo\n\nR=one\n\nChange-Id: > Ixx\nFoo-Bar: baz'), > ('foo\n\nChange-Id: Ixx', 'Foo-Bar: baz', 'foo\n\nChange-Id: Ixx\nFoo-Bar: > baz'), > ('foo', 'Change-Id: Ixx', 'foo\n\nChange-Id: Ixx'), Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/2038673002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2038673002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ nits https://codereview.chromium.org/2038673002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2038673002/diff/20001/git_cl.py#newcode2600 git_cl.py:2600: self._description_lines = (description or '').strip().splitlines() On 2016/06/03 09:34:59, tandrii(chromium) wrote: > see this? Acknowledged. https://codereview.chromium.org/2038673002/diff/20001/git_cl.py#newcode2711 git_cl.py:2711: if not self._description_lines: On 2016/06/03 09:34:59, tandrii(chromium) wrote: > On 2016/06/02 22:59:53, Sergiy Byelozyorov wrote: > > Why are you accessing self._description_lines here and not using > > self.description and self.set_description instead? Please add a comment with > an > > explanation. > > because _description_lines is actual primary storage of information. Why does > that need a comment? I was confused that you use both self._description_lines and self.description in the same function, but now I see that they have different semantic: first is a list of lines , second is just entire description. It's all good now :-). https://codereview.chromium.org/2038673002/diff/20001/git_cl.py#newcode2717 git_cl.py:2717: assert top_lines[-1] == '' and len(top_lines) >= 2 On 2016/06/03 09:34:59, tandrii(chromium) wrote: > On 2016/06/02 22:59:53, Sergiy Byelozyorov wrote: > > Please mention that this is also guaranteed in the git_footers.split_footers > > documentation. When reading this code, first thing that came to my mind was > that > > CL description is specified during upload in which case this assumption may be > > violated by our users. And I needed to read the entire split_footers function > to > > see that indeed this assumption holds. > Added comment. > > > Also after further reading, I've noticed that top_lines may be shorter than 2. > > For example, for description > > description = '\nMy-Footer: foo" > > the function git.footers.split_footers(description) will return: > > ([''], ['My-Footer: foo'], [('My-Footer', 'foo')]) > So, previously that wasn't the case - top_lines was always at least 2. But after > I've modified the previous CL for split_footers implementation, yes, you are > right. refactored. Acknowledged. https://codereview.chromium.org/2038673002/diff/20001/git_cl.py#newcode2720 git_cl.py:2720: insert_index = len(top_lines) On 2016/06/03 09:34:59, tandrii(chromium) wrote: > On 2016/06/02 22:59:53, Sergiy Byelozyorov wrote: > > Shouldn't you also check and if necessary add an empty line here to guarantee > > that footers are separated from the actual descriptions? The whole > insert_index > > logic is quite confusing... please add some comments or even better - try to > > rewrite in a more elegant way. It seems to me that logic in this function is > > deeply intervened and depends on undocumented logic in > > gerrit_footers.split_footers and that makes it really confusing. > > OK, fair. Rewrote it. WDYT? Excellent. Much easier to understand now. https://codereview.chromium.org/2038673002/diff/60001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/2038673002/diff/60001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1034: ('foo\n\nR=one\n\nChange-Id: Ixx', 'Foo-Bar: baz', nit: fix indentation
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yyanagisawa@chromium.org, sergiyb@chromium.org Link to the patchset: https://codereview.chromium.org/2038673002/#ps80001 (title: "nit")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2028303006 Patch 80001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
https://codereview.chromium.org/2038673002/diff/60001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/2038673002/diff/60001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1034: ('foo\n\nR=one\n\nChange-Id: Ixx', 'Foo-Bar: baz', On 2016/06/03 12:35:16, Sergiy Byelozyorov wrote: > nit: fix indentation Done.
The CQ bit was checked by tandrii@chromium.org
The CQ bit was checked by tandrii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038673002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2038673002/80001
Message was sent while issue was closed.
Description was changed from ========== git cl description: avoid appending BUG= after Change-Id. R=yyanagisawa@chromium.org,sergiyb@chromium.org BUG=614587 ========== to ========== git cl description: avoid appending BUG= after Change-Id. R=yyanagisawa@chromium.org,sergiyb@chromium.org BUG=614587 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300692 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300692 |
