{"thread":{"id":"55555","subject":"[PATCH] bisect--helper: use BISECT_TERMS in 'bisect skip' command","startedAt":"2021-04-25T00:18:37Z","lastAt":"2021-04-28T04:09:27Z","messageCount":4,"participants":["Ramsay Jones","Bagas Sanjaya","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"422866","messageId":"fd40e12f-9649-1327-4bdb-dce5b5eed619@ramsayjones.plus.com","threadId":"55555","inReplyTo":null,"subject":"[PATCH] bisect--helper: use BISECT_TERMS in 'bisect skip' command","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2021-04-25T00:18:27Z","receivedAt":"2021-04-25T00:18:37Z","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\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n\nHi Junio,\n\nThis patch was created directly on top of commit e4c7b33747 and tested\nwith the test from Bagas Sanjaya [1] (ie the second version of the\nstand-alone test file t6031-*.sh, rather than the newer patch that\nadds the test to t6030-*.sh). I applied this patch to the current\nmaster branch (@311531c9de55) and it also passed the test in [1].\n\n[I created the patch on top of e4c7b33747 so that it would, hopefully,\neasily backport to the relevant 'maint' branches, should you feel the\nneed. ;-) ]\n\nAt this point, I would normally have looked to see if there were other\nexamples of forgetting to call 'get_terms()' (which seems possible).\nHowever, I am a bit busy, so I will have to add that to my TODO list ...\n:(\n\nATB,\nRamsay Jones\n\n[1] https://lore.kernel.org/git/20210421040808.14185-1-bagasdotme@gmail.com/\n\n builtin/bisect--helper.c | 1 +\n 1 file changed, 1 insertion(+)\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:\n-- \n2.31.0\n"},{"id":"422870","messageId":"473a11db-cbb5-58b9-b05d-cab2072d5d2f@gmail.com","threadId":"55555","inReplyTo":"fd40e12f-9649-1327-4bdb-dce5b5eed619@ramsayjones.plus.com","subject":"Re: [PATCH] bisect--helper: use BISECT_TERMS in 'bisect skip' command","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-04-25T08:16:44Z","receivedAt":"2021-04-25T08:19:14Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 25/04/21 07.18, Ramsay Jones wrote:\n> \n> This patch was created directly on top of commit e4c7b33747 and tested\n> with the test from Bagas Sanjaya [1] (ie the second version of the\n> stand-alone test file t6031-*.sh, rather than the newer patch that\n> adds the test to t6030-*.sh). I applied this patch to the current\n> master branch (@311531c9de55) and it also passed the test in [1].\n\nI have just sent v2 of t6030 version of my test [1]. Please test\nthis patch against that v2 test. And if the test passes (breakage vanished),\nplease update the test by do instructions in the FIXME comment lines of [1].\n\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\nI'm not fluent in C, but I read these lines above as \"ok, we set terms from\n&terms, bad and good as fallback in case the reference is empty; then we get these\nterms from the reference\". Wouldn't it makes sense if we can say \"get the terms\nfrom &terms\" first, then \"set the terms from the reference, if empty use bad\nand good as fallback\"?\n\n[1]: https://lore.kernel.org/git/20210425080508.154159-1-bagasdotme@gmail.com/\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"422912","messageId":"CAP8UFD3hOqeF04Xdjn-o2FShuTPQ9ZUEVCWnhfzCqV6MtbvU4g@mail.gmail.com","threadId":"55555","inReplyTo":"473a11db-cbb5-58b9-b05d-cab2072d5d2f@gmail.com","subject":"Re: [PATCH] bisect--helper: use BISECT_TERMS in 'bisect skip' command","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-04-26T07:06:55Z","receivedAt":"2021-04-26T07:08:03Z","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:18 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:\n>\n> On 25/04/21 07.18, Ramsay Jones wrote:\n> >\n> > This patch was created directly on top of commit e4c7b33747 and tested\n> > with the test from Bagas Sanjaya [1] (ie the second version of the\n> > stand-alone test file t6031-*.sh, rather than the newer patch that\n> > adds the test to t6030-*.sh). I applied this patch to the current\n> > master branch (@311531c9de55) and it also passed the test in [1].\n\nThanks Ramsay for your patch, it looks good to me!\n\n> I have just sent v2 of t6030 version of my test [1]. Please test\n> this patch against that v2 test. And if the test passes (breakage vanished),\n> please update the test by do instructions in the FIXME comment lines of [1].\n\nThanks Bagas for your test! I will take a look at it soon.\n\nMy opinion is that it would be best if both patches (Ramsay's and\nBagas') were in the same patch series or even perhaps in the same\ncommit. If you prefer separate patches, maybe the first one could be\nRamsay's, and the second one Bagas' where indeed the instructions to\nreplace test_expect_failure with test_expect_success have been\nfollowed.\n\n> > @@ -1129,6 +1129,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> >               break;\n> >       case BISECT_SKIP:\n> >               set_terms(&terms, \"bad\", \"good\");\n> > +             get_terms(&terms);\n>\n> I'm not fluent in C, but I read these lines above as \"ok, we set terms from\n> &terms, bad and good as fallback in case the reference is empty; then we get these\n> terms from the reference\". Wouldn't it makes sense if we can say \"get the terms\n> from &terms\" first, then \"set the terms from the reference, if empty use bad\n> and good as fallback\"?\n\nIt might not be the best API for this (or the set_terms() and\nget_terms() function could perhaps have better names), but anyway the\ncurrent situation is that set_terms(&terms, \"bad\", \"good\") is setting\nthe current terms to \"bad\"/\"good\" which is the default, and then\nget_terms(&terms) is reading the terms stored in the BISECT_TERMS file\nand using that to set the current terms. Also if the BISECT_TERMS file\ndoesn't exist, then get_terms(&terms) is doing nothing. So it seems to\nme that Ramsay's patch is doing the right thing.\n\nIf get_terms(&terms) was used before set_terms(&terms, \"bad\", \"good\"),\nthen the current terms would always be \"bad\"/\"good\" even if the\nBISECT_TERMS file contains valid terms different from \"bad\"/\"good\".\n"},{"id":"423170","messageId":"ffd80d10-9939-d31c-324d-3e32c37d2eaf@gmail.com","threadId":"55555","inReplyTo":"CAP8UFD3hOqeF04Xdjn-o2FShuTPQ9ZUEVCWnhfzCqV6MtbvU4g@mail.gmail.com","subject":"Re: [PATCH] bisect--helper: use BISECT_TERMS in 'bisect skip' command","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-04-28T04:09:20Z","receivedAt":"2021-04-28T04:09:27Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 26/04/21 14.06, Christian Couder wrote:\n> Thanks Bagas for your test! I will take a look at it soon.\n> \n> My opinion is that it would be best if both patches (Ramsay's and\n> Bagas') were in the same patch series or even perhaps in the same\n> commit. If you prefer separate patches, maybe the first one could be\n> Ramsay's, and the second one Bagas' where indeed the instructions to\n> replace test_expect_failure with test_expect_success have been\n> followed.\n> \n\nOK. Review ping\n\n> It might not be the best API for this (or the set_terms() and\n> get_terms() function could perhaps have better names), but anyway the\n> current situation is that set_terms(&terms, \"bad\", \"good\") is setting\n> the current terms to \"bad\"/\"good\" which is the default, and then\n> get_terms(&terms) is reading the terms stored in the BISECT_TERMS file\n> and using that to set the current terms. Also if the BISECT_TERMS file\n> doesn't exist, then get_terms(&terms) is doing nothing. So it seems to\n> me that Ramsay's patch is doing the right thing.\n> \n> If get_terms(&terms) was used before set_terms(&terms, \"bad\", \"good\"),\n> then the current terms would always be \"bad\"/\"good\" even if the\n> BISECT_TERMS file contains valid terms different from \"bad\"/\"good\".\n> \n\nOK, thanks for the explanation.\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"}]}