Re: [PATCH v2 2/2] format-patch: add commitListFormat config

[Junio C Hamano <gitster@pobox.com>]

Mirko Faina <mroik@delayed.space> writes:

> On Wed, Feb 25, 2026 at 01:14:13AM +0100, Mirko Faina wrote:
>> > > +		strbuf_release(&tmp);
>> > > +		return 0;
>> > > +	}
>> > >  	if (!strcmp(var, "format.outputdirectory")) {
>> > >  		FREE_AND_NULL(cfg->config_output_directory);
>> > >  		return git_config_string(&cfg->config_output_directory, var, value);
>> > > @@ -2318,6 +2333,13 @@ int cmd_format_patch(int argc,
>> > >  		goto done;
>> > >  	total = list.nr;
>> > >  
>> > > +	if (cover_letter_fmt && (strcmp(cover_letter_fmt, "shortlog") && strncmp(cover_letter_fmt, "log:", 4))) {
>> > 
>> > Overly long line.
>> 
>> Will fix.
>> 
>> > Stepping back a bit, even if we do not validate the format *here*,
>> > shouldn't the code that does use cover_letter_fmt later in the
>> > control flow *already* be checking the validity of the format and
>> > complaining?  If that happens early enough, perhaps we do not want
>> > to have an extra "early check and die" here.
>> 
>> That is true, and initially I did not introduce a check here, but
>> make_cover_letter() is called after the cover letter file has already
>> been created. Failing before format-patch could create a file or print
>> anything on screeen seemed more clean to me, that's the only reason
>> there's a check there.
>
> May I have a confirmation on this. Is it ok to leave the extra check
> here or would you like me to remove it and just let make_cover_letter()
> handle it?

An extra check here would be warranted if that let us avoid
end-users spend extra time and effort before we make a call to
make_cover_letter() and bad arguments cause it to die.  Otherwise,
not.  As the underlying helper function, make_cover_letter() should
be doing its own sanity check on its input anyway, so it is
preferrable not to duplicate the check elsewhere if we do not have
to.