git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] commit: Add -f, --fixes <commit> option to add Fixes: line

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Oct 27, 2013, 05:42 UTC
Message-ID
<526CA7D4.1070904@alum.mit.edu>
In-Reply-To
<20131027013402.GA7146@leaf>
On 10/27/2013 02:34 AM, Josh Triplett wrote:
Show 20 quoted lines
> Linux Kernel Summit 2013 decided on a commit message convention to
> identify commits containing bugs fixed by a commit: a "Fixes:" line,
> included in the standard commit footer (along with "Signed-off-by:" if
> present), containing an abbreviated commit hash (at least 12 characters
> to keep it valid for a long time) and the subject of the commit (for
> human readers).  This helps people (or automated tools) determine how
> far to backport a commit.
> 
> Add a command line option for git commit to automatically construct the
> "Fixes:" line for a commit.  This avoids the need to manually construct
> that line by copy-pasting the commit hash and subject.
> 
> Also works with --amend to modify an existing commit's message.  To add
> a Fixes line to an earlier commit in a series, use rebase -i and add the
> following line after the existing commit:
> x git commit --amend --no-edit -f $commit_containing_bug
> 
> Generalize append_signoff to support appending arbitrary extra lines to
> a commit in the signoff block; this avoids duplicating the logic to find
> or construct that block.
I have a few comments and questions about the design of this feature:

First of all, let me show my ignorance. How formalized is the use of metadata lines at the end of a commit message? I don't remember seeing documentation about such lines in general (as opposed to documentation about particular types of lines). Is the format defined well enough that tools that don't know about a particular line could nonetheless preserve it correctly? Is there/should there be a standard recommended order of metadata lines? (For example, should "Fixes:" lines always appear before "Signed-off-by" lines, or vice versa?) If so, is it documented somewhere and preserved by tools when such lines are added/modified? Should there be support for querying such lines?

There is another thread [1] proposing the addition of a "Change-Id:" metadata line, so maybe now would be a good time to discuss such lines in general.

Too bad your proposed new option sounds so similar to --fixup, which does something conceptually similar albeit very different in effect. This will likely lead to confusion. I wonder if the two features could be combined in some way?

The main difference between the two features is how they are intended to be used: --fixup is to fix a commit that hasn't been pushed yet (where the user intends to squash the commits together), whereas --fixes is to mark a commit as a fix to a commit that has already been pushed (where the commits will remain separate). But there seems to be a common concept here.

For example, what happens if a --fixes commit is "rebase -i"ed at the same time as the commit that it fixes? It might make sense to do the autosquash thing just like with a --fixup/--squash commit. (Otherwise the SHA-1 in the "Fixes:" line will become invalid anyway.)

Conversely, I suppose one could ask whether there should be some way to prevent "fixup!" or "squash!" commits from being pushed, at least without some kind of --force option. This could of course be enforced by a hook but it might be nice to have some protection by default.

I see that there a consistency check that the --fixes argument is a valid commit. But is there/should there be a check that it is an ancestor of the commit being created? Is there/should there be a check that both of these facts remain true if the the commit containing it is rebased, cherry-picked, etc?

In workflows that make more use of cherry-picking, it could be that the original buggy commit was cherry-picked to a different branch. In this case the user would probably want to cherry-pick the fixing commit to the other branch, too. But then the commit that it would be fixing would have a different SHA-1 than it did on the original branch. A check that the "Fixes:" line refers to an ancestor of the current commit could warn against such errors. (In some cases it might be possible to use cherry-pick's "-x" lines to figure out how to rewrite the "Fixes:" line, but I doubt that would work often enough to be worthwhile.)

Show 21 quoted lines
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> ---
>  Documentation/git-commit.txt | 12 ++++++++++--
>  builtin/commit.c             | 29 +++++++++++++++++++++++++++--
>  sequencer.c                  | 31 +++++++++++++++++++++++--------
>  sequencer.h                  |  3 +++
>  t/t7502-commit.sh            | 39 ++++++++++++++++++++++++++++++++++++++-
>  5 files changed, 101 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index 1a7616c..fcc6ed2 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -8,8 +8,8 @@ git-commit - Record changes to the repository
>  SYNOPSIS
>  --------
>  [verse]
> -'git commit' [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]
> -	   [--dry-run] [(-c | -C | --fixup | --squash) <commit>]
> +'git commit' [-a | --interactive | --patch] [-s] [-f <commit>] [-v] [-u<mode>]
> +	   [--amend] [--dry-run] [(-c | -C | --fixup | --squash) <commit>]
You mention only "-f", not "--fixes" here.

But I don't think that this feature should be given the "-f" short option, as (a) -f often means "force"; (b) it will increase the confusion with --fixup; (c) it just doesn't strike me as being likely to be such a frequently-used option (though if this changes over time the "-f" option could always be granted to it later).

Show 15 quoted lines
>  	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
>  	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
>  	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
> @@ -156,6 +156,14 @@ OPTIONS
>  	Add Signed-off-by line by the committer at the end of the commit
>  	log message.
>  
> +-f <commit>::
> +--fixes=<commit>::
> +	Add Fixes line for the specified commit at the end of the commit
> +	log message.  This line includes an abbreviated commit hash for
> +	the specified commit; the `core.abbrev` option determines the
> +	length of the abbreviated commit hash used, with a minimum length
> +	of 12 hex digits.
> +

You might also mention that the "Fixes:" line includes the old commit's subject line.

Show 212 quoted lines
>  -n::
>  --no-verify::
>  	This option bypasses the pre-commit and commit-msg hooks.
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 6ab4605..9bbcd8a 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -123,6 +123,7 @@ static int use_editor = 1, include_status = 1;
>  static int show_ignored_in_status, have_option_m;
>  static const char *only_include_assumed;
>  static struct strbuf message = STRBUF_INIT;
> +static struct strbuf fixes = STRBUF_INIT;
>  
>  static enum status_format {
>  	STATUS_FORMAT_NONE = 0,
> @@ -133,6 +134,28 @@ static enum status_format {
>  	STATUS_FORMAT_UNSPECIFIED
>  } status_format = STATUS_FORMAT_UNSPECIFIED;
>  
> +static int opt_parse_f(const struct option *opt, const char *arg, int unset)
> +{
> +	struct strbuf *sb = opt->value;
> +	if (unset) {
> +		strbuf_setlen(sb, 0);
> +	} else {
> +		struct pretty_print_context ctx = {0};
> +		struct commit *commit;
> +
> +		commit = lookup_commit_reference_by_name(arg);
> +		if (!commit)
> +			die(_("could not lookup commit %s"), arg);
> +		ctx.output_encoding = get_commit_output_encoding();
> +		ctx.abbrev = DEFAULT_ABBREV;
> +		if (ctx.abbrev < 12)
> +			ctx.abbrev = 12;
> +		format_commit_message(commit, "Fixes: %h ('%s')\n", sb, &ctx);
> +	}
> +
> +	return 0;
> +}
> +
>  static int opt_parse_m(const struct option *opt, const char *arg, int unset)
>  {
>  	struct strbuf *buf = opt->value;
> @@ -718,7 +741,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  	if (clean_message_contents)
>  		stripspace(&sb, 0);
>  
> -	if (signoff) {
> +	if (signoff || fixes.len) {
>  		/*
>  		 * See if we have a Conflicts: block at the end. If yes, count
>  		 * its size, so we can ignore it.
> @@ -742,7 +765,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  			previous = eol;
>  		}
>  
> -		append_signoff(&sb, ignore_footer, 0);
> +		append_signoff_extra(&sb, ignore_footer,
> +				     signoff ? 0 : APPEND_EXTRA_ONLY, &fixes);
>  	}
>  
>  	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
> @@ -1463,6 +1487,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_CALLBACK('f', "fixes", &fixes, N_("commit"), N_("add Fixes: for the specified commit"), opt_parse_f),
>  		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
>  		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
>  		OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")),
> diff --git a/sequencer.c b/sequencer.c
> index 06e52b4..f4cf0e1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1135,26 +1135,33 @@ int sequencer_pick_revisions(struct replay_opts *opts)
>  	return pick_commits(todo_list, opts);
>  }
>  
> -void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
> +void append_signoff_extra(struct strbuf *msgbuf, int ignore_footer,
> +			  unsigned flag, struct strbuf *extrabuf)
>  {
>  	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
> +	unsigned append_sob = !(flag & APPEND_EXTRA_ONLY);
>  	struct strbuf sob = STRBUF_INIT;
>  	int has_footer;
>  
> -	strbuf_addstr(&sob, sign_off_header);
> -	strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
> -				getenv("GIT_COMMITTER_EMAIL")));
> -	strbuf_addch(&sob, '\n');
> +	if (append_sob) {
> +		strbuf_addstr(&sob, sign_off_header);
> +		strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
> +					getenv("GIT_COMMITTER_EMAIL")));
> +		strbuf_addch(&sob, '\n');
> +	}
>  
>  	/*
>  	 * If the whole message buffer is equal to the sob, pretend that we
>  	 * found a conforming footer with a matching sob
>  	 */
> -	if (msgbuf->len - ignore_footer == sob.len &&
> +	if (append_sob &&
> +	    msgbuf->len - ignore_footer == sob.len &&
>  	    !strncmp(msgbuf->buf, sob.buf, sob.len))
>  		has_footer = 3;
>  	else
> -		has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);
> +		has_footer = has_conforming_footer(msgbuf,
> +						   append_sob ? &sob : NULL,
> +						   ignore_footer);
>  
>  	if (!has_footer) {
>  		const char *append_newlines = NULL;
> @@ -1193,9 +1200,17 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
>  				append_newlines, strlen(append_newlines));
>  	}
>  
> -	if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
> +	if (append_sob && has_footer != 3 && (!no_dup_sob || has_footer != 2))
>  		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
>  				sob.buf, sob.len);
> +	if (extrabuf)
> +		strbuf_insert(msgbuf, msgbuf->len - ignore_footer,
> +				extrabuf->buf, extrabuf->len);
>  
>  	strbuf_release(&sob);
>  }
> +
> +void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
> +{
> +	append_signoff_extra(msgbuf, ignore_footer, flag, NULL);
> +}
> diff --git a/sequencer.h b/sequencer.h
> index 1fc22dc..8716ad0 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -7,6 +7,7 @@
>  #define SEQ_OPTS_FILE	"sequencer/opts"
>  
>  #define APPEND_SIGNOFF_DEDUP (1u << 0)
> +#define APPEND_EXTRA_ONLY (1u << 1)
>  
>  enum replay_action {
>  	REPLAY_REVERT,
> @@ -51,5 +52,7 @@ int sequencer_pick_revisions(struct replay_opts *opts);
>  extern const char sign_off_header[];
>  
>  void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
> +void append_signoff_extra(struct strbuf *msgbuf, int ignore_footer,
> +			  unsigned flag, struct strbuf *extrabuf);
>  
>  #endif
> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
> index 6313da2..12b123a 100755
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -137,13 +137,50 @@ test_expect_success 'partial removal' '
>  
>  '
>  
> +signoff_ident () {
> +	git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/"
> +}
> +
>  test_expect_success 'sign off' '
>  
>  	>positive &&
>  	git add positive &&
>  	git commit -s -m "thank you" &&
>  	actual=$(git cat-file commit HEAD | sed -ne "s/Signed-off-by: //p") &&
> -	expected=$(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/") &&
> +	expected=$(signoff_ident) &&
> +	test "z$actual" = "z$expected"
> +
> +'
> +
> +fixes_for_commits () {
> +	for commit in "$@"; do
> +		git -c core.abbrev=12 log -1 --pretty=format:"Fixes: %h ('%s')%n" "$commit"
> +	done
> +}
> +
> +test_expect_success '--fixes' '
> +
> +	echo >>positive &&
> +	git add positive &&
> +	git commit -f HEAD -m "fix bug" &&
> +	actual=$(git cat-file commit HEAD | sed -e "1,/^\$/d") &&
> +	expected=$(echo fix bug; echo; fixes_for_commits HEAD^) &&
> +	test "z$actual" = "z$expected"
> +
> +'
> +
> +test_expect_success 'multiple --fixes with signoff' '
> +
> +	echo >>positive &&
> +	git add positive &&
> +	git commit -f HEAD^ -f HEAD -s -m "signed bugfix" &&
> +	actual=$(git cat-file commit HEAD | sed -e "1,/^\$/d") &&
> +	expected=$(
> +		echo signed bugfix
> +		echo
> +		echo "Signed-off-by: $(signoff_ident)"
> +		fixes_for_commits HEAD^^ HEAD^
> +	) &&
>  	test "z$actual" = "z$expected"
>  
>  '
> 
Michael

[1] http://thread.gmane.org/gmane.comp.version-control.git/236429/focus=236582

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Previous: Josh TriplettNext: Theodore Ts'o
Message 2 of 49 in “commit: Add -f, --fixes <commit> option to add Fixes: line”
  1. commit: Add -f, --fixes <commit> option to add Fixes: lineJosh Triplett, Oct 27, 2013
  2. Michael HaggertyOct 27, 2013
  3. Theodore Ts'oOct 27, 2013
  4. Josh TriplettOct 27, 2013
  5. Michel LespinasseOct 27, 2013
  6. Josh TriplettOct 27, 2013
  7. Thomas RastOct 27, 2013
  8. Josh TriplettOct 27, 2013
  9. Johan HerlandOct 27, 2013
  10. Christian CouderOct 27, 2013
  11. Johan HerlandOct 28, 2013
  12. Thomas RastOct 28, 2013
  13. Jeff KingOct 29, 2013
  14. Johan HerlandOct 30, 2013
  15. Christian CouderOct 29, 2013
  16. Johan HerlandOct 30, 2013
  17. Christian CouderNov 2, 2013
  18. Stefan BellerOct 27, 2013
  19. Thomas RastOct 27, 2013
  20. Stefan BellerOct 27, 2013
  21. Stefan BellerOct 31, 2013
  22. Documentation: add a script to generate a (long/short) options overviewStefan Beller, Oct 31, 2013
  23. Stefan BellerOct 31, 2013
  24. brian m. carlsonOct 31, 2013
  25. Junio C HamanoNov 1, 2013
  26. Michael HaggertyOct 28, 2013
  27. Johan HerlandOct 28, 2013
  28. Jeff KingOct 29, 2013
  29. Matthieu MoyOct 29, 2013
  30. Johan HerlandOct 30, 2013
  31. Duy NguyenOct 31, 2013
  32. Junio C HamanoOct 31, 2013
  33. Duy NguyenOct 31, 2013
  34. Johan HerlandNov 1, 2013
  35. Duy NguyenOct 27, 2013
  36. Josh TriplettOct 27, 2013
  37. Jim HillOct 28, 2013
  38. Junio C HamanoOct 28, 2013
  39. Josh TriplettOct 28, 2013
  40. Michael HaggertyOct 28, 2013
  41. Christoph HellwigOct 28, 2013
  42. Benjamin HerrenschmidtOct 28, 2013
  43. Russell King - ARM LinuxOct 28, 2013
  44. Russell King - ARM LinuxOct 28, 2013
  45. Junio C HamanoOct 28, 2013
  46. Christian CouderOct 29, 2013
  47. Junio C HamanoOct 29, 2013
  48. Tony LuckOct 30, 2013
  49. Junio C HamanoOct 30, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.