RFE: support change-id generation natively

RFE: support change-id generation natively

[james.moger]

Hello Git Community,

TL;DR:
It would be a really nice enhancement if the commit command natively
supported _optionally_ injecting a "Change-Id: I000..." footer in the
last paragraph of the commit message template and then substituting the
"I000..." value, on commit, with a generated value _without_ having to
rely on a per-repository, native hook or a global hook that affects
every local repository.

Full Request:
Gerrit has established the change-id footer as a prominent and
wide-spread collaboration identifier.  For those contributing new
patches to a Gerrit server, it is required to either use EGit/JGit
(Eclipse) to generate commits [1] OR to use a commit hook script with
native git to insert a change-id footer during the commit process [2]. 
This per-repository hook script requirement is an obstacle.  These
communities would be better served and it would lower the contribution
barrier for many open source projects if native git supported change-id
generation & injection.

I acknowledge that not everyone uses nor wants to use Gerrit and the
change-id footer.  That is fine, but it would be a _tremendous_
usability improvement for those contributing to open source projects
(myself included) if something like a "--change-id" flag  was
implemented and maybe even a config setting to always generate a
change-id on commit (EGit currently supports this as
"gerrit.createchangeid=true").

Sadly, my C skills are lacking as I live mostly in the world of managed
code, but I'd be very happy to cheer for a change-id champion; I suspect
there are some out there who might rally to this cause.

Thanks for your consideration.
James Moger
gitblit.com

[1]
https://git.eclipse.org/c/jgit/jgit.git/tree/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java?h=stable-3.1#n288
[2]
http://gerrit-documentation.googlecode.com/svn/Documentation/2.0/cmd-hook-commit-msg.html

Re: RFE: support change-id generation natively

[Jeremy Rosen]

for those of us that are not using gerrit...

what is a change-id (semantically, I got from your mail that it is some sort
of unit id set at commit time) and in what way is it different from the 
commit-id ?

Cordialement 

Jérémy Rosen 
+33 (0)1 42 68 28 04

fight key loggers : write some perl using vim 


Open Wide Ingenierie

23, rue Daviel
75012 Paris - France
www.openwide.fr





----- Mail original -----
> Hello Git Community,
> 
> TL;DR:
> It would be a really nice enhancement if the commit command natively
> supported _optionally_ injecting a "Change-Id: I000..." footer in the
> last paragraph of the commit message template and then substituting
> the
> "I000..." value, on commit, with a generated value _without_ having
> to
> rely on a per-repository, native hook or a global hook that affects
> every local repository.
> 
> Full Request:
> Gerrit has established the change-id footer as a prominent and
> wide-spread collaboration identifier.  For those contributing new
> patches to a Gerrit server, it is required to either use EGit/JGit
> (Eclipse) to generate commits [1] OR to use a commit hook script with
> native git to insert a change-id footer during the commit process
> [2].
> This per-repository hook script requirement is an obstacle.  These
> communities would be better served and it would lower the
> contribution
> barrier for many open source projects if native git supported
> change-id
> generation & injection.
> 
> I acknowledge that not everyone uses nor wants to use Gerrit and the
> change-id footer.  That is fine, but it would be a _tremendous_
> usability improvement for those contributing to open source projects
> (myself included) if something like a "--change-id" flag  was
> implemented and maybe even a config setting to always generate a
> change-id on commit (EGit currently supports this as
> "gerrit.createchangeid=true").
> 
> Sadly, my C skills are lacking as I live mostly in the world of
> managed
> code, but I'd be very happy to cheer for a change-id champion; I
> suspect
> there are some out there who might rally to this cause.
> 
> Thanks for your consideration.
> James Moger
> gitblit.com
> 
> [1]
> https://git.eclipse.org/c/jgit/jgit.git/tree/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java?h=stable-3.1#n288
> [2]
> http://gerrit-documentation.googlecode.com/svn/Documentation/2.0/cmd-hook-commit-msg.html
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: RFE: support change-id generation natively

[james.moger]

The change-id is exactly like a commit-id, it is an SHA-1 value, but it
is a constant embedded in the commit message.

Why does Gerrit need this value?
Gerrit is based on the concept of revising/polishing a commit or a
series of commits.

For clarity, consider the case of revising a proposed bug fix.

You checkout the current revision of a proposed bug fix commit that has
a change-id value in it's message.  You revise and _amend_ this commit,
preserving the change-id in the commit message.  Now your commit-id has
changed, but your change-id is still the same.  You then upload your
amended commit to Gerrit which links the amended commit with the
discussion/review.

Commit-ids change all the time because of amend; change-ids are constant
and they are the key that links commit revisions to a discussion.

What I am requesting is a feature that generates and injects the
Change-Id value for the very first commit revision.  This commit is
special because it will create the discussion in Gerrit for the commit. 
Gerrit relies on client-side change-id generation for this initial
commit.  This allows contributors to propose new ideas by implementing
that idea and pushing the proposed implementation to Gerrit.  Gerrit
intercepts this and automatically creates a discussion/review keyed by
the specified Change-Id value.  And now through the --amend process,
this commit can be revised and polished until it is blessed by a
reviewer for merging to some integration branch.

-J



On Mon, Oct 21, 2013, at 10:51 AM, Jeremy Rosen wrote:
> for those of us that are not using gerrit...
> 
> what is a change-id (semantically, I got from your mail that it is some
> sort
> of unit id set at commit time) and in what way is it different from the 
> commit-id ?
> 
> Cordialement 
> 
> Jérémy Rosen 
> +33 (0)1 42 68 28 04
> 
> fight key loggers : write some perl using vim 
> 
> 
> Open Wide Ingenierie
> 
> 23, rue Daviel
> 75012 Paris - France
> www.openwide.fr
> 
> 
> 
> 
> 
> ----- Mail original -----
> > Hello Git Community,
> > 
> > TL;DR:
> > It would be a really nice enhancement if the commit command natively
> > supported _optionally_ injecting a "Change-Id: I000..." footer in the
> > last paragraph of the commit message template and then substituting
> > the
> > "I000..." value, on commit, with a generated value _without_ having
> > to
> > rely on a per-repository, native hook or a global hook that affects
> > every local repository.
> > 
> > Full Request:
> > Gerrit has established the change-id footer as a prominent and
> > wide-spread collaboration identifier.  For those contributing new
> > patches to a Gerrit server, it is required to either use EGit/JGit
> > (Eclipse) to generate commits [1] OR to use a commit hook script with
> > native git to insert a change-id footer during the commit process
> > [2].
> > This per-repository hook script requirement is an obstacle.  These
> > communities would be better served and it would lower the
> > contribution
> > barrier for many open source projects if native git supported
> > change-id
> > generation & injection.
> > 
> > I acknowledge that not everyone uses nor wants to use Gerrit and the
> > change-id footer.  That is fine, but it would be a _tremendous_
> > usability improvement for those contributing to open source projects
> > (myself included) if something like a "--change-id" flag  was
> > implemented and maybe even a config setting to always generate a
> > change-id on commit (EGit currently supports this as
> > "gerrit.createchangeid=true").
> > 
> > Sadly, my C skills are lacking as I live mostly in the world of
> > managed
> > code, but I'd be very happy to cheer for a change-id champion; I
> > suspect
> > there are some out there who might rally to this cause.
> > 
> > Thanks for your consideration.
> > James Moger
> > gitblit.com
> > 
> > [1]
> > https://git.eclipse.org/c/jgit/jgit.git/tree/org.eclipse.jgit/src/org/eclipse/jgit/api/CommitCommand.java?h=stable-3.1#n288
> > [2]
> > http://gerrit-documentation.googlecode.com/svn/Documentation/2.0/cmd-hook-commit-msg.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe git" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

Re: RFE: support change-id generation natively

[Shawn Pearce]

On Mon, Oct 21, 2013 at 8:41 AM,  <james.moger@gitblit.com> wrote:
> The change-id is exactly like a commit-id, it is an SHA-1 value, but it
> is a constant embedded in the commit message.

https://gerrit-review.googlesource.com/Documentation/user-changeid.html
goes into more detail about these.

> Commit-ids change all the time because of amend; change-ids are constant
> and they are the key that links commit revisions to a discussion.

In a mailing list based workflow, when an author revises a patch
series and resends the new patches aren't linked to the old patches in
a MUA, because the Message-Ids of the original versions were not
preserved. Imagine if Git saved that original Message-Id somewhere and
could properly write In-Reply-To headers so that attempt #2 for each
patch replies to the end of the thread discussing attempt #1 of the
same patch. In a 30 patch series. Gerrit does this with Change-Id.


We briefly considered putting the Change-Id into the commit headers
(e.g. below the optional encoding) but could not because `git commit`
doesn't support this. So it went into the footer along with
Signed-off-by provenance data, which is also not expressible in
headers.

Re: RFE: support change-id generation natively

[Ondřej Bílka]

On Mon, Oct 21, 2013 at 09:35:07AM -0700, Shawn Pearce wrote:
> On Mon, Oct 21, 2013 at 8:41 AM,  <james.moger@gitblit.com> wrote:
> > The change-id is exactly like a commit-id, it is an SHA-1 value, but it
> > is a constant embedded in the commit message.
> 
> https://gerrit-review.googlesource.com/Documentation/user-changeid.html
> goes into more detail about these.
> 
> > Commit-ids change all the time because of amend; change-ids are constant
> > and they are the key that links commit revisions to a discussion.
> 
> In a mailing list based workflow, when an author revises a patch
> series and resends the new patches aren't linked to the old patches in
> a MUA, because the Message-Ids of the original versions were not
> preserved. Imagine if Git saved that original Message-Id somewhere and
> could properly write In-Reply-To headers so that attempt #2 for each
> patch replies to the end of the thread discussing attempt #1 of the
> same patch. In a 30 patch series. Gerrit does this with Change-Id.
> 
> 
> We briefly considered putting the Change-Id into the commit headers
> (e.g. below the optional encoding) but could not because `git commit`
> doesn't support this. So it went into the footer along with
> Signed-off-by provenance data, which is also not expressible in
> headers.

What about adding that as Note?

Re: RFE: support change-id generation natively

[Thomas Koch]

On Monday, October 21, 2013 05:41:59 PM james.moger@gitblit.com wrote:
> The change-id is exactly like a commit-id, it is an SHA-1 value, but it
> is a constant embedded in the commit message.

As I understand, a UUID could also be used for the same purbose as the change-
id. How is the change-id generated by the way? Would it be a good english name 
to call it enduring commit identifier?

I had an usage example for such identifiers last week while helping to rebase 
and merge a few too long standing feature branches. After a while we noticed, 
that a few commits where already cherry-picked in a slightly different form and 
with different commit message to the integration branch. If those commit had 
enduring identifiers, it would have been easier to spot this.

Git already has functionality to identify commits that have already been 
cherry-picked or merged in a branch and thus skips them in a rebase. Could 
this functionality benefit from an enduring commit identifier?

Best regards, Thomas Koch

Re: RFE: support change-id generation natively

[james.moger]

On Mon, Oct 21, 2013, at 02:29 PM, Thomas Koch wrote:
> As I understand, a UUID could also be used for the same purbose as the
> change-
> id. How is the change-id generated by the way? Would it be a good english
> name 
> to call it enduring commit identifier?

Here is the algorithm:
https://git.eclipse.org/c/jgit/jgit.git/tree/org.eclipse.jgit/src/org/eclipse/jgit/util/ChangeIdUtil.java#n78

I think "enduring commit id" is a fair interpretation of it's purpose. 
I don't speak for the Gerrit developers so I can not say if they are
interested in alternative id generation.  I come to the list as a
change-id user/consumer.

-J

Re: RFE: support change-id generation natively

[Martin Fick]

On Monday, October 21, 2013 12:40:58 pm 
james.moger@gitblit.com wrote:
> On Mon, Oct 21, 2013, at 02:29 PM, Thomas Koch wrote:
> > As I understand, a UUID could also be used for the same
> > purbose as the change-
> > id. How is the change-id generated by the way? Would it
> > be a good english name
> > to call it enduring commit identifier?
> 
> Here is the algorithm:
> https://git.eclipse.org/c/jgit/jgit.git/tree/org.eclipse.
> jgit/src/org/eclipse/jgit/util/ChangeIdUtil.java#n78
> 
> I think "enduring commit id" is a fair interpretation of
> it's purpose. I don't speak for the Gerrit developers so
> I can not say if they are interested in alternative id
> generation.  I come to the list as a change-id
> user/consumer.

As a Gerrit maintainer, I would suspect that we would 
welcome a way to track "changes" natively in git.  Despite 
any compatibility issues with the current Gerrit 
implementation, I suspect we would be open to new forms if 
the git community has a better proposal than the current 
Change-Id.  Especially if it does reduce the significant 
user pain point of installing a hook!


-Martin


-- 
The Qualcomm Innovation Center, Inc. is a member of Code 
Aurora Forum, hosted by The Linux Foundation
 

Re: RFE: support change-id generation natively

[Shawn Pearce]

On Mon, Oct 21, 2013 at 9:38 AM, Ondřej Bílka <neleai@seznam.cz> wrote:
> On Mon, Oct 21, 2013 at 09:35:07AM -0700, Shawn Pearce wrote:
>> On Mon, Oct 21, 2013 at 8:41 AM,  <james.moger@gitblit.com> wrote:
>> > The change-id is exactly like a commit-id, it is an SHA-1 value, but it
>> > is a constant embedded in the commit message.
>>
>> https://gerrit-review.googlesource.com/Documentation/user-changeid.html
>> goes into more detail about these.
>>
>> > Commit-ids change all the time because of amend; change-ids are constant
>> > and they are the key that links commit revisions to a discussion.
>>
>> In a mailing list based workflow, when an author revises a patch
>> series and resends the new patches aren't linked to the old patches in
>> a MUA, because the Message-Ids of the original versions were not
>> preserved. Imagine if Git saved that original Message-Id somewhere and
>> could properly write In-Reply-To headers so that attempt #2 for each
>> patch replies to the end of the thread discussing attempt #1 of the
>> same patch. In a 30 patch series. Gerrit does this with Change-Id.
>>
>>
>> We briefly considered putting the Change-Id into the commit headers
>> (e.g. below the optional encoding) but could not because `git commit`
>> doesn't support this. So it went into the footer along with
>> Signed-off-by provenance data, which is also not expressible in
>> headers.
>
> What about adding that as Note?

If it was a note, the note would need to be updated every time the
user updated their commit locally. So `git commit --amend` and `git
rebase` (all forms) would be required to update the note with the new
commit SHA-1 so the value isn't lost.

If it was a note, the author would also have to push their notes
branch to the Gerrit server when they push their commits. This is
likely to be forgotten, since its a different branch than the branch
the user is working on. The server only needs notes for the new
incoming commits, but the notes branch will probably bring all
activity the author has been doing. So maybe the author should have
one notes branch per topic branch. And clean up the notes branches
after they delete their local topic branches. Etc.

notes are great, but they get messy. And back when Gerrit introduced
support for Change-Id (more than 4 years ago) I don't think note
support even existed. Or if it did, it was no where near as complete
as it is today.

Re: RFE: support change-id generation natively

[Shawn Pearce]

On Mon, Oct 21, 2013 at 11:40 AM,  <james.moger@gitblit.com> wrote:
>
> On Mon, Oct 21, 2013, at 02:29 PM, Thomas Koch wrote:
>> As I understand, a UUID could also be used for the same purbose as the
>> change-
>> id. How is the change-id generated by the way? Would it be a good english
>> name
>> to call it enduring commit identifier?
>
> Here is the algorithm:
> https://git.eclipse.org/c/jgit/jgit.git/tree/org.eclipse.jgit/src/org/eclipse/jgit/util/ChangeIdUtil.java#n78

For the hyperlink and Java challenged, the Change-Id is essentially
the commit SHA-1 had the Change-Id not been included. The shell script
hook Gerrit recommends for use with `git commit` uses `git
commit-tree` to compute the hash. Which of course later differs after
the Change-Id is inserted.

> I think "enduring commit id" is a fair interpretation of it's purpose.
> I don't speak for the Gerrit developers so I can not say if they are
> interested in alternative id generation.  I come to the list as a
> change-id user/consumer.

As Martin Fick said, we would be open to an alternative if a better
one is presented, especially if it is supported by git commit.

Re: RFE: support change-id generation natively

[Junio C Hamano]

Martin Fick <mfick@codeaurora.org> writes:

> As a Gerrit maintainer, I would suspect that we would 
> welcome a way to track "changes" natively in git.

I would suspect that we would not mind "git commit --change-id" (and
probably "git commit-tree --change-id") option that can be used to
tell the command to add a new Change-Id: trailer at the end, if and
only if there is none in the log message to be recorded (this needs
to happen after the user possibly edits).  We may even want to
introduce commit.changeId boolean configuration variable if we did
so.

"git commit --amend", "git rebase", etc. can be left oblivious to
the "Change-Id:" trailer, as the default mode of operation you guys
want is to leave the existing one as-is, unless the end user really
wants to change it, I think.

It would be just the matter of updating commit_tree_extended() in
commit.c to:

 - detect the need to add a new Change-Id: trailer;

 - call hash_sha1_file() on the commit object buffer (assuming that
   a commit object that you can actually "git cat-file commit" using
   the change Id does not have to exist anywhere for Gerrit to
   work---otherwise you would need to call write_sha1_file()
   instead) before adding Change-Id: trailer;

 - add Change-Id: trailer to the buffer; and then finally

 - let the existing write_sha1_file() to write it out.

I would think.  You might have a funny chicken-and-egg problem with
the signed commit, though.  I didn't think that part through.

RE: RFE: support change-id generation natively

[Pyeron, Jason J CTR (US)]

[-- Attachment #1: Type: text/plain, Size: 646 bytes --]

> -----Original Message-----
> From: Junio C Hamano
> Sent: Tuesday, October 22, 2013 3:51 PM
> 


<snip/>

> I would think.  You might have a funny chicken-and-egg problem with
> the signed commit, though.  I didn't think that part through.

Respectfully, I do not think there is a chicken and egg situation here. Either the user has included a generated id field and value in the portion covered by the signature, or the mutation of the portion covered by the signature has been modified, hence has an invalid signature.

Any user signing their commit, should ensure it is the last operation, or be prepared to resign it later.

Jason Pyeron 


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5615 bytes --]

Re: RFE: support change-id generation natively

[Junio C Hamano]

"Pyeron, Jason J CTR (US)" <jason.j.pyeron.ctr@mail.mil> writes:

>> -----Original Message-----
>> From: Junio C Hamano
>> Sent: Tuesday, October 22, 2013 3:51 PM
>> 
>
>
> <snip/>
>
>> I would think.  You might have a funny chicken-and-egg problem with
>> the signed commit, though.  I didn't think that part through.
>
> Respectfully, I do not think there is a chicken and egg situation
> here. Either the user has included a generated id field and value
> in the portion covered by the signature, or the mutation of the
> portion covered by the signature has been modified, hence has an
> invalid signature.
>
> Any user signing their commit, should ensure it is the last
> operation, or be prepared to resign it later.

Thanks, I think I got what you are saying.

I was coming from the existing code, assuming that you have a single
commit without Change Id but has already called do_sign_commit().
That is what the users today will get out of "commit -S".  But using
the object name of such a commit as the Change Id, and then creating
a new commit by appending a new Change Id trailer will not work, as
that will break the existing signature.

But you can begin from a single commit without Change Id and without
signature---its object name would be the Change Id.  You can add a
new Change Id trailer to record that and sign it while creating a
commit.  It conceptually may be a three-step process, but still can be
done inside a single invocation of "git commit --change-id -S".

So a rough outline of the patch to implement it may look like below.
The parsing and passing down of the "--change-id" option is left as
an exercise to interested readers.  A real patch may have to add an
extra blank line before the strbuf_addf() if buffer.buf does not end
with a trailer to separate the "Change Id" line from the end of the
existing message body.

 commit.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index de16a3c..664ef5d 100644
--- a/commit.c
+++ b/commit.c
@@ -1481,17 +1481,22 @@ static const char commit_utf8_warn[] =
 int commit_tree_extended(const struct strbuf *msg, unsigned char *tree,
 			 struct commit_list *parents, unsigned char *ret,
 			 const char *author, const char *sign_commit,
-			 struct commit_extra_header *extra)
+			 struct commit_extra_header *extra,
+			 unsigned int flags)
 {
 	int result;
 	int encoding_is_utf8;
 	struct strbuf buffer;
+	int add_change_id = !!(flags & COMMIT_ADD_CHANGE_ID);
 
 	assert_sha1_type(tree, OBJ_TREE);
 
 	if (memchr(msg->buf, '\0', msg->len))
 		return error("a NUL byte in commit log message not allowed.");
 
+	if (add_change_id && strstr(msg->buf, "\nChange-Id: "))
+		add_change_id = 0; /* already has one */
+
 	/* Not having i18n.commitencoding is the same as having utf-8 */
 	encoding_is_utf8 = is_encoding_utf8(git_commit_encoding);
 
@@ -1534,6 +1539,13 @@ int commit_tree_extended(const struct strbuf *msg, unsigned char *tree,
 	if (encoding_is_utf8 && !verify_utf8(&buffer))
 		fprintf(stderr, commit_utf8_warn);
 
+	if (add_change_id) {
+		unsigned char change_id[20];
+		if (hash_sha1_file(buffer.buf, buffer.len, commit_type, change_id))
+			return -1;
+		strbuf_addf(&buffer, "Change-Id: %s\n", sha1_to_hex(change_id));
+	}
+
 	if (sign_commit && do_sign_commit(&buffer, sign_commit))
 		return -1;
 

Re: RFE: support change-id generation natively

[Duy Nguyen]

On Wed, Oct 23, 2013 at 2:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> It would be just the matter of updating commit_tree_extended() in
> commit.c to:
>
>  - detect the need to add a new Change-Id: trailer;
>
>  - call hash_sha1_file() on the commit object buffer (assuming that
>    a commit object that you can actually "git cat-file commit" using
>    the change Id does not have to exist anywhere for Gerrit to
>    work---otherwise you would need to call write_sha1_file()
>    instead) before adding Change-Id: trailer;
>
>  - add Change-Id: trailer to the buffer; and then finally
>
>  - let the existing write_sha1_file() to write it out.

I'm not objecting special support for Gerrit, but if the change is
just commit_tree_extended() why don't we just ship the commit hook in
a new "Gerrit" template? It's just a matter of "git init
--template=gerrit" (or something) to create a new repo, or reinit the
current repo and you're set. Of it per-repo is bad, perhaps we could
introduce global hooks (if we haven't had them yet)?
-- 
Duy

Re: RFE: support change-id generation natively

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Oct 23, 2013 at 2:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> It would be just the matter of updating commit_tree_extended() in
>> commit.c to:
>>
>>  - detect the need to add a new Change-Id: trailer;
>>
>>  - call hash_sha1_file() on the commit object buffer (assuming that
>>    a commit object that you can actually "git cat-file commit" using
>>    the change Id does not have to exist anywhere for Gerrit to
>>    work---otherwise you would need to call write_sha1_file()
>>    instead) before adding Change-Id: trailer;
>>
>>  - add Change-Id: trailer to the buffer; and then finally
>>
>>  - let the existing write_sha1_file() to write it out.
>
> I'm not objecting special support for Gerrit, but if the change is
> just commit_tree_extended() why don't we just ship the commit hook in
> a new "Gerrit" template?

It is not clear to me how you envision to make it work.

Naïvely thinking, an obvious place to do this kind of thing may be
the "commit-msg" hook, where the hook reads what the user prepared,
finds that there is no existing "Change-Id:" trailer, and decides to
add one.

But what value would it add on that line as the Id?

It wants to use the name of the commit object that would result if
it were to return without further editing the given message, but we
do not give such a commit object name to the hook, so the hook needs
to duplicate the logic to come up with one.  It may be doable (after
all, builtin/commit.c is open source), but we do not give the hook
the commit object header (i.e. it does not know what the tree,
parent(s), author, committer lines would say, nor it does not know
if we are going to add an encoding line), so the hook needs to guess
what we will put there, too.

Re: RFE: support change-id generation natively

[Duy Nguyen]

On Wed, Oct 23, 2013 at 11:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Wed, Oct 23, 2013 at 2:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> It would be just the matter of updating commit_tree_extended() in
>>> commit.c to:
>>>
>>>  - detect the need to add a new Change-Id: trailer;
>>>
>>>  - call hash_sha1_file() on the commit object buffer (assuming that
>>>    a commit object that you can actually "git cat-file commit" using
>>>    the change Id does not have to exist anywhere for Gerrit to
>>>    work---otherwise you would need to call write_sha1_file()
>>>    instead) before adding Change-Id: trailer;
>>>
>>>  - add Change-Id: trailer to the buffer; and then finally
>>>
>>>  - let the existing write_sha1_file() to write it out.
>>
>> I'm not objecting special support for Gerrit, but if the change is
>> just commit_tree_extended() why don't we just ship the commit hook in
>> a new "Gerrit" template?
>
> It is not clear to me how you envision to make it work.

I don't have the source code. But the commit-msg hook document [1]
describes roughly what you wrote below, except the tree part. And I
suppose the hook has been working fine so far. Reading back the
original post, James ruled out always-active hooks in general and
wanted the control per command line. Perhaps we should add
--no-hooks[=<name>,<name>] to "git commit"? Or maybe it's still
inconvenient and --change-id is best.

[1] http://gerrit-documentation.googlecode.com/svn/Documentation/2.0/cmd-hook-commit-msg.html

> Naïvely thinking, an obvious place to do this kind of thing may be
> the "commit-msg" hook, where the hook reads what the user prepared,
> finds that there is no existing "Change-Id:" trailer, and decides to
> add one.
>
> But what value would it add on that line as the Id?
>
> It wants to use the name of the commit object that would result if
> it were to return without further editing the given message, but we
> do not give such a commit object name to the hook, so the hook needs
> to duplicate the logic to come up with one.  It may be doable (after
> all, builtin/commit.c is open source), but we do not give the hook
> the commit object header (i.e. it does not know what the tree,
> parent(s), author, committer lines would say, nor it does not know
> if we are going to add an encoding line), so the hook needs to guess
> what we will put there, too.
-- 
Duy

Re: RFE: support change-id generation natively

[Nasser Grainawi]

On Oct 23, 2013, at 8:07 PM, Duy Nguyen wrote:

> On Wed, Oct 23, 2013 at 11:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>> 
>>> On Wed, Oct 23, 2013 at 2:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> It would be just the matter of updating commit_tree_extended() in
>>>> commit.c to:
>>>> 
>>>> - detect the need to add a new Change-Id: trailer;
>>>> 
>>>> - call hash_sha1_file() on the commit object buffer (assuming that
>>>>   a commit object that you can actually "git cat-file commit" using
>>>>   the change Id does not have to exist anywhere for Gerrit to
>>>>   work---otherwise you would need to call write_sha1_file()
>>>>   instead) before adding Change-Id: trailer;
>>>> 
>>>> - add Change-Id: trailer to the buffer; and then finally
>>>> 
>>>> - let the existing write_sha1_file() to write it out.
>>> 
>>> I'm not objecting special support for Gerrit, but if the change is
>>> just commit_tree_extended() why don't we just ship the commit hook in
>>> a new "Gerrit" template?
>> 
>> It is not clear to me how you envision to make it work.
> 
> I don't have the source code.

Now you do: https://gerrit.googlesource.com/gerrit/+/master/gerrit-server/src/main/resources/com/google/gerrit/server/tools/root/hooks/commit-msg

> But the commit-msg hook document [1]
> describes roughly what you wrote below, except the tree part. And I
> suppose the hook has been working fine so far. Reading back the
> original post, James ruled out always-active hooks in general and
> wanted the control per command line. Perhaps we should add
> --no-hooks[=<name>,<name>] to "git commit"? Or maybe it's still
> inconvenient and --change-id is best.
> 
> [1] http://gerrit-documentation.googlecode.com/svn/Documentation/2.0/cmd-hook-commit-msg.html
> 
>> Naïvely thinking, an obvious place to do this kind of thing may be
>> the "commit-msg" hook, where the hook reads what the user prepared,
>> finds that there is no existing "Change-Id:" trailer, and decides to
>> add one.
>> 
>> But what value would it add on that line as the Id?
>> 
>> It wants to use the name of the commit object that would result if
>> it were to return without further editing the given message, but we
>> do not give such a commit object name to the hook, so the hook needs
>> to duplicate the logic to come up with one.  It may be doable (after
>> all, builtin/commit.c is open source), but we do not give the hook
>> the commit object header (i.e. it does not know what the tree,
>> parent(s), author, committer lines would say, nor it does not know
>> if we are going to add an encoding line), so the hook needs to guess
>> what we will put there, too.
> -- 
> Duy
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: RFE: support change-id generation natively

[Duy Nguyen]

On Thu, Oct 24, 2013 at 11:11 AM, Nasser Grainawi <nasser@codeaurora.org> wrote:
>>> It is not clear to me how you envision to make it work.
>>
>> I don't have the source code.
>
> Now you do: https://gerrit.googlesource.com/gerrit/+/master/gerrit-server/src/main/resources/com/google/gerrit/server/tools/root/hooks/commit-msg

Thanks. So you do have tree sha-1 by running "git write-tree". But at
that point I'm not sure if cache-tree is written down to disk yet, so
write-tree could be more expensive than necessary (one good point for
building --change-id in).
-- 
Duy

Re: RFE: support change-id generation natively

[Johannes Sixt]

Am 10/24/2013 7:25, schrieb Duy Nguyen:
> On Thu, Oct 24, 2013 at 11:11 AM, Nasser Grainawi <nasser@codeaurora.org> wrote:
>>>> It is not clear to me how you envision to make it work.
>>>
>>> I don't have the source code.
>>
>> Now you do: https://gerrit.googlesource.com/gerrit/+/master/gerrit-server/src/main/resources/com/google/gerrit/server/tools/root/hooks/commit-msg
> 
> Thanks. So you do have tree sha-1 by running "git write-tree". But at
> that point I'm not sure if cache-tree is written down to disk yet, so
> write-tree could be more expensive than necessary (one good point for
> building --change-id in).

Consider that I make a commit with a change-id. Then I rewrite the commit,
but keep the change-id. Then I push the rewritten commit to Gerrit. Gerrit
does not have the objects that the change-id is based on; the change-id is
just a random number and has no other significance. Right?

Why do you go all the length in computing a change-id instead of just
pulling 20 bytes from /dev/random?

That said, I don't think that --change-id option that the user must not
forget to use is any better than a hook that the user must not forget to
install.

-- Hannes

Re: RFE: support change-id generation natively

[james.moger]

> That said, I don't think that --change-id option that the user must not
> forget to use is any better than a hook that the user must not forget to
> install.

Having a --change-id option, to my mind, simplifies use of the patch
workflow as it does not require downloading, copying and setting
executable a hook script per-repository or globally.  I agree that
forgetting to add it on the command-line is a potential problem.  This
could be improved by honoring the "gerrit.createchangeid" value (or
whatever setting name is appropriate).  Of course that still requires
configuring the repo after clone, but it's clean and straight-forward
since it is all plain "git config".

-J

Re: RFE: support change-id generation natively

[Thomas Koch]

On Thursday, October 24, 2013 02:11:05 PM james.moger@gitblit.com wrote:
> > That said, I don't think that --change-id option that the user must not
> > forget to use is any better than a hook that the user must not forget to
> > install.

I'm a bit paranoid. (e.g. I do all my development in a virtual machine and my 
host machine only runs binaries from debian stable.)

A command line option is a big improvement over having to download a random 
script from some potentially untrusted place and executing it probably even 
with the same user that also has access to my GPG key that signs my code and 
my SSH key that has access to the repository.

Regards, Thomas Koch

Re: RFE: support change-id generation natively

[Duy Nguyen]

On Thu, Oct 24, 2013 at 7:11 PM,  <james.moger@gitblit.com> wrote:
>> That said, I don't think that --change-id option that the user must not
>> forget to use is any better than a hook that the user must not forget to
>> install.
>
> Having a --change-id option, to my mind, simplifies use of the patch
> workflow as it does not require downloading, copying and setting
> executable a hook script per-repository or globally.

This could be solved by shipping the hook with git. So all you need to
do is "git init --template=gerrit". --template requires full path, but
I think we can change it to accept a name and look for $datadir/$name.
A more interesting case is removing the hook. I admit I haven't found
any neat way to do it without messing in $GIT_DIR/hooks manually.

> I agree that
> forgetting to add it on the command-line is a potential problem.  This
> could be improved by honoring the "gerrit.createchangeid" value (or
> whatever setting name is appropriate).  Of course that still requires
> configuring the repo after clone, but it's clean and straight-forward
> since it is all plain "git config".
>
> -J



-- 
Duy

Re: RFE: support change-id generation natively

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 10/24/2013 7:25, schrieb Duy Nguyen:
>> On Thu, Oct 24, 2013 at 11:11 AM, Nasser Grainawi <nasser@codeaurora.org> wrote:
>>>>> It is not clear to me how you envision to make it work.
>>>>
>>>> I don't have the source code.
>>>
>>> Now you do: https://gerrit.googlesource.com/gerrit/+/master/gerrit-server/src/main/resources/com/google/gerrit/server/tools/root/hooks/commit-msg
>> 
>> Thanks. So you do have tree sha-1 by running "git write-tree". But at
>> that point I'm not sure if cache-tree is written down to disk yet, so
>> write-tree could be more expensive than necessary (one good point for
>> building --change-id in).
>
> Consider that I make a commit with a change-id. Then I rewrite the commit,
> but keep the change-id. Then I push the rewritten commit to Gerrit. Gerrit
> does not have the objects that the change-id is based on; the change-id is
> just a random number and has no other significance. Right?
>
> Why do you go all the length in computing a change-id instead of just
> pulling 20 bytes from /dev/random?

Very good point.

The quoted script does not necessarily give the right commit object
name at least under three scenarios:

 - when we would need to add encoding header, etc.;

 - when we are recording merges (perhaps merges will not get rebased
   in Gerrit workflow and it does not matter what random garbage
   this script added to them).

 - when we record the commit after 1-sec boundary since _gen_ChangeIdInput
   in the script was called.

I wouldn't call the script "buggy", but I tend to agree with you
that it is an unnecessarily more complex way to spell "grab 20
random bytes" ;-)

> That said, I don't think that --change-id option that the user must not
> forget to use is any better than a hook that the user must not forget to
> install.

That is why I said this in my first response to this thread:

>> ...  We may even want to
>> introduce commit.changeId boolean configuration variable if we did
>> so.

Re: RFE: support change-id generation natively

[Johannes Sixt]

Am 10/24/2013 22:04, schrieb Junio C Hamano:
> Johannes Sixt <j.sixt@viscovery.net> writes:
>> That said, I don't think that --change-id option that the user must not
>> forget to use is any better than a hook that the user must not forget to
>> install.
> 
> That is why I said this in my first response to this thread:
> 
>>> ...  We may even want to
>>> introduce commit.changeId boolean configuration variable if we did
>>> so.

That's only slightly different and still "must not forget to set".

But I am more concerned that a non-volatile change-id is totally outside
the Git data model. After we have git commit --change-id, what will be the
next requests for enhancement? 'git merge' and 'git cherry-pick' take a
change-id? Where will it end?

We could ship a git-gerrit-commit wrapper script in contrib that adds the
change-id and that people can alias their 'git ci' to globally or on a
per-repo basis.

-- Hannes