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

From: Mroik <mroik@delayed.space>
Date: Mar 10, 2026, 21:05 UTC
Message-ID: <abCFlUkXbth08nPS@exploit>
In-Reply-To: <8f49d551-9741-4461-b076-3caba75e6122@gmail.com>

On Tue, Mar 10, 2026 at 02:33:46PM +0000, Phillip Wood wrote:

Show 10 quoted lines

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

Show 12 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.

Will do

Show 15 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.

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.