From: Mirko Faina Date: Thu, 12 Mar 2026 17:18:23 GMT Subject: Re: [PATCH v8 2/4] format-patch: add ability to use alt cover format Message-ID: In-Reply-To: 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. > "--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. > 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 > 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. > 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).