Re: [PATCH v7 3/5] format-patch: add ability to use alt cover format
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Mar 10, 2026, 14:33 UTC
- Message-ID
- <8f49d551-9741-4461-b076-3caba75e6122@gmail.com>
- In-Reply-To
- <316c9e76ee49d73aff75b63299c970e9f55f79b6.1772839973.git.mroik@delayed.space>
On 06/03/2026 23:34, Mirko Faina wrote:
Show 9 quoted lines
> 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.
Show 31 quoted lines
> 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?
Show 5 quoted lines
> @@ -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.
Show 8 quoted lines
> 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
Show 10 quoted lines
> +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.
Show 12 quoted lines
> + 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
Show 12 quoted lines
> + 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.
Show 11 quoted lines
> + 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
Show 18 quoted lines
> + 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