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

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

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