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

[Christian Couder <christian.couder@gmail.com>]

On Sun, Apr 25, 2021 at 10:18 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> 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.

> > @@ -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".