[PATCH v3] bisect: report the found commit with "show"

[PATCH v3] bisect: report the found commit with "show"

[Peter Krefting]

When "git bisect" finds the first bad commit and shows it to the user,
it calls "git diff-tree" to do so, whose output is meant to be stable
and deliberately ignores end-user customizations.

As the output is supposed to be consumed by humans, replace this with
a call to "git show". This command honors configuration options (such
as "log.date" and "log.mailmap") and other UI improvements (renames
are detected).

Pass some hard-coded options to "git show" to make the output similar
to the one we are replacing, such as showing a patch summary only.

Reported-by: Michael Osipov <michael.osipov@innomotics.com>
Signed-off-By: Peter Krefting <peter@softwolves.pp.se>
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Jeff King <peff@peff.net>
Cc: Christian Couder <christian.couder@gmail.com>
---
  bisect.c | 37 ++++++++++++++++++++++---------------
  1 file changed, 22 insertions(+), 15 deletions(-)

Changes compared to v2:
- Adds --no-pager and a comment stating why we do so.
- Adds --diff-merges=first-parent, as per previous discussion.

This seems to generate a final output as close to the previous one as 
possible, while honouring the user's settings for how to display log 
entries and calculating diffs.

diff --git a/bisect.c b/bisect.c
index 8487f8cd1b..3be2460c65 100644
--- a/bisect.c
+++ b/bisect.c
@@ -959,23 +959,30 @@ static enum bisect_error check_good_are_ancestors_of_bad(struct repository *r,
  }

  /*
- * This does "git diff-tree --pretty COMMIT" without one fork+exec.
+ * Display a commit summary to the user.
   */
-static void show_diff_tree(struct repository *r,
-			   const char *prefix,
-			   struct commit *commit)
+static void show_commit(struct commit *commit)
  {
-	const char *argv[] = {
-		"diff-tree", "--pretty", "--stat", "--summary", "--cc", NULL
-	};
-	struct rev_info opt;
+	struct child_process show = CHILD_PROCESS_INIT;

-	git_config(git_diff_ui_config, NULL);
-	repo_init_revisions(r, &opt, prefix);
-
-	setup_revisions(ARRAY_SIZE(argv) - 1, argv, &opt, NULL);
-	log_tree_commit(&opt, commit);
-	release_revisions(&opt);
+	/* Call git show with --no-pager, as it would otherwise
+	 * paginate the "git show" output only, not the output
+	 * from bisect_next_all(); this can be fixed by moving
+	 * it into a --format parameter, but that would override
+	 * the user's default options for "git show", which we
+	 * are trying to honour. */
+	strvec_pushl(&show.args,
+	             "--no-pager",
+	             "show",
+	             "--stat",
+	             "--summary",
+	             "--no-abbrev-commit",
+	             "--diff-merges=first-parent",
+	             oid_to_hex(&commit->object.oid), NULL);
+	show.git_cmd = 1;
+	if (run_command(&show))
+		die(_("unable to start 'show' for object '%s'"),
+		    oid_to_hex(&commit->object.oid));
  }

  /*
@@ -1092,7 +1099,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
  		printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
  			term_bad);

-		show_diff_tree(r, prefix, revs.commits->item);
+		show_commit(revs.commits->item);
  		/*
  		 * This means the bisection process succeeded.
  		 * Using BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10)
-- 
2.39.2

Re: [PATCH v3] bisect: report the found commit with "show"

[Eric Sunshine]

On Sat, Apr 13, 2024 at 5:21 PM Peter Krefting <peter@softwolves.pp.se> wrote:
> When "git bisect" finds the first bad commit and shows it to the user,
> it calls "git diff-tree" to do so, whose output is meant to be stable
> and deliberately ignores end-user customizations.
>
> As the output is supposed to be consumed by humans, replace this with
> a call to "git show". This command honors configuration options (such
> as "log.date" and "log.mailmap") and other UI improvements (renames
> are detected).
>
> Pass some hard-coded options to "git show" to make the output similar
> to the one we are replacing, such as showing a patch summary only.
>
> Signed-off-By: Peter Krefting <peter@softwolves.pp.se>
> ---
> diff --git a/bisect.c b/bisect.c
> @@ -959,23 +959,30 @@ static enum bisect_error check_good_are_ancestors_of_bad(struct repository *r,
> +static void show_commit(struct commit *commit)
>   {
> +       /* Call git show with --no-pager, as it would otherwise
> +        * paginate the "git show" output only, not the output
> +        * from bisect_next_all(); this can be fixed by moving
> +        * it into a --format parameter, but that would override
> +        * the user's default options for "git show", which we
> +        * are trying to honour. */
> +       strvec_pushl(&show.args,
> +                    "--no-pager",
> +                    "show",
> +                    "--stat",
> +                    "--summary",
> +                    "--no-abbrev-commit",
> +                    "--diff-merges=first-parent",
> +                    oid_to_hex(&commit->object.oid), NULL);

Style nit: On this project, multi-line comments are formatted like this:

    /*
     * This is a multi-line
     * comment.
    */

It also feels slightly odd to place each option on its own line in the
call to strvec_pushl() but then place the terminating NULL on the same
line as the oid_to_hex() call. But that's a minor and subjective point
hardly worth mentioning.

Re: [PATCH v3] bisect: report the found commit with "show"

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> Pass some hard-coded options to "git show" to make the output similar
>> to the one we are replacing, such as showing a patch summary only.
>>
>> Signed-off-By: Peter Krefting <peter@softwolves.pp.se>
>> ---

Curious how you trimmed the trailers from the submitted patch ;-)

Although we do not use Cc: in this project, we do recommend use of
the "Reported-by" trailer in Documentation/SubmittingPatches.

>> diff --git a/bisect.c b/bisect.c
>> ...

> Style nit: On this project, multi-line comments are formatted like this:
>
>     /*
>      * This is a multi-line
>      * comment.
>      */

True.

> It also feels slightly odd to place each option on its own line in the
> call to strvec_pushl() but then place the terminating NULL on the same
> line as the oid_to_hex() call. But that's a minor and subjective point
> hardly worth mentioning.

I think that is because we may want to tweak the list of options,
but no matter what change we make to them in the future, the object
name as the last parameter is likely to remain the last one, and
with that reasoning, I agree with the layout in the patch as posted.

What is more problematic is that the message is sent with

	Content-Type: text/plain; format=flowed; charset=US-ASCII

and the contents of the message is in that flawed format, possibly
corrupting whitespaces in irrecoverable ways.

I _think_ "git am" (actually, "git mailsplit" that is called from
it) did a reasonable job this time, but I do not have a lot of
confidence in the resulting commit---I would not be surprised if it
is not identical to what Peter wanted to give us.

Peter, if the resulting commit I push out later today botches some
whitespaces due to this issue, please complain.  I'll fix the
multi-line comment thing on my end before pushing the today's
integration result out.

Thanks.

Re: [PATCH v3] bisect: report the found commit with "show"

[Eric Sunshine]

On Mon, Apr 15, 2024 at 5:24 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> >> Pass some hard-coded options to "git show" to make the output similar
> >> to the one we are replacing, such as showing a patch summary only.
> >>
> >> Signed-off-By: Peter Krefting <peter@softwolves.pp.se>
> >> ---
>
> Curious how you trimmed the trailers from the submitted patch ;-)
>
> Although we do not use Cc: in this project, we do recommend use of
> the "Reported-by" trailer in Documentation/SubmittingPatches.

Sorry if that caused any confusion. I wasn't trying to make some sort
of implicit suggestion to the patch author (such as "don't use Cc:
trailers"). Rather, to ease the reading burden for others, I
habitually trim off chunks of the email which aren't relevant to the
portions about which I'm commenting. So, although I always leave the
Signed-off-by: intact, I often trim off other trailers, sometimes even
parts of the commit message (replacing with "[...]"), and, of course,
entire chunks of the patch body.

Re: [PATCH v3] bisect: report the found commit with "show"

[Jeff King]

On Mon, Apr 15, 2024 at 05:33:29PM -0400, Eric Sunshine wrote:

> > Curious how you trimmed the trailers from the submitted patch ;-)
> >
> > Although we do not use Cc: in this project, we do recommend use of
> > the "Reported-by" trailer in Documentation/SubmittingPatches.
> 
> Sorry if that caused any confusion. I wasn't trying to make some sort
> of implicit suggestion to the patch author (such as "don't use Cc:
> trailers"). Rather, to ease the reading burden for others, I
> habitually trim off chunks of the email which aren't relevant to the
> portions about which I'm commenting. So, although I always leave the
> Signed-off-by: intact, I often trim off other trailers, sometimes even
> parts of the commit message (replacing with "[...]"), and, of course,
> entire chunks of the patch body.

FWIW, as a reader I very much appreciate when people trim their quotes
to the most relevant bits.

-Peff

Re: [PATCH v3] bisect: report the found commit with "show"

[Peter Krefting]

Eric sunshine:

> Style nit: On this project, multi-line comments are formatted like this:

Indeed. Fixed this in v4.

Junio C Hamano:

>>> Signed-off-By: Peter Krefting <peter@softwolves.pp.se>
>>> ---
> Curious how you trimmed the trailers from the submitted patch ;-)

Yeah. I had some extra information in my local commit footer, to make 
sure I remember add the correct Cc in the outgoing mail. I didn't 
intend to remove the Reported-by trailer (and in v4 I see that I 
forgot to trim it at all; so much for posting stuff late in the 
evening, especially just after pushing a localization update).

> What is more problematic is that the message is sent with
>
> 	Content-Type: text/plain; format=flowed; charset=US-ASCII
>
> and the contents of the message is in that flawed format, possibly 
> corrupting whitespaces in irrecoverable ways.

Right. I need to remember to disable that when posting patches. I 
haven't come around to trying to get the Git built-in tools for sending 
patches, as the last patch I submitted this way was in 2009, two mail 
hosts ago. It oughtn't have been corrupted by that as it was just an 
import of the git format-patch output file, and...

> Peter, if the resulting commit I push out later today botches some 
> whitespaces due to this issue, please complain.

...it comes out just fine (as a v3.5 patch, as my v4 also changed the 
command-line options, which we should not).

Eric Sunshine:

> Sorry if that caused any confusion. I wasn't trying to make some 
> sort of implicit suggestion to the patch author (such as "don't use 
> Cc: trailers").

It was not you, it was me trimming the posted patch a little bit too 
much.

-- 
\\// Peter - http://www.softwolves.pp.se/