{"thread":{"id":"56538","subject":"[PATCH v4 0/3] difftool dir-diff symlink bug fix and cleanup patches","startedAt":"2021-09-19T20:38:39Z","lastAt":"2021-09-22T18:43:40Z","messageCount":10,"participants":["David Aguilar","Eric Sunshine","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":3},"messages":[{"id":"436380","messageId":"20210919203832.91207-1-davvid@gmail.com","threadId":"56538","inReplyTo":null,"subject":"[PATCH v4 0/3] difftool dir-diff symlink bug fix and cleanup patches","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-19T20:38:29Z","receivedAt":"2021-09-19T20:38:39Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"This patch series fixes a regression in difftool that can lead to data loss.\nThe symlink-file writing in difftool's dir-diff mode has been fixed to no\nlonger write-through to the symlink targets.\n\nPlease consider patching older Git versions with the fix from 1/3.\n\nChanges since last time:\n- This series has been reordered so that 1/3 is the patch that fixes the bug.\n  The subsequent patches are cleanup.\n- Patch 1/4 from before, which removed the tmp test repos, has been dropped.\n- Patch 1/3 was updated to not remove its tmp test repo.\n- Patch 1/3 was updated to consistently use \"echo\" in its tests.\n- Patch 1/3 was updated to fix a \"syminks\" -> \"symlinks\" test comment typo.\n- Patch 2/3 was reworded to improve the repeated slashes justification.\n- Patch 3/3 \"add a missing space to the ... comments\" is unchanged.\n\nDavid Aguilar (3):\n  difftool: fix symlink-file writing in dir-diff mode\n  difftool: use a strbuf to create a tmpdir path without double-slashes\n  difftool: add a missing space to the run_dir_diff() comments\n\n builtin/difftool.c  | 32 ++++++++++---------\n t/t7800-difftool.sh | 75 +++++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 91 insertions(+), 16 deletions(-)\n\n-- \n2.33.0.720.g5b0b3ce580\n\n"},{"id":"436381","messageId":"20210919203832.91207-2-davvid@gmail.com","threadId":"56538","inReplyTo":"20210919203832.91207-1-davvid@gmail.com","subject":"[PATCH v4 1/3] difftool: fix symlink-file writing in dir-diff mode","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-19T20:38:30Z","receivedAt":"2021-09-19T20:38:40Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"The difftool dir-diff mode handles symlinks by replacing them with their\nreadlink(2) values. This allows diff tools to see changes to symlinks\nas if they were regular text diffs with the old and new path values.\nThis is analogous to what \"git diff\" displays when symlinks change.\n\nThe temporary diff directories that are created initially contain\nsymlinks because they get checked-out using a temporary index that\nretains the original symlinks as checked-in to the repository.\n\nA bug was introduced when difftool was rewritten in C that made\ndifftool write the readlink(2) contents into the pointed-to file rather\nthan the symlink itself. The write was going through the symlink and\nwriting to its target rather than writing to the symlink path itself.\n\nReplace symlinks with raw text files by unlinking the symlink path\nbefore writing the readlink(2) content into them.\n\nWhen 18ec800512eb0634a0bf5e86b36ed156fbee73f3 added handling for\nmodified symlinks this bug got recorded in the test suite. The tests\nincluded the pointed-to symlink target paths. These paths were being\nreported because difftool was erroneously writing to them, but they\nshould have never been reported nor written.\n\nCorrect the modified-symlinks test cases by removing the target files\nfrom the expected output.\n\nAdd a test to ensure that symlinks are written with the readlink(2)\nvalues and that the target files contain their original content.\n\nReported-by: Alan Blotz <work@blotz.org>\nHelped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n builtin/difftool.c  |  2 ++\n t/t7800-difftool.sh | 68 +++++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 68 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex bb9fe7245a..21e055d13a 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -557,11 +557,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\tif (*entry->left) {\n \t\t\tadd_path(&ldir, ldir_len, entry->path);\n \t\t\tensure_leading_directories(ldir.buf);\n+\t\t\tunlink(ldir.buf);\n \t\t\twrite_file(ldir.buf, \"%s\", entry->left);\n \t\t}\n \t\tif (*entry->right) {\n \t\t\tadd_path(&rdir, rdir_len, entry->path);\n \t\t\tensure_leading_directories(rdir.buf);\n+\t\t\tunlink(rdir.buf);\n \t\t\twrite_file(rdir.buf, \"%s\", entry->right);\n \t\t}\n \t}\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex a173f564bc..07a52fb8e1 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -674,7 +674,6 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \trm c &&\n \tln -s d c &&\n \tcat >expect <<-EOF &&\n-\t\tb\n \t\tc\n \n \t\tc\n@@ -710,7 +709,6 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \t# Deleted symlinks\n \trm -f c &&\n \tcat >expect <<-EOF &&\n-\t\tb\n \t\tc\n \n \tEOF\n@@ -723,6 +721,72 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success SYMLINKS 'difftool --dir-diff writes symlinks as raw text' '\n+\t# Start out on a branch called \"branch-init\".\n+\tgit init -b branch-init symlink-files &&\n+\t(\n+\t\tcd ./symlink-files &&\n+\n+\t\t# This test ensures that symlinks are written as raw text.\n+\t\t# The \"cat\" tools output link and file contents.\n+\t\tgit config difftool.cat-left-link.cmd \"cat \\\"\\$LOCAL/link\\\"\" &&\n+\t\tgit config difftool.cat-left-a.cmd \"cat \\\"\\$LOCAL/file-a\\\"\" &&\n+\t\tgit config difftool.cat-right-link.cmd \"cat \\\"\\$REMOTE/link\\\"\" &&\n+\t\tgit config difftool.cat-right-b.cmd \"cat \\\"\\$REMOTE/file-b\\\"\" &&\n+\n+\t\t# Record the empty initial state so that we can come back here\n+\t\t# later and not have to consider the any cases where difftool\n+\t\t# will create symlinks back into the worktree.\n+\t\ttest_tick &&\n+\t\tgit commit --allow-empty -m init &&\n+\n+\t\t# Create a file called \"file-a\" with a symlink pointing to it.\n+\t\tgit switch -c branch-a &&\n+\t\techo a >file-a &&\n+\t\tln -s file-a link &&\n+\t\tgit add file-a link &&\n+\t\ttest_tick &&\n+\t\tgit commit -m link-to-file-a &&\n+\n+\t\t# Create a file called \"file-b\" and point the symlink to it.\n+\t\tgit switch -c branch-b &&\n+\t\techo b >file-b &&\n+\t\trm link &&\n+\t\tln -s file-b link &&\n+\t\tgit add file-b link &&\n+\t\tgit rm file-a &&\n+\t\ttest_tick &&\n+\t\tgit commit -m link-to-file-b &&\n+\n+\t\t# Checkout the initial branch so that the --symlinks behavior is\n+\t\t# not activated. The two directories should be completely\n+\t\t# independent with no symlinks pointing back here.\n+\t\tgit switch branch-init &&\n+\n+\t\t# The left link must be \"file-a\" and \"file-a\" must contain \"a\".\n+\t\techo file-a >expect &&\n+\t\tgit difftool --symlinks --dir-diff --tool cat-left-link \\\n+\t\t\tbranch-a branch-b >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\techo a >expect &&\n+\t\tgit difftool --symlinks --dir-diff --tool cat-left-a \\\n+\t\t\tbranch-a branch-b >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# The right link must be \"file-b\" and \"file-b\" must contain \"b\".\n+\t\techo file-b >expect &&\n+\t\tgit difftool --symlinks --dir-diff --tool cat-right-link \\\n+\t\t\tbranch-a branch-b >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\techo b >expect &&\n+\t\tgit difftool --symlinks --dir-diff --tool cat-right-b \\\n+\t\t\tbranch-a branch-b >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'add -N and difftool -d' '\n \ttest_when_finished git reset --hard &&\n \n-- \n2.33.0.720.g5b0b3ce580\n\n"},{"id":"436382","messageId":"20210919203832.91207-3-davvid@gmail.com","threadId":"56538","inReplyTo":"20210919203832.91207-1-davvid@gmail.com","subject":"[PATCH v4 2/3] difftool: use a strbuf to create a tmpdir path without repeated slashes","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-19T20:38:31Z","receivedAt":"2021-09-19T20:38:47Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"The paths generated by difftool are passed to user-facing diff tools.\nUsing paths with repeated slashes in them is a cosmetic blemish that\nis exposed to users and can be avoided.\n\nUse a strbuf to create the buffer used for the dir-diff tmpdir.\nStrip trailing slashes from the value read from TMPDIR to avoid\nrepeated slashes in the generated paths.\n\nAdd a unit test to ensure that repeated slashes are not present.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n builtin/difftool.c  | 28 +++++++++++++++-------------\n t/t7800-difftool.sh |  7 +++++++\n 2 files changed, 22 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 21e055d13a..daa438be09 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -252,11 +252,10 @@ static void changed_files(struct hashmap *result, const char *index_path,\n \tstrbuf_release(&buf);\n }\n \n-static NORETURN void exit_cleanup(const char *tmpdir, int exit_code)\n+static NORETURN void exit_cleanup(struct strbuf *buf, int exit_code)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tstrbuf_addstr(&buf, tmpdir);\n-\tremove_dir_recursively(&buf, 0);\n+\tremove_dir_recursively(buf, 0);\n+\tstrbuf_release(buf);\n \tif (exit_code)\n \t\twarning(_(\"failed: %d\"), exit_code);\n \texit(exit_code);\n@@ -333,11 +332,11 @@ static int checkout_path(unsigned mode, struct object_id *oid,\n static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\tstruct child_process *child)\n {\n-\tchar tmpdir[PATH_MAX];\n \tstruct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;\n \tstruct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;\n \tstruct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;\n \tstruct strbuf wtdir = STRBUF_INIT;\n+\tstruct strbuf tmpdir = STRBUF_INIT;\n \tchar *lbase_dir, *rbase_dir;\n \tsize_t ldir_len, rdir_len, wtdir_len;\n \tconst char *workdir, *tmp;\n@@ -360,11 +359,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \n \t/* Setup temp directories */\n \ttmp = getenv(\"TMPDIR\");\n-\txsnprintf(tmpdir, sizeof(tmpdir), \"%s/git-difftool.XXXXXX\", tmp ? tmp : \"/tmp\");\n-\tif (!mkdtemp(tmpdir))\n-\t\treturn error(\"could not create '%s'\", tmpdir);\n-\tstrbuf_addf(&ldir, \"%s/left/\", tmpdir);\n-\tstrbuf_addf(&rdir, \"%s/right/\", tmpdir);\n+\tstrbuf_add_absolute_path(&tmpdir, tmp ? tmp : \"/tmp\");\n+\tstrbuf_trim_trailing_dir_sep(&tmpdir);\n+\tstrbuf_addstr(&tmpdir, \"/git-difftool.XXXXXX\");\n+\tif (!mkdtemp(tmpdir.buf))\n+\t\treturn error(\"could not create '%s'\", tmpdir.buf);\n+\tstrbuf_addf(&ldir, \"%s/left/\", tmpdir.buf);\n+\tstrbuf_addf(&rdir, \"%s/right/\", tmpdir.buf);\n \tstrbuf_addstr(&wtdir, workdir);\n \tif (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))\n \t\tstrbuf_addch(&wtdir, '/');\n@@ -614,7 +615,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\tif (!indices_loaded) {\n \t\t\tstruct lock_file lock = LOCK_INIT;\n \t\t\tstrbuf_reset(&buf);\n-\t\t\tstrbuf_addf(&buf, \"%s/wtindex\", tmpdir);\n+\t\t\tstrbuf_addf(&buf, \"%s/wtindex\", tmpdir.buf);\n \t\t\tif (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||\n \t\t\t    write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {\n \t\t\t\tret = error(\"could not write %s\", buf.buf);\n@@ -644,11 +645,11 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t}\n \n \tif (err) {\n-\t\twarning(_(\"temporary files exist in '%s'.\"), tmpdir);\n+\t\twarning(_(\"temporary files exist in '%s'.\"), tmpdir.buf);\n \t\twarning(_(\"you may want to cleanup or recover these.\"));\n \t\texit(1);\n \t} else\n-\t\texit_cleanup(tmpdir, rc);\n+\t\texit_cleanup(&tmpdir, rc);\n \n finish:\n \tif (fp)\n@@ -660,6 +661,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstrbuf_release(&rdir);\n \tstrbuf_release(&wtdir);\n \tstrbuf_release(&buf);\n+\tstrbuf_release(&tmpdir);\n \n \treturn ret;\n }\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 07a52fb8e1..0670b617b4 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -453,6 +453,13 @@ run_dir_diff_test 'difftool --dir-diff' '\n \tgrep \"^file$\" output\n '\n \n+run_dir_diff_test 'difftool --dir-diff avoids repeated slashes in TMPDIR' '\n+\tTMPDIR=\"${TMPDIR:-/tmp}////\" \\\n+\t\tgit difftool --dir-diff $symlinks --extcmd echo branch >output &&\n+\tgrep -v // output >actual &&\n+\ttest_line_count = 1 actual\n+'\n+\n run_dir_diff_test 'difftool --dir-diff ignores --prompt' '\n \tgit difftool --dir-diff $symlinks --prompt --extcmd ls branch >output &&\n \tgrep \"^sub$\" output &&\n-- \n2.33.0.720.g5b0b3ce580\n\n"},{"id":"436383","messageId":"20210919203832.91207-4-davvid@gmail.com","threadId":"56538","inReplyTo":"20210919203832.91207-1-davvid@gmail.com","subject":"[PATCH v4 3/3] difftool: add a missing space to the run_dir_diff() comments","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-19T20:38:32Z","receivedAt":"2021-09-19T20:38:48Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Signed-off-by: David Aguilar <davvid@gmail.com>\n---\n builtin/difftool.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex daa438be09..e225974a2b 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -549,7 +549,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t}\n \n \t/*\n-\t * Symbolic links require special treatment.The standard \"git diff\"\n+\t * Symbolic links require special treatment. The standard \"git diff\"\n \t * shows only the link itself, not the contents of the link target.\n \t * This loop replicates that behavior.\n \t */\n-- \n2.33.0.720.g5b0b3ce580\n\n"},{"id":"436394","messageId":"CAPig+cTBfP5_czsPiALcF3tODJmNfXvNkTjqVFRbHCS535d-ng@mail.gmail.com","threadId":"56538","inReplyTo":"20210919203832.91207-3-davvid@gmail.com","subject":"Re: [PATCH v4 2/3] difftool: use a strbuf to create a tmpdir path without repeated slashes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-20T05:40:38Z","receivedAt":"2021-09-20T05:41:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 19, 2021 at 4:38 PM David Aguilar <davvid@gmail.com> wrote:\n> difftool: use a strbuf to create a tmpdir path without repeated slashes\n>\n> The paths generated by difftool are passed to user-facing diff tools.\n> Using paths with repeated slashes in them is a cosmetic blemish that\n> is exposed to users and can be avoided.\n>\n> Use a strbuf to create the buffer used for the dir-diff tmpdir.\n> Strip trailing slashes from the value read from TMPDIR to avoid\n> repeated slashes in the generated paths.\n\nMentioning strbuf in the commit message misleads the reviewer into\nthinking that it somehow merits extra attention and close scrutiny. In\nfact, the opposite is true: strbuf is just an implementation detail;\nthere is no reason to mention it in the commit message at all. The\ncommit message's emphasis on strbuf distracts the reader from the real\npurpose of this change, which is to fix a cosmetic issue (and maybe a\nreal issue on Windows in which double-slash can have significance?).\nSo, perhaps:\n\n    difftool: fold out repeated slashes from TMPDIR\n\n    Paths generated by difftool are passed to user-facing diff tools.\n    Supplying paths with repeated slashes is a cosmetic blemish that\n    is exposed to users and can be avoided. Therefore, strip trailing\n    slashes from the value of TMPDIR to avoid repeated slashes in the\n    generated paths.\n\n> Add a unit test to ensure that repeated slashes are not present.\n\nUnless there is something unusual or tricky about the new test that\nrequires extra explanation in the commit message, there is little\nreason to mention that you're adding a new test, so I'd probably drop\nthis line, as well. After all, the patch easily speaks for itself, and\na reviewer can see at a glance that you're adding a new test.\n\n> diff --git a/builtin/difftool.c b/builtin/difftool.c\n> @@ -252,11 +252,10 @@ static void changed_files(struct hashmap *result, const char *index_path,\n> -static NORETURN void exit_cleanup(const char *tmpdir, int exit_code)\n> +static NORETURN void exit_cleanup(struct strbuf *buf, int exit_code)\n>  {\n> -       struct strbuf buf = STRBUF_INIT;\n> -       strbuf_addstr(&buf, tmpdir);\n> -       remove_dir_recursively(&buf, 0);\n> +       remove_dir_recursively(buf, 0);\n> +       strbuf_release(buf);\n>         if (exit_code)\n>                 warning(_(\"failed: %d\"), exit_code);\n>         exit(exit_code);\n\nIt feels wrong to be releasing the caller-supplied strbuf; this change\nmakes it harder to reason about ownership. Normally, the entity which\nallocates a resource should be the one to release it. More on this\nbelow...\n\n> @@ -333,11 +332,11 @@ static int checkout_path(unsigned mode, struct object_id *oid,\n> +       struct strbuf tmpdir = STRBUF_INIT;\n> +       strbuf_add_absolute_path(&tmpdir, tmp ? tmp : \"/tmp\");\n> +       strbuf_trim_trailing_dir_sep(&tmpdir);\n> +       strbuf_addstr(&tmpdir, \"/git-difftool.XXXXXX\");\n> +       if (!mkdtemp(tmpdir.buf))\n> +               return error(\"could not create '%s'\", tmpdir.buf);\n\nLeaking the `tmpdir` strbuf here. You'd want:\n\n    if (!mkdtemp(tmpdir.buf)) {\n        error(\"could not create '%s'\", tmpdir.buf);\n        strbuf_release(&tmpdir);\n        return -1;\n    }\n\n> @@ -644,11 +645,11 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n>         if (err) {\n> -               warning(_(\"temporary files exist in '%s'.\"), tmpdir);\n> +               warning(_(\"temporary files exist in '%s'.\"), tmpdir.buf);\n>                 warning(_(\"you may want to cleanup or recover these.\"));\n>                 exit(1);\n>         } else\n> -               exit_cleanup(tmpdir, rc);\n> +               exit_cleanup(&tmpdir, rc);\n\n... [continued from above]\n\nBoth branches in this conditional terminate the program either\ndirectly by `exit(1)` or indirectly through `exit_cleanup(...)`. Yet\nonly the `exit_cleanup(...)` branch releases the strbuf (because you\nupdated `exit_cleanup()` above to do so); the other branch just leaks\nthe strbuf. This is inconsistent.\n\nSince we're exiting anyhow, and since `exit_cleanup()` was already\nleaking its own strbuf even before this patch, and since it feels\nsomewhat dirty to have `exit_cleanup()` responsible for releasing a\nresource it didn't allocate, it may make sense just to maintain the\nstatus-quo and just leak the strbuf before exiting. That is, don't\nmake any changes to `exit_cleanup()`, and let both of these branches\nleak the strbuf.\n\nOn the other hand, if you really do want to release the strbuf, then\nit would be more consistent for both branches in this conditional to\ndo so, not just one. That is, add a strbuf_release() to the `then`\narm.\n\n>  finish:\n>         if (fp)\n> @@ -660,6 +661,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n>         strbuf_release(&rdir);\n>         strbuf_release(&wtdir);\n>         strbuf_release(&buf);\n> +       strbuf_release(&tmpdir);\n\nCorrectly releasing the strbuf. Good.\n"},{"id":"436404","messageId":"87bl4nbrnx.fsf@evledraar.gmail.com","threadId":"56538","inReplyTo":"20210919203832.91207-2-davvid@gmail.com","subject":"Re: [PATCH v4 1/3] difftool: fix symlink-file writing in dir-diff mode","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-20T07:59:51Z","receivedAt":"2021-09-20T08:03:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Sep 19 2021, David Aguilar wrote:\n\n> The difftool dir-diff mode handles symlinks by replacing them with their\n> readlink(2) values. This allows diff tools to see changes to symlinks\n> as if they were regular text diffs with the old and new path values.\n> This is analogous to what \"git diff\" displays when symlinks change.\n>\n> The temporary diff directories that are created initially contain\n> symlinks because they get checked-out using a temporary index that\n> retains the original symlinks as checked-in to the repository.\n>\n> A bug was introduced when difftool was rewritten in C that made\n> difftool write the readlink(2) contents into the pointed-to file rather\n> than the symlink itself. The write was going through the symlink and\n> writing to its target rather than writing to the symlink path itself.\n>\n> Replace symlinks with raw text files by unlinking the symlink path\n> before writing the readlink(2) content into them.\n>\n> When 18ec800512eb0634a0bf5e86b36ed156fbee73f3 added handling for\n\nNit: Please format commits like this: 18ec800512e (difftool: handle\nmodified symlinks in dir-diff mode, 2017-03-15)\n\nSee \"git show -s --pretty=reference\" in Documentation/SubmittingPatches.\n\n> modified symlinks this bug got recorded in the test suite. The tests\n> included the pointed-to symlink target paths. These paths were being\n> reported because difftool was erroneously writing to them, but they\n> should have never been reported nor written.\n>\n> Correct the modified-symlinks test cases by removing the target files\n> from the expected output.\n>\n> Add a test to ensure that symlinks are written with the readlink(2)\n> values and that the target files contain their original content.\n>\n> Reported-by: Alan Blotz <work@blotz.org>\n> Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n>  builtin/difftool.c  |  2 ++\n>  t/t7800-difftool.sh | 68 +++++++++++++++++++++++++++++++++++++++++++--\n>  2 files changed, 68 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/difftool.c b/builtin/difftool.c\n> index bb9fe7245a..21e055d13a 100644\n> --- a/builtin/difftool.c\n> +++ b/builtin/difftool.c\n> @@ -557,11 +557,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n>  \t\tif (*entry->left) {\n>  \t\t\tadd_path(&ldir, ldir_len, entry->path);\n>  \t\t\tensure_leading_directories(ldir.buf);\n> +\t\t\tunlink(ldir.buf);\n>  \t\t\twrite_file(ldir.buf, \"%s\", entry->left);\n>  \t\t}\n>  \t\tif (*entry->right) {\n>  \t\t\tadd_path(&rdir, rdir_len, entry->path);\n>  \t\t\tensure_leading_directories(rdir.buf);\n> +\t\t\tunlink(rdir.buf);\n>  \t\t\twrite_file(rdir.buf, \"%s\", entry->right);\n>  \t\t}\n>  \t}\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index a173f564bc..07a52fb8e1 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -674,7 +674,6 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n>  \trm c &&\n>  \tln -s d c &&\n>  \tcat >expect <<-EOF &&\n> -\t\tb\n>  \t\tc\n>  \n>  \t\tc\n> @@ -710,7 +709,6 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n>  \t# Deleted symlinks\n>  \trm -f c &&\n>  \tcat >expect <<-EOF &&\n> -\t\tb\n>  \t\tc\n>  \n>  \tEOF\n> @@ -723,6 +721,72 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success SYMLINKS 'difftool --dir-diff writes symlinks as raw text' '\n> +\t# Start out on a branch called \"branch-init\".\n> +\tgit init -b branch-init symlink-files &&\n> +\t(\n> +\t\tcd ./symlink-files &&\n\nThe \"./\" isn't needed\n\nNeither of those small comments needs a re-roll I think, just reading\nthrough & noting things I spotted...\n\nI've omitted things that Eric mentioned in\n<CAPig+cTBfP5_czsPiALcF3tODJmNfXvNkTjqVFRbHCS535d-ng@mail.gmail.com>.\n"},{"id":"436450","messageId":"xmqqmto7ay6s.fsf@gitster.g","threadId":"56538","inReplyTo":"20210919203832.91207-1-davvid@gmail.com","subject":"Re: [PATCH v4 0/3] difftool dir-diff symlink bug fix and cleanup patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-20T18:39:39Z","receivedAt":"2021-09-20T18:41:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> This patch series fixes a regression in difftool that can lead to data loss.\n> The symlink-file writing in difftool's dir-diff mode has been fixed to no\n> longer write-through to the symlink targets.\n\nv4 with no backreference?  Do you have a message-id for the previous\nthree rounds handy?\n\n> Please consider patching older Git versions with the fix from 1/3.\n\nmeaing 1/3 would be done on top of maint (v2.33.0), and the other\ntwo can be on a separate branch that starts on a commit that is a\nmerge of the 1/3's branch into 'master'?\n\nThanks.\n"},{"id":"436489","messageId":"CAJDDKr6m9kXKSuQhC5g_OkJa52QRMtf8s8Bd0YbRNnzAgudifQ@mail.gmail.com","threadId":"56538","inReplyTo":"xmqqmto7ay6s.fsf@gitster.g","subject":"Re: [PATCH v4 0/3] difftool dir-diff symlink bug fix and cleanup patches","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-20T21:43:37Z","receivedAt":"2021-09-21T01:43:59Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Mon, Sep 20, 2021 at 11:39 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> David Aguilar <davvid@gmail.com> writes:\n>\n> > This patch series fixes a regression in difftool that can lead to data loss.\n> > The symlink-file writing in difftool's dir-diff mode has been fixed to no\n> > longer write-through to the symlink targets.\n>\n> v4 with no backreference?  Do you have a message-id for the previous\n> three rounds handy?\n\n<20210919015729.98323-4-davvid@gmail.com>\n\nv1 was dead on arrival so v2 was really the first. There was no v3 --\na different patch in the series went to v3 so I bumped the entire\nseries to v4.\n\n\n> > Please consider patching older Git versions with the fix from 1/3.\n>\n> meaing 1/3 would be done on top of maint (v2.33.0), and the other\n> two can be on a separate branch that starts on a commit that is a\n> merge of the 1/3's branch into 'master'?\n>\n> Thanks.\n\nThanks. Patch 3/3 seems trivially correct so I won't resend that either.\n\nÆvar had notes about 2/3 which can be split off to a separate topic,\nso that's the only one I'll plan to resend all by itself --in-reply-to\nthat thread.\n\nI see that it's already in \"seen\". I can send a replacement patch\nshortly if that's not too much trouble; dropping \"strbuf\" from the\nsubject line in the commit message and the note about the test would\nbe good to cleanup. The strbuf leaks will be addressed in the\nreplacement patch.\n--\nDavid\n"},{"id":"436499","messageId":"20210920220855.18637-1-davvid@gmail.com","threadId":"56538","inReplyTo":"CAPig+cTBfP5_czsPiALcF3tODJmNfXvNkTjqVFRbHCS535d-ng@mail.gmail.com","subject":"[PATCH v5] difftool: create a tmpdir path without repeated slashes","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-20T22:08:55Z","receivedAt":"2021-09-21T02:20:10Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"The paths generated by difftool are passed to user-facing diff tools.\nUsing paths with repeated slashes in them is a cosmetic blemish that\nis exposed to users and can be avoided.\n\nUse a strbuf to create the buffer used for the dir-diff tmpdir.\nStrip trailing slashes from the value read from TMPDIR to avoid\nrepeated slashes in the generated paths.\n\nAdjust the error handling to avoid leaking strbufs.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n\nThis is a replacement patch for 976bce8a6d (difftool: use a strbuf to\ncreate a tmpdir path without repeated slashes, 2021-09-19) currently\nin da/difftool (seen).\n\nChanges since last v4:\n- The strbuf leaks noted by Eric have been plugged.\n- More variables are initialized so that the finish: section can run without\n  using unitialized variables.\n- Reworked the control flow so that we always \"goto finish\" on error.\n- Got rid of exit_cleanup() entirely -- there's no need for a separate function.\n- Reworded the commit message to fold in Ævar's sugs.\n\n builtin/difftool.c  | 48 ++++++++++++++++++++++-----------------------\n t/t7800-difftool.sh |  7 +++++++\n 2 files changed, 30 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 2df6af9e5a..a972431f6d 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -252,16 +252,6 @@ static void changed_files(struct hashmap *result, const char *index_path,\n \tstrbuf_release(&buf);\n }\n \n-static NORETURN void exit_cleanup(const char *tmpdir, int exit_code)\n-{\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tstrbuf_addstr(&buf, tmpdir);\n-\tremove_dir_recursively(&buf, 0);\n-\tif (exit_code)\n-\t\twarning(_(\"failed: %d\"), exit_code);\n-\texit(exit_code);\n-}\n-\n static int ensure_leading_directories(char *path)\n {\n \tswitch (safe_create_leading_directories(path)) {\n@@ -333,16 +323,16 @@ static int checkout_path(unsigned mode, struct object_id *oid,\n static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\tstruct child_process *child)\n {\n-\tchar tmpdir[PATH_MAX];\n \tstruct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;\n \tstruct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;\n \tstruct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;\n \tstruct strbuf wtdir = STRBUF_INIT;\n-\tchar *lbase_dir, *rbase_dir;\n+\tstruct strbuf tmpdir = STRBUF_INIT;\n+\tchar *lbase_dir = NULL, *rbase_dir = NULL;\n \tsize_t ldir_len, rdir_len, wtdir_len;\n \tconst char *workdir, *tmp;\n \tint ret = 0, i;\n-\tFILE *fp;\n+\tFILE *fp = NULL;\n \tstruct hashmap working_tree_dups = HASHMAP_INIT(working_tree_entry_cmp,\n \t\t\t\t\t\t\tNULL);\n \tstruct hashmap submodules = HASHMAP_INIT(pair_cmp, NULL);\n@@ -351,7 +341,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstruct pair_entry *entry;\n \tstruct index_state wtindex;\n \tstruct checkout lstate, rstate;\n-\tint rc, flags = RUN_GIT_CMD, err = 0;\n+\tint flags = RUN_GIT_CMD, err = 0;\n \tconst char *helper_argv[] = { \"difftool--helper\", NULL, NULL, NULL };\n \tstruct hashmap wt_modified, tmp_modified;\n \tint indices_loaded = 0;\n@@ -360,11 +350,15 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \n \t/* Setup temp directories */\n \ttmp = getenv(\"TMPDIR\");\n-\txsnprintf(tmpdir, sizeof(tmpdir), \"%s/git-difftool.XXXXXX\", tmp ? tmp : \"/tmp\");\n-\tif (!mkdtemp(tmpdir))\n-\t\treturn error(\"could not create '%s'\", tmpdir);\n-\tstrbuf_addf(&ldir, \"%s/left/\", tmpdir);\n-\tstrbuf_addf(&rdir, \"%s/right/\", tmpdir);\n+\tstrbuf_add_absolute_path(&tmpdir, tmp ? tmp : \"/tmp\");\n+\tstrbuf_trim_trailing_dir_sep(&tmpdir);\n+\tstrbuf_addstr(&tmpdir, \"/git-difftool.XXXXXX\");\n+\tif (!mkdtemp(tmpdir.buf)) {\n+\t\tret = error(\"could not create '%s'\", tmpdir.buf);\n+\t\tgoto finish;\n+\t}\n+\tstrbuf_addf(&ldir, \"%s/left/\", tmpdir.buf);\n+\tstrbuf_addf(&rdir, \"%s/right/\", tmpdir.buf);\n \tstrbuf_addstr(&wtdir, workdir);\n \tif (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))\n \t\tstrbuf_addch(&wtdir, '/');\n@@ -580,7 +574,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\tflags = 0;\n \t} else\n \t\tsetenv(\"GIT_DIFFTOOL_DIRDIFF\", \"true\", 1);\n-\trc = run_command_v_opt(helper_argv, flags);\n+\tret = run_command_v_opt(helper_argv, flags);\n \n \t/* TODO: audit for interaction with sparse-index. */\n \tensure_full_index(&wtindex);\n@@ -614,7 +608,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\tif (!indices_loaded) {\n \t\t\tstruct lock_file lock = LOCK_INIT;\n \t\t\tstrbuf_reset(&buf);\n-\t\t\tstrbuf_addf(&buf, \"%s/wtindex\", tmpdir);\n+\t\t\tstrbuf_addf(&buf, \"%s/wtindex\", tmpdir.buf);\n \t\t\tif (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||\n \t\t\t    write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {\n \t\t\t\tret = error(\"could not write %s\", buf.buf);\n@@ -644,11 +638,14 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t}\n \n \tif (err) {\n-\t\twarning(_(\"temporary files exist in '%s'.\"), tmpdir);\n+\t\twarning(_(\"temporary files exist in '%s'.\"), tmpdir.buf);\n \t\twarning(_(\"you may want to cleanup or recover these.\"));\n-\t\texit(1);\n-\t} else\n-\t\texit_cleanup(tmpdir, rc);\n+\t\tret = 1;\n+\t} else {\n+\t\tremove_dir_recursively(&tmpdir, 0);\n+\t\tif (ret)\n+\t\t\twarning(_(\"failed: %d\"), ret);\n+\t}\n \n finish:\n \tif (fp)\n@@ -660,6 +657,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstrbuf_release(&rdir);\n \tstrbuf_release(&wtdir);\n \tstrbuf_release(&buf);\n+\tstrbuf_release(&tmpdir);\n \n \treturn ret;\n }\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 07a52fb8e1..0670b617b4 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -453,6 +453,13 @@ run_dir_diff_test 'difftool --dir-diff' '\n \tgrep \"^file$\" output\n '\n \n+run_dir_diff_test 'difftool --dir-diff avoids repeated slashes in TMPDIR' '\n+\tTMPDIR=\"${TMPDIR:-/tmp}////\" \\\n+\t\tgit difftool --dir-diff $symlinks --extcmd echo branch >output &&\n+\tgrep -v // output >actual &&\n+\ttest_line_count = 1 actual\n+'\n+\n run_dir_diff_test 'difftool --dir-diff ignores --prompt' '\n \tgit difftool --dir-diff $symlinks --prompt --extcmd ls branch >output &&\n \tgrep \"^sub$\" output &&\n-- \n2.33.0.372.g520515731c\n\n"},{"id":"436747","messageId":"xmqqv92s78od.fsf@gitster.g","threadId":"56538","inReplyTo":"CAJDDKr6m9kXKSuQhC5g_OkJa52QRMtf8s8Bd0YbRNnzAgudifQ@mail.gmail.com","subject":"Re: [PATCH v4 0/3] difftool dir-diff symlink bug fix and cleanup patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-22T18:43:30Z","receivedAt":"2021-09-22T18:43:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n\n> Thanks. Patch 3/3 seems trivially correct so I won't resend that either.\n>\n> Ævar had notes about 2/3 which can be split off to a separate topic,\n> so that's the only one I'll plan to resend all by itself --in-reply-to\n> that thread.\n\nPlease don't do this.  Your topic is not the only one I am looking\nat, and being able to find the whole thing together is always more\nefficient than having to go back, re-read the discussion and pick\nthe latest iteration of each step individually (which will not work\nif the topic needs reordering of the patches in it anyway).\n\n> I see that it's already in \"seen\". I can send a replacement patch\n> shortly ...\n\nPlease do not read anything in a topic being (or not being, for that\nmatter) \"seen\".  It means \"the maintainer has seen it and thought it\nmight be interesting at one point of time in the past\" and nothing\nmore.  It certainly does not mean the maintainer is keeping track of\nhow the topic is evolving and knows which piece will be replaced and\nwhich piece is already done.\n\nThanks.\n"}]}