Re: [PATCH v2 1/2] format-patch: add ability to use alt cover format

[Jeff King <peff@peff.net>]

On Wed, Feb 25, 2026 at 12:54:54AM +0100, Mirko Faina wrote:

> > So a single commit_line is given to repo_format_commit_message()
> > repeatedly to accumulate those lines in it, which makes sense.
> > 
> > We prepare pretty_print_context once above and feed it repeatedly to
> > repo_format_commit_message()---is that intended?  Not a rhetorical
> > question; I do not know the answer.  I guess some existing callers
> > (like builtin/archive.c) do reuse the structure when making multiple
> > calls to the function, so this would be kosher, perhaps?
> 
> Honestly I applied Jeff change trusting his knowledge of the pretty.c
> code. After taking a better look at repo_format_commit_message(), after
> the pretty_print_context is passed to a format_commit_context as
> pretty_ctx, it doesn't seem like this value is modified at all thoughout
> the call. So it should be ok to reuse the same structure in multiple
> calls of repo_format_commit_message().

A dangerous place to put your trust. ;)

I do think it is OK here, though. And in general I'd assume a "context"
variable like this is re-usable unless documented differently. The
shortlog does use a pretty_print_context in a loop like this, too.

> > > +	if (cover_letter && !cover_letter_fmt)
> > > +		cover_letter_fmt = "shortlog";
> > > +
> > 
> > I do not quite see the point of doing this.  There is no law that
> > "cover_letter_fmt != NULL" is a crime when cover_letter is false.
> > 
> > cover_letter_fmt can be initialized to a fixed string "shortlog",
> > and nobody cares what random value cover_letter_fmt has when
> > cover_letter is false.
> 
> Indeed, was the first thing that came to mind at the time. Had I had
> reread a few more times before sending I probably wouldn't have left it
> like this.
> 
> I'll try to take my time with the next version as to not waste your time
> on unnecessary initialization trivialities.

I think that is OK, but FWIW my instinct on reading the patch was to go
the opposite direction: leave it as NULL and fill that in as the default
at the last minute. Like this (on top of your patch 1):

diff --git a/builtin/log.c b/builtin/log.c
index d13fcf830b..6907a680f2 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1360,8 +1360,6 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 	struct commit *head = list[0];
 	char *to_free = NULL;
 
-	assert(format);
-
 	if (!cmit_fmt_is_mail(rev->commit_format))
 		die(_("cover letter needs email format"));
 
@@ -1397,7 +1395,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 	free(pp.after_subject);
 	strbuf_release(&sb);
 
-	if (skip_prefix(format, "log:", &format)) {
+	if (format && skip_prefix(format, "log:", &format)) {
 		generate_commit_list_cover(rev->diffopt.file, format, list, nr);
 	} else {
 		shortlog_init(&log);
@@ -2404,9 +2402,6 @@ int cmd_format_patch(int argc,
 	rev.numbered_files = just_numbers;
 	rev.patch_suffix = fmt_patch_suffix;
 
-	if (cover_letter && !cover_letter_fmt)
-		cover_letter_fmt = "shortlog";
-
 	if (cover_letter) {
 		if (cfg.thread)
 			gen_message_id(&rev, "cover");

I tend to like keeping the sentinel value as long as possible, because
sometimes it later becomes useful to distinguish "the user asked for
shortlog" vs "the user did not ask for anything". We don't need that
now, but I could imagine some hypothetical future where we are going to
switch the default, and start issuing an advice/warning message.

But it is a pretty minor point, and mostly subjective, so I am happy
either way.

However, the patch above does raise another interesting question if you
look closely: why do we never need to check for the string "shortlog" in
this logic?

We just assume anything that does not start with "log:" should show the
shortlog, and that is true both before and after my suggested change
above. What should:

  git format-patch --cover-letter --cover-letter-format=foo

do? With "log:%s" as the syntax, it should probably be an error. And
this seems like a good place to detect it, like:

  if (!format || !strcmp(format, "shortlog"))
	...do shortlog...
  else if (skip_prefix(format, "log:", &format))
	...do format...
  else
	die("what the heck does %s mean?", format);

But that gives me more thoughts (another dangerous habit, I know). Do we
need "log:" at all? Could the value be interpreted as a format, unless
it is the string "shortlog"? We would want to leave room for future
expansion of new hard-coded names (like "shortlog"), but I do not think
there is any problem there. Any reasonable format provided by the user
will have at least one %-placeholder in it, and no hard-coded name we
provide would have a percent in it.

And then you could just do:

  git format-patch --cover-letter --cover-letter-format=%s

Maybe not a big deal, as I'd expect everyone to set it once in config
(or within a script) anyway. But food for thought.

-Peff