{"thread":{"id":"31375","subject":"I think git show is broken","startedAt":"2012-08-28T17:38:51Z","lastAt":"2012-08-28T22:45:50Z","messageCount":5,"participants":["Matthew Caron","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"197996","messageId":"503D022B.6070001@redlion.net","threadId":"31375","inReplyTo":null,"subject":"I think git show is broken","fromName":"Matthew Caron","fromEmail":"matt.caron@redlion.net","sentAt":"2012-08-28T17:38:51Z","receivedAt":"2012-08-28T17:38:51Z","isPatch":false,"sender":{"key":"matt.caron@redlion.net","avatar":null},"body":"(otherwise, there was a very strange change made to its functionality, \nwhich the documentation does not reflect)\n\n\nOld, working git:\n\n===\n$ git --version\ngit version 1.7.0.4\n$ git show --quiet --abbrev-commit --pretty=oneline \n47a7aee54553fb718c376cfa9d7de4389a391e33\n47a7aee Fix hyperlinks for dependent tickets (#7139, #4976).\n===\n\nNew, \"broken\" git:\n\n===\n$ git --version\ngit version 1.7.9.5\n\n$ git show --quiet --abbrev-commit --pretty=oneline \n47a7aee54553fb718c376cfa9d7de4389a391e33\n47a7aee Fix hyperlinks for dependent tickets (#7139, #4976).\ndiff --git a/mastertickets/web_ui.py b/mastertickets/web_ui.py\nindex a91b862..698ed98 100644\n--- a/mastertickets/web_ui.py\n+++ b/mastertickets/web_ui.py\n@@ -32,7 +32,7 @@ class MasterTicketsModule(Component):\n use_gs = BoolOption('mastertickets', 'use_gs', default=False,\n doc='If enabled, use ghostscript to produce \nnicer output.')\n\n- FIELD_XPATH = \n'div[@id=\"ticket\"]/table[@class=\"properties\"]/td[@headers=\"h_%s\"]/text()'\n+ FIELD_XPATH = \n'.//div[@id=\"ticket\"]/table[@class=\"properties\"]//td[@headers=\"h_%s\"]/text()'\n fields = set(['blocking', 'blockedby'])\n\n # IRequestFilter methods\n===\n\nIt appears as though the new functionality always puts out a \"medium\" \nverbosity diff. Though the manpage says that it honors pretty=oneline, \nit does not seem to.\n\nI searched around the message logs, etc. and would have expected this \nchange to have thrown everyone else into as much upheaval as it has in \nmy organization, and found nothing. Am I missing something?\n\nThanks in advance.\n-- \nMatthew Caron, Software Build Engineer\nSixnet, a Red Lion business | www.sixnet.com\n+1 (518) 877-5173 x138 office\n"},{"id":"197997","messageId":"503D046B.7090606@redlion.net","threadId":"31375","inReplyTo":"503D022B.6070001@redlion.net","subject":"Re: I think git show is broken","fromName":"Matthew Caron","fromEmail":"matt.caron@redlion.net","sentAt":"2012-08-28T17:48:27Z","receivedAt":"2012-08-28T17:48:27Z","isPatch":false,"sender":{"key":"matt.caron@redlion.net","avatar":null},"body":"On 08/28/2012 01:38 PM, Matthew Caron wrote:\n> (otherwise, there was a very strange change made to its functionality,\n> which the documentation does not reflect)\n\nNever mind.\n\nI was looking in the wrong spot. The issue is not with --pretty=oneline, \nit's with --quiet. In 1.7.0.4, --quiet worked like -s. It no longer does \nin 1.7.9.5. Switching to -s cures the problem.\n\n-- \nMatthew Caron, Software Build Engineer\nSixnet, a Red Lion business | www.sixnet.com\n+1 (518) 877-5173 x138 office\n"},{"id":"198015","messageId":"20120828212934.GA396@sigill.intra.peff.net","threadId":"31375","inReplyTo":"503D046B.7090606@redlion.net","subject":"Re: I think git show is broken","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-28T21:29:34Z","receivedAt":"2012-08-28T21:29:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 28, 2012 at 01:48:27PM -0400, Matthew Caron wrote:\n\n> On 08/28/2012 01:38 PM, Matthew Caron wrote:\n> >(otherwise, there was a very strange change made to its functionality,\n> >which the documentation does not reflect)\n> \n> Never mind.\n> \n> I was looking in the wrong spot. The issue is not with\n> --pretty=oneline, it's with --quiet. In 1.7.0.4, --quiet worked like\n> -s. It no longer does in 1.7.9.5. Switching to -s cures the problem.\n\nYes, that is what's going on. But it's still a regression. There was\nsome discussion of what --quiet should do here:\n\n http://thread.gmane.org/gmane.comp.version-control.git/171357\n\nwhich resulted in a patch that took away --quiet. But then this thread:\n\n http://thread.gmane.org/gmane.comp.version-control.git/174665\n\nresulted in restoring it as a synonym for \"-s\". Unfortunately there's a\nbug in that fix, which you are seeing. Patch is below.\n\n-- >8 --\nSubject: [PATCH] log: fix --quiet synonym for -s\n\nOriginally the \"--quiet\" option was parsed by the\ndiff-option parser into the internal QUICK option. This had\nthe effect of silencing diff output from the log (which was\nnot intended, but happened to work and people started to\nuse it). But it also had other odd side effects at the diff\nlevel (for example, it would suppress the second commit in\n\"git show A B\").\n\nTo fix this, commit 1c40c36 converted log to parse-options\nand handled the \"quiet\" option separately, not passing it\non to the diff code. However, it simply ignored the option,\nwhich was a regression for people using it as a synonym for\n\"-s\". Commit 01771a8 then fixed that by interpreting the\noption to add DIFF_FORMAT_NO_OUTPUT to the list of output\nformats.\n\nHowever, that commit did not fix it in all cases. It sets\nthe flag after setup_revisions is called. Naively, this\nmakes sense because you would expect the setup_revisions\nparser to overwrite our output format flag if \"-p\" or\nanother output format flag is seen.\n\nHowever, that is not how the NO_OUTPUT flag works. We\nactually store it in the bit-field as just another format.\nAt the end of setup_revisions, we call diff_setup_done,\nwhich post-processes the bitfield and clears any other\nformats if we have set NO_OUTPUT. By setting the flag after\nsetup_revisions is done, diff_setup_done does not have a\nchance to make this tweak, and we end up with other format\noptions still set.\n\nAs a result, the flag would have no effect in \"git log -p\n--quiet\" or \"git show --quiet\". Fix it by setting the\nformat flag before the call to setup_revisions.\n\nSigned-off-by: Jeff King \n---\n builtin/log.c | 2 +-\n t/t7007-show.sh | 12 ++++++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ecc2793..c22469c 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -109,9 +109,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\t\t PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |\n \t\t\t PARSE_OPT_KEEP_DASHDASH);\n \n-\targc = setup_revisions(argc, argv, rev, opt);\n \tif (quiet)\n \t\trev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;\n+\targc = setup_revisions(argc, argv, rev, opt);\n \n \t/* Any arguments at this point are not recognized */\n \tif (argc > 1)\ndiff --git a/t/t7007-show.sh b/t/t7007-show.sh\nindex a40cd36..e41fa00 100755\n--- a/t/t7007-show.sh\n+++ b/t/t7007-show.sh\n@@ -108,4 +108,16 @@ test_expect_success 'showing range' '\n \ttest_cmp expect actual.filtered\n '\n \n+test_expect_success '-s suppresses diff' '\n+\techo main3 >expect &&\n+\tgit show -s --format=%s main3 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--quiet suppresses diff' '\n+\techo main3 >expect &&\n+\tgit show --quiet --format=%s main3 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.11.5.10.g3c8125b\n"},{"id":"198018","messageId":"7v4nnmzlsl.fsf@alter.siamese.dyndns.org","threadId":"31375","inReplyTo":"20120828212934.GA396@sigill.intra.peff.net","subject":"Re: I think git show is broken","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-28T22:36:26Z","receivedAt":"2012-08-28T22:36:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yes, that is what's going on. But it's still a regression. There was\n> some discussion of what --quiet should do here:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/171357\n>\n> which resulted in a patch that took away --quiet. But then this thread:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/174665\n>\n> resulted in restoring it as a synonym for \"-s\". Unfortunately there's a\n> bug in that fix, which you are seeing. Patch is below.\n\nThanks for digging this through to the bottom.\n\n> ...\n> However, that commit did not fix it in all cases. It sets\n> the flag after setup_revisions is called. Naively, this\n> makes sense because you would expect the setup_revisions\n> parser to overwrite our output format flag if \"-p\" or\n> another output format flag is seen.\n>\n> However, that is not how the NO_OUTPUT flag works. We\n> actually store it in the bit-field as just another format.\n> At the end of setup_revisions, we call diff_setup_done,\n> which post-processes the bitfield and clears any other\n> formats if we have set NO_OUTPUT. By setting the flag after\n> setup_revisions is done, diff_setup_done does not have a\n> chance to make this tweak, and we end up with other format\n> options still set.\n>\n> As a result, the flag would have no effect in \"git log -p\n> --quiet\" or \"git show --quiet\". Fix it by setting the\n> format flag before the call to setup_revisions.\n\nThis also means that\n\n\tgit show --name-status --quiet\n\nwill start erroring out, if I am not recalling what diff_setup_done()\ndoes. Which pretty much means \"--quiet\" given to the \"log\" family\nis truly a synonym to \"-s\", as the error condition that triggers is\nexactly the same for this:\n\n\tgit show --name-status -s\n\nwhich is fine, I think.\n\n> Signed-off-by: Jeff King \n> ---\n> builtin/log.c | 2 +-\n> t/t7007-show.sh | 12 ++++++++++++\n> 2 files changed, 13 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index ecc2793..c22469c 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -109,9 +109,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n> \t\t\t PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |\n> \t\t\t PARSE_OPT_KEEP_DASHDASH);\n> \n> -\targc = setup_revisions(argc, argv, rev, opt);\n> \tif (quiet)\n> \t\trev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;\n> +\targc = setup_revisions(argc, argv, rev, opt);\n> \n> \t/* Any arguments at this point are not recognized */\n> \tif (argc > 1)\n> diff --git a/t/t7007-show.sh b/t/t7007-show.sh\n> index a40cd36..e41fa00 100755\n> --- a/t/t7007-show.sh\n> +++ b/t/t7007-show.sh\n> @@ -108,4 +108,16 @@ test_expect_success 'showing range' '\n> \ttest_cmp expect actual.filtered\n> '\n> \n> +test_expect_success '-s suppresses diff' '\n> +\techo main3 >expect &&\n> +\tgit show -s --format=%s main3 >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success '--quiet suppresses diff' '\n> +\techo main3 >expect &&\n> +\tgit show --quiet --format=%s main3 >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"198019","messageId":"20120828224550.GA21940@sigill.intra.peff.net","threadId":"31375","inReplyTo":"7v4nnmzlsl.fsf@alter.siamese.dyndns.org","subject":"Re: I think git show is broken","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-28T22:45:50Z","receivedAt":"2012-08-28T22:45:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 28, 2012 at 03:36:26PM -0700, Junio C Hamano wrote:\n\n> > As a result, the flag would have no effect in \"git log -p\n> > --quiet\" or \"git show --quiet\". Fix it by setting the\n> > format flag before the call to setup_revisions.\n> \n> This also means that\n> \n> \tgit show --name-status --quiet\n> \n> will start erroring out, if I am not recalling what diff_setup_done()\n> does. Which pretty much means \"--quiet\" given to the \"log\" family\n> is truly a synonym to \"-s\", as the error condition that triggers is\n> exactly the same for this:\n> \n> \tgit show --name-status -s\n> \n> which is fine, I think.\n\nYes, I noticed that. I think it is fine for \"--quiet\" to be a true\nsynonym for \"-s\" here.\n\nThough I am puzzled why we would error out on \"--name-status -s\" but not\n\"--patch -s\". What is the difference between \"--name-status\" and\n\"--patch\" here? Shouldn't \"-s\" override all formatting options?\n\nAnd one final thing I noticed that is probably not worth the trouble to\nfix: the position of \"-s\" is independent of its effect. Normally options\nwhich override each other would be position dependent, so:\n\n git log --relative-date --date=local\n\nand\n\n git log --date=local --relative-date\n\nwould both throw away the first option and let the latter take effect.\nBut doing:\n\n git log --patch -s\n\nand\n\n git log -s --patch\n\nwill always have \"-s\" take over. I don't think it's a huge deal, and\nfixing it would be a pain. We'd have to take NO_OUTPUT out of the\nbit-field and make it a special option, and fix any callers who try to\nbe clever about recognizing NO_OUTPUT as a specifically-given option.\nAnd then for \"--quiet\", we'd have to handle it at its proper spot on the\ncommand-line, which would mean converting log's parse-options invocation\nto be step-wise. Probably not worth it for a minor bit of consistency\nthat nobody has actually complained about.\n\n-Peff\n"}]}