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

From: Michael Haggerty
Date: Sun Oct 27 2013 - 01:50:31 EST

Next message: Cong Ding: "[PATCH] btrfs: fix memory leakage"
Previous message: Dave Young: "Re: query for idea about a kexec regression"
In reply to: Josh Triplett: "[PATCH] commit: Add -f, --fixes <commit> option to add Fixes: line"
Next in thread: Theodore Ts'o: "Re: [PATCH] commit: Add -f, --fixes <commit> option to add Fixes:line"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 10/27/2013 02:34 AM, Josh Triplett wrote:
> 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.)

> Signed-off-by: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
> ---
> 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).

> [-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.

> -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@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Cong Ding: "[PATCH] btrfs: fix memory leakage"
Previous message: Dave Young: "Re: query for idea about a kexec regression"
In reply to: Josh Triplett: "[PATCH] commit: Add -f, --fixes <commit> option to add Fixes: line"
Next in thread: Theodore Ts'o: "Re: [PATCH] commit: Add -f, --fixes <commit> option to add Fixes:line"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]