{"thread":{"id":"55557","subject":"[PATCH v2] t6030: add test for git bisect skip started with --term* arguments","startedAt":"2021-04-25T08:06:55Z","lastAt":"2021-04-28T17:49:55Z","messageCount":5,"participants":["Bagas Sanjaya","Christian Couder","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"422869","messageId":"20210425080508.154159-1-bagasdotme@gmail.com","threadId":"55557","inReplyTo":null,"subject":"[PATCH v2] t6030: add test for git bisect skip started with --term* arguments","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-04-25T08:05:09Z","receivedAt":"2021-04-25T08:06:55Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"Trygve Aaberge reported [1] git bisect breakage when the bisection\nis started with --term* arguments (--term-new and --term-old). In that\ncase, skipping with `git bisect skip` should cause HEAD to be changed,\nbut actually the HEAD stayed same after skipping.\n\nLet's add the test to catch the breakage. Because there isn't any fixes\nyet, mark the test as test_expect_failure.\n\n[1]: https://lore.kernel.org/git/20210418151459.GC10839@aaberge.net/\n\nSigned-off-by: Bagas Sanjaya <bagasdotme@gmail.com>\n---\n Changes from v1 [1]:\n\n   * Move the test to the the end of test script (test 74).\n     v1 placed the test as test 26, and when I run the script, there\n     are 9 more failed tests rather than just one (because such tests\n     depended on previous ones). Now the test is placed together with\n     similar tests (git bisect with --terms*).\n   * Begin the test with git bisect reset, as with other nearby tests.\n\n [1]: https://lore.kernel.org/git/20210423070308.85275-1-bagasdotme@gmail.com/\n\n t/t6030-bisect-porcelain.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 32bb66e1ed..b1b847ebbc 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -922,6 +922,19 @@ test_expect_success 'bisect start takes options and revs in any order' '\n \ttest_cmp expected actual\n '\n \n+# Bisect is started with --term-new and --term-old arguments,\n+# then skip. The HEAD should be changed.\n+# FIXME: Mark this test as test_expect_failure. Remove the FIXME and\n+# mark as test_expect_success when this test successes (fixed bug).\n+test_expect_failure '\"bisect skip: bisection is started with --term*\"' '\n+\tgit bisect reset &&\n+\tgit bisect start --term-new=fixed --term-old=unfixed HEAD $HASH1 &&\n+\tHASH_SKIPPED_FROM=$(git rev-parse --verify HEAD) &&\n+\tgit bisect skip &&\n+\tHASH_SKIPPED_TO=$(git rev-parse --verify HEAD) &&\n+\ttest $HASH_SKIPPED_FROM != $HASH_SKIPPED_TO\n+'\n+\n test_expect_success 'git bisect reset cleans bisection state properly' '\n \tgit bisect reset &&\n \tgit bisect start &&\n\nbase-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\n-- \n2.25.1\n\n"},{"id":"423185","messageId":"CAP8UFD0k9fwwW94BKtMPLmA=Qx1ce3Qh3UfF0KiC5+AOj-=MiA@mail.gmail.com","threadId":"55557","inReplyTo":"20210425080508.154159-1-bagasdotme@gmail.com","subject":"Re: [PATCH v2] t6030: add test for git bisect skip started with --term* arguments","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-04-28T08:11:50Z","receivedAt":"2021-04-28T08:12:05Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Apr 25, 2021 at 10:06 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:\n>\n> Trygve Aaberge reported [1] git bisect breakage when the bisection\n> is started with --term* arguments (--term-new and --term-old). In that\n> case, skipping with `git bisect skip` should cause HEAD to be changed,\n> but actually the HEAD stayed same after skipping.\n>\n> Let's add the test to catch the breakage. Because there isn't any fixes\n> yet, mark the test as test_expect_failure.\n>\n> [1]: https://lore.kernel.org/git/20210418151459.GC10839@aaberge.net/\n\nWe prefer when people can give a proper explanation of what happens in\nthe commit message, instead of providing a link to such an\nexplanation. Here it's not so important if this patch (which only adds\ntests) is merged along with (or if it's squashed into) the fix which\nhopefully contains an explanation. Maybe if both patches are sent in\nthe same patch series the commit message might want to only say\nsomething like \"A previous commit fixed a `git bisect skip` breakage\nwhen the bisection is started with --term* arguments, let's add a test\nfor that breakage.\"\n\nA commit message trailer might be the right way to credit Trygve\nAaberge (also please don't forget to put the reporter in Cc like I am\ndoing), for example like:\n\nReported-by: Trygve Aaberge <trygveaa@gmail.com>\n\n> Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>\n> ---\n>  Changes from v1 [1]:\n>\n>    * Move the test to the the end of test script (test 74).\n>      v1 placed the test as test 26, and when I run the script, there\n>      are 9 more failed tests rather than just one (because such tests\n>      depended on previous ones). Now the test is placed together with\n>      similar tests (git bisect with --terms*).\n>    * Begin the test with git bisect reset, as with other nearby tests.\n\nNice!\n\n>  [1]: https://lore.kernel.org/git/20210423070308.85275-1-bagasdotme@gmail.com/\n>\n>  t/t6030-bisect-porcelain.sh | 13 +++++++++++++\n>  1 file changed, 13 insertions(+)\n>\n> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> index 32bb66e1ed..b1b847ebbc 100755\n> --- a/t/t6030-bisect-porcelain.sh\n> +++ b/t/t6030-bisect-porcelain.sh\n> @@ -922,6 +922,19 @@ test_expect_success 'bisect start takes options and revs in any order' '\n>         test_cmp expected actual\n>  '\n>\n> +# Bisect is started with --term-new and --term-old arguments,\n> +# then skip. The HEAD should be changed.\n> +# FIXME: Mark this test as test_expect_failure. Remove the FIXME and\n> +# mark as test_expect_success when this test successes (fixed bug).\n> +test_expect_failure '\"bisect skip: bisection is started with --term*\"' '\n\nI am not sure why there are double quotes inside simple quotes around\nthe name of the test. Aren't simple quotes enough?\n\nAlso what about a simpler name like:\n\ntest_expect_failure 'bisect skip works with --term*' '\n\n?\n\n> +       git bisect reset &&\n> +       git bisect start --term-new=fixed --term-old=unfixed HEAD $HASH1 &&\n> +       HASH_SKIPPED_FROM=$(git rev-parse --verify HEAD) &&\n> +       git bisect skip &&\n> +       HASH_SKIPPED_TO=$(git rev-parse --verify HEAD) &&\n> +       test $HASH_SKIPPED_FROM != $HASH_SKIPPED_TO\n\nIt might be a bit safer and more consistent with the rest of this test\nscript to use double quotes around $HASH_SKIPPED_FROM and\n$HASH_SKIPPED_TO, like:\n\n       test \"$HASH_SKIPPED_FROM\" != \"$HASH_SKIPPED_TO\"\n\nThanks,\nChristian.\n"},{"id":"423202","messageId":"CAPig+cS0yA3TP9PBi2ZtQx5UeKRbFgwXqeqB1sC6QPB7q+Lu2Q@mail.gmail.com","threadId":"55557","inReplyTo":"CAP8UFD0k9fwwW94BKtMPLmA=Qx1ce3Qh3UfF0KiC5+AOj-=MiA@mail.gmail.com","subject":"Re: [PATCH v2] t6030: add test for git bisect skip started with --term* arguments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-04-28T16:32:30Z","receivedAt":"2021-04-28T16:32:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 28, 2021 at 4:12 AM Christian Couder\n<christian.couder@gmail.com> wrote:\n> On Sun, Apr 25, 2021 at 10:06 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:\n> > +       git bisect reset &&\n> > +       git bisect start --term-new=fixed --term-old=unfixed HEAD $HASH1 &&\n> > +       HASH_SKIPPED_FROM=$(git rev-parse --verify HEAD) &&\n> > +       git bisect skip &&\n> > +       HASH_SKIPPED_TO=$(git rev-parse --verify HEAD) &&\n> > +       test $HASH_SKIPPED_FROM != $HASH_SKIPPED_TO\n>\n> It might be a bit safer and more consistent with the rest of this test\n> script to use double quotes around $HASH_SKIPPED_FROM and\n> $HASH_SKIPPED_TO, like:\n>\n>        test \"$HASH_SKIPPED_FROM\" != \"$HASH_SKIPPED_TO\"\n\nAlso, is there a reason for upcasing these variable names\n(HASH_SKIPPED_FROM and HASH_SKIPPED_TO), thus making them appear to be\nglobals even though they are used only in this test? More appropriate\nand less misleading names might be `skipped_from` and `skipped_to`.\n"},{"id":"423205","messageId":"CAP8UFD10vLX_rF=P374BP-CeE5cxs1Rg6faxqf+m3J4MH9Vb8A@mail.gmail.com","threadId":"55557","inReplyTo":"CAPig+cS0yA3TP9PBi2ZtQx5UeKRbFgwXqeqB1sC6QPB7q+Lu2Q@mail.gmail.com","subject":"Re: [PATCH v2] t6030: add test for git bisect skip started with --term* arguments","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-04-28T17:16:52Z","receivedAt":"2021-04-28T17:17:07Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Apr 28, 2021 at 6:32 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Wed, Apr 28, 2021 at 4:12 AM Christian Couder\n> <christian.couder@gmail.com> wrote:\n> > On Sun, Apr 25, 2021 at 10:06 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:\n> > > +       git bisect reset &&\n> > > +       git bisect start --term-new=fixed --term-old=unfixed HEAD $HASH1 &&\n> > > +       HASH_SKIPPED_FROM=$(git rev-parse --verify HEAD) &&\n> > > +       git bisect skip &&\n> > > +       HASH_SKIPPED_TO=$(git rev-parse --verify HEAD) &&\n> > > +       test $HASH_SKIPPED_FROM != $HASH_SKIPPED_TO\n> >\n> > It might be a bit safer and more consistent with the rest of this test\n> > script to use double quotes around $HASH_SKIPPED_FROM and\n> > $HASH_SKIPPED_TO, like:\n> >\n> >        test \"$HASH_SKIPPED_FROM\" != \"$HASH_SKIPPED_TO\"\n>\n> Also, is there a reason for upcasing these variable names\n> (HASH_SKIPPED_FROM and HASH_SKIPPED_TO), thus making them appear to be\n> globals even though they are used only in this test? More appropriate\n> and less misleading names might be `skipped_from` and `skipped_to`.\n\nIn this test script many variables are called HASH1, HASH2, ... or\nPARA_HASH1, PARA_HASH2, ... So in this regard HASH_SKIPPED_FROM and\nHASH_SKIPPED_TO look ok to me.\n"},{"id":"423207","messageId":"CAPig+cSzLFZeOyJAnkXx6M5L+vUKofV+KF8w1MzH+iAzxYycPw@mail.gmail.com","threadId":"55557","inReplyTo":"CAP8UFD10vLX_rF=P374BP-CeE5cxs1Rg6faxqf+m3J4MH9Vb8A@mail.gmail.com","subject":"Re: [PATCH v2] t6030: add test for git bisect skip started with --term* arguments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-04-28T17:48:25Z","receivedAt":"2021-04-28T17:49:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 28, 2021 at 1:17 PM Christian Couder\n<christian.couder@gmail.com> wrote:\n> On Wed, Apr 28, 2021 at 6:32 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > On Wed, Apr 28, 2021 at 4:12 AM Christian Couder\n> > <christian.couder@gmail.com> wrote:\n> > > On Sun, Apr 25, 2021 at 10:06 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:\n> > > > +       HASH_SKIPPED_FROM=$(git rev-parse --verify HEAD) &&\n> > > > +       HASH_SKIPPED_TO=$(git rev-parse --verify HEAD) &&\n> > > > +       test $HASH_SKIPPED_FROM != $HASH_SKIPPED_TO\n> > >\n> > >        test \"$HASH_SKIPPED_FROM\" != \"$HASH_SKIPPED_TO\"\n> >\n> > Also, is there a reason for upcasing these variable names\n> > (HASH_SKIPPED_FROM and HASH_SKIPPED_TO), thus making them appear to be\n> > globals even though they are used only in this test? More appropriate\n> > and less misleading names might be `skipped_from` and `skipped_to`.\n>\n> In this test script many variables are called HASH1, HASH2, ... or\n> PARA_HASH1, PARA_HASH2, ... So in this regard HASH_SKIPPED_FROM and\n> HASH_SKIPPED_TO look ok to me.\n\nMany of the values of the existing uppercase variables -- such as\nHASH1, HASH2 -- in this script are explicitly and intentionally shared\namong tests, so they are necessarily global, thus the conventional\nuppercase names make sense. However, the variables in this test are\nlocal to it (and there is no indication that their values will ever be\nshared with other tests, so using uppercase for the names is\nmisleading and confusing for readers[1].\n\n[1]: It's true that some of the uppercase names in existing tests are\nlocal to those tests, as well, so they ought not use uppercase names,\nand therefore share the same problem of misleading and confusing\nreaders. It would be nice for the naming to be cleaned up, but that's\noutside the scope of this patch. Admittedly, considering the existing\nnaming inconsistency, questioning the uppercase names in this new test\nis a relatively minor point, but as a reader, they confuse me and make\nme spend extra cycles wondering why they need to be global (even\nthough they are not, but their uppercase names mislead me into\nthinking they are).\n"}]}