From: Dragan Simic Date: Wed, 17 Apr 2024 07:11:00 GMT Subject: Re: [PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014 Message-ID: <9a6a9cb1d9dd07bbbbc47616c510779a@manjaro.org> In-Reply-To: On 2024-04-17 08:40, Eric Sunshine wrote: > On Wed, Apr 17, 2024 at 2:34 AM Kristoffer Haugsbakk > 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. Two votes, so the comments goes away. :) >> > +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. Yes, I also wasn't super happy with this new test, as I already noted in one of my replies, but improving this and the other similar tests is most probably something best left for a follow-up series.