[PATCH] commit: provide option to add Fixes tags to log

[PATCH] commit: provide option to add Fixes tags to log

[Edwin Peer]

The Linux style Fixes tag has been adopted by many projects and represents
best practice for referring to previous commits which introduce a bug that
has been fixed by the present commit. Creating these tags manually can be
error prone and doing so using git log -1 --format='Fixes: %h ("%s")' is
cumbersome. It's time the commit command learn to perform this popular
pattern natively.

Implement a convenient command line option to add Fixes tags at the end of
the log message during commmit. The tag refers to the commit hash and
subject line of the given commit reference. This option may appear
multiple times on the command line to reference more than one commit. A
new tag will only be added if it does not already exist in the log message
trailer and will otherwise be added in option order to the beginning of
the trailer section. The minimum number of characters comprising the hash
portion of the tag is 12 by default, but the `core.abbrev` configuration
variable will be honored if it is specified.

Signed-off-by: Edwin Peer <espeer@gmail.com>
---
 Documentation/git-commit.txt |  14 +++-
 builtin/commit.c             |  54 ++++++++++++++
 t/t7527-commit-fixes.sh      | 135 +++++++++++++++++++++++++++++++++++
 3 files changed, 202 insertions(+), 1 deletion(-)
 create mode 100755 t/t7527-commit-fixes.sh

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index a3baea32ae..4acb4b3ac8 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
 	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
 	   [-i | -o] [--pathspec-from-file=<file> [--pathspec-file-nul]]
-	   [-S[<keyid>]] [--] [<pathspec>...]
+	   [-S[<keyid>]] [(-f | --fixes) <commit>] [--] [<pathspec>...]
 
 DESCRIPTION
 -----------
@@ -172,6 +172,18 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.
 	agrees to a Developer Certificate of Origin
 	(see http://developercertificate.org/ for more information).
 
+-f::
+--fixes::
+	Add Linux style Fixes tag at the end of the commit log message. The
+	tag refers to the commit hash and subject line of the given commit
+	reference. This option may appear multiple times on the command
+	line to reference more than one commit. A new tag will only be
+	added if it does not already exist in the log message trailer and
+	will otherwise be added in option order to the beginning of the
+	trailer section. The minimum number of characters comprising the
+	hash portion of the tag is 12 by default, but the `core.abbrev`
+	configuration variable will be honored if it is specified.
+
 -n::
 --no-verify::
 	This option bypasses the pre-commit and commit-msg hooks.
diff --git a/builtin/commit.c b/builtin/commit.c
index 69ac78d5e5..231abe92ff 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -34,6 +34,7 @@
 #include "sequencer.h"
 #include "mailmap.h"
 #include "help.h"
+#include "trailer.h"
 #include "commit-reach.h"
 #include "commit-graph.h"
 
@@ -113,6 +114,7 @@ static int config_commit_verbose = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message, pathspec_file_nul;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
 static char *sign_commit, *pathspec_from_file;
+static struct string_list fixes = STRING_LIST_INIT_NODUP;
 
 /*
  * The default commit message cleanup mode will remove the lines
@@ -651,6 +653,54 @@ static int author_date_is_interesting(void)
 	return author_message || force_date;
 }
 
+static void tag_fixes(struct strbuf *msg)
+{
+	const struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+	struct pretty_print_context pretty = { .abbrev = default_abbrev };
+	const char *format = "Fixes: %h (\"%s\")";
+	const struct string_list_item *fix;
+	struct strbuf tags = STRBUF_INIT;
+	struct trailer_info info;
+	size_t insert_pos;
+
+	/* use at least 12 hash characters unless explicitly configured */
+	if (pretty.abbrev == -1)
+		pretty.abbrev = 12;
+
+	trailer_info_get(&info, msg->buf, &opts);
+
+	for_each_string_list_item(fix, &fixes) {
+		struct strbuf tag = STRBUF_INIT;
+		struct commit *commit;
+		int i;
+
+		commit = lookup_commit_reference_by_name(fix->string);
+		if (!commit)
+			die(_("could not find commit: %s"), fix->string);
+
+		format_commit_message(commit, format, &tag, &pretty);
+
+		/* skip fixes tags that are already present */
+		for (i = 0; i < info.trailer_nr; i++)
+			if (!strncmp(info.trailers[i], tag.buf, tag.len))
+				goto skip;
+
+		strbuf_add(&tags, tag.buf, tag.len);
+		strbuf_addch(&tags, '\n');
+skip:
+		strbuf_release(&tag);
+	}
+
+	insert_pos = info.trailer_start - msg->buf;
+	strbuf_insert(msg, insert_pos, tags.buf, tags.len);
+	if (tags.len != 0 && !info.blank_line_before_trailer) {
+		if (msg->len == tags.len)
+			strbuf_insert(msg, insert_pos, "\n", 1);
+		strbuf_insert(msg, insert_pos, "\n", 1);
+	}
+	strbuf_release(&tags);
+}
+
 static void adjust_comment_line_char(const struct strbuf *sb)
 {
 	char candidates[] = "#;@!$%^&|:";
@@ -829,6 +879,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (signoff)
 		append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);
 
+	if (fixes.nr)
+		tag_fixes(&sb);
+
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
 		die_errno(_("could not write commit template"));
 
@@ -1510,6 +1563,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
 		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
 		OPT_BOOL('s', "signoff", &signoff, N_("add Signed-off-by:")),
+		OPT_STRING_LIST('f', "fixes", &fixes, N_("commit"), N_("add Fixes: tag referencing <commit>")),
 		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
 		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
 		OPT_CLEANUP(&cleanup_arg),
diff --git a/t/t7527-commit-fixes.sh b/t/t7527-commit-fixes.sh
new file mode 100755
index 0000000000..8067cd1ed5
--- /dev/null
+++ b/t/t7527-commit-fixes.sh
@@ -0,0 +1,135 @@
+#!/bin/sh
+#
+# Copyright (c) 2020 Edwin Peer <espeer@gmail.com>
+#
+
+test_description="git commit -f"
+. ./test-lib.sh
+
+test_expect_success 'empty message' '
+	>foo && git add foo &&
+	git commit -m "first" &&
+	echo 1 >foo &&
+	git commit -af 3b6336 &&
+	git log -1 --format="%s" >actual &&
+	echo "Fixes: 3b6336cd156f (\"first\")" >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'invalid commit' '
+	echo 2>foo &&
+	test_must_fail git commit -af abcdef
+'
+
+test_expect_success 'blank line after message' '
+	git commit --amend -m "second commit with spaces" -f 3b6336 &&
+	git log -1 --format="%B" >actual &&
+	cat >expected <<-EOF &&
+		second commit with spaces
+		
+		Fixes: 3b6336cd156f ("first")
+		
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'no duplicate' '
+	git commit --amend -f 3b6336 &&
+	git log -1 --format="%B" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'at start of trailer' '
+	echo 3>foo &&
+	git commit -asm "third
+
+Reviewed-by: John Doe <john@nobody.com>" &&
+	git commit --amend -f HEAD~ &&
+	git log -1 --format="%B" >actual &&
+	cat >expected <<-EOF &&
+		third
+		
+		Fixes: a102cd7a0d16 ("second commit with spaces")
+		Reviewed-by: John Doe <john@nobody.com>
+		Signed-off-by: C O Mitter <committer@example.com>
+		
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'multiple' '
+	echo 4 >foo &&
+	git commit -asm "fourth" -f 3b6336 -f 6b4393 &&
+	git log -1 --format="%B" >actual &&
+	cat >expected <<-EOF &&
+		fourth
+		
+		Fixes: 3b6336cd156f ("first")
+		Fixes: 6b4393deae7c ("third")
+		Signed-off-by: C O Mitter <committer@example.com>
+
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'multiple without duplicates' '
+	git commit --amend -f 6b4393 -f a102cd -f 3b6336 &&
+	git log -1 --format="%B" >actual &&
+	cat >expected <<-EOF &&
+		fourth
+		
+		Fixes: a102cd7a0d16 ("second commit with spaces")
+		Fixes: 3b6336cd156f ("first")
+		Fixes: 6b4393deae7c ("third")
+		Signed-off-by: C O Mitter <committer@example.com>
+		
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'honors core.abbrev' '
+	git config core.abbrev 8 &&
+	echo 5 >foo &&
+	git commit -asm "fifth" -f b06e7571b56c3fc5ac5151ff1b0651debbdada51 &&
+	git log -1 --format="%B" >actual &&
+	cat >expected <<-EOF &&
+		fifth
+		
+		Fixes: b06e7571 ("fourth")
+		Signed-off-by: C O Mitter <committer@example.com>
+		
+	EOF
+	test_cmp expected actual &&
+	git config --unset core.abbrev
+'
+
+test_expect_success 'option order' '
+	echo 6 >foo &&
+	git commit -asm "sixth" -f 3b6336cd -f a102cd7a -f 6b4393de &&
+	git log -1 --format="%B" >actual &&
+	cat >expected <<-EOF &&
+		sixth
+		
+		Fixes: 3b6336cd156f ("first")
+		Fixes: a102cd7a0d16 ("second commit with spaces")
+		Fixes: 6b4393deae7c ("third")
+		Signed-off-by: C O Mitter <committer@example.com>
+		
+	EOF
+	test_cmp expected actual &&
+	echo 7 >foo &&
+	git commit -asm "seventh" -f 6b4393de -f a102cd7a -f 3b6336cd &&
+	git log -1 --format="%B" >actual &&
+	cat >expected <<-EOF &&
+		seventh
+		
+		Fixes: 6b4393deae7c ("third")
+		Fixes: a102cd7a0d16 ("second commit with spaces")
+		Fixes: 3b6336cd156f ("first")
+		Signed-off-by: C O Mitter <committer@example.com>
+		
+	EOF
+	test_cmp expected actual
+'
+
+test_done
-- 
2.28.0

Re: [PATCH] commit: provide option to add Fixes tags to log

[Junio C Hamano]

Edwin Peer <espeer@gmail.com> writes:

> The Linux style Fixes tag has been adopted by many projects and represents
> best practice for referring to previous commits which introduce a bug that
> has been fixed by the present commit. Creating these tags manually can be
> error prone and doing so using git log -1 --format='Fixes: %h ("%s")' is
> cumbersome. It's time the commit command learn to perform this popular
> pattern natively.

Sorry, but not in this form.

It is not a reasonable way forward to add a new "--<trailer>" option
to each and every conceivable "Trailer:" a random person wants to
add to the command line.  The presence of "-s" (signoff) option was
an early "mistake" we made, not something we would want to mimic and
make things worse (besides, "Fixes" is not necessarily used with any
object name---projects can use an identifier used in their bug
trackers).

The "interpret-trailers" (Christian CC'ed) subsystem was an attempt
to create a foundation to consistently treat these lines in the
trailer block and the hope back when it was invented was to
eventually integrate it to these subcommands that want to process
commit log messages (like "rebase", "commit", etc.), but it hasn't
happened yet.  And that may be the approach we would want to take.

For example, imagine that "git commit", "git am", etc. learns to
take a new option whose canonical form is to spell it like so:

	--trailer=<trailer>:<arg>

while (optionally) allowing any unrecognized command line option

	--<trailer>=<arg>

to be internally rewritten into the canonical form, as long as <trailer>
is what the interpret-trailers subsystem recognises as configured.  That
would extend the command line option for "git commit" etc. with new

	--trailer=fixes:foo
	--fixes=foo

(the latter is available only if there is no "--fixes" option used
by the implementation of subcommands for other purposes, but the
former is always available) without having to add any new code. 

That kind of future I'd be happy to see.  Not with individual option
with fixed semantics tailored to a single project's convention.

Thanks.