From: Phillip Wood Date: Tue, 10 Mar 2026 14:33:46 GMT Subject: Re: [PATCH v7 3/5] format-patch: add ability to use alt cover format 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: > 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 > --- > 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