Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
* Using --term-* with bisect breaks skip
@ 2021-04-18 15:14 Trygve Aaberge
  2021-04-19  6:58 ` Bagas Sanjaya
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Trygve Aaberge @ 2021-04-18 15:14 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Using --term-* with bisect breaks skip
  2021-04-18 15:14 Using --term-* with bisect breaks skip Trygve Aaberge
@ 2021-04-19  6:58 ` Bagas Sanjaya
  2021-04-19  8:39   ` Trygve Aaberge
  2021-04-19 18:55 ` Junio C Hamano
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Bagas Sanjaya @ 2021-04-19  6:58 UTC (permalink / raw)
  To: Trygve Aaberge; +Cc: Git Users, Miriam Rubio, Jeff King, Pranit Bauva

On 18/04/21 22.14, Trygve Aaberge wrote:
> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Using --term-* with bisect breaks skip
  2021-04-19  6:58 ` Bagas Sanjaya
@ 2021-04-19  8:39   ` Trygve Aaberge
  2021-04-19 12:50     ` Bagas Sanjaya
  0 siblings, 1 reply; 11+ messages in thread
From: Trygve Aaberge @ 2021-04-19  8:39 UTC (permalink / raw)
  To: Bagas Sanjaya; +Cc: Git Users, Miriam Rubio, Jeff King, Pranit Bauva

On Mon, Apr 19, 2021 at 13:58:20 +0700, Bagas Sanjaya wrote:
> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Using --term-* with bisect breaks skip
  2021-04-19  8:39   ` Trygve Aaberge
@ 2021-04-19 12:50     ` Bagas Sanjaya
  0 siblings, 0 replies; 11+ messages in thread
From: Bagas Sanjaya @ 2021-04-19 12:50 UTC (permalink / raw)
  To: Trygve Aaberge
  Cc: Git Users, Miriam Rubio, Jeff King, Pranit Bauva, Christian Couder

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Using --term-* with bisect breaks skip
  2021-04-18 15:14 Using --term-* with bisect breaks skip Trygve Aaberge
  2021-04-19  6:58 ` Bagas Sanjaya
@ 2021-04-19 18:55 ` Junio C Hamano
  2021-04-19 19:32   ` Trygve Aaberge
  2021-04-20 12:34 ` [PATCH] test: add test for git bisect skip with --term* arguments Bagas Sanjaya
  2021-04-21  4:08 ` [PATCH v2] " Bagas Sanjaya
  3 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2021-04-19 18:55 UTC (permalink / raw)
  To: Trygve Aaberge; +Cc: git, Christian Couder, Pranit Bauva

Trygve Aaberge <trygveaa@gmail.com> writes:

> 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Using --term-* with bisect breaks skip
  2021-04-19 18:55 ` Junio C Hamano
@ 2021-04-19 19:32   ` Trygve Aaberge
  0 siblings, 0 replies; 11+ messages in thread
From: Trygve Aaberge @ 2021-04-19 19:32 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Christian Couder, Pranit Bauva

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] test: add test for git bisect skip with --term* arguments
  2021-04-18 15:14 Using --term-* with bisect breaks skip Trygve Aaberge
  2021-04-19  6:58 ` Bagas Sanjaya
  2021-04-19 18:55 ` Junio C Hamano
@ 2021-04-20 12:34 ` Bagas Sanjaya
  2021-04-20 18:08   ` Junio C Hamano
  2021-04-21  4:08 ` [PATCH v2] " Bagas Sanjaya
  3 siblings, 1 reply; 11+ messages in thread
From: Bagas Sanjaya @ 2021-04-20 12:34 UTC (permalink / raw)
  To: trygveaa; +Cc: git, mirucam, pranit.bauva, christian.couder, Bagas Sanjaya

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] test: add test for git bisect skip with --term* arguments
  2021-04-20 12:34 ` [PATCH] test: add test for git bisect skip with --term* arguments Bagas Sanjaya
@ 2021-04-20 18:08   ` Junio C Hamano
  0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2021-04-20 18:08 UTC (permalink / raw)
  To: Bagas Sanjaya; +Cc: trygveaa, git, mirucam, pranit.bauva, christian.couder

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?

> +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

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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] test: add test for git bisect skip with --term* arguments
  2021-04-18 15:14 Using --term-* with bisect breaks skip Trygve Aaberge
                   ` (2 preceding siblings ...)
  2021-04-20 12:34 ` [PATCH] test: add test for git bisect skip with --term* arguments Bagas Sanjaya
@ 2021-04-21  4:08 ` Bagas Sanjaya
  2021-04-21 17:25   ` Junio C Hamano
  3 siblings, 1 reply; 11+ messages in thread
From: Bagas Sanjaya @ 2021-04-21  4:08 UTC (permalink / raw)
  To: trygveaa
  Cc: git, gitster, larsxschneider, chriscool, Johannes.Schindelin,
	pranit.bauva, tanushreetumane, mirucam, Bagas Sanjaya

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] test: add test for git bisect skip with --term* arguments
  2021-04-21  4:08 ` [PATCH v2] " Bagas Sanjaya
@ 2021-04-21 17:25   ` Junio C Hamano
  2021-04-22  5:16     ` Bagas Sanjaya
  0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2021-04-21 17:25 UTC (permalink / raw)
  To: Bagas Sanjaya
  Cc: trygveaa, git, larsxschneider, chriscool, Johannes.Schindelin,
	pranit.bauva, tanushreetumane, mirucam

Bagas Sanjaya <bagasdotme@gmail.com> writes:

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] test: add test for git bisect skip with --term* arguments
  2021-04-21 17:25   ` Junio C Hamano
@ 2021-04-22  5:16     ` Bagas Sanjaya
  0 siblings, 0 replies; 11+ messages in thread
From: Bagas Sanjaya @ 2021-04-22  5:16 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: trygveaa, git, larsxschneider, chriscool, Johannes.Schindelin,
	pranit.bauva, tanushreetumane, mirucam

On 22/04/21 00.25, Junio C Hamano wrote:
> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-18 15:14 Using --term-* with bisect breaks skip Trygve Aaberge
2021-04-19  6:58 ` Bagas Sanjaya
2021-04-19  8:39   ` Trygve Aaberge
2021-04-19 12:50     ` Bagas Sanjaya
2021-04-19 18:55 ` Junio C Hamano
2021-04-19 19:32   ` Trygve Aaberge
2021-04-20 12:34 ` [PATCH] test: add test for git bisect skip with --term* arguments Bagas Sanjaya
2021-04-20 18:08   ` Junio C Hamano
2021-04-21  4:08 ` [PATCH v2] " Bagas Sanjaya
2021-04-21 17:25   ` Junio C Hamano
2021-04-22  5:16     ` Bagas Sanjaya

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git