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

[PATCH v2] 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'.

Also, add a test[1] to protect against potential future regression.

[1] https://lore.kernel.org/git/xmqqim45h585.fsf@gitster.g/T/#m207791568054b0f8cf1a3942878ea36293273c7d

Reported-by: Trygve Aaberge <trygveaa@gmail.com>
Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---

Hi Junio,

This v2 patch was also created directly on top of commit e4c7b33747 and,
once again, applied to current 'master' (@311531c9de). (I was a little
surprised that it applied clean, given the changes to t6030-*.sh in the
meantime! ;).

I didn't bother with the range-diff[*], since it wasn't very illuminating
(just the addition of a few trailing lines in the commit message and the
addition of the test).

I don't know if I did the 'Co-authorship' bit correctly; I nearly used an
'Co-authored-by:' rather than an 'Signed-off-by:' for Bagas, but I seem to
remember that caused some push-back recently. Anyway, if I messed that up,
please just modify however you see fit.

ATB,
Ramsay Jones

[*] Once I got it to work, that is; for some reason, I was trying smaller
creation-factor values rather than larger! ;-)

 builtin/bisect--helper.c    |  1 +
 t/t6030-bisect-porcelain.sh | 11 +++++++++++
 2 files changed, 12 insertions(+)

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:
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index aa226381be..b707296ae2 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -919,6 +919,17 @@ test_expect_success 'bisect start takes options and revs in any order' '
 	test_cmp expected actual
 '
 
+# Bisect is started with --term-new and --term-old arguments,
+# then skip. The HEAD should be changed.
+test_expect_success 'bisect skip works with --term*' '
+	git bisect reset &&
+	git bisect start --term-new=fixed --term-old=unfixed HEAD $HASH1 &&
+	hash_skipped_from=$(git rev-parse --verify HEAD) &&
+	git bisect skip &&
+	hash_skipped_to=$(git rev-parse --verify HEAD) &&
+	test "$hash_skipped_from" != "$hash_skipped_to"
+'
+
 test_expect_success 'git bisect reset cleans bisection state properly' '
 	git bisect reset &&
 	git bisect start &&
-- 
2.31.0

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

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> This v2 patch was also created directly on top of commit e4c7b33747 and,
> once again, applied to current 'master' (@311531c9de). (I was a little
> surprised that it applied clean, given the changes to t6030-*.sh in the
> meantime! ;).
>
> I didn't bother with the range-diff[*], since it wasn't very illuminating
> (just the addition of a few trailing lines in the commit message and the
> addition of the test).

Thanks.  Let's hear from Bagas that this version looks good and
merge it down.

> I don't know if I did the 'Co-authorship' bit correctly; I nearly used an
> 'Co-authored-by:' rather than an 'Signed-off-by:' for Bagas, but I seem to
> remember that caused some push-back recently. Anyway, if I messed that up,
> please just modify however you see fit.

I am not in principle opposed to the idea of co-authored-by; the
case you remember was where we would have used Helped-by (i.e.  a
reviewer offers "writing it this way is cleaner" suggestions on the
list and then gets credited on the next version), and it wasn't
clear to me if the reviewer consented to be a co-author of that
change.  If the party who were named as a co-author responded that
it is OK, I would have taken it.

> [*] Once I got it to work, that is; for some reason, I was trying smaller
> creation-factor values rather than larger! ;-)
>
>  builtin/bisect--helper.c    |  1 +
>  t/t6030-bisect-porcelain.sh | 11 +++++++++++
>  2 files changed, 12 insertions(+)
>
> 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:
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index aa226381be..b707296ae2 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -919,6 +919,17 @@ test_expect_success 'bisect start takes options and revs in any order' '
>  	test_cmp expected actual
>  '
>  
> +# Bisect is started with --term-new and --term-old arguments,
> +# then skip. The HEAD should be changed.
> +test_expect_success 'bisect skip works with --term*' '
> +	git bisect reset &&
> +	git bisect start --term-new=fixed --term-old=unfixed HEAD $HASH1 &&
> +	hash_skipped_from=$(git rev-parse --verify HEAD) &&
> +	git bisect skip &&
> +	hash_skipped_to=$(git rev-parse --verify HEAD) &&
> +	test "$hash_skipped_from" != "$hash_skipped_to"
> +'
> +
>  test_expect_success 'git bisect reset cleans bisection state properly' '
>  	git bisect reset &&
>  	git bisect start &&

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

[Christian Couder]

On Fri, Apr 30, 2021 at 2:56 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
> > This v2 patch was also created directly on top of commit e4c7b33747 and,
> > once again, applied to current 'master' (@311531c9de). (I was a little
> > surprised that it applied clean, given the changes to t6030-*.sh in the
> > meantime! ;).
> >
> > I didn't bother with the range-diff[*], since it wasn't very illuminating
> > (just the addition of a few trailing lines in the commit message and the
> > addition of the test).
>
> Thanks.  Let's hear from Bagas that this version looks good and
> merge it down.

This version looks good to me.

Thanks Ramsay and Bagas!