{"thread":{"id":"61237","subject":"[PATCH v2] bisect: Honor log.date","startedAt":"2024-03-30T23:10:33Z","lastAt":"2024-04-20T17:14:07Z","messageCount":13,"participants":["Peter Krefting","Junio C Hamano","Jeff King","Christian Couder"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"491912","messageId":"3ec4ec15-8889-913a-1184-72e55a1e0432@softwolves.pp.se","threadId":"61237","inReplyTo":null,"subject":"[PATCH v2] bisect: Honor log.date","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2024-03-30T23:10:24Z","receivedAt":"2024-03-30T23:10:33Z","isPatch":true,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"When bisect finds the target commit to display, it calls git diff-tree\nto do so. This is a plumbing command that is not affected by the user's\nlog.date setting. Switch to instead use \"git show\", which does honor\nit.\n\nReported-by: Michael Osipov <michael.osipov@innomotics.com>\nSigned-off-By: Peter Krefting <peter@softwolves.pp.se>\n---\n  bisect.c | 25 ++++++++++---------------\n  1 file changed, 10 insertions(+), 15 deletions(-)\n\nThis version also uses \"--stat\" which produces an output more like the \none from the diff-tree utility.\n\nGitHub's test run reports a single failed test (7300), but this passes \nwhen I try it locally: \nhttps://github.com/nafmo/git-l10n-sv/commit/2f27ae64064edc5c2570f1c9ea121f3f1a7283d7\n\ndiff --git a/bisect.c b/bisect.c\nindex 8487f8cd1b..3d0100b165 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -959,23 +959,18 @@ 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+ * Runs \"git show\" to display a commit\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+\tstrvec_pushl(&show.args, \"show\", \"--pretty=medium\", \"--stat\", \"--no-abbrev-commit\", \"--no-patch\",\n+\t\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 +1087,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\n"},{"id":"491914","messageId":"xmqqh6gni1ur.fsf@gitster.g","threadId":"61237","inReplyTo":"3ec4ec15-8889-913a-1184-72e55a1e0432@softwolves.pp.se","subject":"Re: [PATCH v2] bisect: Honor log.date","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-31T02:14:52Z","receivedAt":"2024-03-31T02:15:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Krefting <peter@softwolves.pp.se> writes:\n\n> When bisect finds the target commit to display, it calls git diff-tree\n> to do so. This is a plumbing command that is not affected by the user's\n> log.date setting. Switch to instead use \"git show\", which does honor\n> it.\n\nI suspect that log.date is a small tip of an iceberg of the benefit\nwe'll get from this switch.  There is an untold assumption that\nhonoring the user's configuration is a good thing behind the move\nagainst \"plumbing\" in the above description, but singling log.date\nout would give a wrong message.  It makes it harder to answer a\nquestion, \"The commit meant to make the command honor `log.date` and\nmake no other behaviour changes, but there are many small behaviour\nchanges---are they intended?\", when somebody reads this commit log\nmessage after we all forgot about the true motivation behind the\nchange.\n\n    Subject: [PATCH vN] bisect: report the final commit with \"show\"\n\n    When \"git bisect\" finds the first bad commit and shows it to the\n    user, it calls \"git diff-tree\", whose output is meant to be\n    stable and deliberately ignores end-user customizations.\n\n    As this output is meant to be consumed by humans, let's switch\n    it to use \"git show\" so that we honor end-user customizations\n    via the configuration mechanism (e.g., \"log.mailmap\") and\n    benefit from UI improvements meant for human consumption (e.g.,\n    the output is sent to the pager) in \"git show\" relative to \"git\n    diff-tree\".\n\n    We have to give \"git show\" some hardcoded options, like not\n    showing the patch text at all, as the patch is too much for the\n    purpose of \"git bisect\" reporting the final commit.\n\nwould be how I would explain and justify this change.  If we later\nadd more configuration to tweak \"git show\" output, it will affect\nthe output from \"git bisect\" automatically, which is another thing\nyou may want to explain and use as another reason to justify the\nchange (in the second paragraph).\n\nSome differences in the proposed output and the current output I see\nare:\n\n - the output now goes to the pager\n\n - it now honors log.mailmap (which may default to true, so you\n   could disable it with log.mailmap=false).\n\n - it shows the ref decoration by default (when the output goes to\n   terminal).\n\n - the commit object names for the merge parents are abbreviated.\n\n - it no longer shows the change summary (creation, deletion,\n   rename, copy).\n\n - it no longer shows the diffstat when the final commit turns out\n   to be a merge commit.\n\nThere may be other differences.\n\nI personally welcome the first four changes above, which I suspect\nyou didn't intend to make (I suspect that you weren't even aware of\nmaking these changes).\n\nIf there were no existing users of \"git bisect\" other than me, I\nwould even suggest dropping \"--no-abbrev-commit\" from the set of\nhardcoded \"git show\" options, so that the commit object name itself,\njust like the commit object names for the merge parents, gets\nabbreviated.  The abbreviation is designed to give us unique prefix,\nso for the purpose of cutting and pasting from the output to some\nother Git command, it should not break my workflow.  If some tool is\nreading the output and blindly assuming that the object names are\nspelled in full, such a change will break it.\n\nThe final two changes, lack of diffstat for merges, may or may not\nbe considered a regression, depending on the user you ask.  I was\njust surprised by them but personally was not too unhappy with the\nbehaviour change, but reactions from other couple of thousands of\nGit users (we have at least that many users these days, no?) may be\ndifferent from mine, ranging from \"Meh\" to \"you broke my workflow\".\n\nA good test case to try is to do a bisection that finds c2f3bf07\n(GIT 1.0.0, 2005-12-21) with and without your patch and compare\nthe output from them.  I say it is \"good test case\", not because\nI view any difference is a bug in this patch, but because many\ndifferences are probably good things that helps us to promote the\nbehaviour changes.  They just need to be explained in the proposed\nlog message to tell our future developers that we knew about these\nbehaviour changes and we meant to make them.\n\nHaving said all that.\n\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\nIt is very good that we no longer use the separate argv[] array and\nuse the more convenient strvec_pushl() call, which will make it\neasier for us to later tweak the arguments we pass to the command\ninvocation dynamically if needed.\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\nAnd not doing this in process lets us not have to bother with the\nconfiguration and other things we did in the original.  We now spawn\nan extra process to show the final commit, but this is done only at\nthe very end of a bisection session, so it shouldn't matter.\n\n> +\tstrvec_pushl(&show.args, \"show\", \"--pretty=medium\", \"--stat\", \"--no-abbrev-commit\", \"--no-patch\",\n> +\t\t     oid_to_hex(&commit->object.oid), NULL);\n\nI would write it either like this:\n\n\tstrvec_pushl(&show.args, \"show\",\n\t\t     \"--pretty=medium\", \"--stat\",\n\t\t     \"--no-abbrev-commit\", \"--no-patch\",\n\t\t     oid_to_hex(&commit->object.oid), NULL);\n\nin anticipation for changing the set of options over the evolution\nof this code (but the first \"show\" line or the last \"oid_to_hex()\"\nline would have much less chance of needing to change), or even\nspread the middle part one-option-per-line.\n\nAs to the exact set of options to pass to \"git show\", the preference\nwould be different from person to person, but I probably would drop\n\"--pretty=medium\", as it is the default and if/when \"git show\"\nlearns to tweak it via configuration variable, you would want the\noutput from here honor it just like you wanted it honor `log.date`.\nI would not be too unhappy to see `--no-abbrev-commit` to go myself,\nbut some tool authors might hate you if you did so.  I dunno.\n\nIf you add --stat, don't you want to add --summary as well?  Try to\nbisect down to a commit that adds or removes files to see the output\ndifference to decide.\n\nThanks.\n\n\n\n\n"},{"id":"491924","messageId":"5ea0837f-2668-028d-4094-c9400e92fceb@softwolves.pp.se","threadId":"61237","inReplyTo":"xmqqh6gni1ur.fsf@gitster.g","subject":"Re: [PATCH v2] bisect: Honor log.date","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2024-03-31T17:10:32Z","receivedAt":"2024-03-31T17:10:38Z","isPatch":true,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"Junio C Hamano:\n\n> I suspect that log.date is a small tip of an iceberg of the benefit\n> we'll get from this switch.\n\nYeah. I was planning on elaborating a bit on that, but forgot \ncompletely by the time I came around to look at it. I will update the \nmessage with your suggestions for the next version.\n\n> Some differences in the proposed output and the current output I see\n> are:\n>\n> - the output now goes to the pager\n>\n> - it now honors log.mailmap (which may default to true, so you\n>   could disable it with log.mailmap=false).\n>\n> - it shows the ref decoration by default (when the output goes to\n>   terminal).\n>\n> - the commit object names for the merge parents are abbreviated.\n>\n> - it no longer shows the change summary (creation, deletion,\n>   rename, copy).\n>\n> - it no longer shows the diffstat when the final commit turns out\n>   to be a merge commit.\n>\n> There may be other differences.\n>\n> I personally welcome the first four changes above, which I suspect\n> you didn't intend to make (I suspect that you weren't even aware of\n> making these changes).\n\nI hadn't really noticed that the previous implementation *didn't* \ndisplay this. For the most part, the final output of 'bisect' looks \nlike what I expect 'show' to display, to me it was mostly missing the \nother things.\n\n> If there were no existing users of \"git bisect\" other than me, I\n> would even suggest dropping \"--no-abbrev-commit\" from the set of\n> hardcoded \"git show\" options, so that the commit object name itself,\n> just like the commit object names for the merge parents, gets\n> abbreviated.\n\nThe full commit hash is shown in the line above anyway, so that entire \nline is redundant. But since there is no standard format available \nthat omits the commit hash I thought I'd leave it at the full hash to \nbe the most like the previous behaviour as possible.\n\n> The final two changes, lack of diffstat for merges, may or may not\n> be considered a regression, depending on the user you ask.  I was\n> just surprised by them but personally was not too unhappy with the\n> behaviour change, but reactions from other couple of thousands of\n> Git users (we have at least that many users these days, no?) may be\n> different from mine, ranging from \"Meh\" to \"you broke my workflow\".\n\nThose two were not intentional. I'll have to do a few test runs to \ncompare the outputs and try to the change as non-intrusive as \npossible. Thanks.\n\n> If you add --stat, don't you want to add --summary as well?  Try to\n> bisect down to a commit that adds or removes files to see the output\n> difference to decide.\n\nThere are a lot of parameters to show that I have haven't used in my \n14+ years of using Git, --summary is one of them. That's why I didn't \nadd it.\n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"},{"id":"491933","messageId":"xmqq7chif1pu.fsf@gitster.g","threadId":"61237","inReplyTo":"5ea0837f-2668-028d-4094-c9400e92fceb@softwolves.pp.se","subject":"Re: [PATCH v2] bisect: Honor log.date","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-31T22:58:21Z","receivedAt":"2024-03-31T22:58:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Krefting <peter@softwolves.pp.se> writes:\n\n> There are a lot of parameters to show that I have haven't used in my\n> 14+ years of using Git, --summary is one of them. That's why I didn't\n> add it.\n\nYup, that is semi-understandable, but especially given that it is\none of the options used by the original \"diff-tree\"'s invocation,\nand that we are trying to replace it with \"show\" from the same\nfamily of commands, it is a bit of disappointment.\n\nWe know we used to drive \"diff-tree\" with a known set of options,\nand we are replacing the command to use \"show\" with some other set\nof options.  I expected it to be fairly straight-forward and natural\nto feed randomly picked commits to the two commands and compare\ntheir output while deciding what that \"some other set of options\"\nshould be.  It is exactly the reason why I mentioned v1.0.0^0 is a\ngood test case.\n\nAgain, the output from them do not have to be identical---we are\nprimarily after catching unintended loss of informatino in such a\ncomparison, while gaining more confidence that it is a better\napproach to use \"show\" output to produce output for end-user\nconsumption.\n\nWe have changed the bisect output before, as recent as in 2019 with\nb02be8b9 (bisect: make diff-tree output prettier, 2019-02-22), and\nheard nobody complain, so once we get to a reasonable set of options\nand land this patch, maybe we can try improving on it safely.\n\nFYI, attached is a comparison between the diff-tree output and\noutput from show with my choice of options for \"show\" picked from\nthe top of my head.  I do not think I personally like the --stat\noutput applied to a merge (--stat and --summary do not work N-way\nlike --cc does for patch text), but I think these options are the\nclosest parallel to what we have been giving to \"diff-tree\".\n\nThanks.\n\n---------------------- >8 ----------------------\n$ git diff-tree --pretty --stat --summary --cc v1.0.0^0\ncommit c2f3bf071ee90b01f2d629921bb04c4f798f02fa\nMerge: 1ed91937e5cd59fdbdfa5f15f6fac132d2b21ce0 41f93a2c903a45167b26c2dc93d45ffa9a9bbd49\nAuthor: Junio C Hamano <junkio@cox.net>\nDate:   Wed Dec 21 00:01:00 2005 -0800\n\n    GIT 1.0.0\n    \n    Signed-off-by: Junio C Hamano <junkio@cox.net>\n\n .gitignore                                       |   1 -\n Documentation/diff-options.txt                   |   8 +\n ...\n tar-tree.c                                       |   4 +-\n unpack-objects.c                                 |  13 +-\n 66 files changed, 778 insertions(+), 617 deletions(-)\n delete mode 100644 Documentation/git-octopus.txt\n ...\n mode change 100644 => 100755 t/t5500-fetch-pack.sh\n mode change 100644 => 100755 t/t6101-rev-parse-parents.sh\n\n---------------------- >8 ----------------------\n$ git show -s --stat --summary --first-parent v1.0.0^0\ncommit c2f3bf071ee90b01f2d629921bb04c4f798f02fa\nMerge: 1ed91937e5 41f93a2c90\nAuthor: Junio C Hamano <gitster@pobox.com>\nDate:   Wed Dec 21 00:01:00 2005 -0800\n\n    GIT 1.0.0\n    \n    Signed-off-by: Junio C Hamano <junkio@cox.net>\n\n .gitignore                                       |   1 -\n Documentation/diff-options.txt                   |   8 +\n ...\n tar-tree.c                                       |   4 +-\n unpack-objects.c                                 |  13 +-\n 66 files changed, 778 insertions(+), 617 deletions(-)\n delete mode 100644 Documentation/git-octopus.txt\n ...\n mode change 100644 => 100755 t/t5500-fetch-pack.sh\n mode change 100644 => 100755 t/t6101-rev-parse-parents.sh\n"},{"id":"491938","messageId":"20240401023225.GA2639800@coredump.intra.peff.net","threadId":"61237","inReplyTo":"xmqq7chif1pu.fsf@gitster.g","subject":"Re: [PATCH v2] bisect: Honor log.date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-01T02:32:25Z","receivedAt":"2024-04-01T02:32:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 31, 2024 at 03:58:21PM -0700, Junio C Hamano wrote:\n\n> Again, the output from them do not have to be identical---we are\n> primarily after catching unintended loss of informatino in such a\n> comparison, while gaining more confidence that it is a better\n> approach to use \"show\" output to produce output for end-user\n> consumption.\n> \n> We have changed the bisect output before, as recent as in 2019 with\n> b02be8b9 (bisect: make diff-tree output prettier, 2019-02-22), and\n> heard nobody complain, so once we get to a reasonable set of options\n> and land this patch, maybe we can try improving on it safely.\n\nI guess that commit is what brought me into the cc. I have not been\nfollowing this topic too closely, but generally I'm in favor of using\n\"git show\". I even suggested it back then, but I think Christian\npreferred not using an external process if we could avoid it.\n\nThe thread from 2019 is here:\n\n  http://lore.kernel.org/git/20190222061949.GA9875@sigill.intra.peff.net\n\nwhich links to the earlier discussion about \"git show\":\n\n  https://lore.kernel.org/git/CAP8UFD3QhTUj+j3vBGrm0sTQ2dSOLS-m2_PwFj6DZS4VZHKRTQ@mail.gmail.com/\n\nIMHO this config thing is a good example of the strength of the separate\n\"show\" process. If our goal is to trigger all the niceties of \"git\nshow\", it is tricky to catch them all. The revision machinery is pretty\nreusable, but there's no easy way to figure out which config affects\ngit-show and so on. Of course if we had a way to invoke git-show\nin-process that would work, but I suspect there are unexpected corner\ncases that might trigger.\n\n> FYI, attached is a comparison between the diff-tree output and\n> output from show with my choice of options for \"show\" picked from\n> the top of my head.  I do not think I personally like the --stat\n> output applied to a merge (--stat and --summary do not work N-way\n> like --cc does for patch text), but I think these options are the\n> closest parallel to what we have been giving to \"diff-tree\".\n\nI think it was me who added the --cc in 2019; before then we simply\nshowed nothing at all for merges. I am inclined to say that --cc is not\nreally that useful for a bisection at all. If a merge introduces a bug,\nit _might_ come from a resolved hunk that would be shown by --cc\n--patch, but it is just as likely to me that there is some semantic\nconflict between the two sides.\n\nFor a workflow like the one we use in git.git, where we are merging\ntopic branches to a long-running branch, I think that showing the\n-stat/--summary against the first parent is what you want. We know that\nthings worked in merge^1, so we show the changes brought in by merge^2.\nThat does not necessarily mean that the changes in merge^1 are not to\nblame, but at least the changes in merge^2 give you a good place to\nstart.\n\nFor a workflow where you do lots of back-merges, it is less clear that\nshowing the changes against the first parent is better than against\nothers. But I still think the \"well, at least showing the changes\nagainst one parent gives you an idea of where to start looking\" logic\napplies. And showing the \"-m\" diff against every parent often has a lot\nof useless noise.\n\nI don't think I considered all this back when adding --cc in 2019. But I\nbelieve that \"--stat --cc\" is just showing the diff against the first\nparent. Which happens to be what I think this useful, biased of course\nby the fact that projects I work on tend to use a topic-branch workflow. ;)\n\nArguably passing \"--diff-merges=first-parent\" would more directly\nexpress the intent (I don't think that existed back in 2019).\n\nOf course with \"git show\" we do not need to even say anything, since\n\"--cc\" is the default and it does what we (I) want.\n\n(I was puzzled that earlier in the thread you said \"it no longer shows\nthe diffstat when the final commit turns out to be a merge commit\". It\nlooks like it still does?).\n\nI do think keeping --summary is important; it's the only place we show\nmode changes, for example.\n\nThe other changes you outlined all seem like improvements to me.\n\nLooking at Peter's patch, I think:\n\n  - \"--no-patch\" is doing nothing (passing --stat is enough to suppress\n    the default behavior of showing the patch).\n\n  - \"--pretty=medium\" is redundant at best (it's the default), and\n    possibly overriding a different decision \"show\" might make (I don't\n    remember if we have a way for a user to configure the default show\n    format for commits, but if we did, I think users would expect it to\n    kick in here)\n\n  - I'm not sure what the intent is in adding --no-abbrev-commit. It is\n    already the default not to abbreviate it in the \"commit <oid>\" line,\n    and if the user has set log.abbrevcommit, shouldn't we respect that?\n    It seems to me the point of the patch is that \"git show\" represents\n    the way users expect to see commits shown, and we should let it do\n    its thing as much as possible.\n\n-Peff\n"},{"id":"491964","messageId":"c13c0751-0758-e068-282e-eb43496213b8@softwolves.pp.se","threadId":"61237","inReplyTo":"20240401023225.GA2639800@coredump.intra.peff.net","subject":"Re: [PATCH v2] bisect: Honor log.date","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2024-04-01T15:50:32Z","receivedAt":"2024-04-01T15:50:44Z","isPatch":true,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"Junio C Hamano:\n\n> Yup, that is semi-understandable, but especially given that it is \n> one of the options used by the original \"diff-tree\"'s invocation, \n> and that we are trying to replace it with \"show\" from the same \n> family of commands, it is a bit of disappointment.\n\nIndeed. I will make the necessary adjustments.\n\n> FYI, attached is a comparison between the diff-tree output and \n> output from show with my choice of options for \"show\" picked from \n> the top of my head.\n\nI am trying to run some comparisons, but I'm not entirely certain what \nthe parameters are that were passed to \"ls-tree\", as it doesn't \nactually run it through a command line. I tried the v1.0.0^0 and are \nseeing discrepancies in the line count. I need to check if it is my \nconfiguration that causes it, or something else:\n\n   $ git diff-tree --pretty --stat --summary --cc v1.0.0^0 | grep clone-pack.c\n    clone-pack.c                                     | 153 ++----------------\n   $ git show --stat --summary --no-abbrev-commit v1.0.0^0 | grep clone-pack.c\n    clone-pack.c                                     | 151 ++----------------\n\n(these are the options I've currently landed on)\n\n> I do not think I personally like the --stat output applied to a \n> merge (--stat and --summary do not work N-way like --cc does for \n> patch text), but I think these options are the closest parallel to \n> what we have been giving to \"diff-tree\".\n\nI don't really have a preference here. I usually only look at when \nsomething changed (which is why I initially targetted the date format; \nin Sweden the YYYY-MM-DD date format is the most prevalent) and the \ncommit message (for bug tracker and code-review references and so on), \nless so the actual diff details (those I can look into later).\n\n> $ git show -s --stat --summary --first-parent v1.0.0^0\n\nHmm, the git show manual page doesn't document supporting \n\"--first-parent\".\n\nJeff King:\n\n> I guess that commit is what brought me into the cc. I have not been \n> following this topic too closely, but generally I'm in favor of \n> using \"git show\". I even suggested it back then, but I think \n> Christian preferred not using an external process if we could avoid \n> it.\n\nI saw the code that tried to avoid calling one. I don't know the \ninternals well enough here to figure out if we can do without, even \nwhen using git show?\n\n\n\nThat made me realize, if \"git show\" runs things through a pager, \nwouldn't it then lose the \"%s is the first %s commit\\n\" message \nprinted by bisect_next_all() before calling the function to show the \ncontents?\n\nIs that fixable?\n\n> The thread from 2019 is here:\n>\n>  http://lore.kernel.org/git/20190222061949.GA9875@sigill.intra.peff.net\n>\n> which links to the earlier discussion about \"git show\":\n>\n>  https://lore.kernel.org/git/CAP8UFD3QhTUj+j3vBGrm0sTQ2dSOLS-m2_PwFj6DZS4VZHKRTQ@mail.gmail.com/\n\nThese two seems to also get it to honor settings (one to not colorize \noutput, for instance). So this would be a step further.\n\n> I do think keeping --summary is important; it's the only place we show\n> mode changes, for example.\n\nYes, will fix that. I hadn't realized I lost that, since it wasn't \nsomething I have been using myself.\n\n>  - \"--no-patch\" is doing nothing (passing --stat is enough to suppress\n>    the default behavior of showing the patch).\n\nIndeed. And it also negates \"--summary\", so I have dropped that.\n\n>  - \"--pretty=medium\" is redundant at best (it's the default),\n\nDropped.\n\n>  - I'm not sure what the intent is in adding --no-abbrev-commit. It is\n>    already the default not to abbreviate it in the \"commit <oid>\" line,\n>    and if the user has set log.abbrevcommit, shouldn't we respect that?\n\nI think I added it because the diff-tree command did something \nsimilar. I can drop that as well (\"bisect\" displays the full commit \nhash anyway). I guess it mostly is for merges where we show the parent \nhashes?\n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"},{"id":"491967","messageId":"20240401163209.GB3120568@coredump.intra.peff.net","threadId":"61237","inReplyTo":"c13c0751-0758-e068-282e-eb43496213b8@softwolves.pp.se","subject":"Re: [PATCH v2] bisect: Honor log.date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-01T16:32:09Z","receivedAt":"2024-04-01T16:32:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 01, 2024 at 04:50:32PM +0100, Peter Krefting wrote:\n\n> I am trying to run some comparisons, but I'm not entirely certain what the\n> parameters are that were passed to \"ls-tree\", as it doesn't actually run it\n> through a command line. I tried the v1.0.0^0 and are seeing discrepancies in\n> the line count. I need to check if it is my configuration that causes it, or\n> something else:\n> \n>   $ git diff-tree --pretty --stat --summary --cc v1.0.0^0 | grep clone-pack.c\n>    clone-pack.c                                     | 153 ++----------------\n>   $ git show --stat --summary --no-abbrev-commit v1.0.0^0 | grep clone-pack.c\n>    clone-pack.c                                     | 151 ++----------------\n> \n> (these are the options I've currently landed on)\n\nHmm, I get 153 for both. Presumably it's due to some config that only\ngit-show respects...\n\nAha. If I set diff.algorithm to \"patience\", I get 151. And I think\nbisect would produce the same, because it loads diff_ui_config() before\nrunning the internal diff-tree.\n\nSo I think this is fine.\n\n> > $ git show -s --stat --summary --first-parent v1.0.0^0\n> \n> Hmm, the git show manual page doesn't document supporting \"--first-parent\".\n\nI think that's a documentation bug(-ish). We do not include all of the\ntraversal-related options that \"git log\" could use because \"git show\"\ndoes not traverse by default. But it does also affect diffs, per the\ncomment added to git-log's documentation in e58142add4\n(doc/rev-list-options: document --first-parent changes merges format,\n2020-12-21).\n\nBut these days we have \"--diff-merges=first-parent\", which I think is a\nmore intuitive way to specify the same thing for git-show. And it is\ndocumented. So I'd say we could probably continue to not mention\n\"--first-parent\" itself for git-show.\n\n> Jeff King:\n> \n> > I guess that commit is what brought me into the cc. I have not been\n> > following this topic too closely, but generally I'm in favor of using\n> > \"git show\". I even suggested it back then, but I think Christian\n> > preferred not using an external process if we could avoid it.\n> \n> I saw the code that tried to avoid calling one. I don't know the internals\n> well enough here to figure out if we can do without, even when using git\n> show?\n\nThere's not really an easy way.\n\nI think the only thing you could do is call cmd_show(), but I'm\nskeptical of that approach in general. The builtin top-level commands\nare not designed to be run from other spots. And while it will generally\nwork, there will be corner cases (e.g., loading config that touches\nglobals, affecting the calling command in unexpected ways). I suspect\nyou could largely get away with it here where showing the commit is the\nlast thing we do, but I don't think it's a good pattern to get into.\n\n> That made me realize, if \"git show\" runs things through a pager, wouldn't it\n> then lose the \"%s is the first %s commit\\n\" message printed by\n> bisect_next_all() before calling the function to show the contents?\n> \n> Is that fixable?\n\nGood catch. IMHO we should disable the pager entirely by sticking\n\"--no-pager\" at the front of the child argv. But then, maybe somebody\nwould like the output to be paged? I wouldn't.\n\nIf we really wanted to keep the pager for git-show, I guess we'd need to\nhave it print the \"%s is the first %s commit\" message. The only way I\ncan think to do that is to pass it as a custom --format. But then we'd\nneed to additionally specify all of the usual \"medium\" format as a\ncustom format, too, which is quite ugly.\n\n> I think I added it because the diff-tree command did something similar. I\n> can drop that as well (\"bisect\" displays the full commit hash anyway). I\n> guess it mostly is for merges where we show the parent hashes?\n\nNo, I don't think the \"Merge:\" lines are affected by it either way.\nThose are always abbreviated, and looking at pretty.c's\nadd_merge_info(), I don't think there is any config that affects it.\n\n-Peff\n"},{"id":"491968","messageId":"xmqqmsqd9fse.fsf@gitster.g","threadId":"61237","inReplyTo":"20240401163209.GB3120568@coredump.intra.peff.net","subject":"Re: [PATCH v2] bisect: Honor log.date","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-01T17:03:13Z","receivedAt":"2024-04-01T17:03:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So I think this is fine.\n>\n>> > $ git show -s --stat --summary --first-parent v1.0.0^0\n>> \n>> Hmm, the git show manual page doesn't document supporting \"--first-parent\".\n>\n> I think that's a documentation bug(-ish). We do not include all of the\n> traversal-related options that \"git log\" could use because \"git show\"\n> does not traverse by default. But it does also affect diffs, per the\n> comment added to git-log's documentation in e58142add4\n> (doc/rev-list-options: document --first-parent changes merges format,\n> 2020-12-21).\n\nIt's one of the \"show is a command in the log family, so some of the\noptions that are appropriate to log applies there\".  The ones that\nare not useful are the ones about commit walking (e.g., \"git show\n--no-merges seen\" would probably show nothing), but many are still\nrelevant.  After all \"git show\" is a \"git log --no-walk --cc\" in\ndisguise.  The \"--first-parent\" option affects both traversal (which\nis useless in the context of \"git show\" that does not walk) and also\ndiff generation (which does make it show the diffstat/summary/patch\nrelative to the first parent), as you two saw.\n\n>> I saw the code that tried to avoid calling one. I don't know the internals\n>> well enough here to figure out if we can do without, even when using git\n>> show?\n>\n> There's not really an easy way.\n\nTrue, but this is \"we show the single commit we found before\nexiting\"; executing \"git show\" as an external program is fine and\nnot worth \"optimizing out\" the cost of starting another process.\n\n> I think the only thing you could do is call cmd_show(), but I'm\n> skeptical of that approach in general. The builtin top-level commands\n> are not designed to be run from other spots. And while it will generally\n> work, there will be corner cases (e.g., loading config that touches\n> globals, affecting the calling command in unexpected ways). I suspect\n> you could largely get away with it here where showing the commit is the\n> last thing we do, but I don't think it's a good pattern to get into.\n\nExactly.  Anybody who turns run_command(\"foo\") into blindly calling\ncmd_foo() should be shot, twice ;-).  The right way to turn\nrun_command(\"foo\") into an internal call is not to call cmd_foo(),\nbut to refactor cmd_foo() into the part that sets up the global\nstate and the part that does the \"foo\" thing, and make the latter a\nreusable function.\n\nIn the longer run, if we had infinite engineering resources, it\nwould be nice to have everything callable by everything else\ninternally, is it worth doing for this case?  I dunno.\n\n>> That made me realize, if \"git show\" runs things through a pager, wouldn't it\n>> then lose the \"%s is the first %s commit\\n\" message printed by\n>> bisect_next_all() before calling the function to show the contents?\n>> \n>> Is that fixable?\n>\n> Good catch. IMHO we should disable the pager entirely by sticking\n> \"--no-pager\" at the front of the child argv. But then, maybe somebody\n> would like the output to be paged? I wouldn't.\n\nHardcoded --no-pager is a good workaround.  But if the output is\nlong and needs paging, wouldn't we see what was shown before we\nspawned \"less\" on the screen when we quit it?  Running\n\n    $ (echo message here ; git log --help)\n\nand then saying 'q' to exit the pager leaves me \"message\" after that\ncommand line.\n\n> If we really wanted to keep the pager for git-show, I guess we'd need to\n> have it print the \"%s is the first %s commit\" message. The only way I\n> can think to do that is to pass it as a custom --format. But then we'd\n> need to additionally specify all of the usual \"medium\" format as a\n> custom format, too, which is quite ugly.\n\n;-)  Ugly but fun.\n\nI wonder how hard it is to add %(default-output) placeholder for the\npretty machinery.\n\nThanks.\n\n"},{"id":"492128","messageId":"20240403012701.GC892394@coredump.intra.peff.net","threadId":"61237","inReplyTo":"xmqqmsqd9fse.fsf@gitster.g","subject":"Re: [PATCH v2] bisect: Honor log.date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-03T01:27:01Z","receivedAt":"2024-04-03T01:27:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 01, 2024 at 10:03:13AM -0700, Junio C Hamano wrote:\n\n> >> That made me realize, if \"git show\" runs things through a pager, wouldn't it\n> >> then lose the \"%s is the first %s commit\\n\" message printed by\n> >> bisect_next_all() before calling the function to show the contents?\n> >> \n> >> Is that fixable?\n> >\n> > Good catch. IMHO we should disable the pager entirely by sticking\n> > \"--no-pager\" at the front of the child argv. But then, maybe somebody\n> > would like the output to be paged? I wouldn't.\n> \n> Hardcoded --no-pager is a good workaround.  But if the output is\n> long and needs paging, wouldn't we see what was shown before we\n> spawned \"less\" on the screen when we quit it?  Running\n> \n>     $ (echo message here ; git log --help)\n> \n> and then saying 'q' to exit the pager leaves me \"message\" after that\n> command line.\n\nThat depends on your \"less\" options and your terminal, I think. Aren't\nthere some combinations where the terminal deinit sequence clears the\nscreen? It has been a while since I've run into that, though, so I might\nbe misremembering.\n\nAt any rate, my concerns are more:\n\n  1. You wouldn't see it while the pager is active, so you are missing\n     some context.\n\n  2. If you don't use LESS=F, then it may be annoying to invoke the\n     pager at all.\n\n-Peff\n\n> > If we really wanted to keep the pager for git-show, I guess we'd need to\n> > have it print the \"%s is the first %s commit\" message. The only way I\n> > can think to do that is to pass it as a custom --format. But then we'd\n> > need to additionally specify all of the usual \"medium\" format as a\n> > custom format, too, which is quite ugly.\n> \n> ;-)  Ugly but fun.\n> \n> I wonder how hard it is to add %(default-output) placeholder for the\n> pretty machinery.\n\nI have a dream that all of the pretty formats could be implemented in\nterms of %-placeholders. But yeah, even without that, being able to do\n\"%(pretty:medium)\" would be cool. \"Pretty\" cool, even. (Sorry, I could\nnot resist).\n\n-Peff\n"},{"id":"492992","messageId":"CAP8UFD0W7PUHTg2NwuVkQJik2+HqTDF6KRZZ8tA_dW7-YZtsbQ@mail.gmail.com","threadId":"61237","inReplyTo":"20240401023225.GA2639800@coredump.intra.peff.net","subject":"Re: [PATCH v2] bisect: Honor log.date","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-04-16T11:01:34Z","receivedAt":"2024-04-16T11:01:47Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Apr 1, 2024 at 4:32 AM Jeff King <peff@peff.net> wrote:\n\n> I guess that commit is what brought me into the cc. I have not been\n> following this topic too closely, but generally I'm in favor of using\n> \"git show\". I even suggested it back then, but I think Christian\n> preferred not using an external process if we could avoid it.\n>\n> The thread from 2019 is here:\n>\n>   http://lore.kernel.org/git/20190222061949.GA9875@sigill.intra.peff.net\n>\n> which links to the earlier discussion about \"git show\":\n>\n>   https://lore.kernel.org/git/CAP8UFD3QhTUj+j3vBGrm0sTQ2dSOLS-m2_PwFj6DZS4VZHKRTQ@mail.gmail.com/\n>\n> IMHO this config thing is a good example of the strength of the separate\n> \"show\" process. If our goal is to trigger all the niceties of \"git\n> show\", it is tricky to catch them all. The revision machinery is pretty\n> reusable, but there's no easy way to figure out which config affects\n> git-show and so on. Of course if we had a way to invoke git-show\n> in-process that would work, but I suspect there are unexpected corner\n> cases that might trigger.\n\nSorry for not following the topic closely and for replying to this so\nlate, but I think that by now we should have some kind of guidelines\nabout when forking a new process is Ok and when it's not.\n\nIt seems to me that there was already some amount of back and forth on\nthis topic when bisect and other shell commands were ported to C a\nlong time ago. There weren't clearly written guidelines, but it seems\nto me that at that time we thought that forking a new process was\ngenerally bad, especially for performance reasons, but also because\nthey showed a bad example and didn't go in the right direction. It\nseems to me that people who reviewed code that ported some commands to\nC sometimes asked contributors to not fork processes, and efforts were\nmade by contributors, like GSoC or Outreachy contributors I mentored,\nto go in this direction. At one point there was even a microproject\nabout replacing code that forked a process with function calls.\n\nThese days there are also talks and patches around about libification\nand about passing around a \"repository\" variable and other such\nvariables, so that C code does not need to fork processes to be able\nto work more broadly, for example in submodules. And again it seems to\nme that such changes (adding code which starts a new process to\nreplace code which doesn't) doesn't go in the same direction as the\nlibification and similar goals.\n"},{"id":"492996","messageId":"xmqq8r1dfh65.fsf@gitster.g","threadId":"61237","inReplyTo":"CAP8UFD0W7PUHTg2NwuVkQJik2+HqTDF6KRZZ8tA_dW7-YZtsbQ@mail.gmail.com","subject":"Re: [PATCH v2] bisect: Honor log.date","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-16T15:42:10Z","receivedAt":"2024-04-16T15:42:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n>> IMHO this config thing is a good example of the strength of the separate\n>> \"show\" process. If our goal is to trigger all the niceties of \"git\n>> show\", it is tricky to catch them all. The revision machinery is pretty\n>> reusable, but there's no easy way to figure out which config affects\n>> git-show and so on. Of course if we had a way to invoke git-show\n>> in-process that would work, but I suspect there are unexpected corner\n>> cases that might trigger.\n>\n> Sorry for not following the topic closely and for replying to this so\n> late, but I think that by now we should have some kind of guidelines\n> about when forking a new process is Ok and when it's not.\n\nI thought we had passed that stage long ago.  A case like this one\nwe see in this patch, where it is run just once immediately before\nwe give control back to the end-user (as opposed to \"gets run each\ntime in a tight loop\"), I would see it a no-brainer to discount the\n\"fork+exec is so expensive\" objection more than we would otherwise,\nespecially when the upside of running an external command is so much\nbigger.\n\nThere actually should be a different level of \"running it as a\nseparate command\" that we do not have.  If we can split out and\nencapsulate the global execution context sufficiently into a \"bag of\nstate variables\" structure, and rewrite cmd_foo() for each such\ncommand we wish to be able to run from inside an executing Git into\ntwo parts:\n\n - cmd_foo() that prepares the global execution context to a\n   \"pristine\" state, calls into cmd__foo() with that \"bag of state\n   variables\" structure as one of the parameters, and exits when\n   everything is done.\n\n - cmd__foo() that does the rest, including reading the\n   configuration files, parsing of the command line arguments to\n   override them, doing the actual work.\n\nthen the codepath we are changing from using diff-tree to show can\ndo something like:\n\n\tstruct git_global_state state = GIT_GLOBAL_STATE_INIT;\n\tstruct strvec args = STRVEC_INIT;\n\n        strvec_pushl(&args, ...);\n        cmd__show(&state, args.nr , args.v);\n\nand expect that cmd__show() will do the _right thing_, right?\n\nAnd to reach that ultimate goal, I do not think using run_command()\nAPI in the meantime poses hindrance.  The real work should be in the\nimplementation of cmd__show(), not the open-coded use of revisions\nAPI at each such point where you are tempted to spawn an external\ncommand via run_command() API, which will have to be consolidated\nand replaced with a call to cmd__show() anyway.\n"},{"id":"493005","messageId":"4f0456c0-e926-ee60-4e14-6b8ed80d2ace@softwolves.pp.se","threadId":"61237","inReplyTo":"xmqq8r1dfh65.fsf@gitster.g","subject":"Re: [PATCH v2] bisect: Honor log.date","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2024-04-16T19:53:40Z","receivedAt":"2024-04-16T19:53:44Z","isPatch":true,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"Junio C Hamano:\n\n> then the codepath we are changing from using diff-tree to show can \n> do something like:\n>\n> \tstruct git_global_state state = GIT_GLOBAL_STATE_INIT;\n> \tstruct strvec args = STRVEC_INIT;\n>\n>        strvec_pushl(&args, ...);\n>        cmd__show(&state, args.nr , args.v);\n>\n> and expect that cmd__show() will do the _right thing_, right?\n\nIn this particular case, calling \"git show\" is really the last thing \nwe want to do; so if we can move the cleanup that happens after it \n(that ends the bisect), it should be able to just take over the \ncurrent process with a call to show, without needing to re-exec.\n\nAnd calling back to the libification question, I would see this part \nof the bisect command to be something that would run *on top of* the \nlibrary (with possibly an API to poke bad/good states into it), so I \ndon't think that objection holds for this particular case.\n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"},{"id":"493248","messageId":"xmqqjzks3qjt.fsf@gitster.g","threadId":"61237","inReplyTo":"4f0456c0-e926-ee60-4e14-6b8ed80d2ace@softwolves.pp.se","subject":"Re: [PATCH v2] bisect: Honor log.date","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-20T17:13:58Z","receivedAt":"2024-04-20T17:14:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Krefting <peter@softwolves.pp.se> writes:\n\n> In this particular case, calling \"git show\" is really the last thing\n> we want to do; so if we can move the cleanup that happens after it\n> (that ends the bisect), it should be able to just take over the\n> current process with a call to show, without needing to re-exec.\n\nThe cmd_foo() functions are also expected to either return to their\ncallers or call exit() themselves, and it is true that as the very\nlast step before giving the control back to the end-user in the\ncurrent \"bisect\" process, we could make an internal call to\ncmd_foo() and let it exit with its own exit status.\n\nBut that is only the latter half of a story.\n\nThe cmd_show() (or any cmd_foo() in general) function expects to\nstart from within a pristine environment.  Calling them _after_\nsomebody else (in this case everything called from cmd_bisect())\nclobbered the global state may or may not work (and in general we\nshould assume it would not work) correctly.\n\nThe outline of the envisioned end state of libification I gave was\nabout an arrangement to ensure that we can give such an pristine\nstate when we make a call to such \"top level\" entry point of \"foo\"\ncommand (in this case, \"show\"), from a different command (in this\ncase, \"bisect\").  It is very much orthogonal to what you are talking\nabout, I think.  We need both.\n\n> And calling back to the libification question, I would see this part\n> of the bisect command to be something that would run *on top of* the\n> library (with possibly an API to poke bad/good states into it), so I\n> don't think that objection holds for this particular case.\n\nThere was no objection.  I was just pointing out that the infrastructure\nis not ready to do so.\n"}]}