[RFC PATCH] diff --no-index: test for pager after option parsing

[RFC PATCH] diff --no-index: test for pager after option parsing

[Thomas Rast]

We need to parse options before we can see if --exit-code was
provided.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>

---

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)

Re: [RFC PATCH] diff --no-index: test for pager after option parsing

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 859 bytes --]

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



[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [RFC PATCH] diff --no-index: test for pager after option parsing

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> 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?

Re: [RFC PATCH] diff --no-index: test for pager after option parsing

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

On Tue, Jan 06, 2009 at 04:09:18PM -0800, Junio C Hamano <gitster@pobox.com> 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).

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [RFC PATCH] diff --no-index: test for pager after option parsing

[Jeff King]

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 <gitster@pobox.com> 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

Re: [RFC PATCH] diff --no-index: test for pager after option parsing

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> On Tue, Jan 06, 2009 at 04:09:18PM -0800, Junio C Hamano <gitster@pobox.com> 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.

[PATCH] diff --no-index -q: fix endless loop

[Thomas Rast]

We forgot to move to the next argument when parsing -q, getting stuck
in an endless loop.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>

---

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)

Re: [PATCH] diff --no-index -q: fix endless loop

[Junio C Hamano]

Thanks.