Re: [PATCH v7 3/5] format-patch: add ability to use alt cover format

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

On 06/03/2026 23:34, Mirko Faina wrote:
> Often when sending patch series there's a need to clarify to the
> reviewer what's the purpose of said series, since it might be difficult
> to understand it from reading the commits messages one by one.
> 
> "git format-patch" provides the useful "--cover-letter" flag to declare
> if we want it to generate a template for us to use. By default it will
> generate a "git shortlog" of the changes, which developers find less
> useful than they'd like, mainly because the shortlog groups commits by
> author, and gives no obvious chronological order.

I think we probably care more about topological order for format-patch. 
In practice that's the same as chronological order by commit date, but 
it does not necessarily match chronological order by author date.

> Give format-patch the ability to specify an alternative format spec
> through the "--cover-letter-format" option. This option either takes
> "shortlog", which is the current format, or a format spec prefixed with
> "log:".

That sounds like a nice improvement over using the shortlog output. 
However it is rather cumbersome to have to type "log:" each time. As 
--cover-letter-format=shortlog is a nonsensical format I don't think we 
need to require the "log:" prefix. --no-cover-letter-format should 
behave like --cover-letter-format=shortlog.

> Example:
>      git format-patch --cover-letter \
>          --cover-letter-format="log:[%(count)/%(total)] %s (%an)" HEAD~3
> 
>      [1/3] this is a commit summary (Mirko Faina)
>      [2/3] this is another commit summary (Mirko Faina)
>      ...
> 
> Signed-off-by: Mirko Faina <mroik@delayed.space>
> ---
>   builtin/log.c           | 40 +++++++++++++++++++++++++++++++---
>   t/t4014-format-patch.sh | 48 +++++++++++++++++++++++++++++++++++++++++
>   t/t9902-completion.sh   |  1 +
>   3 files changed, 86 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/log.c b/builtin/log.c
> index 0d12272031..95e5d9755f 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1343,13 +1343,36 @@ static void generate_shortlog_cover_letter(struct shortlog *log,
>   	shortlog_output(log);
>   }
>   
> +static void generate_commit_list_cover(FILE *cover_file, const char *format,
> +				       struct commit **list, int n)
> +{
> +	struct strbuf commit_line = STRBUF_INIT;
> +	struct pretty_print_context ctx = {0};
> +	struct rev_info rev = REV_INFO_INIT;
> +
> +	strbuf_init(&commit_line, 0);

This is unnecessary as commit_line is initialized in the declaration above.

> +	rev.total = n;
> +	ctx.rev = &rev;
> +	for (int i = n - 1; i >= 0; i--) {
> +		rev.nr = n - i;
 > +		repo_format_commit_message(the_repository, list[i], format,
 > +				&commit_line, &ctx);

This loop is a bit confusing, I wonder if it would be simpler to count up
	for (int i = 1; i <= n; i++) {
		rev.nr = i
		repo_format_commit_message(the_repository, list[n - i], ...);

The shortlog sets some wrapping and indent options, do we want to do 
something similar here?
> @@ -2297,6 +2328,7 @@ int cmd_format_patch(int argc,
>   		/* nothing to do */
>   		goto done;
>   	total = list.nr;
> +

Please don't mix unrelated whitespace changes in with your changes.

>   	if (cover_letter == -1) {
>   		if (cfg.config_cover_letter == COVER_AUTO)
>   			cover_letter = (total > 1);
> @@ -2383,12 +2415,14 @@ int cmd_format_patch(int argc,
>   	}
>   	rev.numbered_files = just_numbers;
>   	rev.patch_suffix = fmt_patch_suffix;
> +

The same here

> +test_expect_success 'cover letter with subject, author and count' '
> +	rm -rf patches &&
> +	test_when_finished "git reset --hard HEAD~1" &&
> +	test_when_finished "rm -rf patches result test_file" &&
> +	touch test_file &&
> +	git add test_file &&
> +	git commit -m "This is a subject" &&
> +	git format-patch --cover-letter \
> +	--cover-letter-format="log:[%(count)/%(total)] %s (%an)" -o patches HEAD~1 &&
> +	grep "^\[1/1\] This is a subject (A U Thor)$" patches/0000-cover-letter.patch >result &&

using test_grep here would make it easier to debug test failures.

> +	test_line_count = 1 result
> +'
> +
> +test_expected_success 'cover letter with author and count' '
> +	test_when_finished "git reset --hard HEAD~1" &&
> +	test_when_finished "rm -rf patches result test_file" &&
> +	touch test_file &&
> +	git add test_file &&
> +	git commit -m "This is a subject" &&
> +	git format-patch --cover-letter \
> +	--cover-letter-format="log:[%(count)/%(total)] %an" -o patches HEAD~1 &&
> +	grep "^\[1/1\] A U Thor$" patches/0000-cover-letter.patch >result &&

I'm not clear what new coverage this test adds

> +	test_line_count = 1 result
> +'
> +
> +test_expect_success 'cover letter shortlog' '
> +	test_when_finished "git reset --hard HEAD~1" &&
> +	test_when_finished "rm -rf patches result test_file" &&
> +	touch test_file &&
> +	git add test_file &&
> +	git commit -m "This is a subject" &&
> +	git format-patch --cover-letter --cover-letter-format=shortlog \
> +	-o patches HEAD~1 &&
> +	sed -n -e "/^A U Thor/p;" patches/0000-cover-letter.patch >result &&

This just checks that the author name appears in the coverletter, not 
that the patches are formatted with shortlog.

> +	test_line_count = 1 result
> +'
> +
> +test_expect_success 'cover letter no format' '
> +	test_when_finished "git reset --hard HEAD~1" &&
> +	test_when_finished "rm -rf patches result test_file" &&
> +	touch test_file &&
> +	git add test_file &&
> +	git commit -m "This is a subject" &&
> +	git format-patch --cover-letter -o patches HEAD~1 &&
> +	sed -n -e "/^A U Thor/p;" patches/0000-cover-letter.patch >result &&

Don't we already have test coverage for the case where 
--cover-letter-format isn't given? Testing that --no-cover-letter-format 
works as expected would be useful.

I think this is a useful improvement to the cover letter generated by 
"git format-patch"

Thanks

Phillip

> +	test_line_count = 1 result
> +'
> +
>   test_expect_success 'reroll count' '
>   	rm -fr patches &&
>   	git format-patch -o patches --cover-letter --reroll-count 4 main..side >list &&
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 964e1f1569..4f760a7468 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2774,6 +2774,7 @@ test_expect_success PERL 'send-email' '
>   	test_completion "git send-email --cov" <<-\EOF &&
>   	--cover-from-description=Z
>   	--cover-letter Z
> +	--cover-letter-format=Z
>   	EOF
>   	test_completion "git send-email --val" <<-\EOF &&
>   	--validate Z