{"thread":{"id":"17029","subject":"[RFC PATCH] diff --no-index: test for pager after option parsing","startedAt":"2009-01-06T23:56:03Z","lastAt":"2009-01-07T21:30:58Z","messageCount":8,"participants":["Thomas Rast","Junio C Hamano","Miklos Vajna","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"99523","messageId":"1231286163-9422-1-git-send-email-trast@student.ethz.ch","threadId":"17029","inReplyTo":null,"subject":"[RFC PATCH] diff --no-index: test for pager after option parsing","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2009-01-06T23:56:03Z","receivedAt":"2009-01-06T23:56:03Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"We need to parse options before we can see if --exit-code was\nprovided.\n\nSigned-off-by: Thomas Rast \n\n---\n\nI noticed this while working on the earlier patch for diff --no-index.\nIt seems like the right thing to do (and passes tests), but I don't\nhave a clue about git's normal setup sequences, so I'm flagging it\nRFC.\n\n\n diff-no-index.c | 14 +++++++-------\n 1 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex b60d345..f655f64 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -198,13 +198,6 @@ void diff_no_index(struct rev_info *revs,\n \t\tdie(\"git diff %s takes two paths\",\n \t\t no_index ? \"--no-index\" : \"[--no-index]\");\n \n-\t/*\n-\t * If the user asked for our exit code then don't start a\n-\t * pager or we would end up reporting its exit code instead.\n-\t */\n-\tif (!DIFF_OPT_TST(&revs->diffopt, EXIT_WITH_STATUS))\n-\t\tsetup_pager();\n-\n \tdiff_setup(&revs->diffopt);\n \tif (!revs->diffopt.output_format)\n \t\trevs->diffopt.output_format = DIFF_FORMAT_PATCH;\n@@ -222,6 +215,13 @@ void diff_no_index(struct rev_info *revs,\n \t\t}\n \t}\n \n+\t/*\n+\t * If the user asked for our exit code then don't start a\n+\t * pager or we would end up reporting its exit code instead.\n+\t */\n+\tif (!DIFF_OPT_TST(&revs->diffopt, EXIT_WITH_STATUS))\n+\t\tsetup_pager();\n+\n \tif (prefix) {\n \t\tint len = strlen(prefix);\n \n-- \ntg: (e9b8523..) t/diff-no-index-status (depends on: origin/master)\n"},{"id":"99524","messageId":"200901070109.25579.trast@student.ethz.ch","threadId":"17029","inReplyTo":"1231286163-9422-1-git-send-email-trast@student.ethz.ch","subject":"Re: [RFC PATCH] diff --no-index: test for pager after option parsing","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2009-01-07T00:09:03Z","receivedAt":"2009-01-07T00:09:03Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Speaking of diff --no-index code, there's also this bit:\n\n\t/* diff-no-index.c:206 */\n\tfor (i = 1; i < argc - 2; ) {\n\t\tint j;\n\t\tif (!strcmp(argv[i], \"--no-index\"))\n\t\t\ti++;\n\t\telse if (!strcmp(argv[1], \"-q\"))\n\t\t\toptions |= DIFF_SILENT_ON_REMOVED;\n\nNote the argv[i] vs. argv[1]. The entire block is from 0569e9b (\"git\ndiff\": do not ignore index without --no-index, 2008-05-23).\n\nWhile it seems obvious that this should be argv[i], I'm rather\nconfused by the option itself. It is not documented in my version of\ngit-diff(1). Furthermore, I can't see what being silent about removed\npaths (which relates to the index?) has to do with a diff --no-index\n(which takes two paths that must exist).\n\nOr perhaps I should take the \"no mails/patches after midnight\" rule a\ntad bit more serious...\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n\n\n"},{"id":"99525","messageId":"7vfxjwf041.fsf@gitster.siamese.dyndns.org","threadId":"17029","inReplyTo":"1231286163-9422-1-git-send-email-trast@student.ethz.ch","subject":"Re: [RFC PATCH] diff --no-index: test for pager after option parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-07T00:09:18Z","receivedAt":"2009-01-07T00:09:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> I noticed this while working on the earlier patch for diff --no-index.\n> It seems like the right thing to do (and passes tests), but I don't\n> have a clue about git's normal setup sequences, so I'm flagging it\n> RFC.\n\nI think the patch itself makes sense from the logic flow point of view.\n\nBut I wonder if it still makes a difference in real life.idn't we stop\nreporting the exit status from the pager some time ago?\n"},{"id":"99533","messageId":"20090107032013.GO21154@genesis.frugalware.org","threadId":"17029","inReplyTo":"7vfxjwf041.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC PATCH] diff --no-index: test for pager after option parsing","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2009-01-07T03:20:13Z","receivedAt":"2009-01-07T03:20:13Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Tue, Jan 06, 2009 at 04:09:18PM -0800, Junio C Hamano wrote:\n> But I wonder if it still makes a difference in real life.idn't we stop\n> reporting the exit status from the pager some time ago?\n\nI just wanted to write this, I think that code could be just removed\nsince ea27a18 (spawn pager via run_command interface, 2008-07-22).\n"},{"id":"99548","messageId":"20090107064248.GC22616@coredump.intra.peff.net","threadId":"17029","inReplyTo":"20090107032013.GO21154@genesis.frugalware.org","subject":"Re: [RFC PATCH] diff --no-index: test for pager after option parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-07T06:42:48Z","receivedAt":"2009-01-07T06:42:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 07, 2009 at 04:20:13AM +0100, Miklos Vajna wrote:\n\n> On Tue, Jan 06, 2009 at 04:09:18PM -0800, Junio C Hamano wrote:\n> > But I wonder if it still makes a difference in real life.idn't we stop\n> > reporting the exit status from the pager some time ago?\n> \n> I just wanted to write this, I think that code could be just removed\n> since ea27a18 (spawn pager via run_command interface, 2008-07-22).\n\nI don't think just removing it is right. You would also need to put\nSETUP_PAGER into the flags for calling cmd_diff.\n\nWe do pass along the error code properly these days, but I think it is\nnice that --exit-code always just suppresses the pager. Otherwise a\nscript like this:\n\n if git diff --exit-code $x $y; then\n do something\n fi\n\nwill invoke the pager (and not everybody's setup immediately exits if\nthere is no output, either because they have different LESS options or\nbecause they use a different pager). Of course one might argue that the\nscript should not be using \"git diff\" porcelain at all, but I don't\nthink there is another way to get a --no-index diff.\n\n-Peff\n"},{"id":"99550","messageId":"7veizfbnuw.fsf@gitster.siamese.dyndns.org","threadId":"17029","inReplyTo":"20090107032013.GO21154@genesis.frugalware.org","subject":"Re: [RFC PATCH] diff --no-index: test for pager after option parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-07T07:02:15Z","receivedAt":"2009-01-07T07:02:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna writes:\n\n> On Tue, Jan 06, 2009 at 04:09:18PM -0800, Junio C Hamano wrote:\n>> But I wonder if it still makes a difference in real life.idn't we stop\n>> reporting the exit status from the pager some time ago?\n>\n> I just wanted to write this, I think that code could be just removed\n> since ea27a18 (spawn pager via run_command interface, 2008-07-22).\n\nI think we shouldn't.\n\nPeople may already have got used to \"git diff --exit-code\" to disable the\npager, and doing the same for \"git diff --exit-code --no-index\" should be\nwith less surprises.\n\nI'll queue the \"--\" fix, \"-q\" fix and this pager fix. Thanks.\n"},{"id":"99570","messageId":"1231326930-7132-1-git-send-email-trast@student.ethz.ch","threadId":"17029","inReplyTo":"7veizfbnuw.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] diff --no-index -q: fix endless loop","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2009-01-07T11:15:30Z","receivedAt":"2009-01-07T11:15:30Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"We forgot to move to the next argument when parsing -q, getting stuck\nin an endless loop.\n\nSigned-off-by: Thomas Rast \n\n---\n\nJunio C Hamano wrote:\n> I'll queue the \"--\" fix, \"-q\" fix and this pager fix. Thanks.\n\nSeems the after-midnight rule indeed has some value. I pointed out\nthe argv[i] bug, which I see you have squashed into the -- fix, but\nfailed to notice that the -q parsing is also missing an i++.\n\nI'm still not convinced of the option's value, but this patch at least\nfixes the bug.\n\n\n diff-no-index.c | 4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 12ff1f1..60ed174 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -207,8 +207,10 @@ void diff_no_index(struct rev_info *revs,\n \t\tint j;\n \t\tif (!strcmp(argv[i], \"--no-index\"))\n \t\t\ti++;\n-\t\telse if (!strcmp(argv[i], \"-q\"))\n+\t\telse if (!strcmp(argv[i], \"-q\")) {\n \t\t\toptions |= DIFF_SILENT_ON_REMOVED;\n+\t\t\ti++;\n+\t\t}\n \t\telse if (!strcmp(argv[i], \"--\"))\n \t\t\ti++;\n \t\telse {\n-- \ntg: (3bbe36c..) t/diff-q-endless (depends on: next)\n"},{"id":"99618","messageId":"7vk596952l.fsf@gitster.siamese.dyndns.org","threadId":"17029","inReplyTo":"1231326930-7132-1-git-send-email-trast@student.ethz.ch","subject":"Re: [PATCH] diff --no-index -q: fix endless loop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-07T21:30:58Z","receivedAt":"2009-01-07T21:30:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"}]}