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

[Erik Cervin Edin <erik@cervined.in>]

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

Fair, "poorly suited" is more accurate. It's not impossible, just very
awkward.

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

No, I was mistaken. The message is kept until autosquash.

    The `-m` option may be used to supplement the log message of the
    created commit, but the additional commentary will be thrown away
    once the "fixup!" commit is squashed into _<commit>_ by `git rebase
    --autosquash`.

I was trying to fill in the gaps here on the intent of the pre-existing
behavior (to reject -F with --fixup) and I kind of assumed the message
was being discarded.

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

I think it would. I kept the pre-existing behavior because I wasn't sure
if the rejection meant "Error. You are doing something that doesn't make
sense -- you probably meant to do something else" or "Sorry. What you're
trying to do is not supported"

A closer look at the original implementation 30884c9afc (commit: add
support for --fixup <commit> -m"<extra message>", 2017-12-22) makes it
clear the intent here is the latter:

    Those options could also support combining with -m, but given what
    they do I can't think of a good use-case for doing that, so I have not
    made the more invasive change of splitting up the logic in commit.c to
    first act on those, and then on -m options.

There is a case to not reject them, it was just deemed unnecessary
complex for something without a clear use-case.

In the ideal case, given that -m works (and does something useful), it's
reasonable to expect -F to do the same (for the same reasons as
--fixup=reword:.) Although, it's arguably less crucial in this
usecase. Given what its ephemeral nature, such a message is likely a
terse comment, -m "forgot to format" or similar.

I think it makes sense to allow -F for all --fixup variations, for
consistency. For the plain --fixup, -c/-C are probably less justifiable,
but -F mirroring -m seems worthwhile for consistency's sake in all
variations.

> > A potential problem of the above code is if we find something wrong
> > in message and complain later in the control flow
> > 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.

Now that you mention this, I guess a message on stdin can be arbitrarily
large, have null bytes and maybe some other oddities which the -m
would never have.

> I wonder how hard it would be to refactor prepare_to_commit()
> so that it can accommodate "--fixup=amend:<commit> -F"

I think this is doable.

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

Mea culpa. I overlooked this distinction.

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

That would also work (except it has to be HEAD~2, since the reword
commit advances HEAD by one)

Thank you both for the review. I will reroll as a V2 taking your
suggestions into account.

- Erik