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

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

[Bagas Sanjaya]

Trygve Aaberge reported [1] git bisect breakage when the bisection
is started with --term* arguments (--term-new and --term-old). In that
case, skipping with `git bisect skip` should cause HEAD to be changed,
but actually the HEAD stayed same after skipping.

Let's add the test to catch the breakage. Because there isn't any fixes
yet, mark the test as test_expect_failure.

[1]: https://lore.kernel.org/git/20210418151459.GC10839@aaberge.net/

Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
---
 Changes from v1 [1]:

   * Move the test to the the end of test script (test 74).
     v1 placed the test as test 26, and when I run the script, there
     are 9 more failed tests rather than just one (because such tests
     depended on previous ones). Now the test is placed together with
     similar tests (git bisect with --terms*).
   * Begin the test with git bisect reset, as with other nearby tests.

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

 t/t6030-bisect-porcelain.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 32bb66e1ed..b1b847ebbc 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -922,6 +922,19 @@ 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.
+# FIXME: Mark this test as test_expect_failure. Remove the FIXME and
+# mark as test_expect_success when this test successes (fixed bug).
+test_expect_failure '"bisect skip: bisection is started 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 &&

base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a
-- 
2.25.1

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

[Christian Couder]

On Sun, Apr 25, 2021 at 10:06 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> Trygve Aaberge reported [1] git bisect breakage when the bisection
> is started with --term* arguments (--term-new and --term-old). In that
> case, skipping with `git bisect skip` should cause HEAD to be changed,
> but actually the HEAD stayed same after skipping.
>
> Let's add the test to catch the breakage. Because there isn't any fixes
> yet, mark the test as test_expect_failure.
>
> [1]: https://lore.kernel.org/git/20210418151459.GC10839@aaberge.net/

We prefer when people can give a proper explanation of what happens in
the commit message, instead of providing a link to such an
explanation. Here it's not so important if this patch (which only adds
tests) is merged along with (or if it's squashed into) the fix which
hopefully contains an explanation. Maybe if both patches are sent in
the same patch series the commit message might want to only say
something like "A previous commit fixed a `git bisect skip` breakage
when the bisection is started with --term* arguments, let's add a test
for that breakage."

A commit message trailer might be the right way to credit Trygve
Aaberge (also please don't forget to put the reporter in Cc like I am
doing), for example like:

Reported-by: Trygve Aaberge <trygveaa@gmail.com>

> Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
> ---
>  Changes from v1 [1]:
>
>    * Move the test to the the end of test script (test 74).
>      v1 placed the test as test 26, and when I run the script, there
>      are 9 more failed tests rather than just one (because such tests
>      depended on previous ones). Now the test is placed together with
>      similar tests (git bisect with --terms*).
>    * Begin the test with git bisect reset, as with other nearby tests.

Nice!

>  [1]: https://lore.kernel.org/git/20210423070308.85275-1-bagasdotme@gmail.com/
>
>  t/t6030-bisect-porcelain.sh | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index 32bb66e1ed..b1b847ebbc 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -922,6 +922,19 @@ 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.
> +# FIXME: Mark this test as test_expect_failure. Remove the FIXME and
> +# mark as test_expect_success when this test successes (fixed bug).
> +test_expect_failure '"bisect skip: bisection is started with --term*"' '

I am not sure why there are double quotes inside simple quotes around
the name of the test. Aren't simple quotes enough?

Also what about a simpler name like:

test_expect_failure '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

It might be a bit safer and more consistent with the rest of this test
script to use double quotes around $HASH_SKIPPED_FROM and
$HASH_SKIPPED_TO, like:

       test "$HASH_SKIPPED_FROM" != "$HASH_SKIPPED_TO"

Thanks,
Christian.

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

[Eric Sunshine]

On Wed, Apr 28, 2021 at 4:12 AM Christian Couder
<christian.couder@gmail.com> wrote:
> On Sun, Apr 25, 2021 at 10:06 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> > +       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
>
> It might be a bit safer and more consistent with the rest of this test
> script to use double quotes around $HASH_SKIPPED_FROM and
> $HASH_SKIPPED_TO, like:
>
>        test "$HASH_SKIPPED_FROM" != "$HASH_SKIPPED_TO"

Also, is there a reason for upcasing these variable names
(HASH_SKIPPED_FROM and HASH_SKIPPED_TO), thus making them appear to be
globals even though they are used only in this test? More appropriate
and less misleading names might be `skipped_from` and `skipped_to`.

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

[Christian Couder]

On Wed, Apr 28, 2021 at 6:32 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Wed, Apr 28, 2021 at 4:12 AM Christian Couder
> <christian.couder@gmail.com> wrote:
> > On Sun, Apr 25, 2021 at 10:06 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> > > +       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
> >
> > It might be a bit safer and more consistent with the rest of this test
> > script to use double quotes around $HASH_SKIPPED_FROM and
> > $HASH_SKIPPED_TO, like:
> >
> >        test "$HASH_SKIPPED_FROM" != "$HASH_SKIPPED_TO"
>
> Also, is there a reason for upcasing these variable names
> (HASH_SKIPPED_FROM and HASH_SKIPPED_TO), thus making them appear to be
> globals even though they are used only in this test? More appropriate
> and less misleading names might be `skipped_from` and `skipped_to`.

In this test script many variables are called HASH1, HASH2, ... or
PARA_HASH1, PARA_HASH2, ... So in this regard HASH_SKIPPED_FROM and
HASH_SKIPPED_TO look ok to me.

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

[Eric Sunshine]

On Wed, Apr 28, 2021 at 1:17 PM Christian Couder
<christian.couder@gmail.com> wrote:
> On Wed, Apr 28, 2021 at 6:32 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Wed, Apr 28, 2021 at 4:12 AM Christian Couder
> > <christian.couder@gmail.com> wrote:
> > > On Sun, Apr 25, 2021 at 10:06 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> > > > +       HASH_SKIPPED_FROM=$(git rev-parse --verify HEAD) &&
> > > > +       HASH_SKIPPED_TO=$(git rev-parse --verify HEAD) &&
> > > > +       test $HASH_SKIPPED_FROM != $HASH_SKIPPED_TO
> > >
> > >        test "$HASH_SKIPPED_FROM" != "$HASH_SKIPPED_TO"
> >
> > Also, is there a reason for upcasing these variable names
> > (HASH_SKIPPED_FROM and HASH_SKIPPED_TO), thus making them appear to be
> > globals even though they are used only in this test? More appropriate
> > and less misleading names might be `skipped_from` and `skipped_to`.
>
> In this test script many variables are called HASH1, HASH2, ... or
> PARA_HASH1, PARA_HASH2, ... So in this regard HASH_SKIPPED_FROM and
> HASH_SKIPPED_TO look ok to me.

Many of the values of the existing uppercase variables -- such as
HASH1, HASH2 -- in this script are explicitly and intentionally shared
among tests, so they are necessarily global, thus the conventional
uppercase names make sense. However, the variables in this test are
local to it (and there is no indication that their values will ever be
shared with other tests, so using uppercase for the names is
misleading and confusing for readers[1].

[1]: It's true that some of the uppercase names in existing tests are
local to those tests, as well, so they ought not use uppercase names,
and therefore share the same problem of misleading and confusing
readers. It would be nice for the naming to be cleaned up, but that's
outside the scope of this patch. Admittedly, considering the existing
naming inconsistency, questioning the uppercase names in this new test
is a relatively minor point, but as a reader, they confuse me and make
me spend extra cycles wondering why they need to be global (even
though they are not, but their uppercase names mislead me into
thinking they are).