Re: RFE: support change-id generation natively

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 22, 2013, 20:43 UTC
Message-ID: <xmqqppqxrq8q.fsf@gitster.dls.corp.google.com>
In-Reply-To: <871B6C10EBEFE342A772D1159D1320855772CBAD@umechphj.easf.csd.disa.mil>

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

Show 19 quoted lines

>> -----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;