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

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

[Ramsay Jones]

Commit e4c7b33747 ("bisect--helper: reimplement `bisect_skip` shell
function in C", 2021-02-03), as part of the shell-to-C conversion,
forgot to read the 'terms' file (.git/BISECT_TERMS) during the new
'bisect skip' command implementation. As a result, the 'bisect skip'
command will use the default 'bad'/'good' terms. If the bisection
terms have been set to non-default values (for example by the
'bisect start' command), then the 'bisect skip' command will fail.

In order to correct this problem, we insert a call to the get_terms()
function, which reads the non-default terms from that file (if set),
in the '--bisect-skip' command implementation of 'bisect--helper'.

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---

Hi Junio,

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].

[I created the patch on top of e4c7b33747 so that it would, hopefully,
easily backport to the relevant 'maint' branches, should you feel the
need. ;-) ]

At this point, I would normally have looked to see if there were other
examples of forgetting to call 'get_terms()' (which seems possible).
However, I am a bit busy, so I will have to add that to my TODO list ...
:(

ATB,
Ramsay Jones

[1] https://lore.kernel.org/git/20210421040808.14185-1-bagasdotme@gmail.com/

 builtin/bisect--helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 7ad9b4d55b..49c07f0710 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -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);
 		res = bisect_skip(&terms, argv, argc);
 		break;
 	default:
-- 
2.31.0

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

[Bagas Sanjaya]

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].

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].

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

[1]: https://lore.kernel.org/git/20210425080508.154159-1-bagasdotme@gmail.com/
-- 
An old man doll... just what I always wanted! - Clara

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

[Christian Couder]

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

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

[Bagas Sanjaya]

On 26/04/21 14.06, Christian Couder wrote:
> 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.
> 

OK. Review ping

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

OK, thanks for the explanation.

-- 
An old man doll... just what I always wanted! - Clara