# Using --term-* with bisect breaks skip 11 messages from 2021-04-18 to 2021-04-22. Participants: Trygve Aaberge, Bagas Sanjaya, Junio C Hamano. Thread: https://gitlist.dev/t/55512 ## Trygve Aaberge, 2021-04-18 15:14 Subject: Using --term-* with bisect breaks skip Message-ID: <20210418151459.GC10839@aaberge.net> URL: https://gitlist.dev/e/20210418151459.GC10839%40aaberge.net ``` 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, 2021-04-19 06:58 Subject: Re: Using --term-* with bisect breaks skip Message-ID: URL: https://gitlist.dev/e/c29ef929-4744-a498-540d-1b7df0afa0a0%40gmail.com In-Reply-To: <20210418151459.GC10839@aaberge.net> ``` 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 ``` ## Trygve Aaberge, 2021-04-19 08:39 Subject: Re: Using --term-* with bisect breaks skip Message-ID: <20210419083926.GD10839@aaberge.net> URL: https://gitlist.dev/e/20210419083926.GD10839%40aaberge.net In-Reply-To: ``` 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 ``` ## Bagas Sanjaya, 2021-04-19 12:50 Subject: Re: Using --term-* with bisect breaks skip Message-ID: URL: https://gitlist.dev/e/f5368d12-81d9-8591-53f3-44e2e31a89dd%40gmail.com In-Reply-To: <20210419083926.GD10839@aaberge.net> ``` 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, 2021-04-19 18:55 Subject: Re: Using --term-* with bisect breaks skip Message-ID: URL: https://gitlist.dev/e/xmqqlf9e9k9d.fsf%40gitster.g In-Reply-To: <20210418151459.GC10839@aaberge.net> ``` Trygve Aaberge 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 ``` ## Trygve Aaberge, 2021-04-19 19:32 Subject: Re: Using --term-* with bisect breaks skip Message-ID: <20210419193203.GE10839@aaberge.net> URL: https://gitlist.dev/e/20210419193203.GE10839%40aaberge.net In-Reply-To: ``` 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 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 Mentored-by: Christian Couder Mentored-by: Johannes Schindelin Signed-off-by: Pranit Bauva Signed-off-by: Tanushree Tumane Signed-off-by: Miriam Rubio Signed-off-by: Junio C Hamano -- Trygve Aaberge ``` ## Bagas Sanjaya, 2021-04-20 12:34 Subject: [PATCH] test: add test for git bisect skip with --term* arguments Message-ID: <20210420123435.35936-1-bagasdotme@gmail.com> URL: https://gitlist.dev/e/20210420123435.35936-1-bagasdotme%40gmail.com In-Reply-To: <20210418151459.GC10839@aaberge.net> ``` 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 --- 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 > 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, 2021-04-20 18:08 Subject: Re: [PATCH] test: add test for git bisect skip with --term* arguments Message-ID: URL: https://gitlist.dev/e/xmqqa6ps7rrc.fsf%40gitster.g In-Reply-To: <20210420123435.35936-1-bagasdotme@gmail.com> ``` Bagas Sanjaya 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 +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. ``` ## Bagas Sanjaya, 2021-04-21 04:08 Subject: [PATCH v2] test: add test for git bisect skip with --term* arguments Message-ID: <20210421040808.14185-1-bagasdotme@gmail.com> URL: https://gitlist.dev/e/20210421040808.14185-1-bagasdotme%40gmail.com In-Reply-To: <20210418151459.GC10839@aaberge.net> ``` 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 --- Changes from v1: * style changes requested by Junio * rename test script and edit test description to be more descriptive * remove exec >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, 2021-04-21 17:25 Subject: Re: [PATCH v2] test: add test for git bisect skip with --term* arguments Message-ID: URL: https://gitlist.dev/e/xmqqa6przh08.fsf%40gitster.g In-Reply-To: <20210421040808.14185-1-bagasdotme@gmail.com> ``` Bagas Sanjaya 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. ``` ## Bagas Sanjaya, 2021-04-22 05:16 Subject: Re: [PATCH v2] test: add test for git bisect skip with --term* arguments Message-ID: <3bbc9350-60cd-9fb2-117a-add3dc5cf77e@gmail.com> URL: https://gitlist.dev/e/3bbc9350-60cd-9fb2-117a-add3dc5cf77e%40gmail.com In-Reply-To: ``` 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 ```