Re: [RFC PATCH 2/2] history: add --verbose option for reword

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

Hi Jelle

On 07/11/2026 19:30, Jelle van der Waa wrote:
> When rewording a commit message it is often useful to have context of
> the changes. 

Indeed - I'm still using a wrapper around "git rebase -i" to reword 
commits because "git history reword" does not show the diff. Thanks for 
working on this.

> This commit implements the --verbose option of git commit
> in git history reword with the only difference in that it does not
> folllow the commit.verbose configuration setting.

As it is easy to support commit.verbose, I think we should. In my ideal 
world we'd default to showing the diff when rewording but that's not 
really compatible with the way "commit.verbose" works.

> diff --git a/Documentation/git-history.adoc b/Documentation/git-history.adoc
> index 1fcc30150a..a476780623 100644
> --- a/Documentation/git-history.adoc
> +++ b/Documentation/git-history.adoc
> @@ -10,7 +10,7 @@ SYNOPSIS
>   [synopsis]
>   git history drop <commit> [--dry-run] [--update-refs=(branches|head)] [--empty=(drop|keep|abort)] [--[no-]gpg-sign[=<key-id>]]
>   git history fixup <commit> [--dry-run] [--update-refs=(branches|head)] [--reedit-message] [--empty=(drop|keep|abort)] [--[no-]gpg-sign[=<key-id>]]
> -git history reword <commit> [--dry-run] [--update-refs=(branches|head)] [--[no-]gpg-sign[=<key-id>]]
> +git history reword <commit> [--dry-run] [--update-refs=(branches|head)] [-v | --verbose] [--[no-]gpg-sign[=<key-id>]]

It is not a new problem, but these lines are getting very long.
> diff --git a/builtin/history.c b/builtin/history.c
> index cdb0d30784..e2e0aba4a9 100644
> --- a/builtin/history.c
> +++ b/builtin/history.c
> @@ -34,7 +36,7 @@
>   #define GIT_HISTORY_FIXUP_USAGE \
>   	N_("git history fixup <commit> [--dry-run] [--update-refs=(branches|head)] [--reedit-message] [--empty=(drop|keep|abort)] [--[no-]gpg-sign[=<key-id>]]")
>   #define GIT_HISTORY_REWORD_USAGE \
> -	N_("git history reword <commit> [--dry-run] [--update-refs=(branches|head)] [--[no-]gpg-sign[=<key-id>]]")
> +	N_("git history reword <commit> [--dry-run] [--update-refs=(branches|head)] [-v | --verbose] [--[no-]gpg-sign[=<key-id>]]")

As above, it is not a new problem, but these lines are very long.

>   #define GIT_HISTORY_SPLIT_USAGE \
>   	N_("git history split <commit> [--dry-run] [--update-refs=(branches|head)] [--[no-]gpg-sign[=<key-id>]] [--] [<pathspec>...]")
>   #define GIT_HISTORY_SQUASH_USAGE \

>   static int fill_commit_message(struct repository *repo,
>   			       const struct object_id *old_tree,
>   			       const struct object_id *new_tree,
>   			       const char *default_message,
>   			       const char *action,
> +			       int verbose,

If this was a bool, we could get rid of the "!!()" in the caller.

>   			       struct strbuf *out)
>   {
>   	const char *path = git_path_commit_editmsg();
> @@ -84,6 +107,10 @@ static int fill_commit_message(struct repository *repo,
>   
>   	wt_status_collect_changes_trees(&s, old_tree, new_tree);
>   	wt_status_print(&s);
> +	if (verbose) {
> +		wt_status_add_cut_line(&s);
> +		print_verbose_diff(repo, old_tree, new_tree, s.fp);

We already reuse some of the code in wt-status.c that "git commit" uses 
to generate the template message, how hard would it be to teach the 
status code to generate the diff for us as it does for "git commit"? 
"git commit" also respects status.relativePaths, status.renames and 
status.renameLimit in addition to the equivalent diff settings - should 
we be doing the same here?

> +static int reword_config(const char *var, const char *value,
> +			 const struct config_context *ctx, void *data)
> +{
> +	const char **sign_commit = data;
> +
> +	if (!strcmp(var, "commit.gpgsign")) {
> +		*sign_commit = git_config_bool(var, value) ? "" : NULL;
> +		return 0;
> +	}
> +
> +	return git_diff_ui_config(var, value, ctx, NULL);
> +}

I'm in two minds about this - it would be simpler to share the same 
config callback between all subcommands, even if it means we read a bit 
more config that we really need to for some commands.

> diff --git a/t/t3451-history-reword.sh b/t/t3451-history-reword.sh
> index 6dbe2143d3..4ec07fb493 100755
> --- a/t/t3451-history-reword.sh
> +++ b/t/t3451-history-reword.sh
> @@ -16,6 +16,19 @@ reword_with_message () {
>   	rm fake-editor.sh message
>   }
>   
> +reword_and_capture () {
> +	write_script fake-editor.sh <<-\EOF &&
> +	cp "$1" captured &&
> +	{
> +		echo reworded &&
> +		sed -n "/^# -* >8 -*$/,\$p" "$1"

We preserve the scissors line and the diff to check the message is 
cleaned up as we expect - good.

> +test_expect_success '--verbose shows the diff of the reworded commit' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit first &&
> +		test_commit second &&
> +		test_commit third &&
> +		echo unstaged >first.t &&
> +		echo staged >third.t &&
> +		git add third.t &&
> +		git show --format= HEAD~ >expect &&

This looks a bit strange - if we just want a diff why don't we create it 
by running "git diff"?

	git diff HEAD~^- >expect

> +
> +		reword_and_capture --verbose HEAD~ &&
> +
> +		test_grep "^# -* >8 -*\$" captured &&

If this line is not in the output then the sed command in the fake 
editor wont have printed the diff so I'm not sure this actually adds 
anything.

> +		sed -n "/^diff --git/,\$p" captured >actual &&
> +		test_cmp expect actual &&
> +
> +		test_commit_message HEAD~ <<-\EOF
> +		reworded
> +		EOF

We check the diff and the reworded message - good. We do not check that 
the user sees the original message though - perhaps we'd be better to 
just compare captured against what we expect the user to see.

> +test_expect_success 'no diff is shown without --verbose' '

Why do we need a separate test to do this, rather than just checking 
there is no diff in one of the existing tests?

> +test_expect_success '--verbose works in a bare repository' '

I'm not sure this adds much value - we already have a test to check 
reword works in a bare repository and there is nothing in the 
implementation of verbose that is conditional on the whether the 
repository is bare or not. What failure mode do we expect this test to 
detect that the existing test wont?

> +test_expect_success '--verbose on a merge diffs against the first parent' '

Can we extend the existing test for rewording a merge commit instead of 
adding a new one? Our test suite is slow and the way the history tests 
are written makes it even slower because they initialize a new 
repository and create a bunch of new commits for every single test.

Thanks for working on this - it is a really useful improvement

Phillip

>   test_expect_success 'retains changes in the worktree and index' '
>   	test_when_finished "rm -rf repo" &&
>   	git init repo &&