From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 18 May 2026 15:27:24 GMT
Subject: Re: [PATCH 1/1] commit: allow -m/-F with --fixup=amend: or reword:
Message-ID: <ac6aaaca-2b7c-4892-ba93-0dc3e3c18ff7@gmail.com>
In-Reply-To: <xmqqik8kc2nj.fsf@gitster.g>

On 18/05/2026 13:39, Junio C Hamano wrote:
> 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.

Indeed

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

Yes, I find it quite useful to make a note of what the fixup is doing if 
I know I'm not going to squash it for a while.

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

Yes, looking at the way the code is structured I wonder if these options 
were made incompatible to simplify the implementation, or maybe the 
implementation merely reflects those restrictions.

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

It is indeed unfortunate that we end up duplicating the code to read the 
logfile here. I wonder how hard it would be to refactor 
prepare_to_commit() so that it can accommodate "--fixup=amend:<commit> -F"

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

In this case it would be better to use

	test_commit_message HEAD <<-EOF
	amend! $(git log -1 --format=%s HEAD~)

	amend commit message
	EOF

and avoid creating actual and expect all together.

Thanks

Phillip