threads / discuss / 55512

Using --term-* with bisect breaks skip

Subject: Using --term-* with bisect breaks skip

## tl;dr

11 messages between Apr 18, 2021 and Apr 22, 2021.

replies: 10people: 3as markdown or json

Trygve Aaberge· Apr 18, 2021, 15:14 UTC · lore
What did you do before the bug happened? (Steps to reproduce your issue)
1. git bisect start --term-new=fixed --term-old=unfixed master HEAD~10
2. git bisect skip

What did you expect to happen? (Expected behavior) Git should mark the commit as skipped and change HEAD to a different commit.

What happened instead? (Actual behavior) The commit was marked as skipped, but HEAD was not changed.

What's different between what you expected and what actually happened? After running bisect skip, HEAD was still at the same commit as before, instead of having changed to a new that I can test. The usual output about steps left to test and the new commit was also missing, skip did not output anything.

Anything else you want to add:
- If I don't provide any --term-* options, skip works as expected.
- The revisions provided in the reproduction steps doesn't matter, they're
  just an example.
- I tried running from the next branch, and it happened there too.

[System Info] git version: git version 2.31.1 cpu: x86_64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh uname: Linux 5.10.29-1-lts #1 SMP Sat, 10 Apr 2021 14:40:41 +0000 x86_64 compiler info: gnuc: 10.2 libc info: glibc: 2.33 $SHELL (typically, interactive shell): /usr/bin/zsh

[Enabled Hooks]
-- 
Trygve Aaberge
Bagas Sanjaya· Apr 19, 2021, 06:58 UTC · re: Trygve Aaberge · lore

Re: Using --term-* with bisect breaks skip

On 18/04/21 22.14, Trygve Aaberge wrote:
Show 5 quoted lines
> What's different between what you expected and what actually happened?
> After running bisect skip, HEAD was still at the same commit as before,
> instead of having changed to a new that I can test. The usual output about
> steps left to test and the new commit was also missing, skip did not output
> anything.
I can reproduce the issue, thanks.
> Anything else you want to add:
> - If I don't provide any --term-* options, skip works as expected.
The issue still persists without --term-* options on my computer.
To reproduce in git.git:
   1. git bisect start
   2. git bisect new v2.31.0
   3. git bisect old v2.30.0
   4. git bisect skip

[CC] I'd CCed Miriam and Pranit (who submitted git-bisect in C) and Jeff for the issue.

-- 
An old man doll... just what I always wanted! - Clara
Trygve Aaberge· Apr 19, 2021, 08:39 UTC · re: Bagas Sanjaya · lore

Re: Using --term-* with bisect breaks skip

On Mon, Apr 19, 2021 at 13:58:20 +0700, Bagas Sanjaya wrote:
Show 7 quoted lines
> The issue still persists without --term-* options on my computer.
> 
> To reproduce in git.git:
>   1. git bisect start
>   2. git bisect new v2.31.0
>   3. git bisect old v2.30.0
>   4. git bisect skip

Right, the issue persists with those commands here too. So it apparently happens as long as you don't use good/bad, but with good/bad it works as expected.

-- 
Trygve Aaberge
Bagas Sanjaya· Apr 19, 2021, 12:50 UTC · re: Trygve Aaberge · lore

Re: Using --term-* with bisect breaks skip

On 19/04/21 15.39, Trygve Aaberge wrote:
> Right, the issue persists with those commands here too. So it apparently
> happens as long as you don't use good/bad, but with good/bad it works as
> expected.
>

Seems interesting, git bisect skip only works on good/bad terms but not on other terms (and not even on old/new)

I saw similar test on t/t6030-bisect-porcelain.sh (hint: find test that test for skip) , but I don't know whether that test will pass. I CC'ed Christian Couder for now.

-- 
An old man doll... just what I always wanted! - Clara
Junio C Hamano· Apr 19, 2021, 18:55 UTC · re: Trygve Aaberge · lore

Re: Using --term-* with bisect breaks skip

Trygve Aaberge <trygveaa@gmail.com> writes:
Show 25 quoted lines
> What did you do before the bug happened? (Steps to reproduce your issue)
> 1. git bisect start --term-new=fixed --term-old=unfixed master HEAD~10
> 2. git bisect skip
>
> What did you expect to happen? (Expected behavior)
> Git should mark the commit as skipped and change HEAD to a different commit.
>
> What happened instead? (Actual behavior)
> The commit was marked as skipped, but HEAD was not changed.
>
> What's different between what you expected and what actually happened?
> After running bisect skip, HEAD was still at the same commit as before,
> instead of having changed to a new that I can test. The usual output about
> steps left to test and the new commit was also missing, skip did not output
> anything.
>
> Anything else you want to add:
> - If I don't provide any --term-* options, skip works as expected.
> - The revisions provided in the reproduction steps doesn't matter, they're
>   just an example.
> - I tried running from the next branch, and it happened there too.
>
> [System Info]
> git version:
> git version 2.31.1
Thanks for a report.

I suspect that this is a fairly recent regression. With Git 2.30.2, "bisect skip" does flip HEAD and mark the commit to as untestable.

Can you "bisect" the problem? There aren't that many commits that touched bisection code during the period.

$ git log --format='%aN %s' --no-merges v2.30.2..v2.31.1 -- \
  bisect.c builtin/bisect--helper.c git-bisect.sh
Jeff King bisect: peel annotated tags to commits
René Scharfe use CALLOC_ARRAY
Johannes Sixt replace "parameters" by "arguments" in error messages
Pranit Bauva bisect--helper: retire `--check-and-set-terms` subcommand
Pranit Bauva bisect--helper: reimplement `bisect_skip` shell function in C
Pranit Bauva bisect--helper: retire `--bisect-auto-next` subcommand
Pranit Bauva bisect--helper: use `res` instead of return in BISECT_RESET case option
Pranit Bauva bisect--helper: retire `--bisect-write` subcommand
Pranit Bauva bisect--helper: reimplement `bisect_replay` shell function in C
Pranit Bauva bisect--helper: reimplement `bisect_log` shell function in C
Martin Ågren hash-lookup: rename from sha1-lookup
Trygve Aaberge· Apr 19, 2021, 19:32 UTC · re: Junio C Hamano · lore

Re: Using --term-* with bisect breaks skip

On Mon, Apr 19, 2021 at 11:55:42 -0700, Junio C Hamano wrote:
> Can you "bisect" the problem?  There aren't that many commits that
> touched bisection code during the period.
Yes, I ran it now. The commit introducing the regression is:
commit e4c7b33747dccc6600bc528b51e91c11b474eb04
Author: Pranit Bauva <pranit.bauva@gmail.com>
Date:   Wed Feb 3 22:54:37 2021 +0100
    bisect--helper: reimplement `bisect_skip` shell function in C
    
    Reimplement the `bisect_skip()` shell function in C and also add
    `bisect-skip` subcommand to `git bisect--helper` to call it from
    git-bisect.sh
    
    Using `--bisect-skip` subcommand is a temporary measure to port shell
    function to C so as to use the existing test suite.
    
    Mentored-by: Lars Schneider <larsxschneider@gmail.com>
    Mentored-by: Christian Couder <chriscool@tuxfamily.org>
    Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
    Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
    Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
    Signed-off-by: Miriam Rubio <mirucam@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
-- 
Trygve Aaberge
Bagas Sanjaya· Apr 20, 2021, 12:34 UTC · re: Trygve Aaberge · lore

[PATCH] test: add test for git bisect skip with --term* arguments

The current git bisect breakage happens when skipping in the middle of bisect session which is started with --term-new and --term-old arguments. This test expect that HEAD changes after skipping, in other words HEAD before and after skipping must be different.

Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
---
NOTE: this patch is not intended for integrating into git.git, but
rather this patch is written to demonstrate this breakage. I hope that
the test can be added to t6030-bisect-porcelain.sh, and make this patch
redundant.  
 t/t6031-bisect-skip.sh | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100755 t/t6031-bisect-skip.sh
diff --git a/t/t6031-bisect-skip.sh b/t/t6031-bisect-skip.sh
new file mode 100755
index 0000000000..0dfc6f0928
--- /dev/null
+++ b/t/t6031-bisect-skip.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+#
+# Copyright (c) 2021 Bagas Sanjaya
+#
+
+test_description='Tests git bisect --skip'
+
+exec </dev/null
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+# hash variables
+HASH_SKIPPED_FROM=
+HASH_SKIPPED_TO=
+
+# initialize testing repo
+init() {
+	for i in `seq 1 20`; do
+		echo $i >> test &&
+		git add test && git commit -m $i
+	done
+}
+
+init
+
+test_expect_success 'test moving HEAD when skip bisecting' '
+	git bisect start --term-new=ok --term-old=whoops HEAD HEAD~9 &&
+	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
+'

-- 
2.25.1
Junio C Hamano· Apr 20, 2021, 18:08 UTC · re: Bagas Sanjaya · lore

Re: [PATCH] test: add test for git bisect skip with --term* arguments

Bagas Sanjaya <bagasdotme@gmail.com> writes:
> NOTE: this patch is not intended for integrating into git.git, but
> rather this patch is written to demonstrate this breakage. I hope that
> the test can be added to t6030-bisect-porcelain.sh, and make this patch
> redundant.

Then perhaps add it to where you think it belongs to before you post?

In any case, let's critique the patch a bit for future reference, with a hope that you'd be contributing more ;-).

>  t/t6031-bisect-skip.sh | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100755 t/t6031-bisect-skip.sh
Good; many new people forget to make these executable.
> +test_description='Tests git bisect --skip'
> +
> +exec </dev/null
Why?
Show 11 quoted lines
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +. ./test-lib.sh
> +
> +# hash variables
> +HASH_SKIPPED_FROM=
> +HASH_SKIPPED_TO=
> +
> +# initialize testing repo
> +init() {
Style.
> +	for i in `seq 1 20`; do
Use $(test_seq ...)
> +		echo $i >> test &&
Style
Show 5 quoted lines
> +		git add test && git commit -m $i
> +	done
> +}
> +
> +init

We do that in the first "test_expect_success setup '... code ...'" block to catch breakages in the initialization.

Show 7 quoted lines
> +test_expect_success 'test moving HEAD when skip bisecting' '
> +	git bisect start --term-new=ok --term-old=whoops HEAD HEAD~9 &&
> +	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
> +'
Missing "test_done".
Thanks.
Bagas Sanjaya· Apr 21, 2021, 04:08 UTC · re: Trygve Aaberge · lore

[PATCH v2] test: add test for git bisect skip with --term* arguments

Trygve Aaberge reported the current git bisect breakage on [1]. After starting bisection with --term-new and --term-old arguments to git bisect start, skipping with git bisect skip does not change HEAD as expected.

Let's add the test to catch this breakage.
[1]: https://lore.kernel.org/git/20210418151459.GC10839@aaberge.net/
Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
---
Changes from v1:
  * style changes requested by Junio
  * rename test script and edit test description to be more descriptive
  * remove exec </dev/null (I don't know what it means)
  * repo initialization is now on test_expect_success block (as
    requested by Junio)
  
 t/t6031-bisect-skip-with-term.sh | 36 ++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100755 t/t6031-bisect-skip-with-term.sh
diff --git a/t/t6031-bisect-skip-with-term.sh b/t/t6031-bisect-skip-with-term.sh
new file mode 100755
index 0000000000..f1e1c4c1a2
--- /dev/null
+++ b/t/t6031-bisect-skip-with-term.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+#
+# Copyright (c) 2021 Bagas Sanjaya
+#
+
+test_description='Tests skipping bisect which the bisection is started with --term* arguments'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+# hash variables to be used in bisection test
+HASH_SKIPPED_FROM=
+HASH_SKIPPED_TO=
+
+# test repo initialization
+test_expect_success 'initialize testing repo with 20 commits' '
+	for i in $(test_seq 1 20); do
+		echo $i >>test &&
+		git add test && git commit -m "commit $i" &&
+		test_tick
+	done
+'
+
+# actual bisection test
+test_expect_success 'test moving HEAD when skip bisecting' '
+	git bisect start --term-new=ok --term-old=whoops HEAD HEAD~9 &&
+	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_done

base-commit: b0c09ab8796fb736efa432b8e817334f3e5ee75a

-- 
2.25.1
Junio C Hamano· Apr 21, 2021, 17:25 UTC · re: Bagas Sanjaya · lore

Re: [PATCH v2] test: add test for git bisect skip with --term* arguments

Bagas Sanjaya <bagasdotme@gmail.com> writes:
Show 6 quoted lines
> Trygve Aaberge reported the current git bisect breakage on [1].
> After starting bisection with --term-new and --term-old arguments to git
> bisect start, skipping with git bisect skip does not change HEAD as
> expected.
>
> Let's add the test to catch this breakage.

I expected, as you said in your first version, that it would be added as part of an existing test script for "git bisect". And I suspect that you can reuse the history the existing tests in the script already use, so you won't have to add the first "initialize" piece. If the tested sequence should work but does not yet work due to lack of a fix to a known bug, the test should be marked as test_expect_failure instead of test_expect_success.

>   * style changes requested by Junio

Heh, I didn't request anything. I merely pointed out the parts that violate the style guide, so if anything, the guide requested you and I was just a messenger ;-).

Thanks.
Bagas Sanjaya· Apr 22, 2021, 05:16 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] test: add test for git bisect skip with --term* arguments

On 22/04/21 00.25, Junio C Hamano wrote:
Show 7 quoted lines
> I expected, as you said in your first version, that it would be
> added as part of an existing test script for "git bisect".  And I
> suspect that you can reuse the history the existing tests in the
> script already use, so you won't have to add the first "initialize"
> piece.  If the tested sequence should work but does not yet work due
> to lack of a fix to a known bug, the test should be marked as
> test_expect_failure instead of test_expect_success.

So will this patch be queued? Or should I send the patch adding this test to t6030 instead of separate test script here?

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

← back to recent threads