My favorites | Sign in
Project Home Downloads Wiki Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 663: Change-Id parsing should ignore empty lines
3 people starred this issue and may be notified of changes. Back to list
Status:  Duplicate
Merged:  issue 606
Owner:  ----
Closed:  May 2011


Sign in to add a comment
 
Reported by di...@google.com, Aug 17, 2010
Affected Version:
2.1.2.5

What steps will reproduce the problem?
1. Create a Git commit with a description like:
line1

line2
line3

Change-Id: <valid-changeId>

Some-Other-Footer: foobar

2. Push it for code review

What is the expected output? What do you see instead?
Gerrit should try to update the referenced change from the Change-Id footer. Instead it creates a new change ignoring the footer. My explanation for this is that it's only considering footers up to the first empty line. This is probably in line with the upstream kernel commit description guidelines but it bites our users who don't follow those guidelines.

Please provide any additional information below.
Changing the pattern matching to look for a Change-Id footer from the end of the description, ignoring new lines, until it finds a line that doesn't match the general footer regex (^[a-zA-Z0-9_-]+: .*$) or until it runs out of the description to parse would work for the above case and still be strict enough (ie it won't match random "Change-Id: foobar" inside the description body if there is at least one non empty line between the footers and this broken Change-Id that doesn't match the footers general regex).

This could arguably be considered a feature request instead tho to our users it seems like a bug.
Aug 17, 2010
Project Member #1 bklarson@gmail.com
Changing this concerns me a little.  Sometimes our users will reference other Change-Id's in the commit message.  It is a corner case, but I could see that causing problems if they don't have a change-id in their footer.

Perhaps  Issue 557  would take care of this issue?
Aug 17, 2010
#2 di...@google.com
Yes, we could enable mandatory Change-Id (assuming it will use the same logic to check for their presence) and that should work around the issues we face.

To be clear, you are worried that you may have a description like this:
Fixed nasty bug.

Fixed nasty bug in some nasty part of code introduced by
Change-Id: Iabcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbc when the moon phase was wrong.

Signed-off-by: foo@example.com

(I mean including the "Change-Id: " before the change ID value).
Aug 17, 2010
Project Member #3 bklarson@gmail.com
Exactly.  It is a real corner-case, probably ok to ignore.
Dec 21, 2010
#4 di...@google.com
A related issue (see issue 606) suggests something that seems to be even less likely to cause false positives: it asks for Gerrit to support a "^Change-Id: ..." footer anywhere in the commit message. What are the chances that someone has Change-Id lines in the middle of commit messages that are not intended to be such? One case I can think of is with squashes (when using "git rebase") and commit messages being appended automatically (including the Change-Id). We could have Gerrit look for the first "^Change-Id: ..." line starting from the last line and going up (so that it stops if there is a Change-Id in the last footer and considers that one instead of other Change-Id values included in the commit message from squashes and such).
May 15, 2011
#5 sop@google.com
(No comment was entered for this change.)
Status: Duplicate
Mergedinto: 606
Jan 9, 2013
#6 pvoi...@can2go.com
It is definitely annoying that it does need to be at the very end of commit messages... What if any other tool makes such an assumption with another footer?

In our case we had anticipated the move to Gerrit by using the commit-msg hook some time ago but we have other footers which got added to our commits... after Change-Id. We never thought it would be harmful to mix Change-Id with other footers and I don't have any clean solution to work around that problem.
Jan 9, 2013
#7 di...@google.com
A weird workaround that we've been using is to enable all repositories (where code reviews are going to be used) to require Change-Ids. This way, the strict check for Change-Ids is enforced early and it's much better to get an error when trying to upload a "broken" patch than to succeed to upload it, only to get a different Change-Id for it because the Change-Id included in the patch does not follow some strict rules that Gerrit works with.
Jan 9, 2013
#8 pvoi...@can2go.com
Indeed, this is a useful precaution but it doesn't really help for all those existing commits that I now want to push to my Gerrit instance... :)
Jan 9, 2013
Project Member #9 bklarson@gmail.com
@pvoisin - there are APIs to get a map of <string, string> for the name-value pairs contained in the commit footer.  It is a pretty standardized thing, just like the commit subject is one line by itself with a blank line following.

Adding other name-value pairs isn't a problem as long as they are all in the footer.  The Change-Id can be anywhere in the footer.  Just keep them all as one 'paragraph' (no blank lines between name-value pairs).
Jan 10, 2013
#10 pvoi...@can2go.com
Ah, nice! I'll pack every keys into one unique paragraph then. Thanks!
Sign in to add a comment

Powered by Google Project Hosting