Re: [PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014

> On Wed, Apr 17, 2024 at 2:34 AM Kristoffer Haugsbakk
> <code@khaugsbakk.name> wrote:
>> On Wed, Apr 17, 2024, at 05:32, Dragan Simic wrote:
>> > Fix a bug that allows --rfc and -k options to be specified together when
>> > executing "git format-patch".  This bug was introduced back in the commit
>> > e0d7db7423a9 ("format-patch: --rfc honors what --subject-prefix sets"),
>> > about eight months ago, but it has remained undetected so far, presumably
>> > because of no associated test coverage.
>> 
>> I don’t think speculating on why the bug is still there improves the
>> commit message.
>> 
>> This paragraph could perhaps be rewritten to
>> 
>>   “ Fix a bug from e0d7db7423a (format-patch: --rfc honors what
>>     --subject-prefix sets, 2023-08-30) that allows --rfc and -k 
>> options
>>     to be specified together when executing "git format-patch".
>> 
>> The extra sentence in the original doesn’t really explain anything 
>> more
>> about the commit. Except the “eight months ago”, but here I’ve used 
>> the
>> “reference” style (not the Linux-style) which contains the date.
>> > @@ -2050,8 +2050,11 @@ int cmd_format_patch(int argc, const char
>> > -     if (rfc)
>> > +     /* Also mark the subject prefix as modified, for later checks */
>> 
>> I think the code speaks for itself in this case.
> 
> Apparently we're thinking along the same lines since we both said
> essentially the same things in our reviews.

>> > +test_expect_success '--rfc and -k cannot be used together' '
>> > +     test_must_fail git format-patch -1 --stdout --rfc -k >patch
>> 
>> I don’t understand why you redirect to `patch` if you only check the
>> exit code. (I don’t expect any stdout since it will fail.)
> 
> I had the same question but left it unwritten since I noticed that
> this new test is modelled after the test immediately following it in
> the script, and the existing test also redirects to "patch"
> unnecessarily. So, if it's done this way for consistency with existing
> tests, I don't mind letting it slide.