{"thread":{"id":"59035","subject":"Problem with git diff --relative, diff.external, run from a sub-directory","startedAt":"2023-01-04T22:03:36Z","lastAt":"2023-01-06T13:10:17Z","messageCount":7,"participants":["Carl Baldwin","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"469799","messageId":"CALiLy7raQsK3j+f6+dpYrEiegvFZRra5F9JwPWu---4h_AR49w@mail.gmail.com","threadId":"59035","inReplyTo":null,"subject":"Problem with git diff --relative, diff.external, run from a sub-directory","fromName":"Carl Baldwin","fromEmail":"carl@ecbaldwin.net","sentAt":"2023-01-04T22:03:17Z","receivedAt":"2023-01-04T22:03:36Z","isPatch":false,"sender":{"key":"carl@ecbaldwin.net","avatar":null},"body":"What did you do before the bug happened? (Steps to reproduce your issue)\n\n    I created a simple external diff program called `mydiff` and added it to\n    $PATH with exe permissions. It looks like this:\n\n    #!/bin/bash\n    # So that I can see how git calls me\n    echo $0 ${1+\"$@\"}\n    # || true is just so that git doesn't think that the external diff failed\n    diff -u $2 $5 || true\n\n    Next, I created a new repository with one file in a sub-directory. I put\n    five paragraphs of generated lorem ipsum in the file. Here are the precise\n    commands I ran to do this:\n\n    $ mkdir lorem-ipsum\n    $ cd lorem-ipsum\n    $ git init\n    $ mkdir subdir\n    $ : Add some text to this file ... (I added the five paragraphs here)\n    $ paste > subdir/text\n    $ git add subdir/text\n    $ git commit -m \"initial commit\"\n    $ : Make a minor change to the file\n    $ vim subdir/text\n    $ : With the local modification in place, the following looks correct\n    $ git diff --relative\n    $ : This also looks correct\n    $ GIT_EXTERNAL_DIFF=mydiff git diff --relative\n    $ : However, cd to the sub-directory and try it again...\n    $ cd subdir\n    $ : The following looks as if the file was deleted\n    $ GIT_EXTERNAL_DIFF=mydiff git diff --relative\n    $ : However, this still looks correct\n    $ git diff --relative\n\nWhat did you expect to happen? (Expected behavior)\n\n    When using a diff.external command with --relative, the diff output should\n    show the minor change that I made to the file.\n\nWhat happened instead? (Actual behavior)\n\n    The diff output shows the entire old contents of the file as deleted. The\n    header of the patch looked like this (indent added for this report):\n\n        --- /private/tmp/git-blob-DQP9aO/text   2023-01-04\n14:33:00.000000000 -0700\n        +++ /dev/null   2023-01-04 14:33:00.000000000 -0700\n\n    Here are the arguments that git passed to mydiff:\n\n        /<path>/<to>/bin/mydiff text /private/tmp/git-blob-KTGAg0/text\n0515a69f4ce73e295f4a7824f475bf75793cadb9 100644 /dev/null . .\n\nWhat's different between what you expected and what actually happened?\n\n    Git failed to pass the new contents of the file as the fifth argument to\n    the external diff command which led to showing that the file had apparently\n    been deleted when it had not. The sixth and seventh arguments aren't\n    correct either but mydiff doesn't use them.\n\nAnything else you want to add:\n\n    The following three things ALL appear to be necessary to reprouduce this\n    behavior:\n\n    1. Setting an external diff program (either with env var or\nthrough .gitconfig)\n    2. Using --relative (either on the command line or using .gitconfig)\n    3. Running `git diff` from a sub-directory under the root of the repository.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.39.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nuname: Darwin 21.6.0 Darwin Kernel Version 21.6.0: Thu Sep 29 20:12:57\nPDT 2022; root:xnu-8020.240.7~1/RELEASE_X86_64 x86_64\ncompiler info: clang: 14.0.0 (clang-1400.0.29.202)\nlibc info: no libc information available\n$SHELL (typically, interactive shell): /bin/zsh\n\n\n[Enabled Hooks]\n"},{"id":"469821","messageId":"Y7f/YiVu1TgbucDI@coredump.intra.peff.net","threadId":"59035","inReplyTo":"CALiLy7raQsK3j+f6+dpYrEiegvFZRra5F9JwPWu---4h_AR49w@mail.gmail.com","subject":"[PATCH 0/3] fixing \"diff --relative\" with external diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-06T11:00:50Z","receivedAt":"2023-01-06T11:01:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 04, 2023 at 03:03:17PM -0700, Carl Baldwin wrote:\n\n> What did you expect to happen? (Expected behavior)\n> \n>     When using a diff.external command with --relative, the diff output should\n>     show the minor change that I made to the file.\n> \n> What happened instead? (Actual behavior)\n> \n>     The diff output shows the entire old contents of the file as deleted. The\n>     header of the patch looked like this (indent added for this report):\n\nNice catch, and thank you for a clear reproduction recipe. It looks like\nthis bug has been lurking since --relative was introduced in 2008. :)\n\nHere's a patch series which fixes it. The first one is the fix itself,\nand the other two are some cleanups we can do on top (I almost squashed\nthem in, but their diffs are rather noisy and make it harder to see the\nactual fix).\n\n  [1/3]: diff: use filespec path to set up tempfiles for ext-diff\n  [2/3]: diff: clean up external-diff argv setup\n  [3/3]: diff: drop \"name\" parameter from prepare_temp_file()\n\n diff.c                   | 30 +++++++++++++-----------------\n t/t4045-diff-relative.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 17 deletions(-)\n\n-Peff\n"},{"id":"469822","messageId":"Y7gAHenwmIo4gXTb@coredump.intra.peff.net","threadId":"59035","inReplyTo":"Y7f/YiVu1TgbucDI@coredump.intra.peff.net","subject":"[PATCH 1/3] diff: use filespec path to set up tempfiles for ext-diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-06T11:03:57Z","receivedAt":"2023-01-06T11:04:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we're going to run an external diff, we have to make the contents\nof the pre- and post-images available either by dumping them to a\ntempfile, or by pointing at a valid file in the worktree. The logic of\nthis is all handled by prepare_temp_file(), and we just pass in the\nfilename and the diff_filespec.\n\nBut there's a gotcha here. The \"filename\" we have is a logical filename\nand not necessarily a path on disk or in the repository. This matters in\nat least one case: when using \"--relative\", we may have a name like\n\"foo\", even though the file content is found at \"subdir/foo\". As a\nresult, we look for the wrong path, fail to find \"foo\", and claim that\nthe file has been deleted (passing \"/dev/null\" to the external diff,\nrather than the correct worktree path).\n\nWe can fix this by passing the pathname from the diff_filespec, which\nshould always be a full repository path (and that's what we want even if\nreusing a worktree file, since we're always operating from the top-level\nof the working tree).\n\nThe breakage seems to go all the way back to cd676a5136 (diff\n--relative: output paths as relative to the current subdirectory,\n2008-02-12). As far as I can tell, before then \"name\" would always have\nbeen the same as the filespec's \"path\".\n\nThere are two related cases I looked at that aren't buggy:\n\n  1. the only other caller of prepare_temp_file() is run_textconv(). But\n     it always passes the filespec's path field, so it's OK.\n\n  2. I wondered if file renames/copies might cause similar confusion.\n     But they don't, because run_external_diff() receives two names in\n     that case: \"name\" and \"other\", which correspond to the two sides of\n     the diff. And we did correctly pass \"other\" when handling the\n     post-image side. Barring the use of \"--relative\", that would always\n     match \"two->path\", the path of the second filespec (and the rename\n     destination).\n\nSo the only bug is just the interaction with external diff drivers and\n--relative.\n\nReported-by: Carl Baldwin <carl@ecbaldwin.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c                   |  2 +-\n t/t4045-diff-relative.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 9b14543e6e..59039773a1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4281,7 +4281,7 @@ static void add_external_diff_name(struct repository *r,\n \t\t\t\t   const char *name,\n \t\t\t\t   struct diff_filespec *df)\n {\n-\tstruct diff_tempfile *temp = prepare_temp_file(r, name, df);\n+\tstruct diff_tempfile *temp = prepare_temp_file(r, df->path, df);\n \tstrvec_push(argv, temp->name);\n \tstrvec_push(argv, temp->hex);\n \tstrvec_push(argv, temp->mode);\ndiff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\nindex 198dfc9190..9b46c4c1be 100755\n--- a/t/t4045-diff-relative.sh\n+++ b/t/t4045-diff-relative.sh\n@@ -164,6 +164,35 @@ check_diff_relative_option subdir file2 true --no-relative --relative\n check_diff_relative_option . file2 false --no-relative --relative=subdir\n check_diff_relative_option . file2 true --no-relative --relative=subdir\n \n+test_expect_success 'external diff with --relative' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo changed >file1 &&\n+\techo changed >subdir/file2 &&\n+\n+\twrite_script mydiff <<-\\EOF &&\n+\t# hacky pretend diff; the goal here is just to make sure we got\n+\t# passed sensible input that we _could_ diff, without relying on\n+\t# the specific output of a system diff tool.\n+\techo \"diff a/$1 b/$1\" &&\n+\techo \"--- a/$1\" &&\n+\techo \"+++ b/$1\" &&\n+\techo \"@@ -1 +0,0 @@\" &&\n+\tsed \"s/^/-/\" \"$2\" &&\n+\tsed \"s/^/+/\" \"$5\"\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\tdiff a/file2 b/file2\n+\t--- a/file2\n+\t+++ b/file2\n+\t@@ -1 +0,0 @@\n+\t-other content\n+\t+changed\n+\tEOF\n+\tGIT_EXTERNAL_DIFF=./mydiff git diff --relative=subdir >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'setup diff --relative unmerged' '\n \ttest_commit zero file0 &&\n \ttest_commit base subdir/file0 &&\n-- \n2.39.0.463.g3774f23bc9\n\n"},{"id":"469823","messageId":"Y7gAMoEpF4k3hc92@coredump.intra.peff.net","threadId":"59035","inReplyTo":"Y7f/YiVu1TgbucDI@coredump.intra.peff.net","subject":"[PATCH 2/3] diff: clean up external-diff argv setup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-06T11:04:18Z","receivedAt":"2023-01-06T11:04:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since the previous commit, setting up the tempfile for an external diff\nuses df->path from the diff_filespec, rather than the logical name. This\nmeans add_external_diff_name() does not need to take a \"name\" parameter\nat all, and we can drop it. And that in turn lets us simplify the\nconditional for handling renames (when the \"other\" name is non-NULL).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 59039773a1..72bed1d0a3 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4278,7 +4278,6 @@ static struct diff_tempfile *prepare_temp_file(struct repository *r,\n \n static void add_external_diff_name(struct repository *r,\n \t\t\t\t   struct strvec *argv,\n-\t\t\t\t   const char *name,\n \t\t\t\t   struct diff_filespec *df)\n {\n \tstruct diff_tempfile *temp = prepare_temp_file(r, df->path, df);\n@@ -4308,11 +4307,9 @@ static void run_external_diff(const char *pgm,\n \tstrvec_push(&cmd.args, name);\n \n \tif (one && two) {\n-\t\tadd_external_diff_name(o->repo, &cmd.args, name, one);\n-\t\tif (!other)\n-\t\t\tadd_external_diff_name(o->repo, &cmd.args, name, two);\n-\t\telse {\n-\t\t\tadd_external_diff_name(o->repo, &cmd.args, other, two);\n+\t\tadd_external_diff_name(o->repo, &cmd.args, one);\n+\t\tadd_external_diff_name(o->repo, &cmd.args, two);\n+\t\tif (other) {\n \t\t\tstrvec_push(&cmd.args, other);\n \t\t\tstrvec_push(&cmd.args, xfrm_msg);\n \t\t}\n-- \n2.39.0.463.g3774f23bc9\n\n"},{"id":"469824","messageId":"Y7gAXK9GQ/XIY19e@coredump.intra.peff.net","threadId":"59035","inReplyTo":"Y7f/YiVu1TgbucDI@coredump.intra.peff.net","subject":"[PATCH 3/3] diff: drop \"name\" parameter from prepare_temp_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-06T11:05:00Z","receivedAt":"2023-01-06T11:05:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The prepare_temp_file() function takes a diff_filespec as well as a\nfilename. But it is almost certainly an error to pass in a name that\nisn't the filespec's \"path\" parameter, since that is the only thing that\nreliably tells us how to find the content (and indeed, this was the\nsource of a recently-fixed bug).\n\nSo let's drop the redundant \"name\" parameter and just use one->path\nthroughout the function. This simplifies the interface a little bit, and\nmakes it impossible for calling code to get it wrong.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c | 21 ++++++++++-----------\n 1 file changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 72bed1d0a3..329eebf16a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4213,7 +4213,6 @@ static void prep_temp_blob(struct index_state *istate,\n }\n \n static struct diff_tempfile *prepare_temp_file(struct repository *r,\n-\t\t\t\t\t       const char *name,\n \t\t\t\t\t       struct diff_filespec *one)\n {\n \tstruct diff_tempfile *temp = claim_diff_tempfile();\n@@ -4231,18 +4230,18 @@ static struct diff_tempfile *prepare_temp_file(struct repository *r,\n \n \tif (!S_ISGITLINK(one->mode) &&\n \t    (!one->oid_valid ||\n-\t     reuse_worktree_file(r->index, name, &one->oid, 1))) {\n+\t     reuse_worktree_file(r->index, one->path, &one->oid, 1))) {\n \t\tstruct stat st;\n-\t\tif (lstat(name, &st) < 0) {\n+\t\tif (lstat(one->path, &st) < 0) {\n \t\t\tif (errno == ENOENT)\n \t\t\t\tgoto not_a_valid_file;\n-\t\t\tdie_errno(\"stat(%s)\", name);\n+\t\t\tdie_errno(\"stat(%s)\", one->path);\n \t\t}\n \t\tif (S_ISLNK(st.st_mode)) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\tif (strbuf_readlink(&sb, name, st.st_size) < 0)\n-\t\t\t\tdie_errno(\"readlink(%s)\", name);\n-\t\t\tprep_temp_blob(r->index, name, temp, sb.buf, sb.len,\n+\t\t\tif (strbuf_readlink(&sb, one->path, st.st_size) < 0)\n+\t\t\t\tdie_errno(\"readlink(%s)\", one->path);\n+\t\t\tprep_temp_blob(r->index, one->path, temp, sb.buf, sb.len,\n \t\t\t\t       (one->oid_valid ?\n \t\t\t\t\t&one->oid : null_oid()),\n \t\t\t\t       (one->oid_valid ?\n@@ -4251,7 +4250,7 @@ static struct diff_tempfile *prepare_temp_file(struct repository *r,\n \t\t}\n \t\telse {\n \t\t\t/* we can borrow from the file in the work tree */\n-\t\t\ttemp->name = name;\n+\t\t\ttemp->name = one->path;\n \t\t\tif (!one->oid_valid)\n \t\t\t\toid_to_hex_r(temp->hex, null_oid());\n \t\t\telse\n@@ -4269,7 +4268,7 @@ static struct diff_tempfile *prepare_temp_file(struct repository *r,\n \telse {\n \t\tif (diff_populate_filespec(r, one, NULL))\n \t\t\tdie(\"cannot read data blob for %s\", one->path);\n-\t\tprep_temp_blob(r->index, name, temp,\n+\t\tprep_temp_blob(r->index, one->path, temp,\n \t\t\t       one->data, one->size,\n \t\t\t       &one->oid, one->mode);\n \t}\n@@ -4280,7 +4279,7 @@ static void add_external_diff_name(struct repository *r,\n \t\t\t\t   struct strvec *argv,\n \t\t\t\t   struct diff_filespec *df)\n {\n-\tstruct diff_tempfile *temp = prepare_temp_file(r, df->path, df);\n+\tstruct diff_tempfile *temp = prepare_temp_file(r, df);\n \tstrvec_push(argv, temp->name);\n \tstrvec_push(argv, temp->hex);\n \tstrvec_push(argv, temp->mode);\n@@ -7031,7 +7030,7 @@ static char *run_textconv(struct repository *r,\n \tstruct strbuf buf = STRBUF_INIT;\n \tint err = 0;\n \n-\ttemp = prepare_temp_file(r, spec->path, spec);\n+\ttemp = prepare_temp_file(r, spec);\n \tstrvec_push(&child.args, pgm);\n \tstrvec_push(&child.args, temp->name);\n \n-- \n2.39.0.463.g3774f23bc9\n"},{"id":"469829","messageId":"xmqqbknbsu9y.fsf@gitster.g","threadId":"59035","inReplyTo":"Y7gAHenwmIo4gXTb@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] diff: use filespec path to set up tempfiles for ext-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-06T12:48:57Z","receivedAt":"2023-01-06T12:49:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We can fix this by passing the pathname from the diff_filespec, which\n> should always be a full repository path (and that's what we want even if\n> reusing a worktree file, since we're always operating from the top-level\n> of the working tree).\n\nVery sensible.\n\n> The breakage seems to go all the way back to cd676a5136 (diff\n> --relative: output paths as relative to the current subdirectory,\n> 2008-02-12).\n\nNot surprising.  When I wrote all the rest of \"diff\", I didn't\nplan to do \"--relative\" ;-)\n\n> So the only bug is just the interaction with external diff drivers and\n> --relative.\n>\n> Reported-by: Carl Baldwin <carl@ecbaldwin.net>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  diff.c                   |  2 +-\n>  t/t4045-diff-relative.sh | 29 +++++++++++++++++++++++++++++\n>  2 files changed, 30 insertions(+), 1 deletion(-)\n\nThanks for a clear description.  The fix looks trivially obvious and\ncorrect.\n\n> diff --git a/diff.c b/diff.c\n> index 9b14543e6e..59039773a1 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4281,7 +4281,7 @@ static void add_external_diff_name(struct repository *r,\n>  \t\t\t\t   const char *name,\n>  \t\t\t\t   struct diff_filespec *df)\n>  {\n> -\tstruct diff_tempfile *temp = prepare_temp_file(r, name, df);\n> +\tstruct diff_tempfile *temp = prepare_temp_file(r, df->path, df);\n"},{"id":"469831","messageId":"Y7gdsxNT2TM/5M17@coredump.intra.peff.net","threadId":"59035","inReplyTo":"xmqqbknbsu9y.fsf@gitster.g","subject":"Re: [PATCH 1/3] diff: use filespec path to set up tempfiles for ext-diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-06T13:10:11Z","receivedAt":"2023-01-06T13:10:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 06, 2023 at 09:48:57PM +0900, Junio C Hamano wrote:\n\n> > The breakage seems to go all the way back to cd676a5136 (diff\n> > --relative: output paths as relative to the current subdirectory,\n> > 2008-02-12).\n> \n> Not surprising.  When I wrote all the rest of \"diff\", I didn't\n> plan to do \"--relative\" ;-)\n\n:) Commit cd676a5136 mentions that \"diff --relative\" does not interact\nwell with \"--no-index\", and that was one of the things I tested while\npoking (to make sure I did not make anything worse). And indeed, it\nseems that --relative is mostly ignored there. We could follow through\non the plan from the end of the commit message to forbid combining the\ntwo, but it may not be that important given how long it has been an\nissue (and that I think people may set diff.relative in their config\nthese days).\n\n> Thanks for a clear description.  The fix looks trivially obvious and\n> correct.\n> [...]\n> > -\tstruct diff_tempfile *temp = prepare_temp_file(r, name, df);\n> > +\tstruct diff_tempfile *temp = prepare_temp_file(r, df->path, df);\n\nOne nagging concern I had is whether \"df->path\" might ever point to\nsomething unexpected (or even be NULL). The fact that textconv\nunconditionally passes it made me feel a lot better. But I also ended up\nwalking back to the source of the \"name\" and \"other\" fields, which is\nthis code in run_diff():\n\n\tname  = one->path;\n\tother = (strcmp(name, two->path) ? two->path : NULL);\n\tattr_path = name;\n\tif (o->prefix_length)\n\t\tstrip_prefix(o->prefix_length, &name, &other);\n\nSo those values really are just aliases for one->path and two->path,\nmodulo the prefix stripping (which as you might guess, came from\ncd676a5136 itself). And using them directly instead of the stripped\nversions is definitely the right thing to do.\n\n-Peff\n"}]}