Re: [PATCH v3 0/2] commit: allow -m/-F/-c/-C for all --fixup variations

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Erik

Sorry it has taken me so long to get round to commenting on these 
patches. I think that supporting -m and -F with --fixup is useful and I 
cannot see a reason not to support -C as well. I've left a couple of 
comments, but the only thing I'm really worried about is calling 
test_set_editor() outside of a subshell in the second patch.

Thanks

Phillip

On 22/09/2026 14:04, erik@cervined.in wrote:
> From: Erik Cervin-Edin <erik@cervined.in>
> 
> Changes since v2:
> 
>    * 2/2: Junio noted [1] that after dropping --fixup from the
>      -C/-c/--fixup incompatibility check, what remained (-C, -c, -F)
>      was already rejected by the neighbouring die_for_incompatible_opt4()
>      that groups those options with -m.  The check is now removed
>      entirely instead of being narrowed.
> 
>    * 2/2: reworded the log message to lead with why -c/-C are the odd
>      ones out once 1/2 is in place, rather than with the check that
>      happened to block them.
> 
>    * Rebased onto current master.  1/2 is unchanged apart from that.
> 
> Background
> ----------
> 
> Today only -m works with plain --fixup, and neither -m nor -F works
> with --fixup=amend: or --fixup=reword:, which makes those modes
> awkward to drive non-interactively.  1/2 threads -m and -F through
> prepare_to_commit() for all --fixup variations.  2/2 does the same for
> -c/-C, which were blocked by the same incompatibility check that
> caught -F, so that all four message sources behave consistently across
> the plain, amend, and reword forms.
> 
> 2/2 lives in its own patch and I won't object if reviewers would
> rather drop it or take it separately.
> 
> A note on one design decision in 2/2.  -c/-C go through
> prepare_amend_commit(), so if the source commit is itself an "amend!"
> commit, its subject line is dropped and only the body is copied.  This
> is what a no-source --fixup=amend:<commit> already does.
> 
> I kept that on purpose.  Under --fixup the subject is always generated
> by git, so whatever -C supplies ends up in the body, and the body of an
> amend! commit is the message destined for its target.  Copying the
> "amend! X" line along with it would leave that line in the final
> message after autosquash.  It also means
> 
>      git commit --fixup=amend:foo -C foo
> 
> and
> 
>      GIT_EDITOR=: git commit --fixup=amend:foo
> 
> produce the same commit.
> 
> The helper only knows about "amend!" and not "fixup!" or "squash!".
> That is pre-existing and I have left it alone here.
> 
> The v1 -> v2 changes (refactoring prepare_to_commit() instead of the
> special-cased file slurp in cmd_commit(), extending scope to -c/-C,
> and the smaller review fixes) are described in the v2 cover letter [2].
> 
> [1] https://lore.kernel.org/git/xmqq1pbkefh0.fsf@gitster.g/
> [2] https://lore.kernel.org/git/cover.1779792311.git.erik@cervined.in/
> 
> Erik Cervin-Edin (2):
>    commit: allow -m/-F for all kinds of --fixup
>    commit: allow -c/-C for all kinds of --fixup
> 
>   Documentation/git-commit.adoc             |  22 +++--
>   builtin/commit.c                          |  40 ++++----
>   t/t7500-commit-template-squash-signoff.sh | 114 +++++++++++++++++++---
>   3 files changed, 131 insertions(+), 45 deletions(-)
> 
> Range-diff against v2:
> 1:  e9f07d49ee = 1:  4581f97cb7 commit: allow -m/-F for all kinds of --fixup
> 2:  b3fc743abf ! 2:  39e408e147 commit: allow -c/-C for all kinds of --fixup
>      @@ Metadata
>        ## Commit message ##
>           commit: allow -c/-C for all kinds of --fixup
>       
>      -    The previous commit allowed -m and -F for all --fixup variations.  The
>      -    -c/-C flags were blocked by the same higher-layer incompatibility check
>      -    that previously caught -F, namely die_for_incompatible_opt4() grouping
>      -    them with --fixup.
>      +    The fixup code path in prepare_to_commit() only knows how to take a
>      +    message body from -m or -F.  Now that the previous commit allows -m and
>      +    -F for all --fixup variations, -c/-C are the odd ones out.
>       
>      -    Drop --fixup from that check and route the resolved commit through
>      -    prepare_amend_commit() in the fixup path, mirroring the no-message-source
>      -    behaviour of --fixup=amend.  With this in place, -m/-F/-c/-C all behave
>      -    consistently across the plain, amend, and reword --fixup forms.
>      +    Teach the fixup path to look up the ref given to -c/-C and seed its
>      +    message when preparing to amend the commit, mirroring what --fixup=amend
>      +    does when no message source is given.  With this in place, -m/-F/-c/-C
>      +    all behave consistently across the plain, amend, and reword --fixup
>      +    forms.
>       
>      +    The incompatibility check grouping -C, -c, and --fixup can go away
>      +    entirely, as -C and -c are already rejected together by the check that
>      +    groups them with -m and -F.
>      +
>      +    Helped-by: Junio C Hamano <gitster@pobox.com>
>           Signed-off-by: Erik Cervin-Edin <erik@cervined.in>
>       
>        ## Documentation/git-commit.adoc ##
>      @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const cha
>        			prepare_amend_commit(commit, &sb, &ctx);
>        		}
>       @@ builtin/commit.c: 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_opt3(!!use_message, "-C",
>      - 				  !!edit_message, "-c",
>      +-	die_for_incompatible_opt3(!!use_message, "-C",
>      +-				  !!edit_message, "-c",
>       -				  !!fixup_message, "--fixup");
>      -+				  !!logfile, "-F");
>        	die_for_incompatible_opt4(have_option_m, "-m",
>        				  !!edit_message, "-c",
>        				  !!use_message, "-C",
> 
> base-commit: d38352cd43ab9745686d697872408bc3249a153f