{"thread":{"id":"25500","subject":"[PATCH] fix color.pager = false with \"git diff\"","startedAt":"2010-10-21T15:02:45Z","lastAt":"2010-10-22T01:49:07Z","messageCount":4,"participants":["Jeff King","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"153969","messageId":"20101021150244.GA18426@sigill.intra.peff.net","threadId":"25500","inReplyTo":null,"subject":"[PATCH] fix color.pager = false with \"git diff\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-10-21T15:02:45Z","receivedAt":"2010-10-21T15:02:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The color code makes a decision early on about whether to\nuse colors based on the config and whether we are using a\npager. For the most part, this works, because if we are\nusing a pager, we will start it more or less immediately.\n\nIn the case of diff, however, we delay starting the pager in\ncase --exit-code is being used. If this happens, then the\ncolor code makes the wrong decision (because it doesn't\nyet realize we are using a pager), and we need to correct\nthe decision after deciding whether to use a pager.\n\nSigned-off-by: Jeff King \n---\nOriginal discussion here:\n\n http://thread.gmane.org/gmane.comp.version-control.git/89599\n\nI have mixed feelings on this one. It's kind of a hack. A more elegant\nsolution would be totally rewriting the color code to check for the\npager at first output.\n\nIn favor of this patch:\n\n 1. It fixes a real bug.\n\n 2. Perfect is the enemy of the good, and I don't care enough about\n this case to refactor the color code.\n\nAgainst:\n\n 1. It does nothing to fix other times when this ordering problem may\n arise (I don't think there are others, but I didn't check very\n thoroughly. And of course new ones may appear).\n\n 2. The bug was reported over 2 years ago, and hasn't come up since,\n despite remaining unfixed. Maybe nobody actually cares.\n\n builtin/diff.c | 15 +++++++++++++++\n 1 files changed, 15 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex a43d326..cbe15c9 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -318,6 +318,21 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tsetup_pager();\n \n \t/*\n+\t * Special case: we have already examined the config for\n+\t * color, but we didn't know at that point whether we were\n+\t * going to start a pager. The only case that we care about is\n+\t * when we turned on color, but shouldn't have (we will never\n+\t * \"should have but didn't\" because of the pager, since\n+\t * a lack of a pager implies either the tty is stdout, in\n+\t * which case we do turn on color, or it is not, in which\n+\t * case we don't start a pager).\n+\t */\n+\tif (DIFF_OPT_TST(&rev.diffopt, COLOR_DIFF) &&\n+\t pager_in_use() &&\n+\t !pager_use_color)\n+\t\tDIFF_OPT_CLR(&rev.diffopt, COLOR_DIFF);\n+\n+\t/*\n \t * Do we have --cached and not have a pending object, then\n \t * default to HEAD by hand. Eek.\n \t */\n-- \n1.7.3.1.235.gdd6c0\n"},{"id":"153999","messageId":"20101021190258.GA11759@burratino","threadId":"25500","inReplyTo":"20101021150244.GA18426@sigill.intra.peff.net","subject":"Re: [PATCH] fix color.pager = false with \"git diff\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-21T19:02:58Z","receivedAt":"2010-10-21T19:02:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"[reordered for convenience]\nJeff King wrote:\n\n> In favor of this patch:\n>\n> 1. It fixes a real bug.\n>\n> 2. Perfect is the enemy of the good, and I don't care enough about\n> this case to refactor the color code.\n>\n> Against:\n>\n> 1. It does nothing to fix other times when this ordering problem may\n> arise (I don't think there are others, but I didn't check very\n> thoroughly. And of course new ones may appear).\n>\n> 2. The bug was reported over 2 years ago, and hasn't come up since,\n> despite remaining unfixed. Maybe nobody actually cares.\n\nI'd say, go for it. At least one person (Benjamin Kudria, cc-ed) cared\nat some point. And including the code means that maybe some future hero\nrefactoring \"git diff\" will notice and implement the proper solution.\n\nThough to that end, wouldn't it make sense to include\n\n> A more elegant\n> solution would be totally rewriting the color code to check for the\n> pager at first output.\n\nin the commit message?\n"},{"id":"154041","messageId":"7vzku7rwcv.fsf@alter.siamese.dyndns.org","threadId":"25500","inReplyTo":"20101021150244.GA18426@sigill.intra.peff.net","subject":"Re: [PATCH] fix color.pager = false with \"git diff\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-21T22:53:36Z","receivedAt":"2010-10-21T22:53:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The color code makes a decision early on about whether to\n> use colors based on the config and whether we are using a\n> pager. For the most part, this works, because if we are\n> using a pager, we will start it more or less immediately.\n>\n> In the case of diff, however, we delay starting the pager in\n> case --exit-code is being used. If this happens, then the\n> color code makes the wrong decision (because it doesn't\n> yet realize we are using a pager), and we need to correct\n> the decision after deciding whether to use a pager.\n>\n> Signed-off-by: Jeff King \n> ---\n> Original discussion here:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/89599\n>\n> I have mixed feelings on this one. It's kind of a hack. A more elegant\n> solution would be totally rewriting the color code to check for the\n> pager at first output.\n>\n> In favor of this patch:\n>\n> 1. It fixes a real bug.\n>\n> 2. Perfect is the enemy of the good, and I don't care enough about\n> this case to refactor the color code.\n\nHmm, with \"[color] pager = false\", what should\n\n $ git diff --color\n\ndo?\n"},{"id":"154088","messageId":"20101022014907.GA30532@sigill.intra.peff.net","threadId":"25500","inReplyTo":"7vzku7rwcv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fix color.pager = false with \"git diff\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-10-22T01:49:07Z","receivedAt":"2010-10-22T01:49:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 21, 2010 at 03:53:36PM -0700, Junio C Hamano wrote:\n\n> > Original discussion here:\n> >\n> > http://thread.gmane.org/gmane.comp.version-control.git/89599\n> >\n> > I have mixed feelings on this one. It's kind of a hack. A more elegant\n> > solution would be totally rewriting the color code to check for the\n> > pager at first output.\n> >\n> > In favor of this patch:\n> >\n> > 1. It fixes a real bug.\n> >\n> > 2. Perfect is the enemy of the good, and I don't care enough about\n> > this case to refactor the color code.\n> \n> Hmm, with \"[color] pager = false\", what should\n> \n> $ git diff --color\n> \n> do?\n\nUgh. It should definitely turn color on, which my patch breaks. And\nthere's currently no way to know at that point in the code whether the\nCOLOR_DIFF bit came from --color or from the config.\n\nSo we would need to start keeping that information through the\ncallchain, or we need to refactor the color code to push the color\ndecision until later (probably first use). The latter is the right way,\nI think, but it's going to take some surgery.\n\nEither way, let's scrap my patch. The bug it introduces is just as bad\nas the bug it fixes.\n\nThanks for noticing.\n\n-Peff\n"}]}