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

From: Mirko Faina <mroik@delayed.space>
Date: Mar 12, 2026, 17:18 UTC
Message-ID: <abLw6vUUh36zFK4n@exploit2>
In-Reply-To: <xmqq5x71gfci.fsf@gitster.g>

On Thu, Mar 12, 2026 at 09:52:29AM -0700, Junio C Hamano wrote:

> The example does not use a format spec 'prefixed with "log:"',
> though?

Yes, I fixed the example but forgot fix the paragraph when rewording. Will fix

> The second sentence reads funny.  The option is available whether
> the user wants to use it or not.  I'd suggest dropping the sentence,
> without which the paragraph reads just fine.

Will do

> OK, so we are not requiring "log:"?  This robs extensibility from

Like I said above, we won't require "log:" as per the discussion with Phillip.

>                                      This robs extensibility from
> future developers to introduce something other than "shortlog", no?
> If the version of Git in 'next' supports "longlog" and user gives

Not really, anyone can introduce new formats, it's just an additional if statement.

Show 5 quoted lines

> "--cover-letter-format=longlog" to their version that does not yet
> support it, it would be mistaken by the version of the code here as
> a "log:longlog" without any placeholder that shows a fixed string
> "longlog" for each commit in the series?  We'd rather want such an
> input to cause failure, no?

Isn't that the same for any feature that is in next but not merged in master yet? I wouldn't expect subcommands of history not yet merged in master to work either if I'm using a version built from master. This is an issue with the user and I don't think it's grounds for any issue.

Show 6 quoted lines

> s/If defined, d\(efaults.*variable\)\./D\1, if defined./ would avoid
> "if I define --cover-letter-format, why does it default to a
> configuration?  do you mean 'if not given'?"
> 
> So, "If defined," -> "If not given" would be another possible
> improvement.  I think I like it better, actually.

Will do

Show 13 quoted lines

> Hmph, an alternative that may make it easier to use is to make the
> command line option _imply_ "--cover-letter", so that the user does
> not have to give two similar looking command line options.
> 
>     git format-patch --cover-letter --cover-letter-format=...
> 
> Of course, the presence of the configuration variable should not
> imply generation of a cover letter. I.e.
> 
>     git -c format.commitListInCoverLetterFormat=shortlog \
> 	    format-patch -1 HEAD
> 
> should not imply --cover-letter.

Yes, that'd be a nice behaviour to have, it is indeed obvious that I want a cover letter if I'm specifying a format from command line.

Show 17 quoted lines

> Many issues.
> 
> In modern tests (written within the past 10 years), we try not to
> execute things outside text_expect_foo blocks.  The golden output
> to compare with is customary called 'expect' (not 'expected').  A
> redirection operator ">", "<<", etc. has a single SP before but no
> SP after it, when there is no parameter interpolation happens in a
> HERE document, quote the "EOF" marker to show the intention of the
> author of the test that no parameter interpolation is expected.
> 
> i.e.,
> 
>     cat >expect <<\-EOF
> 	...
>     EOF
> 
> and do so inside a set-up test_expect_success block.

Will fix

> Why do we have so many blank lines?  Are the number of blank lines
> significant?  Such a hidden and hard to count dependency would hurt
> maintainability of this test script.

In the beginning I thought about stripping the empty lines, but doing so would not ensure that those two lines were next to each other. grep matches line by line so I couldn't ensure that they'd be next to each other like that.

If I were to strip the empty lines the test would be not better (imo) than the previous series (where it was flagged down).