Re: [PATCH] Improve errors from 'git diff --no-index'.

From: Jeff King <peff@peff.net>
Date: May 23, 2011, 20:50 UTC
Message-ID: <20110523205038.GB6281@sigill.intra.peff.net>
In-Reply-To: <7vpqn9usqt.fsf@alter.siamese.dyndns.org>

On Mon, May 23, 2011 at 12:22:34PM -0700, Junio C Hamano wrote:

Show 6 quoted lines

> > $ # with my patch:
> > $ ../git/git-diff /tmp/{foo,bar}
> > warning: neither '/tmp/foo' nor '/tmp/bar' are tracked, forcing --no-index
> 
> I actually consider this a regression. We are giving an output that the
> user wanted to see, and I do not see a reason why we need to warn.

Agreed.

Show 6 quoted lines

> One thing that I have always been unhappy about --no-index is that it does
> not really mesh well with the notion of what a "git diff" is. "git diff"
> inherently is an operation to take two collections of contents labeled
> with paths, and show series of patch output between corresponding paths in
> these collections, while rename/copy detection may affect the definition
> of "correspoinding paths".

It's not just --no-index that breaks this abstraction. How about:

  git diff HEAD:Makefile v1.7.5:Makefile

or even

  git diff :1:Makefile :2:Makefile

So I think even without --no-index, we have the concept that diff compares two "things", and that only some of those things are a collection of paths (either a tree-ish, or possibly a directory, but as you note below, I suspect the latter is broken).

And obviously the two "things" must either both be single items, or must both be collections, as any other comparison doesn't really make sense. We do catch this, but it's another place where it might be nice to have better error messages:

  $ git diff HEAD:Makefile HEAD
  usage: git diff [<options>] [<commit> [<commit>]] [--] [<path>...]

Show 9 quoted lines

> 	$ git diff [--no-index] /tmp/foo /tmp/bar
> 
> but such a request does not compare "two collections of paths that have
> corresponding paths" at all. We could say we are comparing a collection
> that has tmp/foo with another that has tmp/bar, but then we should either
> emit a delete patch for tmp/foo and a create patch for tmp/bar, or emit a
> rename patch to create tmp/bar out of tmp/foo if we want to be consistent.
> 
> But that consistency goes totally against what the users would expect.

Right. If I give git two single items to compare, rather than collections, I am not going to expect renames at all, which fundamentally involve path movement. Those are a collection thing, and I asked for a much narrower scope.

Show 12 quoted lines

> The _only_ use of --no-index that is in line with what "git diff" does is
> to compare two directories as the "two collections of contents" above,
> i.e.
> 
> 	$ git diff --no-index old/ new/
> 
> and then support pathspecs, like this:
> 
> 	$ git diff --no-index old/ new/ -- Makefile '*.c'
> 
> But I do not think the current implementation does not even support this
> only sane usecase.

Yeah, I would expect git to support that, but knowing the builtin/diff.c code, I am not too surprised if it doesn't. :)

On the subject of things git diff should probably do but I doubt anybody cares enough to implement, it would be nice to be able to do:

  git diff $merge^1 $merge^2 $merge

and get the combined diff that "git show $merge" would give you, but without the log cruft.

I also wonder if the reverse would be useful:

  git diff $ours $theirs `git merge-base $ours $theirs`

which would produce something like a reverse combined diff, or "how have we diverged". E.g.,:

  diff --combined file
  index $base..$ours,$theirs
  --- a/file
  --- b/file
  @@@ -1,1 +1,1 +1,1 @@@
  --what was in the base
  + what is in ours
   +what is in theirs

But that is more than just a git-diff interface problem; we would actually have to write the reverse-combined diff code (I _think_ it should be a pretty simple reversal of the regular combined diff, but I haven't really thought it through).

-Peff