Problem with git diff --relative, diff.external, run from a sub-directory

Problem with git diff --relative, diff.external, run from a sub-directory

[Carl Baldwin]

What did you do before the bug happened? (Steps to reproduce your issue)

    I created a simple external diff program called `mydiff` and added it to
    $PATH with exe permissions. It looks like this:

    #!/bin/bash
    # So that I can see how git calls me
    echo $0 ${1+"$@"}
    # || true is just so that git doesn't think that the external diff failed
    diff -u $2 $5 || true

    Next, I created a new repository with one file in a sub-directory. I put
    five paragraphs of generated lorem ipsum in the file. Here are the precise
    commands I ran to do this:

    $ mkdir lorem-ipsum
    $ cd lorem-ipsum
    $ git init
    $ mkdir subdir
    $ : Add some text to this file ... (I added the five paragraphs here)
    $ paste > subdir/text
    $ git add subdir/text
    $ git commit -m "initial commit"
    $ : Make a minor change to the file
    $ vim subdir/text
    $ : With the local modification in place, the following looks correct
    $ git diff --relative
    $ : This also looks correct
    $ GIT_EXTERNAL_DIFF=mydiff git diff --relative
    $ : However, cd to the sub-directory and try it again...
    $ cd subdir
    $ : The following looks as if the file was deleted
    $ GIT_EXTERNAL_DIFF=mydiff git diff --relative
    $ : However, this still looks correct
    $ git diff --relative

What did you expect to happen? (Expected behavior)

    When using a diff.external command with --relative, the diff output should
    show the minor change that I made to the file.

What happened instead? (Actual behavior)

    The diff output shows the entire old contents of the file as deleted. The
    header of the patch looked like this (indent added for this report):

        --- /private/tmp/git-blob-DQP9aO/text   2023-01-04
14:33:00.000000000 -0700
        +++ /dev/null   2023-01-04 14:33:00.000000000 -0700

    Here are the arguments that git passed to mydiff:

        /<path>/<to>/bin/mydiff text /private/tmp/git-blob-KTGAg0/text
0515a69f4ce73e295f4a7824f475bf75793cadb9 100644 /dev/null . .

What's different between what you expected and what actually happened?

    Git failed to pass the new contents of the file as the fifth argument to
    the external diff command which led to showing that the file had apparently
    been deleted when it had not. The sixth and seventh arguments aren't
    correct either but mydiff doesn't use them.

Anything else you want to add:

    The following three things ALL appear to be necessary to reprouduce this
    behavior:

    1. Setting an external diff program (either with env var or
through .gitconfig)
    2. Using --relative (either on the command line or using .gitconfig)
    3. Running `git diff` from a sub-directory under the root of the repository.

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.39.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Darwin 21.6.0 Darwin Kernel Version 21.6.0: Thu Sep 29 20:12:57
PDT 2022; root:xnu-8020.240.7~1/RELEASE_X86_64 x86_64
compiler info: clang: 14.0.0 (clang-1400.0.29.202)
libc info: no libc information available
$SHELL (typically, interactive shell): /bin/zsh


[Enabled Hooks]

[PATCH 0/3] fixing "diff --relative" with external diff

[Jeff King]

On Wed, Jan 04, 2023 at 03:03:17PM -0700, Carl Baldwin wrote:

> What did you expect to happen? (Expected behavior)
> 
>     When using a diff.external command with --relative, the diff output should
>     show the minor change that I made to the file.
> 
> What happened instead? (Actual behavior)
> 
>     The diff output shows the entire old contents of the file as deleted. The
>     header of the patch looked like this (indent added for this report):

Nice catch, and thank you for a clear reproduction recipe. It looks like
this bug has been lurking since --relative was introduced in 2008. :)

Here's a patch series which fixes it. The first one is the fix itself,
and the other two are some cleanups we can do on top (I almost squashed
them in, but their diffs are rather noisy and make it harder to see the
actual fix).

  [1/3]: diff: use filespec path to set up tempfiles for ext-diff
  [2/3]: diff: clean up external-diff argv setup
  [3/3]: diff: drop "name" parameter from prepare_temp_file()

 diff.c                   | 30 +++++++++++++-----------------
 t/t4045-diff-relative.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 17 deletions(-)

-Peff

[PATCH 1/3] diff: use filespec path to set up tempfiles for ext-diff

[Jeff King]

When we're going to run an external diff, we have to make the contents
of the pre- and post-images available either by dumping them to a
tempfile, or by pointing at a valid file in the worktree. The logic of
this is all handled by prepare_temp_file(), and we just pass in the
filename and the diff_filespec.

But there's a gotcha here. The "filename" we have is a logical filename
and not necessarily a path on disk or in the repository. This matters in
at least one case: when using "--relative", we may have a name like
"foo", even though the file content is found at "subdir/foo". As a
result, we look for the wrong path, fail to find "foo", and claim that
the file has been deleted (passing "/dev/null" to the external diff,
rather than the correct worktree path).

We can fix this by passing the pathname from the diff_filespec, which
should always be a full repository path (and that's what we want even if
reusing a worktree file, since we're always operating from the top-level
of the working tree).

The breakage seems to go all the way back to cd676a5136 (diff
--relative: output paths as relative to the current subdirectory,
2008-02-12). As far as I can tell, before then "name" would always have
been the same as the filespec's "path".

There are two related cases I looked at that aren't buggy:

  1. the only other caller of prepare_temp_file() is run_textconv(). But
     it always passes the filespec's path field, so it's OK.

  2. I wondered if file renames/copies might cause similar confusion.
     But they don't, because run_external_diff() receives two names in
     that case: "name" and "other", which correspond to the two sides of
     the diff. And we did correctly pass "other" when handling the
     post-image side. Barring the use of "--relative", that would always
     match "two->path", the path of the second filespec (and the rename
     destination).

So the only bug is just the interaction with external diff drivers and
--relative.

Reported-by: Carl Baldwin <carl@ecbaldwin.net>
Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c                   |  2 +-
 t/t4045-diff-relative.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 9b14543e6e..59039773a1 100644
--- a/diff.c
+++ b/diff.c
@@ -4281,7 +4281,7 @@ static void add_external_diff_name(struct repository *r,
 				   const char *name,
 				   struct diff_filespec *df)
 {
-	struct diff_tempfile *temp = prepare_temp_file(r, name, df);
+	struct diff_tempfile *temp = prepare_temp_file(r, df->path, df);
 	strvec_push(argv, temp->name);
 	strvec_push(argv, temp->hex);
 	strvec_push(argv, temp->mode);
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 198dfc9190..9b46c4c1be 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -164,6 +164,35 @@ check_diff_relative_option subdir file2 true --no-relative --relative
 check_diff_relative_option . file2 false --no-relative --relative=subdir
 check_diff_relative_option . file2 true --no-relative --relative=subdir
 
+test_expect_success 'external diff with --relative' '
+	test_when_finished "git reset --hard" &&
+	echo changed >file1 &&
+	echo changed >subdir/file2 &&
+
+	write_script mydiff <<-\EOF &&
+	# hacky pretend diff; the goal here is just to make sure we got
+	# passed sensible input that we _could_ diff, without relying on
+	# the specific output of a system diff tool.
+	echo "diff a/$1 b/$1" &&
+	echo "--- a/$1" &&
+	echo "+++ b/$1" &&
+	echo "@@ -1 +0,0 @@" &&
+	sed "s/^/-/" "$2" &&
+	sed "s/^/+/" "$5"
+	EOF
+
+	cat >expect <<-\EOF &&
+	diff a/file2 b/file2
+	--- a/file2
+	+++ b/file2
+	@@ -1 +0,0 @@
+	-other content
+	+changed
+	EOF
+	GIT_EXTERNAL_DIFF=./mydiff git diff --relative=subdir >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'setup diff --relative unmerged' '
 	test_commit zero file0 &&
 	test_commit base subdir/file0 &&
-- 
2.39.0.463.g3774f23bc9

[PATCH 2/3] diff: clean up external-diff argv setup

[Jeff King]

Since the previous commit, setting up the tempfile for an external diff
uses df->path from the diff_filespec, rather than the logical name. This
means add_external_diff_name() does not need to take a "name" parameter
at all, and we can drop it. And that in turn lets us simplify the
conditional for handling renames (when the "other" name is non-NULL).

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index 59039773a1..72bed1d0a3 100644
--- a/diff.c
+++ b/diff.c
@@ -4278,7 +4278,6 @@ static struct diff_tempfile *prepare_temp_file(struct repository *r,
 
 static void add_external_diff_name(struct repository *r,
 				   struct strvec *argv,
-				   const char *name,
 				   struct diff_filespec *df)
 {
 	struct diff_tempfile *temp = prepare_temp_file(r, df->path, df);
@@ -4308,11 +4307,9 @@ static void run_external_diff(const char *pgm,
 	strvec_push(&cmd.args, name);
 
 	if (one && two) {
-		add_external_diff_name(o->repo, &cmd.args, name, one);
-		if (!other)
-			add_external_diff_name(o->repo, &cmd.args, name, two);
-		else {
-			add_external_diff_name(o->repo, &cmd.args, other, two);
+		add_external_diff_name(o->repo, &cmd.args, one);
+		add_external_diff_name(o->repo, &cmd.args, two);
+		if (other) {
 			strvec_push(&cmd.args, other);
 			strvec_push(&cmd.args, xfrm_msg);
 		}
-- 
2.39.0.463.g3774f23bc9

[PATCH 3/3] diff: drop "name" parameter from prepare_temp_file()

[Jeff King]

The prepare_temp_file() function takes a diff_filespec as well as a
filename. But it is almost certainly an error to pass in a name that
isn't the filespec's "path" parameter, since that is the only thing that
reliably tells us how to find the content (and indeed, this was the
source of a recently-fixed bug).

So let's drop the redundant "name" parameter and just use one->path
throughout the function. This simplifies the interface a little bit, and
makes it impossible for calling code to get it wrong.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/diff.c b/diff.c
index 72bed1d0a3..329eebf16a 100644
--- a/diff.c
+++ b/diff.c
@@ -4213,7 +4213,6 @@ static void prep_temp_blob(struct index_state *istate,
 }
 
 static struct diff_tempfile *prepare_temp_file(struct repository *r,
-					       const char *name,
 					       struct diff_filespec *one)
 {
 	struct diff_tempfile *temp = claim_diff_tempfile();
@@ -4231,18 +4230,18 @@ static struct diff_tempfile *prepare_temp_file(struct repository *r,
 
 	if (!S_ISGITLINK(one->mode) &&
 	    (!one->oid_valid ||
-	     reuse_worktree_file(r->index, name, &one->oid, 1))) {
+	     reuse_worktree_file(r->index, one->path, &one->oid, 1))) {
 		struct stat st;
-		if (lstat(name, &st) < 0) {
+		if (lstat(one->path, &st) < 0) {
 			if (errno == ENOENT)
 				goto not_a_valid_file;
-			die_errno("stat(%s)", name);
+			die_errno("stat(%s)", one->path);
 		}
 		if (S_ISLNK(st.st_mode)) {
 			struct strbuf sb = STRBUF_INIT;
-			if (strbuf_readlink(&sb, name, st.st_size) < 0)
-				die_errno("readlink(%s)", name);
-			prep_temp_blob(r->index, name, temp, sb.buf, sb.len,
+			if (strbuf_readlink(&sb, one->path, st.st_size) < 0)
+				die_errno("readlink(%s)", one->path);
+			prep_temp_blob(r->index, one->path, temp, sb.buf, sb.len,
 				       (one->oid_valid ?
 					&one->oid : null_oid()),
 				       (one->oid_valid ?
@@ -4251,7 +4250,7 @@ static struct diff_tempfile *prepare_temp_file(struct repository *r,
 		}
 		else {
 			/* we can borrow from the file in the work tree */
-			temp->name = name;
+			temp->name = one->path;
 			if (!one->oid_valid)
 				oid_to_hex_r(temp->hex, null_oid());
 			else
@@ -4269,7 +4268,7 @@ static struct diff_tempfile *prepare_temp_file(struct repository *r,
 	else {
 		if (diff_populate_filespec(r, one, NULL))
 			die("cannot read data blob for %s", one->path);
-		prep_temp_blob(r->index, name, temp,
+		prep_temp_blob(r->index, one->path, temp,
 			       one->data, one->size,
 			       &one->oid, one->mode);
 	}
@@ -4280,7 +4279,7 @@ static void add_external_diff_name(struct repository *r,
 				   struct strvec *argv,
 				   struct diff_filespec *df)
 {
-	struct diff_tempfile *temp = prepare_temp_file(r, df->path, df);
+	struct diff_tempfile *temp = prepare_temp_file(r, df);
 	strvec_push(argv, temp->name);
 	strvec_push(argv, temp->hex);
 	strvec_push(argv, temp->mode);
@@ -7031,7 +7030,7 @@ static char *run_textconv(struct repository *r,
 	struct strbuf buf = STRBUF_INIT;
 	int err = 0;
 
-	temp = prepare_temp_file(r, spec->path, spec);
+	temp = prepare_temp_file(r, spec);
 	strvec_push(&child.args, pgm);
 	strvec_push(&child.args, temp->name);
 
-- 
2.39.0.463.g3774f23bc9

Re: [PATCH 1/3] diff: use filespec path to set up tempfiles for ext-diff

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We can fix this by passing the pathname from the diff_filespec, which
> should always be a full repository path (and that's what we want even if
> reusing a worktree file, since we're always operating from the top-level
> of the working tree).

Very sensible.

> The breakage seems to go all the way back to cd676a5136 (diff
> --relative: output paths as relative to the current subdirectory,
> 2008-02-12).

Not surprising.  When I wrote all the rest of "diff", I didn't
plan to do "--relative" ;-)

> So the only bug is just the interaction with external diff drivers and
> --relative.
>
> Reported-by: Carl Baldwin <carl@ecbaldwin.net>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  diff.c                   |  2 +-
>  t/t4045-diff-relative.sh | 29 +++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 1 deletion(-)

Thanks for a clear description.  The fix looks trivially obvious and
correct.

> diff --git a/diff.c b/diff.c
> index 9b14543e6e..59039773a1 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4281,7 +4281,7 @@ static void add_external_diff_name(struct repository *r,
>  				   const char *name,
>  				   struct diff_filespec *df)
>  {
> -	struct diff_tempfile *temp = prepare_temp_file(r, name, df);
> +	struct diff_tempfile *temp = prepare_temp_file(r, df->path, df);

Re: [PATCH 1/3] diff: use filespec path to set up tempfiles for ext-diff

[Jeff King]

On Fri, Jan 06, 2023 at 09:48:57PM +0900, Junio C Hamano wrote:

> > The breakage seems to go all the way back to cd676a5136 (diff
> > --relative: output paths as relative to the current subdirectory,
> > 2008-02-12).
> 
> Not surprising.  When I wrote all the rest of "diff", I didn't
> plan to do "--relative" ;-)

:) Commit cd676a5136 mentions that "diff --relative" does not interact
well with "--no-index", and that was one of the things I tested while
poking (to make sure I did not make anything worse). And indeed, it
seems that --relative is mostly ignored there. We could follow through
on the plan from the end of the commit message to forbid combining the
two, but it may not be that important given how long it has been an
issue (and that I think people may set diff.relative in their config
these days).

> Thanks for a clear description.  The fix looks trivially obvious and
> correct.
> [...]
> > -	struct diff_tempfile *temp = prepare_temp_file(r, name, df);
> > +	struct diff_tempfile *temp = prepare_temp_file(r, df->path, df);

One nagging concern I had is whether "df->path" might ever point to
something unexpected (or even be NULL). The fact that textconv
unconditionally passes it made me feel a lot better. But I also ended up
walking back to the source of the "name" and "other" fields, which is
this code in run_diff():

	name  = one->path;
	other = (strcmp(name, two->path) ? two->path : NULL);
	attr_path = name;
	if (o->prefix_length)
		strip_prefix(o->prefix_length, &name, &other);

So those values really are just aliases for one->path and two->path,
modulo the prefix stripping (which as you might guess, came from
cd676a5136 itself). And using them directly instead of the stripped
versions is definitely the right thing to do.

-Peff