CandyShop / gerrit

Automatically exported from code.google.com/p/gerrit
Apache License 2.0
1 stars 0 forks source link

Accept Change-Id: tag at any place within commit message #606

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
I am seeing confusion where people add various kinds of information towards the 
end of their commit messages:

- bug IDs
- Signed-off-by and similar tags
- Describe how they tested the commit
- gerrit Change-Id: tag

All these things tend to be pushed towards the end of a commit message, but not 
necessarily in a precise order. But gerrit only considers the Change-Id: tag if 
it's part of the last paragraph, so it will silently ignore it if it's placed 
somewhere else, and people get confused.

Could gerrit be made to accept Change-Id: lines anywhere in the commit message ?

Original issue reported on code.google.com by wal...@google.com on 23 Jun 2010 at 12:29

GoogleCodeExporter commented 9 years ago
It would also cause new changes to generated, when instead a new patchset 
should be generated, and the changeid line moved in a subsequent patchset.

Original comment by antony.s...@gmail.com on 27 Jul 2010 at 11:56

GoogleCodeExporter commented 9 years ago
Antony - I'm not sure I understand your comment.

What I am seeing *now* is that new changes are generated instead of new 
patchsets, because gerrit ignores the Change-Id: header when it's not in the 
last paragraph.

Original comment by wal...@google.com on 28 Jul 2010 at 12:17

GoogleCodeExporter commented 9 years ago
Yah, what you said :) 

I was just trying to add that, the author should be given a change to correct 
his mistake, without causing a new "change".

Original comment by antony.s...@gmail.com on 28 Jul 2010 at 12:20

GoogleCodeExporter commented 9 years ago
While it will not stop the problem from happening in the first place (as this 
issues is suggesting), Issue 653 suggests a method to deal with this problem 
after the fact.

Original comment by mf...@codeaurora.org on 17 Aug 2010 at 9:31

GoogleCodeExporter commented 9 years ago
I'm sure there is a bug in JGit at the minute to do with reading comment tags 
(if that's the right name). I remember having a quick look into it, but not 
having time to make a fix.

Original comment by em...@pete-woods.com on 7 Feb 2011 at 2:56

GoogleCodeExporter commented 9 years ago
I don't think it is a good idea to allow a Change-Id tag anywhere in the commit 
message.  Having a commit footer with various tags is a pretty standard thing 
in git.  The rest of the commit message text should describe the change, and it 
is reasonable to reference another Change-Id in that text.

Would turning on the 'Require a Change-Id' option for your project in Gerrit 
solve this issue for your team?  That way a commit would be rejected during 
upload if it didn't have a valid Change-Id tag.

Original comment by bklarson@gmail.com on 7 Feb 2011 at 4:11

GoogleCodeExporter commented 9 years ago
git itself actually has no notion of footers - they are only a convention 
implemented among humans on top of git. Human conventions tend to be flexible, 
and not break just because a newline was inserted at the wrong place. Saying 
'git does it already!' to justify current gerrit footers behavior seems 
fallacious to me.

Original comment by wal...@google.com on 7 Feb 2011 at 9:59

GoogleCodeExporter commented 9 years ago
I respectfully disagree - see the git commit -s argument.  Also jgit has an API 
for pulling out the commit footer and parsing the tags 
(http://lists-archives.org/git/694524-add-parsing-support-for-signed-off-by-line
s-in-commit-messages.html).  IMO, a commit footer is just as much a git 
convention as the commit header.

Original comment by bklarson@gmail.com on 7 Feb 2011 at 10:19

GoogleCodeExporter commented 9 years ago
Issue 663 has been merged into this issue.

Original comment by sop@google.com on 15 May 2011 at 9:29

GoogleCodeExporter commented 9 years ago
Since there is a split group of people that want the feature and people that 
don't, then make it configurable? In my use case it is much less of an issue 
for a Change-Id to be mistakenly quoted in the middle of the message than for 
Gerrit to ignore an intended Change-Id somewhere not in the last paragraph. It 
has become less important since we enabled to require Change-Id to all reviews 
but until that happened it has produced pain for more than an year.

Original comment by di...@google.com on 16 May 2011 at 4:43

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
I don't really care where the Change-Id and Signed-off-by tags need to be, but 
the error message returned when rejecting due to incorrect placement *MUST* 
indicate where they need to be - It took me quite a while to figure out what 
was required...

Original comment by ehun...@broadcom.com on 25 Nov 2011 at 1:23

GoogleCodeExporter commented 9 years ago
ehunter: What version of Gerrit are you using?  If you are using 2.2.1 or later 
and you still don't like the error messages for this, please suggest an 
improvement.

Original comment by mf...@codeaurora.org on 25 Nov 2011 at 1:43

GoogleCodeExporter commented 9 years ago
Hi mfick, I'm using version 2.2.1 (on http://openocd.zylin.com) 

The error messages given were: 
! [remote rejected] HEAD -> refs/for/master (not Signed-off-by 
author/committer/uploader)

or

! [remote rejected] HEAD -> refs/for/master (missing Change-Id in commit 
message )

Both messages are incorrect - I did have Change-Id and Signed-off-by, but they 
were not in the correct position. The error message should be something like :

! [remote rejected] HEAD -> refs/for/master (not Signed-off-by 
author/committer/uploader in last paragraph of commit message)
! [remote rejected] HEAD -> refs/for/master (Change-Id not found in last 
paragraph of commit message )

Also, it would be nice if it said which commit was being rejected (where there 
are multiple commits being pushed)

Original comment by ehun...@broadcom.com on 25 Nov 2011 at 4:24

GoogleCodeExporter commented 9 years ago
I am fairly certain that those error messages where improved quite a bit back 
in May, I thought in 2.2.1, but I guess not.  Perhaps they are still in master 
unreleased (there are well over 300 unreleased changes I think there!).  It 
would probably be worth your while to check out the head.

Original comment by mf...@codeaurora.org on 29 Nov 2011 at 6:22

GoogleCodeExporter commented 9 years ago
the hick is that git-am -s add a new line after the ChangeId in an 'applied' 
patch which cause troubles as the commit hook then try to add a new ChangeId.

Original comment by nthieb...@gmail.com on 3 May 2012 at 7:33

GoogleCodeExporter commented 9 years ago
Issue 663 has been merged into this issue.

Original comment by bklarson@gmail.com on 10 Jan 2013 at 2:12