| Message ID | 965ae345-fd58-c46c-5a7a-de181e901f21@softwolves.pp.se (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] bisect: report the found commit with "show" | expand |
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.
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.
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.
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
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.
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)
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.