{"thread":{"id":"61315","subject":"[PATCH v3] bisect: report the found commit with \"show\"","startedAt":"2024-04-13T21:21:19Z","lastAt":"2024-04-16T19:44:30Z","messageCount":6,"participants":["Peter Krefting","Eric Sunshine","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"492908","messageId":"965ae345-fd58-c46c-5a7a-de181e901f21@softwolves.pp.se","threadId":"61315","inReplyTo":null,"subject":"[PATCH v3] bisect: report the found commit with \"show\"","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2024-04-13T20:14:48Z","receivedAt":"2024-04-13T21:21:19Z","isPatch":true,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"When \"git bisect\" finds the first bad commit and shows it to the user,\nit calls \"git diff-tree\" to do so, whose output is meant to be stable\nand deliberately ignores end-user customizations.\n\nAs the output is supposed to be consumed by humans, replace this with\na call to \"git show\". This command honors configuration options (such\nas \"log.date\" and \"log.mailmap\") and other UI improvements (renames\nare detected).\n\nPass some hard-coded options to \"git show\" to make the output similar\nto the one we are replacing, such as showing a patch summary only.\n\nReported-by: Michael Osipov \nSigned-off-By: Peter Krefting \nCc: Junio C Hamano \nCc: Jeff King \nCc: Christian Couder \n---\n bisect.c | 37 ++++++++++++++++++++++---------------\n 1 file changed, 22 insertions(+), 15 deletions(-)\n\nChanges compared to v2:\n- Adds --no-pager and a comment stating why we do so.\n- Adds --diff-merges=first-parent, as per previous discussion.\n\nThis seems to generate a final output as close to the previous one as \npossible, while honouring the user's settings for how to display log \nentries and calculating diffs.\n\ndiff --git a/bisect.c b/bisect.c\nindex 8487f8cd1b..3be2460c65 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -959,23 +959,30 @@ static enum bisect_error check_good_are_ancestors_of_bad(struct repository *r,\n }\n\n /*\n- * This does \"git diff-tree --pretty COMMIT\" without one fork+exec.\n+ * Display a commit summary to the user.\n */\n-static void show_diff_tree(struct repository *r,\n-\t\t\t const char *prefix,\n-\t\t\t struct commit *commit)\n+static void show_commit(struct commit *commit)\n {\n-\tconst char *argv[] = {\n-\t\t\"diff-tree\", \"--pretty\", \"--stat\", \"--summary\", \"--cc\", NULL\n-\t};\n-\tstruct rev_info opt;\n+\tstruct child_process show = CHILD_PROCESS_INIT;\n\n-\tgit_config(git_diff_ui_config, NULL);\n-\trepo_init_revisions(r, &opt, prefix);\n-\n-\tsetup_revisions(ARRAY_SIZE(argv) - 1, argv, &opt, NULL);\n-\tlog_tree_commit(&opt, commit);\n-\trelease_revisions(&opt);\n+\t/* Call git show with --no-pager, as it would otherwise\n+\t * paginate the \"git show\" output only, not the output\n+\t * from bisect_next_all(); this can be fixed by moving\n+\t * it into a --format parameter, but that would override\n+\t * the user's default options for \"git show\", which we\n+\t * are trying to honour. */\n+\tstrvec_pushl(&show.args,\n+\t \"--no-pager\",\n+\t \"show\",\n+\t \"--stat\",\n+\t \"--summary\",\n+\t \"--no-abbrev-commit\",\n+\t \"--diff-merges=first-parent\",\n+\t oid_to_hex(&commit->object.oid), NULL);\n+\tshow.git_cmd = 1;\n+\tif (run_command(&show))\n+\t\tdie(_(\"unable to start 'show' for object '%s'\"),\n+\t\t oid_to_hex(&commit->object.oid));\n }\n\n /*\n@@ -1092,7 +1099,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n \t\tprintf(\"%s is the first %s commit\\n\", oid_to_hex(bisect_rev),\n \t\t\tterm_bad);\n\n-\t\tshow_diff_tree(r, prefix, revs.commits->item);\n+\t\tshow_commit(revs.commits->item);\n \t\t/*\n \t\t * This means the bisection process succeeded.\n \t\t * Using BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10)\n-- \n2.39.2\n"},{"id":"492918","messageId":"CAPig+cQu15HzZkeT3+oG3U7iFax5_GYUB=uqwuJxshw-PD=VHQ@mail.gmail.com","threadId":"61315","inReplyTo":"965ae345-fd58-c46c-5a7a-de181e901f21@softwolves.pp.se","subject":"Re: [PATCH v3] bisect: report the found commit with \"show\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-14T01:28:40Z","receivedAt":"2024-04-14T01:28:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 13, 2024 at 5:21 PM Peter Krefting wrote:\n> When \"git bisect\" finds the first bad commit and shows it to the user,\n> it calls \"git diff-tree\" to do so, whose output is meant to be stable\n> and deliberately ignores end-user customizations.\n>\n> As the output is supposed to be consumed by humans, replace this with\n> a call to \"git show\". This command honors configuration options (such\n> as \"log.date\" and \"log.mailmap\") and other UI improvements (renames\n> are detected).\n>\n> Pass some hard-coded options to \"git show\" to make the output similar\n> to the one we are replacing, such as showing a patch summary only.\n>\n> Signed-off-By: Peter Krefting \n> ---\n> diff --git a/bisect.c b/bisect.c\n> @@ -959,23 +959,30 @@ static enum bisect_error check_good_are_ancestors_of_bad(struct repository *r,\n> +static void show_commit(struct commit *commit)\n> {\n> + /* Call git show with --no-pager, as it would otherwise\n> + * paginate the \"git show\" output only, not the output\n> + * from bisect_next_all(); this can be fixed by moving\n> + * it into a --format parameter, but that would override\n> + * the user's default options for \"git show\", which we\n> + * are trying to honour. */\n> + strvec_pushl(&show.args,\n> + \"--no-pager\",\n> + \"show\",\n> + \"--stat\",\n> + \"--summary\",\n> + \"--no-abbrev-commit\",\n> + \"--diff-merges=first-parent\",\n> + oid_to_hex(&commit->object.oid), NULL);\n\nStyle nit: On this project, multi-line comments are formatted like this:\n\n /*\n * This is a multi-line\n * comment.\n */\n\nIt also feels slightly odd to place each option on its own line in the\ncall to strvec_pushl() but then place the terminating NULL on the same\nline as the oid_to_hex() call. But that's a minor and subjective point\nhardly worth mentioning.\n"},{"id":"492966","messageId":"xmqq7cgyl3pr.fsf@gitster.g","threadId":"61315","inReplyTo":"CAPig+cQu15HzZkeT3+oG3U7iFax5_GYUB=uqwuJxshw-PD=VHQ@mail.gmail.com","subject":"Re: [PATCH v3] bisect: report the found commit with \"show\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-15T21:24:00Z","receivedAt":"2024-04-15T21:24:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> Pass some hard-coded options to \"git show\" to make the output similar\n>> to the one we are replacing, such as showing a patch summary only.\n>>\n>> Signed-off-By: Peter Krefting \n>> ---\n\nCurious how you trimmed the trailers from the submitted patch ;-)\n\nAlthough we do not use Cc: in this project, we do recommend use of\nthe \"Reported-by\" trailer in Documentation/SubmittingPatches.\n\n>> diff --git a/bisect.c b/bisect.c\n>> ...\n\n> Style nit: On this project, multi-line comments are formatted like this:\n>\n> /*\n> * This is a multi-line\n> * comment.\n> */\n\nTrue.\n\n> It also feels slightly odd to place each option on its own line in the\n> call to strvec_pushl() but then place the terminating NULL on the same\n> line as the oid_to_hex() call. But that's a minor and subjective point\n> hardly worth mentioning.\n\nI think that is because we may want to tweak the list of options,\nbut no matter what change we make to them in the future, the object\nname as the last parameter is likely to remain the last one, and\nwith that reasoning, I agree with the layout in the patch as posted.\n\nWhat is more problematic is that the message is sent with\n\n\tContent-Type: text/plain; format=flowed; charset=US-ASCII\n\nand the contents of the message is in that flawed format, possibly\ncorrupting whitespaces in irrecoverable ways.\n\nI _think_ \"git am\" (actually, \"git mailsplit\" that is called from\nit) did a reasonable job this time, but I do not have a lot of\nconfidence in the resulting commit---I would not be surprised if it\nis not identical to what Peter wanted to give us.\n\nPeter, if the resulting commit I push out later today botches some\nwhitespaces due to this issue, please complain. I'll fix the\nmulti-line comment thing on my end before pushing the today's\nintegration result out.\n\nThanks.\n"},{"id":"492967","messageId":"CAPig+cSY3vp6V=4SWmxyCi+7QY74eGnzPM6bu5LU5j00n8-j5g@mail.gmail.com","threadId":"61315","inReplyTo":"xmqq7cgyl3pr.fsf@gitster.g","subject":"Re: [PATCH v3] bisect: report the found commit with \"show\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-15T21:33:29Z","receivedAt":"2024-04-15T21:33:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 15, 2024 at 5:24 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> >> Pass some hard-coded options to \"git show\" to make the output similar\n> >> to the one we are replacing, such as showing a patch summary only.\n> >>\n> >> Signed-off-By: Peter Krefting \n> >> ---\n>\n> Curious how you trimmed the trailers from the submitted patch ;-)\n>\n> Although we do not use Cc: in this project, we do recommend use of\n> the \"Reported-by\" trailer in Documentation/SubmittingPatches.\n\nSorry if that caused any confusion. I wasn't trying to make some sort\nof implicit suggestion to the patch author (such as \"don't use Cc:\ntrailers\"). Rather, to ease the reading burden for others, I\nhabitually trim off chunks of the email which aren't relevant to the\nportions about which I'm commenting. So, although I always leave the\nSigned-off-by: intact, I often trim off other trailers, sometimes even\nparts of the commit message (replacing with \"[...]\"), and, of course,\nentire chunks of the patch body.\n"},{"id":"492980","messageId":"20240416051358.GB1745631@coredump.intra.peff.net","threadId":"61315","inReplyTo":"CAPig+cSY3vp6V=4SWmxyCi+7QY74eGnzPM6bu5LU5j00n8-j5g@mail.gmail.com","subject":"Re: [PATCH v3] bisect: report the found commit with \"show\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-16T05:13:58Z","receivedAt":"2024-04-16T05:14:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 15, 2024 at 05:33:29PM -0400, Eric Sunshine wrote:\n\n> > Curious how you trimmed the trailers from the submitted patch ;-)\n> >\n> > Although we do not use Cc: in this project, we do recommend use of\n> > the \"Reported-by\" trailer in Documentation/SubmittingPatches.\n> \n> Sorry if that caused any confusion. I wasn't trying to make some sort\n> of implicit suggestion to the patch author (such as \"don't use Cc:\n> trailers\"). Rather, to ease the reading burden for others, I\n> habitually trim off chunks of the email which aren't relevant to the\n> portions about which I'm commenting. So, although I always leave the\n> Signed-off-by: intact, I often trim off other trailers, sometimes even\n> parts of the commit message (replacing with \"[...]\"), and, of course,\n> entire chunks of the patch body.\n\nFWIW, as a reader I very much appreciate when people trim their quotes\nto the most relevant bits.\n\n-Peff\n"},{"id":"493004","messageId":"70e5d02e-7728-260b-40c3-a19cf68d9b0e@softwolves.pp.se","threadId":"61315","inReplyTo":"20240416051358.GB1745631@coredump.intra.peff.net","subject":"Re: [PATCH v3] bisect: report the found commit with \"show\"","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2024-04-16T19:44:21Z","receivedAt":"2024-04-16T19:44:30Z","isPatch":true,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"Eric sunshine:\n\n> Style nit: On this project, multi-line comments are formatted like this:\n\nIndeed. Fixed this in v4.\n\nJunio C Hamano:\n\n>>> Signed-off-By: Peter Krefting \n>>> ---\n> Curious how you trimmed the trailers from the submitted patch ;-)\n\nYeah. I had some extra information in my local commit footer, to make \nsure I remember add the correct Cc in the outgoing mail. I didn't \nintend to remove the Reported-by trailer (and in v4 I see that I \nforgot to trim it at all; so much for posting stuff late in the \nevening, especially just after pushing a localization update).\n\n> What is more problematic is that the message is sent with\n>\n> \tContent-Type: text/plain; format=flowed; charset=US-ASCII\n>\n> and the contents of the message is in that flawed format, possibly \n> corrupting whitespaces in irrecoverable ways.\n\nRight. I need to remember to disable that when posting patches. I \nhaven't come around to trying to get the Git built-in tools for sending \npatches, as the last patch I submitted this way was in 2009, two mail \nhosts ago. It oughtn't have been corrupted by that as it was just an \nimport of the git format-patch output file, and...\n\n> Peter, if the resulting commit I push out later today botches some \n> whitespaces due to this issue, please complain.\n\n...it comes out just fine (as a v3.5 patch, as my v4 also changed the \ncommand-line options, which we should not).\n\nEric Sunshine:\n\n> Sorry if that caused any confusion. I wasn't trying to make some \n> sort of implicit suggestion to the patch author (such as \"don't use \n> Cc: trailers\").\n\nIt was not you, it was me trimming the posted patch a little bit too \nmuch.\n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"}]}