RFC patchdiff --no-index: test for pager after option parsing

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(-)

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@student.ethz.ch> writes:

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?

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

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).

On Wed, Jan 07, 2009 at 04:20:13AM +0100, Miklos Vajna wrote:

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

Miklos Vajna <vmiklos@frugalware.org> writes:

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.

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:

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(-)

Thanks.