diff mbox series

Fix git-bisect when show-branch is configured to run with pager

Message ID pull.1003.git.1627311659384.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series Fix git-bisect when show-branch is configured to run with pager | expand

Commit Message

Oded Shimon July 26, 2021, 3 p.m. UTC
From: Oded Shimon <oded@istraresearch.com>

Signed-off-by: Oded Shimon <oded@istraresearch.com>
---
    Fix git-bisect when show-branch is configured to run with pager
    
    Signed-off-by: Oded Shimon oded@istraresearch.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1003%2Foded-ist%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1003/oded-ist/master-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1003

 bisect.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


base-commit: eb27b338a3e71c7c4079fbac8aeae3f8fbb5c687

Comments

Christian Couder July 26, 2021, 6:13 p.m. UTC | #1
(Sorry, I forgot to keep the mailing list in Cc, in my first reply, so
I am resending it...)

On Mon, Jul 26, 2021 at 5:03 PM Oded S via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Oded Shimon <oded@istraresearch.com>

Could you explain a bit more here what is the unwanted behavior this
patch fixes?

> Signed-off-by: Oded Shimon <oded@istraresearch.com>
> ---
>     Fix git-bisect when show-branch is configured to run with pager

Usually subjects are something like:

bisect: make sure show-branch doesn't use a pager

Otherwise your patch looks good to me!

Thanks!
Junio C Hamano July 26, 2021, 6:39 p.m. UTC | #2
"Oded S via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Subject: Re: [PATCH] Fix git-bisect when show-branch is configured to run with pager

Perhaps

    Subject: bisect: disable pager while invoking show-branch

cf. Documentation/SubmittingPatches (describe-changes)

> From: Oded Shimon <oded@istraresearch.com>

Here is the space for you to answer these potential questions by
future readers of your code in "git log -p" output in advance:

 * The title says "fix", but how is it broken?  If the user prefers
   to run show-branch with their pager, why would it be a good
   change to unilaterally countermand that preference?

 * When is show-branch invoked in the "git bisect" session?  Perhaps
   that justifies the unilateral disabling of the pager, but it is
   not explained here so we cannot tell why the author of this
   change thought it was a good idea.

 * We see in the patch context that "checkout" is also invoked
   somewhere in the same program, but it does not gain the
   "--no-pager" option.  Why?  If those who prefer show-branch to
   page have trouble using "bisect", wouldn't those who prefer
   "checkout" to page have the same trouble?

> Signed-off-by: Oded Shimon <oded@istraresearch.com>

Thanks for trying to make Git better.  I cannot quite tell without
these questions (and there may be others) answered in the proposed
log message if the proposed change is a good one.

Also, in the longer term, I suspect that we probably should stop
calling show-branch from this codepath and here is why.

If we look at "git show v1.5.3:git-bisect.sh" and look for
invocation of show-branch, and look for show-branch in the current
codebase wrt bisect, i.e.

    $ git grep show-branch bisect.c git-bisect.sh builtin/bisect--helper.c

we notice that we used to call the command in many more places to
write into BISECT_LOG and also after checking out the commit to be
tested.  In today's code, the latter is the only place that still
uses show-branch.  What happend to the use of the other one?  IOW,
how do we write BISECT_LOG these days in such a way that it is
compatible/comparable to the output we got from show-branch in olden
times?  Would it easy to emulate it so that we do not use show-branch
at all?  If so, reusing how the part that writes BISECT_LOG does to
show the revision after checking it out may be a good clean-up,
regardless of "show-branch pages" issue.

In any case, if the "for those with show-branch configured to page,
the current behaviour of bisect needs fixing" is true, then I think
your patch may even be worth applying before such a longer-term
change.

Thanks.


>     Fix git-bisect when show-branch is configured to run with pager
>     
>     Signed-off-by: Oded Shimon oded@istraresearch.com
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1003%2Foded-ist%2Fmaster-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1003/oded-ist/master-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1003
>
>  bisect.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index af2863d044b..c02bcc3359f 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -23,7 +23,7 @@ static struct oid_array skipped_revs;
>  static struct object_id *current_bad_oid;
>  
>  static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
> -static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
> +static const char *argv_show_branch[] = {"-P", "show-branch", NULL, NULL};
>  
>  static const char *term_bad;
>  static const char *term_good;
> @@ -748,7 +748,7 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int
>  			return -abs(res);
>  	}
>  
> -	argv_show_branch[1] = bisect_rev_hex;
> +	argv_show_branch[2] = bisect_rev_hex;
>  	res = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
>  	/*
>  	 * Errors in `run_command()` itself, signaled by res < 0,
>
> base-commit: eb27b338a3e71c7c4079fbac8aeae3f8fbb5c687
Junio C Hamano July 27, 2021, 6:22 p.m. UTC | #3
Junio C Hamano <gitster@pobox.com> writes:

> Also, in the longer term, I suspect that we probably should stop
> calling show-branch from this codepath and here is why.

I wonder if it is just a simple matter of a few lines of code, like
this?

---- >8 ------- >8 ------- >8 ------- >8 ------- >8 ----
Subject: [PATCH] bisect: do not run show-branch just to show the current  commit

In scripted versions of "git bisect", we used "git show-branch" to
describe single commit in the bisect log and also to the interactive
user after checking out the next version to be tested.  

The former use of "git show-branch" was lost when the helper
function that wrote bisect log entries was rewritten at 0f30233a
(bisect--helper: `bisect_write` shell function in C, 2019-01-02) in
C

But we've kept the latter ever since 0871984d (bisect: make "git
bisect" use new "--next-all" bisect-helper function, 2009-05-09)
started using the faithful C-rewrite introduced at ef24c7ca
(bisect--helper: add "--next-exit" to output bisect results,
2009-04-19).

Showing "[<full hex>] <subject>" is simple enough with our helper
pretty.c::format_commit_message() and spawning show-branch is an
overkill.  Let's lose one external process.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 bisect.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/bisect.c b/bisect.c
index af2863d044..2b8b6546e9 100644
--- a/bisect.c
+++ b/bisect.c
@@ -23,7 +23,6 @@ static struct oid_array skipped_revs;
 static struct object_id *current_bad_oid;
 
 static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
-static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
 
 static const char *term_bad;
 static const char *term_good;
@@ -729,6 +728,9 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int
 {
 	char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
 	enum bisect_error res = BISECT_OK;
+	struct commit *commit;
+	struct pretty_print_context pp = {0};
+	struct strbuf commit_msg = STRBUF_INIT;
 
 	oid_to_hex_r(bisect_rev_hex, bisect_rev);
 	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
@@ -748,13 +750,11 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int
 			return -abs(res);
 	}
 
-	argv_show_branch[1] = bisect_rev_hex;
-	res = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
-	/*
-	 * Errors in `run_command()` itself, signaled by res < 0,
-	 * and errors in the child process, signaled by res > 0
-	 * can both be treated as regular BISECT_FAILURE (-1).
-	 */
+	commit = lookup_commit_reference(the_repository, bisect_rev);
+	format_commit_message(commit, "[%H] %s%n", &commit_msg, &pp);
+	fputs(commit_msg.buf, stdout);
+	strbuf_release(&commit_msg);
+
 	return -abs(res);
 }
Christian Couder July 28, 2021, 6:37 a.m. UTC | #4
On Tue, Jul 27, 2021 at 8:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Also, in the longer term, I suspect that we probably should stop
> > calling show-branch from this codepath and here is why.
>
> I wonder if it is just a simple matter of a few lines of code, like
> this?

Yeah, I also think it's a good idea.

> ---- >8 ------- >8 ------- >8 ------- >8 ------- >8 ----
> Subject: [PATCH] bisect: do not run show-branch just to show the current  commit
>
> In scripted versions of "git bisect", we used "git show-branch" to
> describe single commit in the bisect log and also to the interactive

s/single/ a single/

> user after checking out the next version to be tested.

[...]

> diff --git a/bisect.c b/bisect.c
> index af2863d044..2b8b6546e9 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -23,7 +23,6 @@ static struct oid_array skipped_revs;
>  static struct object_id *current_bad_oid;
>
>  static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
> -static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
>
>  static const char *term_bad;
>  static const char *term_good;
> @@ -729,6 +728,9 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int
>  {
>         char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
>         enum bisect_error res = BISECT_OK;
> +       struct commit *commit;
> +       struct pretty_print_context pp = {0};
> +       struct strbuf commit_msg = STRBUF_INIT;
>
>         oid_to_hex_r(bisect_rev_hex, bisect_rev);
>         update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
> @@ -748,13 +750,11 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int
>                         return -abs(res);
>         }
>
> -       argv_show_branch[1] = bisect_rev_hex;
> -       res = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
> -       /*
> -        * Errors in `run_command()` itself, signaled by res < 0,
> -        * and errors in the child process, signaled by res > 0
> -        * can both be treated as regular BISECT_FAILURE (-1).
> -        */
> +       commit = lookup_commit_reference(the_repository, bisect_rev);
> +       format_commit_message(commit, "[%H] %s%n", &commit_msg, &pp);
> +       fputs(commit_msg.buf, stdout);
> +       strbuf_release(&commit_msg);
> +
>         return -abs(res);

Nice! Now, the above line can be simplified to:

         return BISECT_OK;

And the declaration of the `res` variable can be moved into the else
clause where it is used.
>  }
Junio C Hamano July 28, 2021, 4:41 p.m. UTC | #5
Christian Couder <christian.couder@gmail.com> writes:

>> I wonder if it is just a simple matter of a few lines of code, like
>> this?
>
> Yeah, I also think it's a good idea.
>
>> ---- >8 ------- >8 ------- >8 ------- >8 ------- >8 ----
>> Subject: [PATCH] bisect: do not run show-branch just to show the current  commit
>>
>> In scripted versions of "git bisect", we used "git show-branch" to
>> describe single commit in the bisect log and also to the interactive
>
> s/single/ a single/

Thanks.

>>         enum bisect_error res = BISECT_OK;
>> +       struct commit *commit;
>> +       struct pretty_print_context pp = {0};
>> +       struct strbuf commit_msg = STRBUF_INIT;
>> ...
>> +       commit = lookup_commit_reference(the_repository, bisect_rev);
>> +       format_commit_message(commit, "[%H] %s%n", &commit_msg, &pp);
>> +       fputs(commit_msg.buf, stdout);
>> +       strbuf_release(&commit_msg);
>> +
>>         return -abs(res);
>
> Nice! Now, the above line can be simplified to:
>
>          return BISECT_OK;
>
> And the declaration of the `res` variable can be moved into the else
> clause where it is used.

Thanks, again.
diff mbox series

Patch

diff --git a/bisect.c b/bisect.c
index af2863d044b..c02bcc3359f 100644
--- a/bisect.c
+++ b/bisect.c
@@ -23,7 +23,7 @@  static struct oid_array skipped_revs;
 static struct object_id *current_bad_oid;
 
 static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
-static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
+static const char *argv_show_branch[] = {"-P", "show-branch", NULL, NULL};
 
 static const char *term_bad;
 static const char *term_good;
@@ -748,7 +748,7 @@  static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int
 			return -abs(res);
 	}
 
-	argv_show_branch[1] = bisect_rev_hex;
+	argv_show_branch[2] = bisect_rev_hex;
 	res = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
 	/*
 	 * Errors in `run_command()` itself, signaled by res < 0,