{"thread":{"id":"56161","subject":"[PATCH] Fix git-bisect when show-branch is configured to run with pager","startedAt":"2021-07-26T15:01:33Z","lastAt":"2021-07-29T02:34:28Z","messageCount":10,"participants":["Oded S via GitGitGadget","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"431192","messageId":"pull.1003.git.1627311659384.gitgitgadget@gmail.com","threadId":"56161","inReplyTo":null,"subject":"[PATCH] Fix git-bisect when show-branch is configured to run with pager","fromName":"Oded S via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-26T15:00:59Z","receivedAt":"2021-07-26T15:01:33Z","isPatch":true,"sender":{"key":"name:Oded S","avatar":null},"body":"From: Oded Shimon \n\nSigned-off-by: Oded Shimon \n---\n Fix git-bisect when show-branch is configured to run with pager\n \n Signed-off-by: Oded Shimon oded@istraresearch.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1003%2Foded-ist%2Fmaster-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1003/oded-ist/master-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1003\n\n bisect.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex af2863d044b..c02bcc3359f 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -23,7 +23,7 @@ static struct oid_array skipped_revs;\n static struct object_id *current_bad_oid;\n \n static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n-static const char *argv_show_branch[] = {\"show-branch\", NULL, NULL};\n+static const char *argv_show_branch[] = {\"-P\", \"show-branch\", NULL, NULL};\n \n static const char *term_bad;\n static const char *term_good;\n@@ -748,7 +748,7 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int\n \t\t\treturn -abs(res);\n \t}\n \n-\targv_show_branch[1] = bisect_rev_hex;\n+\targv_show_branch[2] = bisect_rev_hex;\n \tres = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);\n \t/*\n \t * Errors in `run_command()` itself, signaled by res < 0,\n\nbase-commit: eb27b338a3e71c7c4079fbac8aeae3f8fbb5c687\n-- \ngitgitgadget\n"},{"id":"431208","messageId":"CAP8UFD37EyNcjPBeSmpAaryv9M0zqrZ98aQ36O6NXNZ9t_7CBw@mail.gmail.com","threadId":"56161","inReplyTo":"pull.1003.git.1627311659384.gitgitgadget@gmail.com","subject":"Re: [PATCH] Fix git-bisect when show-branch is configured to run with pager","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-07-26T18:13:45Z","receivedAt":"2021-07-26T18:13:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"(Sorry, I forgot to keep the mailing list in Cc, in my first reply, so\nI am resending it...)\n\nOn Mon, Jul 26, 2021 at 5:03 PM Oded S via GitGitGadget\n wrote:\n>\n> From: Oded Shimon \n\nCould you explain a bit more here what is the unwanted behavior this\npatch fixes?\n\n> Signed-off-by: Oded Shimon \n> ---\n> Fix git-bisect when show-branch is configured to run with pager\n\nUsually subjects are something like:\n\nbisect: make sure show-branch doesn't use a pager\n\nOtherwise your patch looks good to me!\n\nThanks!\n"},{"id":"431215","messageId":"xmqq4kcgkjhk.fsf@gitster.g","threadId":"56161","inReplyTo":"pull.1003.git.1627311659384.gitgitgadget@gmail.com","subject":"Re: [PATCH] Fix git-bisect when show-branch is configured to run with pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-26T18:39:19Z","receivedAt":"2021-07-26T18:39:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Oded S via GitGitGadget\" writes:\n\n> Subject: Re: [PATCH] Fix git-bisect when show-branch is configured to run with pager\n\nPerhaps\n\n Subject: bisect: disable pager while invoking show-branch\n\ncf. Documentation/SubmittingPatches (describe-changes)\n\n> From: Oded Shimon \n\nHere is the space for you to answer these potential questions by\nfuture readers of your code in \"git log -p\" output in advance:\n\n * The title says \"fix\", but how is it broken? If the user prefers\n to run show-branch with their pager, why would it be a good\n change to unilaterally countermand that preference?\n\n * When is show-branch invoked in the \"git bisect\" session? Perhaps\n that justifies the unilateral disabling of the pager, but it is\n not explained here so we cannot tell why the author of this\n change thought it was a good idea.\n\n * We see in the patch context that \"checkout\" is also invoked\n somewhere in the same program, but it does not gain the\n \"--no-pager\" option. Why? If those who prefer show-branch to\n page have trouble using \"bisect\", wouldn't those who prefer\n \"checkout\" to page have the same trouble?\n\n> Signed-off-by: Oded Shimon \n\nThanks for trying to make Git better. I cannot quite tell without\nthese questions (and there may be others) answered in the proposed\nlog message if the proposed change is a good one.\n\nAlso, in the longer term, I suspect that we probably should stop\ncalling show-branch from this codepath and here is why.\n\nIf we look at \"git show v1.5.3:git-bisect.sh\" and look for\ninvocation of show-branch, and look for show-branch in the current\ncodebase wrt bisect, i.e.\n\n $ git grep show-branch bisect.c git-bisect.sh builtin/bisect--helper.c\n\nwe notice that we used to call the command in many more places to\nwrite into BISECT_LOG and also after checking out the commit to be\ntested. In today's code, the latter is the only place that still\nuses show-branch. What happend to the use of the other one? IOW,\nhow do we write BISECT_LOG these days in such a way that it is\ncompatible/comparable to the output we got from show-branch in olden\ntimes? Would it easy to emulate it so that we do not use show-branch\nat all? If so, reusing how the part that writes BISECT_LOG does to\nshow the revision after checking it out may be a good clean-up,\nregardless of \"show-branch pages\" issue.\n\nIn any case, if the \"for those with show-branch configured to page,\nthe current behaviour of bisect needs fixing\" is true, then I think\nyour patch may even be worth applying before such a longer-term\nchange.\n\nThanks.\n\n\n> Fix git-bisect when show-branch is configured to run with pager\n> \n> Signed-off-by: Oded Shimon oded@istraresearch.com\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1003%2Foded-ist%2Fmaster-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1003/oded-ist/master-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1003\n>\n> bisect.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index af2863d044b..c02bcc3359f 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -23,7 +23,7 @@ static struct oid_array skipped_revs;\n> static struct object_id *current_bad_oid;\n> \n> static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n> -static const char *argv_show_branch[] = {\"show-branch\", NULL, NULL};\n> +static const char *argv_show_branch[] = {\"-P\", \"show-branch\", NULL, NULL};\n> \n> static const char *term_bad;\n> static const char *term_good;\n> @@ -748,7 +748,7 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int\n> \t\t\treturn -abs(res);\n> \t}\n> \n> -\targv_show_branch[1] = bisect_rev_hex;\n> +\targv_show_branch[2] = bisect_rev_hex;\n> \tres = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);\n> \t/*\n> \t * Errors in `run_command()` itself, signaled by res < 0,\n>\n> base-commit: eb27b338a3e71c7c4079fbac8aeae3f8fbb5c687\n"},{"id":"431291","messageId":"pull.1003.v2.git.1627373560881.gitgitgadget@gmail.com","threadId":"56161","inReplyTo":"pull.1003.git.1627311659384.gitgitgadget@gmail.com","subject":"[PATCH v2] bisect: disable pager while invoking show-branch","fromName":"Oded S via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-27T08:12:40Z","receivedAt":"2021-07-27T08:12:47Z","isPatch":true,"sender":{"key":"name:Oded S","avatar":null},"body":"From: Oded Shimon \n\ngit-bisect uses show-branch for logging during the bisect process. If the user\nsets an interactive pager for show-branch, this makes bisect hang (wait for\nuser input) unexpectedly - so we disable pager with -P.\n\nIt's possible that the user would set a pager for git-checkout as well, but an\ninteractive pager there would break many more scripts.\n\nSigned-off-by: Oded Shimon \n---\n Fix git-bisect when show-branch is configured to run with pager\n \n Signed-off-by: Oded Shimon oded@istraresearch.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1003%2Foded-ist%2Fmaster-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1003/oded-ist/master-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1003\n\nRange-diff vs v1:\n\n 1: 52eff18191b ! 1: c193f3a556f Fix git-bisect when show-branch is configured to run with pager\n @@ Metadata\n Author: Oded Shimon \n \n ## Commit message ##\n - Fix git-bisect when show-branch is configured to run with pager\n + bisect: disable pager while invoking show-branch\n +\n + git-bisect uses show-branch for logging during the bisect process. If the user\n + sets an interactive pager for show-branch, this makes bisect hang (wait for\n + user input) unexpectedly - so we disable pager with -P.\n +\n + It's possible that the user would set a pager for git-checkout as well, but an\n + interactive pager there would break many more scripts.\n \n Signed-off-by: Oded Shimon \n \n\n\n bisect.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex af2863d044b..c02bcc3359f 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -23,7 +23,7 @@ static struct oid_array skipped_revs;\n static struct object_id *current_bad_oid;\n \n static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n-static const char *argv_show_branch[] = {\"show-branch\", NULL, NULL};\n+static const char *argv_show_branch[] = {\"-P\", \"show-branch\", NULL, NULL};\n \n static const char *term_bad;\n static const char *term_good;\n@@ -748,7 +748,7 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int\n \t\t\treturn -abs(res);\n \t}\n \n-\targv_show_branch[1] = bisect_rev_hex;\n+\targv_show_branch[2] = bisect_rev_hex;\n \tres = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);\n \t/*\n \t * Errors in `run_command()` itself, signaled by res < 0,\n\nbase-commit: eb27b338a3e71c7c4079fbac8aeae3f8fbb5c687\n-- \ngitgitgadget\n"},{"id":"431332","messageId":"xmqq7dhbehwl.fsf@gitster.g","threadId":"56161","inReplyTo":"xmqq4kcgkjhk.fsf@gitster.g","subject":"Re: [PATCH] Fix git-bisect when show-branch is configured to run with pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-27T18:22:18Z","receivedAt":"2021-07-27T18:22:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Also, in the longer term, I suspect that we probably should stop\n> calling show-branch from this codepath and here is why.\n\nI wonder if it is just a simple matter of a few lines of code, like\nthis?\n\n---- >8 ------- >8 ------- >8 ------- >8 ------- >8 ----\nSubject: [PATCH] bisect: do not run show-branch just to show the current commit\n\nIn scripted versions of \"git bisect\", we used \"git show-branch\" to\ndescribe single commit in the bisect log and also to the interactive\nuser after checking out the next version to be tested. \n\nThe former use of \"git show-branch\" was lost when the helper\nfunction that wrote bisect log entries was rewritten at 0f30233a\n(bisect--helper: `bisect_write` shell function in C, 2019-01-02) in\nC\n\nBut we've kept the latter ever since 0871984d (bisect: make \"git\nbisect\" use new \"--next-all\" bisect-helper function, 2009-05-09)\nstarted using the faithful C-rewrite introduced at ef24c7ca\n(bisect--helper: add \"--next-exit\" to output bisect results,\n2009-04-19).\n\nShowing \"[] \" is simple enough with our helper\npretty.c::format_commit_message() and spawning show-branch is an\noverkill. Let's lose one external process.\n\nSigned-off-by: Junio C Hamano \n---\n bisect.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex af2863d044..2b8b6546e9 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -23,7 +23,6 @@ static struct oid_array skipped_revs;\n static struct object_id *current_bad_oid;\n \n static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n-static const char *argv_show_branch[] = {\"show-branch\", NULL, NULL};\n \n static const char *term_bad;\n static const char *term_good;\n@@ -729,6 +728,9 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int\n {\n \tchar bisect_rev_hex[GIT_MAX_HEXSZ + 1];\n \tenum bisect_error res = BISECT_OK;\n+\tstruct commit *commit;\n+\tstruct pretty_print_context pp = {0};\n+\tstruct strbuf commit_msg = STRBUF_INIT;\n \n \toid_to_hex_r(bisect_rev_hex, bisect_rev);\n \tupdate_ref(NULL, \"BISECT_EXPECTED_REV\", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n@@ -748,13 +750,11 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int\n \t\t\treturn -abs(res);\n \t}\n \n-\targv_show_branch[1] = bisect_rev_hex;\n-\tres = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);\n-\t/*\n-\t * Errors in `run_command()` itself, signaled by res < 0,\n-\t * and errors in the child process, signaled by res > 0\n-\t * can both be treated as regular BISECT_FAILURE (-1).\n-\t */\n+\tcommit = lookup_commit_reference(the_repository, bisect_rev);\n+\tformat_commit_message(commit, \"[%H] %s%n\", &commit_msg, &pp);\n+\tfputs(commit_msg.buf, stdout);\n+\tstrbuf_release(&commit_msg);\n+\n \treturn -abs(res);\n }\n \n-- \n2.32.0-555-g350b5add0b\n\n"},{"id":"431333","messageId":"xmqq5ywvehla.fsf@gitster.g","threadId":"56161","inReplyTo":"pull.1003.v2.git.1627373560881.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] bisect: disable pager while invoking show-branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-27T18:29:05Z","receivedAt":"2021-07-27T18:29:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Oded S via GitGitGadget\" writes:\n\n> From: Oded Shimon \n>\n> git-bisect uses show-branch for logging during the bisect process. If the user\n> sets an interactive pager for show-branch, this makes bisect hang (wait for\n> user input) unexpectedly - so we disable pager with -P.\n>\n> It's possible that the user would set a pager for git-checkout as well, but an\n> interactive pager there would break many more scripts.\n>\n> Signed-off-by: Oded Shimon \n> ---\n\nNicely described. Now we can discuss if the thought process behind\nthis change makes sense or not with such a clear description.\n\nI do not know if \"unexpectedly\" is truly unexpected for those who\nconfigure show-branch to page, though. After all they wanted their\npager to kick in.\n\nIn any case, such users are probably better off configuring their\npager not to prompt and wait when the output is less than pageful\n(e.g. \"less\" has \"--quit-if-one-screen\" option and 'F' in $LESS\nenvironemnt variable triggers this behaviour).\n\nThe patch looks good to me.\n\nThanks.\n\n\n\n> diff --git a/bisect.c b/bisect.c\n> index af2863d044b..c02bcc3359f 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -23,7 +23,7 @@ static struct oid_array skipped_revs;\n> static struct object_id *current_bad_oid;\n> \n> static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n> -static const char *argv_show_branch[] = {\"show-branch\", NULL, NULL};\n> +static const char *argv_show_branch[] = {\"-P\", \"show-branch\", NULL, NULL};\n> \n> static const char *term_bad;\n> static const char *term_good;\n> @@ -748,7 +748,7 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int\n> \t\t\treturn -abs(res);\n> \t}\n> \n> -\targv_show_branch[1] = bisect_rev_hex;\n> +\targv_show_branch[2] = bisect_rev_hex;\n> \tres = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);\n> \t/*\n> \t * Errors in `run_command()` itself, signaled by res < 0,\n>\n> base-commit: eb27b338a3e71c7c4079fbac8aeae3f8fbb5c687\n"},{"id":"431385","messageId":"CAP8UFD26AXTYPvSzBD1x6hSvK5SNbM0F5BPX5bLEbxUZa2dAUg@mail.gmail.com","threadId":"56161","inReplyTo":"xmqq7dhbehwl.fsf@gitster.g","subject":"Re: [PATCH] Fix git-bisect when show-branch is configured to run with pager","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-07-28T06:37:01Z","receivedAt":"2021-07-28T06:37:16Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Jul 27, 2021 at 8:26 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > Also, in the longer term, I suspect that we probably should stop\n> > calling show-branch from this codepath and here is why.\n>\n> I wonder if it is just a simple matter of a few lines of code, like\n> this?\n\nYeah, I also think it's a good idea.\n\n> ---- >8 ------- >8 ------- >8 ------- >8 ------- >8 ----\n> Subject: [PATCH] bisect: do not run show-branch just to show the current commit\n>\n> In scripted versions of \"git bisect\", we used \"git show-branch\" to\n> describe single commit in the bisect log and also to the interactive\n\ns/single/ a single/\n\n> user after checking out the next version to be tested.\n\n[...]\n\n> diff --git a/bisect.c b/bisect.c\n> index af2863d044..2b8b6546e9 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -23,7 +23,6 @@ static struct oid_array skipped_revs;\n> static struct object_id *current_bad_oid;\n>\n> static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n> -static const char *argv_show_branch[] = {\"show-branch\", NULL, NULL};\n>\n> static const char *term_bad;\n> static const char *term_good;\n> @@ -729,6 +728,9 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int\n> {\n> char bisect_rev_hex[GIT_MAX_HEXSZ + 1];\n> enum bisect_error res = BISECT_OK;\n> + struct commit *commit;\n> + struct pretty_print_context pp = {0};\n> + struct strbuf commit_msg = STRBUF_INIT;\n>\n> oid_to_hex_r(bisect_rev_hex, bisect_rev);\n> update_ref(NULL, \"BISECT_EXPECTED_REV\", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n> @@ -748,13 +750,11 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int\n> return -abs(res);\n> }\n>\n> - argv_show_branch[1] = bisect_rev_hex;\n> - res = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);\n> - /*\n> - * Errors in `run_command()` itself, signaled by res < 0,\n> - * and errors in the child process, signaled by res > 0\n> - * can both be treated as regular BISECT_FAILURE (-1).\n> - */\n> + commit = lookup_commit_reference(the_repository, bisect_rev);\n> + format_commit_message(commit, \"[%H] %s%n\", &commit_msg, &pp);\n> + fputs(commit_msg.buf, stdout);\n> + strbuf_release(&commit_msg);\n> +\n> return -abs(res);\n\nNice! Now, the above line can be simplified to:\n\n return BISECT_OK;\n\nAnd the declaration of the `res` variable can be moved into the else\nclause where it is used.\n> }\n"},{"id":"431403","messageId":"xmqq8s1qbdcr.fsf@gitster.g","threadId":"56161","inReplyTo":"CAP8UFD26AXTYPvSzBD1x6hSvK5SNbM0F5BPX5bLEbxUZa2dAUg@mail.gmail.com","subject":"Re: [PATCH] Fix git-bisect when show-branch is configured to run with pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-28T16:41:08Z","receivedAt":"2021-07-28T16:41:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>> I wonder if it is just a simple matter of a few lines of code, like\n>> this?\n>\n> Yeah, I also think it's a good idea.\n>\n>> ---- >8 ------- >8 ------- >8 ------- >8 ------- >8 ----\n>> Subject: [PATCH] bisect: do not run show-branch just to show the current commit\n>>\n>> In scripted versions of \"git bisect\", we used \"git show-branch\" to\n>> describe single commit in the bisect log and also to the interactive\n>\n> s/single/ a single/\n\nThanks.\n\n>> enum bisect_error res = BISECT_OK;\n>> + struct commit *commit;\n>> + struct pretty_print_context pp = {0};\n>> + struct strbuf commit_msg = STRBUF_INIT;\n>> ...\n>> + commit = lookup_commit_reference(the_repository, bisect_rev);\n>> + format_commit_message(commit, \"[%H] %s%n\", &commit_msg, &pp);\n>> + fputs(commit_msg.buf, stdout);\n>> + strbuf_release(&commit_msg);\n>> +\n>> return -abs(res);\n>\n> Nice! Now, the above line can be simplified to:\n>\n> return BISECT_OK;\n>\n> And the declaration of the `res` variable can be moved into the else\n> clause where it is used.\n\nThanks, again.\n"},{"id":"431405","messageId":"xmqq4kcebc5l.fsf_-_@gitster.g","threadId":"56161","inReplyTo":"xmqq8s1qbdcr.fsf@gitster.g","subject":"Re* [PATCH] Fix git-bisect when show-branch is configured to run with pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-28T17:07:02Z","receivedAt":"2021-07-28T17:07:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I'll fix the missing 'a' in the log message, but the \"res\"\nsimplification is probably better done as a separate patch on top.\n\nHow does this look?\n\nThanks.\n\n---- >8 ------- >8 ------- >8 ------- >8 ------- >8 ------- >8 ----\nSubject: [PATCH] bisect: simplify return code from bisect_checkout()\n\nThe function was designed to return only BISECT_OK (0) or\nBISECT_FAILED (-1) and no other values, but there were two issues:\n\n - The comment misspelled BISECT_FAILED as BISECT_FAILURE, even\n though the logic it described (i.e. any non-zero return should be\n reported as a single BISECT_FAILED) was correct.\n\n - It took the return value from run_command_v_opt(), and assumed it\n was either -1 or 1 upon error, which is not the case; it can relay\n errors from wait_or_whine(), which can report exit status of the\n child process.\n\nTranslate any error return from run_command_v_opt() to BISECT_FAILED,\nand simplify the resulting code by losing the 'res' variable that is\nno longer needed.\n\nSigned-off-by: Junio C Hamano \n---\n bisect.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 2b8b6546e9..888949fba6 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -727,7 +727,6 @@ static int is_expected_rev(const struct object_id *oid)\n static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int no_checkout)\n {\n \tchar bisect_rev_hex[GIT_MAX_HEXSZ + 1];\n-\tenum bisect_error res = BISECT_OK;\n \tstruct commit *commit;\n \tstruct pretty_print_context pp = {0};\n \tstruct strbuf commit_msg = STRBUF_INIT;\n@@ -740,14 +739,13 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int\n \t\tupdate_ref(NULL, \"BISECT_HEAD\", bisect_rev, NULL, 0,\n \t\t\t UPDATE_REFS_DIE_ON_ERR);\n \t} else {\n-\t\tres = run_command_v_opt(argv_checkout, RUN_GIT_CMD);\n-\t\tif (res)\n+\t\tif (run_command_v_opt(argv_checkout, RUN_GIT_CMD))\n \t\t\t/*\n \t\t\t * Errors in `run_command()` itself, signaled by res < 0,\n \t\t\t * and errors in the child process, signaled by res > 0\n-\t\t\t * can both be treated as regular BISECT_FAILURE (-1).\n+\t\t\t * can both be treated as regular BISECT_FAILED (-1).\n \t\t\t */\n-\t\t\treturn -abs(res);\n+\t\t\treturn BISECT_FAILED;\n \t}\n \n \tcommit = lookup_commit_reference(the_repository, bisect_rev);\n@@ -755,7 +753,7 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int\n \tfputs(commit_msg.buf, stdout);\n \tstrbuf_release(&commit_msg);\n \n-\treturn -abs(res);\n+\treturn BISECT_OK;\n }\n \n static struct commit *get_commit_reference(struct repository *r,\n-- \n2.32.0-561-g6177dfa0d2\n\n"},{"id":"431455","messageId":"CAP8UFD2Qp-yBP1-YNFx9UG89J3QcLinY99edjtXPo=SqR21-9Q@mail.gmail.com","threadId":"56161","inReplyTo":"xmqq4kcebc5l.fsf_-_@gitster.g","subject":"Re: Re* [PATCH] Fix git-bisect when show-branch is configured to run with pager","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-07-29T02:34:13Z","receivedAt":"2021-07-29T02:34:28Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jul 28, 2021 at 7:07 PM Junio C Hamano wrote:\n>\n> I'll fix the missing 'a' in the log message, but the \"res\"\n> simplification is probably better done as a separate patch on top.\n\nOk.\n\n> How does this look?\n>\n> ---- >8 ------- >8 ------- >8 ------- >8 ------- >8 ------- >8 ----\n> Subject: [PATCH] bisect: simplify return code from bisect_checkout()\n\n[...]\n\nThis LGTM, thanks!\n"}]}