Re: [PATCH] bisect--helper: use BISECT_TERMS in 'bisect skip' command

From: Christian Couder <christian.couder@gmail.com>
Date: Apr 26, 2021, 07:06 UTC
Message-ID: <CAP8UFD3hOqeF04Xdjn-o2FShuTPQ9ZUEVCWnhfzCqV6MtbvU4g@mail.gmail.com>
In-Reply-To: <473a11db-cbb5-58b9-b05d-cab2072d5d2f@gmail.com>

On Sun, Apr 25, 2021 at 10:18 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:

Show 8 quoted lines

>
> On 25/04/21 07.18, Ramsay Jones wrote:
> >
> > This patch was created directly on top of commit e4c7b33747 and tested
> > with the test from Bagas Sanjaya [1] (ie the second version of the
> > stand-alone test file t6031-*.sh, rather than the newer patch that
> > adds the test to t6030-*.sh). I applied this patch to the current
> > master branch (@311531c9de55) and it also passed the test in [1].

Thanks Ramsay for your patch, it looks good to me!

> I have just sent v2 of t6030 version of my test [1]. Please test
> this patch against that v2 test. And if the test passes (breakage vanished),
> please update the test by do instructions in the FIXME comment lines of [1].

Thanks Bagas for your test! I will take a look at it soon.

My opinion is that it would be best if both patches (Ramsay's and Bagas') were in the same patch series or even perhaps in the same commit. If you prefer separate patches, maybe the first one could be Ramsay's, and the second one Bagas' where indeed the instructions to replace test_expect_failure with test_expect_success have been followed.

Show 11 quoted lines

> > @@ -1129,6 +1129,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> >               break;
> >       case BISECT_SKIP:
> >               set_terms(&terms, "bad", "good");
> > +             get_terms(&terms);
>
> I'm not fluent in C, but I read these lines above as "ok, we set terms from
> &terms, bad and good as fallback in case the reference is empty; then we get these
> terms from the reference". Wouldn't it makes sense if we can say "get the terms
> from &terms" first, then "set the terms from the reference, if empty use bad
> and good as fallback"?

It might not be the best API for this (or the set_terms() and get_terms() function could perhaps have better names), but anyway the current situation is that set_terms(&terms, "bad", "good") is setting the current terms to "bad"/"good" which is the default, and then get_terms(&terms) is reading the terms stored in the BISECT_TERMS file and using that to set the current terms. Also if the BISECT_TERMS file doesn't exist, then get_terms(&terms) is doing nothing. So it seems to me that Ramsay's patch is doing the right thing.

If get_terms(&terms) was used before set_terms(&terms, "bad", "good"), then the current terms would always be "bad"/"good" even if the BISECT_TERMS file contains valid terms different from "bad"/"good".