Re: [PATCH 0/2] Demonstrate and partially work around a gitattributes problem

From: Jeff King <peff@peff.net>
Date: May 16, 2017, 07:54 UTC
Message-ID: <20170516075418.m3knwvdsr5ab6vzy@sigill.intra.peff.net>
In-Reply-To: <cover.1494861793.git.johannes.schindelin@gmx.de>

On Mon, May 15, 2017 at 05:23:44PM +0200, Johannes Schindelin wrote:

Show 15 quoted lines

> Granted, it is a bit of a less common use case to call
> 
> 	git diff <commit1>:<file1> <commit2>:<file2>
> 
> There are valid scenarios for such calls, of course.
> 
> And sometimes, one may want to compare even files that are stored in
> subdirectories instead of the top-level directory. Example:
> 
> 	git diff cafebabe:t/README bee7e55:t/README
> 
> Now, the surprising part is that Git tries to read a .gitattributes
> files interpreting the *entire* prefix `cafebabe:t/` as a *directory
> path*. I.e. it will try to read the *file* (not the blob)
> `cafebabe:t/.gitattributes`.

You also get a very silly-looking diff header:

  $ git diff HEAD:Makefile HEAD~100:Makefile
  diff --git a/HEAD:Makefile b/HEAD~100:Makefile
  index e35542e63..9b36068ac 100644
  --- a/HEAD:Makefile
  +++ b/HEAD~100:Makefile
  [...]

I think the problem is that diff's blob-compare should be feeding the actual path to the diff machinery, rather than the original name. True, that carries less information, but it matches the other cases. For instance:

  git diff HEAD~100:Makefile Makefile

will diff a blob and a file, and uses the file's path (so "HEAD~100" does not appear at all in the output).

Likewise, doing:

  git diff HEAD~100 HEAD -- Makefile

will show you the diff over "Makefile", without bothering to mention the commit endpoints.

So I think we should just be feeding "Makefile" to the diff code even for the 2-blob case.

Something like the patch below almost gets us there, but it suffers from the fact that the revision machinery doesn't record the path for the blobs it parses. So:

  1. It's a little inefficient, because it has to re-resolve the names
     again.

  2. It works for "git diff $commit:file1 $commit:file2", because each
     of the blobs has a name we can re-resolve. But connecting them with
     "$commit:file1..$commit:file2" doesn't work, because that name is
     given to _both_ blobs, and can't be resolved as a single sha1. I'd
     argue that this is actually a bug in the revision code.

Both would be solved if handle_revision_arg used get_sha1_with_context to record the path while resolving (struct object_entry already has a slot for it, but the revision code never sets it).

-Peff

---

diff --git a/builtin/diff.c b/builtin/diff.c
index d184aafab..e8c541f00 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -406,10 +406,30 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 			obj->flags |= flags;
 			add_object_array(obj, name, &ent);
 		} else if (obj->type == OBJ_BLOB) {
+			struct object_context oc;
+
 			if (2 <= blobs)
 				die(_("more than two blobs given: '%s'"), name);
-			hashcpy(blob[blobs].oid.hash, obj->oid.hash);
-			blob[blobs].name = name;
+
+			/*
+			 * We already resolved this once as a blob via the
+			 * rev_info machinery, but it did not bother to collect
+			 * any path information. Let's re-resolve to get it.
+			 *
+			 * There are a few corner cases where there is no path
+			 * information (e.g., a tag pointing straight at a
+			 * blob). In that case we'll fall back to just using
+			 * the name the user gave us.
+			 */
+			if (!get_sha1_with_context(entry->name, 0, blob[blobs].oid.hash, &oc) &&
+			    !hashcmp(blob[blobs].oid.hash, obj->oid.hash) &&
+			    oc.path[0]) {
+				blob[blobs].name = xstrdup(oc.path);
+			} else {
+				blob[blobs].name = name;
+				hashcpy(blob[blobs].oid.hash, obj->oid.hash);
+			}
+
 			blob[blobs].mode = entry->mode;
 			blobs++;