# [RFC PATCH] diff --no-index: test for pager after option parsing 8 messages from 2009-01-06 to 2009-01-07. Participants: Thomas Rast, Junio C Hamano, Miklos Vajna, Jeff King. Thread: https://gitlist.dev/t/17029 ## Thomas Rast, 2009-01-06 23:56 Subject: [RFC PATCH] diff --no-index: test for pager after option parsing Message-ID: <1231286163-9422-1-git-send-email-trast@student.ethz.ch> URL: https://gitlist.dev/e/1231286163-9422-1-git-send-email-trast%40student.ethz.ch ``` We need to parse options before we can see if --exit-code was provided. Signed-off-by: Thomas Rast --- I noticed this while working on the earlier patch for diff --no-index. It seems like the right thing to do (and passes tests), but I don't have a clue about git's normal setup sequences, so I'm flagging it RFC. diff-no-index.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-) diff --git a/diff-no-index.c b/diff-no-index.c index b60d345..f655f64 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -198,13 +198,6 @@ void diff_no_index(struct rev_info *revs, die("git diff %s takes two paths", no_index ? "--no-index" : "[--no-index]"); - /* - * If the user asked for our exit code then don't start a - * pager or we would end up reporting its exit code instead. - */ - if (!DIFF_OPT_TST(&revs->diffopt, EXIT_WITH_STATUS)) - setup_pager(); - diff_setup(&revs->diffopt); if (!revs->diffopt.output_format) revs->diffopt.output_format = DIFF_FORMAT_PATCH; @@ -222,6 +215,13 @@ void diff_no_index(struct rev_info *revs, } } + /* + * If the user asked for our exit code then don't start a + * pager or we would end up reporting its exit code instead. + */ + if (!DIFF_OPT_TST(&revs->diffopt, EXIT_WITH_STATUS)) + setup_pager(); + if (prefix) { int len = strlen(prefix); -- tg: (e9b8523..) t/diff-no-index-status (depends on: origin/master) ``` ## Thomas Rast, 2009-01-07 00:09 Subject: Re: [RFC PATCH] diff --no-index: test for pager after option parsing Message-ID: <200901070109.25579.trast@student.ethz.ch> URL: https://gitlist.dev/e/200901070109.25579.trast%40student.ethz.ch In-Reply-To: <1231286163-9422-1-git-send-email-trast@student.ethz.ch> ``` Speaking of diff --no-index code, there's also this bit: /* diff-no-index.c:206 */ for (i = 1; i < argc - 2; ) { int j; if (!strcmp(argv[i], "--no-index")) i++; else if (!strcmp(argv[1], "-q")) options |= DIFF_SILENT_ON_REMOVED; Note the argv[i] vs. argv[1]. The entire block is from 0569e9b ("git diff": do not ignore index without --no-index, 2008-05-23). While it seems obvious that this should be argv[i], I'm rather confused by the option itself. It is not documented in my version of git-diff(1). Furthermore, I can't see what being silent about removed paths (which relates to the index?) has to do with a diff --no-index (which takes two paths that must exist). Or perhaps I should take the "no mails/patches after midnight" rule a tad bit more serious... -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Junio C Hamano, 2009-01-07 00:09 Subject: Re: [RFC PATCH] diff --no-index: test for pager after option parsing Message-ID: <7vfxjwf041.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vfxjwf041.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1231286163-9422-1-git-send-email-trast@student.ethz.ch> ``` Thomas Rast writes: > I noticed this while working on the earlier patch for diff --no-index. > It seems like the right thing to do (and passes tests), but I don't > have a clue about git's normal setup sequences, so I'm flagging it > RFC. I think the patch itself makes sense from the logic flow point of view. But I wonder if it still makes a difference in real life.idn't we stop reporting the exit status from the pager some time ago? ``` ## Miklos Vajna, 2009-01-07 03:20 Subject: Re: [RFC PATCH] diff --no-index: test for pager after option parsing Message-ID: <20090107032013.GO21154@genesis.frugalware.org> URL: https://gitlist.dev/e/20090107032013.GO21154%40genesis.frugalware.org In-Reply-To: <7vfxjwf041.fsf@gitster.siamese.dyndns.org> ``` On Tue, Jan 06, 2009 at 04:09:18PM -0800, Junio C Hamano wrote: > But I wonder if it still makes a difference in real life.idn't we stop > reporting the exit status from the pager some time ago? I just wanted to write this, I think that code could be just removed since ea27a18 (spawn pager via run_command interface, 2008-07-22). ``` ## Jeff King, 2009-01-07 06:42 Subject: Re: [RFC PATCH] diff --no-index: test for pager after option parsing Message-ID: <20090107064248.GC22616@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090107064248.GC22616%40coredump.intra.peff.net In-Reply-To: <20090107032013.GO21154@genesis.frugalware.org> ``` On Wed, Jan 07, 2009 at 04:20:13AM +0100, Miklos Vajna wrote: > On Tue, Jan 06, 2009 at 04:09:18PM -0800, Junio C Hamano wrote: > > But I wonder if it still makes a difference in real life.idn't we stop > > reporting the exit status from the pager some time ago? > > I just wanted to write this, I think that code could be just removed > since ea27a18 (spawn pager via run_command interface, 2008-07-22). I don't think just removing it is right. You would also need to put SETUP_PAGER into the flags for calling cmd_diff. We do pass along the error code properly these days, but I think it is nice that --exit-code always just suppresses the pager. Otherwise a script like this: if git diff --exit-code $x $y; then do something fi will invoke the pager (and not everybody's setup immediately exits if there is no output, either because they have different LESS options or because they use a different pager). Of course one might argue that the script should not be using "git diff" porcelain at all, but I don't think there is another way to get a --no-index diff. -Peff ``` ## Junio C Hamano, 2009-01-07 07:02 Subject: Re: [RFC PATCH] diff --no-index: test for pager after option parsing Message-ID: <7veizfbnuw.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7veizfbnuw.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20090107032013.GO21154@genesis.frugalware.org> ``` Miklos Vajna writes: > On Tue, Jan 06, 2009 at 04:09:18PM -0800, Junio C Hamano wrote: >> But I wonder if it still makes a difference in real life.idn't we stop >> reporting the exit status from the pager some time ago? > > I just wanted to write this, I think that code could be just removed > since ea27a18 (spawn pager via run_command interface, 2008-07-22). I think we shouldn't. People may already have got used to "git diff --exit-code" to disable the pager, and doing the same for "git diff --exit-code --no-index" should be with less surprises. I'll queue the "--" fix, "-q" fix and this pager fix. Thanks. ``` ## Thomas Rast, 2009-01-07 11:15 Subject: [PATCH] diff --no-index -q: fix endless loop Message-ID: <1231326930-7132-1-git-send-email-trast@student.ethz.ch> URL: https://gitlist.dev/e/1231326930-7132-1-git-send-email-trast%40student.ethz.ch In-Reply-To: <7veizfbnuw.fsf@gitster.siamese.dyndns.org> ``` We forgot to move to the next argument when parsing -q, getting stuck in an endless loop. Signed-off-by: Thomas Rast --- Junio C Hamano wrote: > I'll queue the "--" fix, "-q" fix and this pager fix. Thanks. Seems the after-midnight rule indeed has some value. I pointed out the argv[i] bug, which I see you have squashed into the -- fix, but failed to notice that the -q parsing is also missing an i++. I'm still not convinced of the option's value, but this patch at least fixes the bug. diff-no-index.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/diff-no-index.c b/diff-no-index.c index 12ff1f1..60ed174 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -207,8 +207,10 @@ void diff_no_index(struct rev_info *revs, int j; if (!strcmp(argv[i], "--no-index")) i++; - else if (!strcmp(argv[i], "-q")) + else if (!strcmp(argv[i], "-q")) { options |= DIFF_SILENT_ON_REMOVED; + i++; + } else if (!strcmp(argv[i], "--")) i++; else { -- tg: (3bbe36c..) t/diff-q-endless (depends on: next) ``` ## Junio C Hamano, 2009-01-07 21:30 Subject: Re: [PATCH] diff --no-index -q: fix endless loop Message-ID: <7vk596952l.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk596952l.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1231326930-7132-1-git-send-email-trast@student.ethz.ch> ``` Thanks. ```