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

[Mirko Faina <mroik@delayed.space>]

On Tue, Feb 24, 2026 at 09:40:54AM -0800, Junio C Hamano wrote:
> > +				       struct commit **list, int n)
> > +{
> > +	struct strbuf commit_line = STRBUF_INIT;
> > +	struct pretty_print_context ctx = {0};
> > +
> > +	strbuf_init(&commit_line, 0);
> 
> 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().

> > +	for (int i = n - 1; i >= 0; i--) {
> > +		strbuf_addf(&commit_line, "[%0*d/%d] ", decimal_width(n), n - i, n);
> > +		repo_format_commit_message(the_repository, list[i], format, &commit_line, &ctx);
> 
> Let's line-wrap this overly long line (my lithmus test to complain
> about "overly long lines" is after losing three columns to the left
> for "> +" in e-mail quote like the above the right edge of the line
> does not fit on my 92-column terminal, which will never happen if
> you stick to the official "fit 80-column after quoted for a few
> times in e-mail" guideline).
> 
> 		repo_format_commit_message(the_repository, list[i], format,
> 					   &commit_line, &ctx);
> 
> perhaps.

My bad, didn't realise it went over. I'll change settings on my editor
to ensure that I can visually see it going over and do an overall coding
style check.

> > +		fprintf(cover_file, "%s\n", commit_line.buf);
> 
> I somehow would have expected that as we internally prepare "format"
> string given to this function , we ensure it ends with "\n" so we do
> not have to do a fprintf() here.

The value of format is user defined, I'm not doing any pre proccessing
to it apart from stripping the prefix. Would it be much different had I
appended a newline here? I personally think it's fine doing a fprintf
here.

> > +	strbuf_release(&commit_line);
> > +}
> > +
> >  static void make_cover_letter(struct rev_info *rev, int use_separate_file,
> >  			      struct commit *origin,
> >  			      int nr, struct commit **list,
> >  			      const char *description_file,
> >  			      const char *branch_name,
> >  			      int quiet,
> > -			      const struct format_config *cfg)
> > +			      const struct format_config *cfg,
> > +			      const char *format)
> >  {
> >  	const char *from;
> >  	struct shortlog log;
> > @@ -1342,6 +1361,8 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
> >  	struct commit *head = list[0];
> >  	char *to_free = NULL;
> >  
> > +	assert(format);
> > +
> 
> Curious.  We do not assert() for any other pointer arguments.  What
> makes this so special?

It's a leftover of code that should've been stripped. Sorry for this.

> >  	if (!cmit_fmt_is_mail(rev->commit_format))
> >  		die(_("cover letter needs email format"));
> >  
> > @@ -1377,18 +1398,22 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
> >  	free(pp.after_subject);
> >  	strbuf_release(&sb);
> >  
> > -	shortlog_init(&log);
> > -	log.wrap_lines = 1;
> > -	log.wrap = MAIL_DEFAULT_WRAP;
> > -	log.in1 = 2;
> > -	log.in2 = 4;
> > -	log.file = rev->diffopt.file;
> > -	log.groups = SHORTLOG_GROUP_AUTHOR;
> > -	shortlog_finish_setup(&log);
> > -	for (i = 0; i < nr; i++)
> > -		shortlog_add_commit(&log, list[i]);
> 
> It would have been much nicer to first create a short helper
> function generate_shortlog_cover(), move the above plus a call to
> shortlog_output(&log) to that helper function, without doing
> anything else and make it a preliminary patch [1/n].  Then introduce
> the corresponding generate_commit_list_cover() function in patch
> [2/n] (which is what this step is doing), which would have resulted
> in the body of the make_cover_letter() around here a short-and-sweet
> 
> 	if (... we are doing shortlog style ...)
> 		generate_shortlog_cover(...);
> 	else
> 		generate_commit_list_cover(...);
> 
> Just like readers of _this_ function are helped by not having to
> know the details of what happens inside commit-list style cover
> generation, they can concentratre on the flow without having to care
> about details on shortlog side that way.

Yes, it does read nicer. Will do.

> > @@ -1906,6 +1931,7 @@ int cmd_format_patch(int argc,
> >  	int just_numbers = 0;
> >  	int ignore_if_in_upstream = 0;
> >  	int cover_letter = -1;
> > +	char *cover_letter_fmt = NULL;
> >  	int boundary_count = 0;
> >  	int no_binary_diff = 0;
> >  	int zero_commit = 0;
> > @@ -1952,6 +1978,8 @@ int cmd_format_patch(int argc,
> >  			    N_("print patches to standard out")),
> >  		OPT_BOOL(0, "cover-letter", &cover_letter,
> >  			    N_("generate a cover letter")),
> > +		OPT_STRING(0, "cover-letter-format", &cover_letter_fmt, N_("format-spec"),
> > +			    N_("format spec used for the commit list in the cover letter")),
> >  		OPT_BOOL(0, "numbered-files", &just_numbers,
> >  			    N_("use simple number sequence for output file names")),
> >  		OPT_STRING(0, "suffix", &fmt_patch_suffix, N_("sfx"),
> > @@ -2289,13 +2317,14 @@ int cmd_format_patch(int argc,
> >  		/* nothing to do */
> >  		goto done;
> >  	total = list.nr;
> > +
> 
> What is this churn about?

This is introducing "--cover-letter-format"

> >  	if (cover_letter == -1) {
> >  		if (cfg.config_cover_letter == COVER_AUTO)
> > -			cover_letter = (total > 1);
> > +			cover_letter = total > 1;
> 
> What is this churn about?
> 
> >  		else if ((idiff_prev.nr || rdiff_prev) && (total > 1))
> > -			cover_letter = (cfg.config_cover_letter != COVER_OFF);
> > +			cover_letter = cfg.config_cover_letter != COVER_OFF;
> 
> What is this churn about?
> 
> >  		else
> > -			cover_letter = (cfg.config_cover_letter == COVER_ON);
> > +			cover_letter = cfg.config_cover_letter == COVER_ON;
> 
> What is this churn about?
> 
> Please do not distract reviewers by mixing immaterial "style fixes"
> on existing code to a patch whose primary purpose is to introduce
> new code.  A separate "preliminary fix and/or clean-up" patch before
> the main series begins is often a welcome addition, though.

This was not inteded. Is the result of me copy-pasting and then
restoring when I changed my mind on having the format spec be passed
through "--cover-letter".

Sorry for the unnecessary noise, will restore.

> > @@ -2375,12 +2404,16 @@ 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";
> > +
> 
> 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.

> >  	if (cover_letter) {
> >  		if (cfg.thread)
> >  			gen_message_id(&rev, "cover");
> >  		make_cover_letter(&rev, !!output_directory,
> >  				  origin, list.nr, list.items,
> > -				  description_file, branch_name, quiet, &cfg);
> > +				  description_file, branch_name, quiet, &cfg, cover_letter_fmt);
> 
> This line has become overly long.  Please wrap it.

Will do.

> New set of tests should come together with implementation of a new
> feature in the same patch.

Yes, this is something you told me about on the other patch series as
well, sorry for forgetting. Will try to include them right away next
time (along with the necessary docs update).