{"thread":{"id":"55592","subject":"[PATCH v2] bisect--helper: use BISECT_TERMS in 'bisect skip' command","startedAt":"2021-04-29T20:15:30Z","lastAt":"2021-04-30T04:34:11Z","messageCount":3,"participants":["Ramsay Jones","Junio C Hamano","Christian Couder"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"423299","messageId":"fa098676-a56c-cbb9-28cb-e57d4a8a5d77@ramsayjones.plus.com","threadId":"55592","inReplyTo":null,"subject":"[PATCH v2] bisect--helper: use BISECT_TERMS in 'bisect skip' command","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2021-04-29T20:15:15Z","receivedAt":"2021-04-29T20:15:30Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nCommit e4c7b33747 (\"bisect--helper: reimplement `bisect_skip` shell\nfunction in C\", 2021-02-03), as part of the shell-to-C conversion,\nforgot to read the 'terms' file (.git/BISECT_TERMS) during the new\n'bisect skip' command implementation. As a result, the 'bisect skip'\ncommand will use the default 'bad'/'good' terms. If the bisection\nterms have been set to non-default values (for example by the\n'bisect start' command), then the 'bisect skip' command will fail.\n\nIn order to correct this problem, we insert a call to the get_terms()\nfunction, which reads the non-default terms from that file (if set),\nin the '--bisect-skip' command implementation of 'bisect--helper'.\n\nAlso, add a test[1] to protect against potential future regression.\n\n[1] https://lore.kernel.org/git/xmqqim45h585.fsf@gitster.g/T/#m207791568054b0f8cf1a3942878ea36293273c7d\n\nReported-by: Trygve Aaberge \nSigned-off-by: Bagas Sanjaya \nSigned-off-by: Ramsay Jones \n---\n\nHi Junio,\n\nThis v2 patch was also created directly on top of commit e4c7b33747 and,\nonce again, applied to current 'master' (@311531c9de). (I was a little\nsurprised that it applied clean, given the changes to t6030-*.sh in the\nmeantime! ;).\n\nI didn't bother with the range-diff[*], since it wasn't very illuminating\n(just the addition of a few trailing lines in the commit message and the\naddition of the test).\n\nI don't know if I did the 'Co-authorship' bit correctly; I nearly used an\n'Co-authored-by:' rather than an 'Signed-off-by:' for Bagas, but I seem to\nremember that caused some push-back recently. Anyway, if I messed that up,\nplease just modify however you see fit.\n\nATB,\nRamsay Jones\n\n[*] Once I got it to work, that is; for some reason, I was trying smaller\ncreation-factor values rather than larger! ;-)\n\n builtin/bisect--helper.c | 1 +\n t/t6030-bisect-porcelain.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 7ad9b4d55b..49c07f0710 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -1129,6 +1129,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \tcase BISECT_SKIP:\n \t\tset_terms(&terms, \"bad\", \"good\");\n+\t\tget_terms(&terms);\n \t\tres = bisect_skip(&terms, argv, argc);\n \t\tbreak;\n \tdefault:\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex aa226381be..b707296ae2 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -919,6 +919,17 @@ 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+test_expect_success 'bisect skip works 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-- \n2.31.0\n"},{"id":"423310","messageId":"xmqqo8dwegk0.fsf@gitster.g","threadId":"55592","inReplyTo":"fa098676-a56c-cbb9-28cb-e57d4a8a5d77@ramsayjones.plus.com","subject":"Re: [PATCH v2] bisect--helper: use BISECT_TERMS in 'bisect skip' command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-30T00:56:15Z","receivedAt":"2021-04-30T00:56:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> This v2 patch was also created directly on top of commit e4c7b33747 and,\n> once again, applied to current 'master' (@311531c9de). (I was a little\n> surprised that it applied clean, given the changes to t6030-*.sh in the\n> meantime! ;).\n>\n> I didn't bother with the range-diff[*], since it wasn't very illuminating\n> (just the addition of a few trailing lines in the commit message and the\n> addition of the test).\n\nThanks. Let's hear from Bagas that this version looks good and\nmerge it down.\n\n> I don't know if I did the 'Co-authorship' bit correctly; I nearly used an\n> 'Co-authored-by:' rather than an 'Signed-off-by:' for Bagas, but I seem to\n> remember that caused some push-back recently. Anyway, if I messed that up,\n> please just modify however you see fit.\n\nI am not in principle opposed to the idea of co-authored-by; the\ncase you remember was where we would have used Helped-by (i.e. a\nreviewer offers \"writing it this way is cleaner\" suggestions on the\nlist and then gets credited on the next version), and it wasn't\nclear to me if the reviewer consented to be a co-author of that\nchange. If the party who were named as a co-author responded that\nit is OK, I would have taken it.\n\n> [*] Once I got it to work, that is; for some reason, I was trying smaller\n> creation-factor values rather than larger! ;-)\n>\n> builtin/bisect--helper.c | 1 +\n> t/t6030-bisect-porcelain.sh | 11 +++++++++++\n> 2 files changed, 12 insertions(+)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 7ad9b4d55b..49c07f0710 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -1129,6 +1129,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\tbreak;\n> \tcase BISECT_SKIP:\n> \t\tset_terms(&terms, \"bad\", \"good\");\n> +\t\tget_terms(&terms);\n> \t\tres = bisect_skip(&terms, argv, argc);\n> \t\tbreak;\n> \tdefault:\n> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> index aa226381be..b707296ae2 100755\n> --- a/t/t6030-bisect-porcelain.sh\n> +++ b/t/t6030-bisect-porcelain.sh\n> @@ -919,6 +919,17 @@ 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> +test_expect_success 'bisect skip works 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"},{"id":"423318","messageId":"CAP8UFD08kH5k1wrjo0pHJ3KH2iW4=DDzePxiZXvYbDSCOqQSEw@mail.gmail.com","threadId":"55592","inReplyTo":"xmqqo8dwegk0.fsf@gitster.g","subject":"Re: [PATCH v2] bisect--helper: use BISECT_TERMS in 'bisect skip' command","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-04-30T04:33:55Z","receivedAt":"2021-04-30T04:34:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Apr 30, 2021 at 2:56 AM Junio C Hamano wrote:\n>\n> Ramsay Jones writes:\n>\n> > This v2 patch was also created directly on top of commit e4c7b33747 and,\n> > once again, applied to current 'master' (@311531c9de). (I was a little\n> > surprised that it applied clean, given the changes to t6030-*.sh in the\n> > meantime! ;).\n> >\n> > I didn't bother with the range-diff[*], since it wasn't very illuminating\n> > (just the addition of a few trailing lines in the commit message and the\n> > addition of the test).\n>\n> Thanks. Let's hear from Bagas that this version looks good and\n> merge it down.\n\nThis version looks good to me.\n\nThanks Ramsay and Bagas!\n"}]}