Re: [PATCH 1/1] commit: allow -m/-F with --fixup=amend: or reword:

[Junio C Hamano <gitster@pobox.com>]

erik@cervined.in writes:

> From: Erik Cervin-Edin <erik@cervined.in>

The name on this overriding in-body From: line, and the name on
Signed-off-by: line below, must match.  Please pick a name with or
without hyphen and stick to it.

> --fixup=amend: and --fixup=reword: require an editor to supply the
> replacement commit message. The -m and -F flags are rejected: -m is
> caught by a die() in prepare_to_commit(), and -F is caught by
> die_for_incompatible_opt4() which groups -F with --fixup as mutually
> exclusive. This makes these modes unusable in non-interactive
> workflows -- notably AI coding agents.

"Unusable" may be stronger than reality, as you can make creatie use
of GIT_EDITOR to achieve what you want.  "awkward" or "poorly suited"
would be more fitting.

> Plain --fixup (without amend: or reword:) continues to reject -F but
> still accepts -m (even though it's practically a no-op).

Is it "practically a no-op"?  Wouldn't

   $ git commit --fixup <commit> -m "message body"

be useful to leave a message in the resulting commit, which is later
to be squashed into the named <commit>?  Actually squashing with "fixup!"
may lose the message supplied here, but wouldn't people use this
facility to more easily identify what each of the fixups are about?

For the same reason, "-F" would be just as useful as "-m" in this context,
and it feels a bit inconsistent to allow one while rejecting the other.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 28f6174503..269c2d782b 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -837,21 +837,19 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		hook_arg1 = "message";
>  
>  		/*
> -		 * Only `-m` commit message option is checked here, as
> -		 * it supports `--fixup` to append the commit message.
> -		 *
> -		 * The other commit message options `-c`/`-C`/`-F` are
> -		 * incompatible with all the forms of `--fixup` and
> -		 * have already errored out while parsing the `git commit`
> -		 * options.
> +		 * `-m` (and `-F`, converted to `-m` earlier for
> +		 * amend/reword) appends the message body here.
> +		 * `-c`/`-C` are still incompatible with all forms
> +		 * of `--fixup`.
>  		 */
>  		if (have_option_m && !strcmp(fixup_prefix, "fixup"))
>  			strbuf_addbuf(&sb, &message);
>  
>  		if (!strcmp(fixup_prefix, "amend")) {
>  			if (have_option_m)
> -				die(_("options '%s' and '%s:%s' cannot be used together"), "-m", "--fixup", fixup_message);

Good that you got rid of this overly long die() message line.

> -			prepare_amend_commit(commit, &sb, &ctx);
> +				strbuf_addbuf(&sb, &message);
> +			else
> +				prepare_amend_commit(commit, &sb, &ctx);
>  		}
>  	} else if (!stat(git_path_merge_msg(the_repository), &statbuf)) {
>  		size_t merge_msg_start;
> @@ -1338,10 +1336,12 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  	}
>  	if (fixup_message && squash_message)
>  		die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup");
> -	die_for_incompatible_opt4(!!use_message, "-C",
> +	die_for_incompatible_opt3(!!use_message, "-C",
>  				  !!edit_message, "-c",
> -				  !!logfile, "-F",
>  				  !!fixup_message, "--fixup");
> +	die_for_incompatible_opt3(!!use_message, "-C",
> +				  !!edit_message, "-c",
> +				  !!logfile, "-F");
>  	die_for_incompatible_opt4(have_option_m, "-m",
>  				  !!edit_message, "-c",
>  				  !!use_message, "-C",
> @@ -1410,6 +1410,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  		}
>  	}
>  
> +	if (logfile && fixup_message && !strcmp(fixup_prefix, "fixup"))
> +		die(_("options '%s' and '%s' cannot be used together"), "-F", "--fixup");
> +
>  	if (0 <= edit_flag)
>  		use_editor = edit_flag;
>  
> @@ -1821,6 +1824,22 @@ int cmd_commit(int argc,
>  	argc = parse_and_validate_options(argc, argv, builtin_commit_options,
>  					  builtin_commit_usage,
>  					  prefix, current_head, &s);
> +
> +	if (logfile && fixup_message && !strcmp(fixup_prefix, "amend")) {
> +		if (!strcmp(logfile, "-")) {
> +			if (isatty(0))
> +				fprintf(stderr, _("(reading log message from standard input)\n"));
> +			if (strbuf_read(&message, 0, 0) < 0)
> +				die_errno(_("could not read log from standard input"));
> +		} else {
> +			if (strbuf_read_file(&message, logfile, 0) < 0)
> +				die_errno(_("could not read log file '%s'"), logfile);
> +		}
> +		strbuf_complete_line(&message);
> +		have_option_m = 1;
> +		FREE_AND_NULL(logfile);
> +	}
> +

It is curious that for this new feature alone, but not the other
existing code paths, "-m" and "-F" options reads from file in the
new code here, instead of letting the existing code for "-F" to read
(which happens inside prepare_to_commit(), I presume?).

A potential problem of the above code is if we find something wrong
in message and complain later in the control flow, we have long lost
where the message came from, as the point of the above code is
exactly to pretend that "--fixup:amend/reword -F" message did *not*
come from a file with the "-F" option, but from the command line via
the "-m" option.

> +test_expect_success '--fixup=amend: with -m option' '
>  	commit_for_rebase_autosquash_setup &&
> -	echo "fatal: options '\''-m'\'' and '\''--fixup:reword'\'' cannot be used together" >expect &&
> -	test_must_fail git commit --fixup=reword:HEAD~ -m "reword commit message" 2>actual &&
> -	test_cmp expect actual
> +	cat >expected <<-EOF &&

This comment is not about the added logic, but I notice that among
86 hits with string "expect" in this file in today's "master", only
14 hits are with string "expected", i.e., the prevalent name for the
"golden copy result" that is compared with the actula result (called
"actual") is "expect", not "expected".  Please do not make the
situation worse.

> -	test_cmp expect actual
> +	cat >expected <<-EOF &&

Ditto.