{"thread":{"id":"47512","subject":"[BUG] git bisect colour output contrary to configuration","startedAt":"2017-12-29T20:03:43Z","lastAt":"2017-12-30T23:01:09Z","messageCount":10,"participants":["Zefram","Ævar Arnfjörð Bjarmason","Todd Zullinger","Jeff King","Christian Couder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"335543","messageId":"20171229194712.GA15930@fysh.org","threadId":"47512","inReplyTo":null,"subject":"[BUG] git bisect colour output contrary to configuration","fromName":"Zefram","fromEmail":"zefram@fysh.org","sentAt":"2017-12-29T19:47:12Z","receivedAt":"2017-12-29T20:03:43Z","isPatch":false,"sender":{"key":"zefram@fysh.org","avatar":null},"body":"My ~/.gitconfig sets color.ui=never, which should prevent attempts\nat colouring output from all git commands.  I do not have any git\nconfiguration enabling colour in any situation (such as for specific\ncommands).  But when a git bisect completes, the output identifying\nthe first bad commit includes escape sequences to colour the \"commit\n3e6...\" line yellow.  Excerpt of strace output (with many irrelevant\nlines omitted):\n\n23851 write(1, \"3e6fc602e433dbd76941ac0ef7a438a77fbe9a05 is the first bad commit\\n\", 65) = 65\n23851 open(\"/home/zefram/.gitconfig\", O_RDONLY) = 3\n23851 read(3, \"[user]\\n\\tname = Zefram\\n\\temail = zefram@fysh.org\\n\\tsigningkey = 0x8E1E1EC1\\n\\n[color]\\n\\tui = never\\n\", 4096) = 93\n23851 write(1, \"\\33[33mcommit 3e6fc602e433dbd76941ac0ef7a438a77fbe9a05\\33[m\\n\", 56) = 56\n\nGiven the configuration, that line should be free of escape sequences.\n\nI'm mainly using git 2.1.4 via Debian, but I've also\nreproduced this problem with the latest from git.git (commit\n1eaabe34fc6f486367a176207420378f587d3b48, tagged v2.16.0-rc0).\n\n-zefram\n"},{"id":"335546","messageId":"87zi616vgf.fsf@evledraar.gmail.com","threadId":"47512","inReplyTo":"20171229194712.GA15930@fysh.org","subject":"Re: [BUG] git bisect colour output contrary to configuration","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-29T22:05:52Z","receivedAt":"2017-12-29T22:06:00Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 29 2017, zefram@fysh.org jotted:\n\n> My ~/.gitconfig sets color.ui=never, which should prevent attempts\n> at colouring output from all git commands.  I do not have any git\n> configuration enabling colour in any situation (such as for specific\n> commands).  But when a git bisect completes, the output identifying\n> the first bad commit includes escape sequences to colour the \"commit\n> 3e6...\" line yellow.  Excerpt of strace output (with many irrelevant\n> lines omitted):\n>\n> 23851 write(1, \"3e6fc602e433dbd76941ac0ef7a438a77fbe9a05 is the first bad commit\\n\", 65) = 65\n> 23851 open(\"/home/zefram/.gitconfig\", O_RDONLY) = 3\n> 23851 read(3, \"[user]\\n\\tname = Zefram\\n\\temail = zefram@fysh.org\\n\\tsigningkey = 0x8E1E1EC1\\n\\n[color]\\n\\tui = never\\n\", 4096) = 93\n> 23851 write(1, \"\\33[33mcommit 3e6fc602e433dbd76941ac0ef7a438a77fbe9a05\\33[m\\n\", 56) = 56\n>\n> Given the configuration, that line should be free of escape sequences.\n>\n> I'm mainly using git 2.1.4 via Debian, but I've also\n> reproduced this problem with the latest from git.git (commit\n> 1eaabe34fc6f486367a176207420378f587d3b48, tagged v2.16.0-rc0).\n\nThis issue is a bug, but has nothing do do with bisect per-se, but is a\nbug in diff-tree, compare these two:\n\n    git -c color.ui=never diff-tree --pretty --stat HEAD\n    git -c color.ui=never show --pretty --stat HEAD\n\ndiff-tree will incorrectly show colored output here despite\nui.color=never.\n"},{"id":"335548","messageId":"20171229225121.13805-1-avarab@gmail.com","threadId":"47512","inReplyTo":"87zi616vgf.fsf@evledraar.gmail.com","subject":"[PATCH] diff-tree: obey the color.ui configuration","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-29T22:51:21Z","receivedAt":"2017-12-29T22:51:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Before git-bisect exits it calls `diff-tree --pretty --stat $commit`\non the bad commit. This would always print the \"commit\" line with\ncoloring despite color.ui being set to \"never\".\n\nTeach diff-tree to look at the git_color_config() configuration. I\ninitially tried to add this to git_diff_basic_config itself, but it\nmakes other unrelated things fail, and this is a more isolated change\nthat solves the issue.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nNo idea how to test this, in particular trying to pipe the output of\ncolor.ui=never v.s. color.ui=auto to a file as \"auto\" will disable\ncoloring when it detects a pipe, but this fixes the issue.\n\n builtin/diff-tree.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex b775a75647..0311c01a87 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -97,6 +97,15 @@ static void diff_tree_tweak_rev(struct rev_info *rev, struct setup_revision_opt\n \t}\n }\n \n+\n+static int diff_tree_config(const char *var, const char *value, void *cb)\n+{\n+\tif (git_color_config(var, value, cb) < 0)\n+\t\treturn -1;\n+\n+\treturn git_diff_basic_config(var, value, cb);\n+}\n+\n int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n {\n \tchar line[1000];\n@@ -108,7 +117,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(diff_tree_usage);\n \n-\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\tgit_config(diff_tree_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(opt, prefix);\n \tif (read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n-- \n2.15.1.424.g9478a66081\n\n"},{"id":"335550","messageId":"20171229231631.GS3693@zaya.teonanacatl.net","threadId":"47512","inReplyTo":"20171229225121.13805-1-avarab@gmail.com","subject":"Re: [PATCH] diff-tree: obey the color.ui configuration","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2017-12-29T23:16:31Z","receivedAt":"2017-12-29T23:16:42Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> No idea how to test this, in particular trying to pipe the output of\n> color.ui=never v.s. color.ui=auto to a file as \"auto\" will disable\n> coloring when it detects a pipe, but this fixes the issue.\n\nYou might be able to use similar methods as those Jeff used\nin the series merged from jk/ui-color-always-to-auto:\n\nhttps://github.com/gitster/git/tree/jk/ui-color-always-to-auto\n\nHe may also have some ideas about this issue in general.\n(Or they could be tramatic memories, depending on how\npainful it was to dig into the color code.)\n\n-- \nTodd\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nSubtlety is the art of saying what you think and getting out of the\nway before it is understood.\n    -- Anonymous\n\n"},{"id":"335555","messageId":"20171230015533.GA27130@sigill.intra.peff.net","threadId":"47512","inReplyTo":"20171229231631.GS3693@zaya.teonanacatl.net","subject":"Re: [PATCH] diff-tree: obey the color.ui configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-30T01:55:33Z","receivedAt":"2017-12-30T01:55:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 29, 2017 at 06:16:31PM -0500, Todd Zullinger wrote:\n\n> Ævar Arnfjörð Bjarmason wrote:\n> > No idea how to test this, in particular trying to pipe the output of\n> > color.ui=never v.s. color.ui=auto to a file as \"auto\" will disable\n> > coloring when it detects a pipe, but this fixes the issue.\n> \n> You might be able to use similar methods as those Jeff used\n> in the series merged from jk/ui-color-always-to-auto:\n> \n> https://github.com/gitster/git/tree/jk/ui-color-always-to-auto\n\nYeah, test_terminal is the solution to testing. But...\n\n> He may also have some ideas about this issue in general.\n> (Or they could be tramatic memories, depending on how\n> painful it was to dig into the color code.)\n\nYep. If we make diff-tree support color.ui, it's going to break a bunch\nof other stuff (like add--interactive) for people who set color.ui=always.\nI know this empirically, because we did that in v2.13, and a bunch of\npeople complained. ;)\n\nThe root of the problem is that the plumbing diff-tree defaults its\ninternal color variable to \"auto\" in the first place. In theory the best\nway forward is fixing that, but it's likely to have a bunch of fallouts\nitself (scripts which use plumbing and where the user _does_ want color\nwill stop showing it). This bug has been around since v1.8.4, I think,\nso it's hard to say how many people are depending on it at this point.\n\nA hackier option which would probably make most people happy would be to\nhave plumbing respect \"color.ui=never\", but not any other values.\n\nI think the history of the back and forth is:\n\n  - 4c7f1819b3 (make color.ui default to 'auto', 2013-06-10) introduced\n    the problem of plumbing defaulting to \"auto\". This was in v1.8.4.\n\n  - we did something similar to Ævar's patch in 136c8c8b8f (color: check\n    color.ui in git_default_config(), 2017-07-13). That shipped in\n    v2.14.2, and people with color.ui=always complained, because things\n    like add--interactive broke for them.\n\n  - we tried fixing it with 6be4595edb (color: make \"always\" the same as\n    \"auto\" in config, 2017-10-03), but that broke people doing \"git -c\n    color.ui=always\" as an equivalent of \"--color\". We talked about\n    making the \"-c\" config behave differently from on-disk config, but\n    got pretty disgusted at the weird hacks. And so...\n\n  - we ended up with 33c643bb08 (Revert \"color: check color.ui in\n    git_default_config()\", 2017-10-13), which just reverts the whole\n    mess back to the pre-v2.14 state. This shipped in v2.15.\n\nSo I don't think we want to go down that road again. If anything, we\nwant to either fix the original sin from 4c7f1819b3, or we want to do\nthe \"respect only never\" hack.\n\n-Peff\n"},{"id":"335558","messageId":"87tvw875vh.fsf@evledraar.gmail.com","threadId":"47512","inReplyTo":"20171230015533.GA27130@sigill.intra.peff.net","subject":"Re: [PATCH] diff-tree: obey the color.ui configuration","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-30T12:33:06Z","receivedAt":"2017-12-30T12:33:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Dec 30 2017, Jeff King jotted:\n\n> On Fri, Dec 29, 2017 at 06:16:31PM -0500, Todd Zullinger wrote:\n>\n>> Ævar Arnfjörð Bjarmason wrote:\n>> > No idea how to test this, in particular trying to pipe the output of\n>> > color.ui=never v.s. color.ui=auto to a file as \"auto\" will disable\n>> > coloring when it detects a pipe, but this fixes the issue.\n>>\n>> You might be able to use similar methods as those Jeff used\n>> in the series merged from jk/ui-color-always-to-auto:\n>>\n>> https://github.com/gitster/git/tree/jk/ui-color-always-to-auto\n>\n> Yeah, test_terminal is the solution to testing. But...\n>\n>> He may also have some ideas about this issue in general.\n>> (Or they could be tramatic memories, depending on how\n>> painful it was to dig into the color code.)\n>\n> Yep. If we make diff-tree support color.ui, it's going to break a bunch\n> of other stuff (like add--interactive) for people who set color.ui=always.\n> I know this empirically, because we did that in v2.13, and a bunch of\n> people complained. ;)\n>\n> The root of the problem is that the plumbing diff-tree defaults its\n> internal color variable to \"auto\" in the first place. In theory the best\n> way forward is fixing that, but it's likely to have a bunch of fallouts\n> itself (scripts which use plumbing and where the user _does_ want color\n> will stop showing it). This bug has been around since v1.8.4, I think,\n> so it's hard to say how many people are depending on it at this point.\n>\n> A hackier option which would probably make most people happy would be to\n> have plumbing respect \"color.ui=never\", but not any other values.\n>\n> I think the history of the back and forth is:\n>\n>   - 4c7f1819b3 (make color.ui default to 'auto', 2013-06-10) introduced\n>     the problem of plumbing defaulting to \"auto\". This was in v1.8.4.\n>\n>   - we did something similar to Ævar's patch in 136c8c8b8f (color: check\n>     color.ui in git_default_config(), 2017-07-13). That shipped in\n>     v2.14.2, and people with color.ui=always complained, because things\n>     like add--interactive broke for them.\n>\n>   - we tried fixing it with 6be4595edb (color: make \"always\" the same as\n>     \"auto\" in config, 2017-10-03), but that broke people doing \"git -c\n>     color.ui=always\" as an equivalent of \"--color\". We talked about\n>     making the \"-c\" config behave differently from on-disk config, but\n>     got pretty disgusted at the weird hacks. And so...\n>\n>   - we ended up with 33c643bb08 (Revert \"color: check color.ui in\n>     git_default_config()\", 2017-10-13), which just reverts the whole\n>     mess back to the pre-v2.14 state. This shipped in v2.15.\n\nThanks. What a mess.\n\nI haven't tried that add-interactive case you mentioned, an earlier\nversion of this patch where I tried adding the color detection in\ngit_diff_basic_config() did break one of its tests, but not my ptch, but\nit's probably still broken with =always (haven't tested.\n\n> So I don't think we want to go down that road again. If anything, we\n> want to either fix the original sin from 4c7f1819b3, or we want to do\n> the \"respect only never\" hack.\n\nGetting back to the bug report that prompted this whole thing, wouldn't\nthe easiest solution just to run \"git show --stat $commit\" instead of\n\"git diff-tree --pretty $commit\" when bisect wants to report the commit\nit found?\n\nI've always thought the output was a bit ugly, it's plumbing command, so\nwhy wouldn't we just show the commit as the user usually prefers to see\ncommits?\n"},{"id":"335561","messageId":"20171230144505.GA29252@sigill.intra.peff.net","threadId":"47512","inReplyTo":"87tvw875vh.fsf@evledraar.gmail.com","subject":"Re: [PATCH] diff-tree: obey the color.ui configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-30T14:45:05Z","receivedAt":"2017-12-30T14:45:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 30, 2017 at 01:33:06PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> >   - we ended up with 33c643bb08 (Revert \"color: check color.ui in\n> >     git_default_config()\", 2017-10-13), which just reverts the whole\n> >     mess back to the pre-v2.14 state. This shipped in v2.15.\n> \n> Thanks. What a mess.\n> \n> I haven't tried that add-interactive case you mentioned, an earlier\n> version of this patch where I tried adding the color detection in\n> git_diff_basic_config() did break one of its tests, but not my ptch, but\n> it's probably still broken with =always (haven't tested.\n\nIt should break a test, since I added one in 33c643bb083. :)\n\nThat covers \"add -p\", though, which only does diff-files under the hood.\nYou can convince it to run \"diff-index\", too, but I don't think\ndiff-tree. So technically your patch doesn't break add--interactive, but\nprobably does break some other script we don't know about. ;)\n\n> > So I don't think we want to go down that road again. If anything, we\n> > want to either fix the original sin from 4c7f1819b3, or we want to do\n> > the \"respect only never\" hack.\n> \n> Getting back to the bug report that prompted this whole thing, wouldn't\n> the easiest solution just to run \"git show --stat $commit\" instead of\n> \"git diff-tree --pretty $commit\" when bisect wants to report the commit\n> it found?\n> \n> I've always thought the output was a bit ugly, it's plumbing command, so\n> why wouldn't we just show the commit as the user usually prefers to see\n> commits?\n\nI like that solution. I've often found the output ugly, too. And in\nparticular, it doesn't show any output at all for merge commits. Doing\n\"diff-tree --cc --stat\" would be the minimal output improvement there.\n\nI do like the idea of using \"show\", though. We know the point is to show\nthe output to the user, so we don't mind at all if the behavior or\noutput of show changes in future versions (unless we consider the final\noutput of bisect to be machine-readable, but I certainly don't).\n\n-Peff\n"},{"id":"335563","messageId":"87po6w6yul.fsf@evledraar.gmail.com","threadId":"47512","inReplyTo":"20171230144505.GA29252@sigill.intra.peff.net","subject":"Re: [PATCH] diff-tree: obey the color.ui configuration","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-30T15:04:50Z","receivedAt":"2017-12-30T15:04:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Dec 30 2017, Jeff King jotted:\n\n> On Sat, Dec 30, 2017 at 01:33:06PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> >   - we ended up with 33c643bb08 (Revert \"color: check color.ui in\n>> >     git_default_config()\", 2017-10-13), which just reverts the whole\n>> >     mess back to the pre-v2.14 state. This shipped in v2.15.\n>>\n>> Thanks. What a mess.\n>>\n>> I haven't tried that add-interactive case you mentioned, an earlier\n>> version of this patch where I tried adding the color detection in\n>> git_diff_basic_config() did break one of its tests, but not my ptch, but\n>> it's probably still broken with =always (haven't tested.\n>\n> It should break a test, since I added one in 33c643bb083. :)\n\nRight, the one I tried first broke that, but not this version....\n\n> That covers \"add -p\", though, which only does diff-files under the hood.\n> You can convince it to run \"diff-index\", too, but I don't think\n> diff-tree. So technically your patch doesn't break add--interactive, but\n> probably does break some other script we don't know about. ;)\n\n...Yeah, for sure.\n\n>> > So I don't think we want to go down that road again. If anything, we\n>> > want to either fix the original sin from 4c7f1819b3, or we want to do\n>> > the \"respect only never\" hack.\n>>\n>> Getting back to the bug report that prompted this whole thing, wouldn't\n>> the easiest solution just to run \"git show --stat $commit\" instead of\n>> \"git diff-tree --pretty $commit\" when bisect wants to report the commit\n>> it found?\n>>\n>> I've always thought the output was a bit ugly, it's plumbing command, so\n>> why wouldn't we just show the commit as the user usually prefers to see\n>> commits?\n>\n> I like that solution. I've often found the output ugly, too. And in\n> particular, it doesn't show any output at all for merge commits. Doing\n> \"diff-tree --cc --stat\" would be the minimal output improvement there.\n>\n> I do like the idea of using \"show\", though. We know the point is to show\n> the output to the user, so we don't mind at all if the behavior or\n> output of show changes in future versions (unless we consider the final\n> output of bisect to be machine-readable, but I certainly don't).\n\nNot knowing the internal APIs for that well, is this basically a matter\nof copy/pasting (or factoring out into a function), some of this:\n\n    git grep -W cmd_show -- builtin/log.c\n\nI.e. boilerplate + calling cmd_log_walk() to yield a result similar to\ne22278c0a0 (\"bisect: display first bad commit without forking a new\nprocess\", 2009-05-28).\n\nOr is it preferred to just fake up argc/argv and call cmd_show()\ndirectly? I haven't seen many examples of that in the codebase:\n\n    git grep -W '(return|=)\\s*cmd.*argc' -- '*.c'\n\nBut I don't see why it wouldn't work, the cmd_show() doesn't call exit()\nitself, and we're right about to call exit anyway when our current\ndiff-tree invocation is called.\n"},{"id":"335564","messageId":"20171230181557.GA30351@sigill.intra.peff.net","threadId":"47512","inReplyTo":"87po6w6yul.fsf@evledraar.gmail.com","subject":"Re: [PATCH] diff-tree: obey the color.ui configuration","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-30T18:15:57Z","receivedAt":"2017-12-30T18:16:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 30, 2017 at 04:04:50PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > I do like the idea of using \"show\", though. We know the point is to show\n> > the output to the user, so we don't mind at all if the behavior or\n> > output of show changes in future versions (unless we consider the final\n> > output of bisect to be machine-readable, but I certainly don't).\n> \n> Not knowing the internal APIs for that well, is this basically a matter\n> of copy/pasting (or factoring out into a function), some of this:\n> \n>     git grep -W cmd_show -- builtin/log.c\n> \n> I.e. boilerplate + calling cmd_log_walk() to yield a result similar to\n> e22278c0a0 (\"bisect: display first bad commit without forking a new\n> process\", 2009-05-28).\n> \n> Or is it preferred to just fake up argc/argv and call cmd_show()\n> directly? I haven't seen many examples of that in the codebase:\n> \n>     git grep -W '(return|=)\\s*cmd.*argc' -- '*.c'\n> \n> But I don't see why it wouldn't work, the cmd_show() doesn't call exit()\n> itself, and we're right about to call exit anyway when our current\n> diff-tree invocation is called.\n\nHmm, I just assumed we were actually calling diff-tree. But looking at\nthat code in bisect, it literally is calling log_tree_commit(), which is\nthe same thing that git-show is doing.\n\nSo yet another option is to just set up our options similarly:\n\ndiff --git a/bisect.c b/bisect.c\nindex 0fca17c02b..1eadecd42a 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -893,9 +893,11 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n \n \t/* diff-tree init */\n \tinit_revisions(&opt, prefix);\n-\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\tgit_config(git_diff_ui_config, NULL);\n \topt.abbrev = 0;\n \topt.diff = 1;\n+\topt.combine_merges = 1;\n+\topt.dense_combined_merges = 1;\n \n \t/* This is what \"--pretty\" does */\n \topt.verbose_header = 1;\n\nThough I do kind of like the idea of just delegating to git-show.\nThere's no real need for us to have our own logic.\n\nI think calling cmd_show() from bisect.c is supposed to be forbidden\n(library code shouldn't call up to builtin code). I was going to suggest\njust using run_command() to call git-show. After all, we do this only\nonce at the very end of the bisection (which is pretty heavy-weight, as\nit surely has forked a lot of processes to do the actual testing).\n\nBut that would be directly undoing Christian's e22278c0a0 (bisect:\ndisplay first bad commit without forking a new process, 2009-05-28). I'm\nof the opinion that would be OK, but maybe Christian has input. :)\n\n-Peff\n"},{"id":"335567","messageId":"CAP8UFD3QhTUj+j3vBGrm0sTQ2dSOLS-m2_PwFj6DZS4VZHKRTQ@mail.gmail.com","threadId":"47512","inReplyTo":"20171230181557.GA30351@sigill.intra.peff.net","subject":"Re: [PATCH] diff-tree: obey the color.ui configuration","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-12-30T23:01:02Z","receivedAt":"2017-12-30T23:01:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Dec 30, 2017 at 7:15 PM, Jeff King <peff@peff.net> wrote:\n> On Sat, Dec 30, 2017 at 04:04:50PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > I do like the idea of using \"show\", though. We know the point is to show\n>> > the output to the user, so we don't mind at all if the behavior or\n>> > output of show changes in future versions (unless we consider the final\n>> > output of bisect to be machine-readable, but I certainly don't).\n\nI think that the first line that gives the sha1 of the first bad\ncommit (XXX is the first bad commit) should be machine-readable,\nbecause there have been people writing scripts on top of git bisect.\nBelow that I am ok if it is more fancy, especially because it looks\nlike it already used some coloring.\n\n>> Not knowing the internal APIs for that well, is this basically a matter\n>> of copy/pasting (or factoring out into a function), some of this:\n>>\n>>     git grep -W cmd_show -- builtin/log.c\n>>\n>> I.e. boilerplate + calling cmd_log_walk() to yield a result similar to\n>> e22278c0a0 (\"bisect: display first bad commit without forking a new\n>> process\", 2009-05-28).\n>>\n>> Or is it preferred to just fake up argc/argv and call cmd_show()\n>> directly? I haven't seen many examples of that in the codebase:\n>>\n>>     git grep -W '(return|=)\\s*cmd.*argc' -- '*.c'\n>>\n>> But I don't see why it wouldn't work, the cmd_show() doesn't call exit()\n>> itself, and we're right about to call exit anyway when our current\n>> diff-tree invocation is called.\n>\n> Hmm, I just assumed we were actually calling diff-tree. But looking at\n> that code in bisect, it literally is calling log_tree_commit(), which is\n> the same thing that git-show is doing.\n\nNice. This means that we should be able to make small changes to move\nfrom a diff-tree like output to a show like output.\n\n> So yet another option is to just set up our options similarly:\n>\n> diff --git a/bisect.c b/bisect.c\n> index 0fca17c02b..1eadecd42a 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -893,9 +893,11 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n>\n>         /* diff-tree init */\n>         init_revisions(&opt, prefix);\n> -       git_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> +       git_config(git_diff_ui_config, NULL);\n>         opt.abbrev = 0;\n>         opt.diff = 1;\n> +       opt.combine_merges = 1;\n> +       opt.dense_combined_merges = 1;\n>\n>         /* This is what \"--pretty\" does */\n>         opt.verbose_header = 1;\n\nI am ok with that kind of changes.\n\n> Though I do kind of like the idea of just delegating to git-show.\n> There's no real need for us to have our own logic.\n\nI don't think there is a lot of logic in the above. We are mostly just\nsetting parameters before calling setup_revisions() and\nlog_tree_commit().\n\nIf we think that the above is too much \"logic\", then we should\nprobably try to refactor the revision walking interface to make it\nsimpler, so that not as much \"logic\" is needed to call it. If this\nsucceeds, then this could help simplify a lot of things throughout the\ncode base.\n\n> I think calling cmd_show() from bisect.c is supposed to be forbidden\n> (library code shouldn't call up to builtin code). I was going to suggest\n> just using run_command() to call git-show. After all, we do this only\n> once at the very end of the bisection (which is pretty heavy-weight, as\n> it surely has forked a lot of processes to do the actual testing).\n>\n> But that would be directly undoing Christian's e22278c0a0 (bisect:\n> display first bad commit without forking a new process, 2009-05-28). I'm\n> of the opinion that would be OK, but maybe Christian has input. :)\n\nIn general I am against adding forks that are not necessary and not\nspecially useful, as they don't perform well on some platforms or some\nmachines (like big servers where new processes tend to be allocated to\na different CPU).\n\nI know that in this case it happens only once after usually a lot of\nprocessing and forking, but I don't think it gives a good example and\ngoes in the right direction.\n\nYes, it looks ugly to have 10 or 20 lines of code to just set\nparameters for setup_revisions() and log_tree_commit(), and yes I\ndon't like the revision walking interface, but I think this is a\ndifferent problem that we should take care of separately.\n\nThanks for working on this.\n"}]}