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

[Mroik <mroik@delayed.space>]

On Tue, Mar 10, 2026 at 02:33:46PM +0000, Phillip Wood wrote:
> > 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.

This was done to avoid having "anything that is not shortlog should be
in pretty-print format". Having it this way in the future we can check
for the prefix and see if the user indeed would like the pretty-print
format, otherwise check againts other formats available (so far only
shortlog). If "log:" is too cumbersome maybe we can drop it and check
instead for existing presets first, "shortlog, etc..." and then when
nothing is found interpret it as pp format. I like the explicit prefix
but I guess it is unlikely that a user will want to print "shortlog" on
every line.

Do we all agree on dropping the prefix?

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

Will remove

> 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], ...);

Will rewrite for readability

> > +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.

Will do

> > +	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.

The name at the start of the line can appear only if the foramt is
shortlog, but if in the future it we do decide to support more prefix
that might not be true anymore. Will rewrite this test to be more
robust.