{"thread":{"id":"56536","subject":"[PATCH 1/4] t7800-difftool: cleanup temporary repositories used by tests","startedAt":"2021-09-19T01:57:45Z","lastAt":"2021-09-20T18:37:20Z","messageCount":16,"participants":["David Aguilar","Jeff King","Johannes Sixt","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"436339","messageId":"20210919015729.98323-1-davvid@gmail.com","threadId":"56536","inReplyTo":null,"subject":"[PATCH 1/4] t7800-difftool: cleanup temporary repositories used by tests","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-19T01:57:26Z","receivedAt":"2021-09-19T01:57:45Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"The \"dirlinks\" and \"growing\" repositories should not outlive the\ntests that use them.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n t/t7800-difftool.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex a173f564bc..a923f193da 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -414,6 +414,7 @@ test_expect_success 'setup change in subdirectory' '\n test_expect_success 'difftool -d with growing paths' '\n \ta=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n \tgit init growing &&\n+\ttest_when_finished rm -rf growing &&\n \t(\n \t\tcd growing &&\n \t\techo \"test -f \\\"\\$2/b\\\"\" | write_script .git/test-for-b.sh &&\n@@ -646,6 +647,7 @@ test_expect_success 'difftool properly honors gitlink and core.worktree' '\n test_expect_success SYMLINKS 'difftool --dir-diff symlinked directories' '\n \ttest_when_finished git reset --hard &&\n \tgit init dirlinks &&\n+\ttest_when_finished rm -rf dirlinks &&\n \t(\n \t\tcd dirlinks &&\n \t\tgit config diff.tool checktrees &&\n-- \n2.30.1 (Apple Git-130)\n\n"},{"id":"436340","messageId":"20210919015729.98323-2-davvid@gmail.com","threadId":"56536","inReplyTo":"20210919015729.98323-1-davvid@gmail.com","subject":"[PATCH 2/4] difftool: add a missing space to the run_dir_diff() comments","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-19T01:57:27Z","receivedAt":"2021-09-19T01:57:45Z","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 bb9fe7245a..4d2e772031 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -548,7 +548,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.30.1 (Apple Git-130)\n\n"},{"id":"436341","messageId":"20210919015729.98323-3-davvid@gmail.com","threadId":"56536","inReplyTo":"20210919015729.98323-1-davvid@gmail.com","subject":"[PATCH 3/4] difftool: use a strbuf to create the tmpdir path","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-19T01:57:28Z","receivedAt":"2021-09-19T01:57:45Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Use a strbuf to create the buffer used for the dir-diff tmpdir.\nStrip trailing slashes \"/\" from the value read from TMPDIR to avoid\ndouble-slashes in the calculated paths.\n\nAdd a unit test to ensure that double-slashes are not present.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n builtin/difftool.c  | 32 +++++++++++++++++++-------------\n t/t7800-difftool.sh |  7 +++++++\n 2 files changed, 26 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 4d2e772031..2014a2bb9e 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,17 @@ 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+\t/* Remove trailing slashes when $TMPDIR ends in '/'. */\n+\twhile (tmpdir.len > 0 && tmpdir.buf[tmpdir.len - 1] == '/') {\n+\t\tstrbuf_setlen(&tmpdir, tmpdir.len - 1);\n+\t}\n+\tstrbuf_addstr(&tmpdir, \"/git-difftool.XXXXXX\");\n+\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@@ -612,7 +617,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@@ -642,11 +647,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@@ -658,6 +663,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 a923f193da..3863afcaac 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -454,6 +454,13 @@ run_dir_diff_test 'difftool --dir-diff' '\n \tgrep \"^file$\" output\n '\n \n+run_dir_diff_test 'difftool --dir-diff avoids double-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.30.1 (Apple Git-130)\n\n"},{"id":"436342","messageId":"20210919015729.98323-4-davvid@gmail.com","threadId":"56536","inReplyTo":"20210919015729.98323-1-davvid@gmail.com","subject":"[PATCH 4/4] difftool: fix symlink-file writing in dir-diff mode","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-19T01:57:29Z","receivedAt":"2021-09-19T01:57:45Z","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 | 70 +++++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 70 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 2014a2bb9e..4cf454eca4 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -562,11 +562,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 3863afcaac..97077f34a5 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -683,7 +683,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@@ -719,7 +718,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@@ -732,6 +730,74 @@ 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 \"init\".\n+\tgit init -b branch-init symlink-files &&\n+\ttest_when_finished rm -rf 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\" tool cats the file only if it is not a symlink.\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 start so that we can come back here later and\n+\t\t# not have to consider the any cases where difftool will create\n+\t\t# 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 syminks 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\tprintf \"%s\\n\" 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\tprintf \"%s\\n\" 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+\n test_expect_success 'add -N and difftool -d' '\n \ttest_when_finished git reset --hard &&\n \n-- \n2.30.1 (Apple Git-130)\n\n"},{"id":"436343","messageId":"20210919020235.98799-1-davvid@gmail.com","threadId":"56536","inReplyTo":"20210919015729.98323-4-davvid@gmail.com","subject":"[PATCH v2 4/4] difftool: fix symlink-file writing in dir-diff mode","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-19T02:02:35Z","receivedAt":"2021-09-19T02:02:41Z","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\nPlease drop the previous patch.\nv2 removes a spurious extra newline that was added after the new test.\nNo changes otherwise.\n\n builtin/difftool.c  |  2 ++\n t/t7800-difftool.sh | 69 +++++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 69 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 2014a2bb9e..4cf454eca4 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -562,11 +562,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 3863afcaac..d9f6d15183 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -683,7 +683,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@@ -719,7 +718,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@@ -732,6 +730,73 @@ 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 \"init\".\n+\tgit init -b branch-init symlink-files &&\n+\ttest_when_finished rm -rf 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\" tool cats the file only if it is not a symlink.\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 start so that we can come back here later and\n+\t\t# not have to consider the any cases where difftool will create\n+\t\t# 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 syminks 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\tprintf \"%s\\n\" 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\tprintf \"%s\\n\" 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.30.1 (Apple Git-130)\n\n"},{"id":"436344","messageId":"YUaduDIYoW8Fo+5s@coredump.intra.peff.net","threadId":"56536","inReplyTo":"20210919015729.98323-3-davvid@gmail.com","subject":"Re: [PATCH 3/4] difftool: use a strbuf to create the tmpdir path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-19T02:17:28Z","receivedAt":"2021-09-19T02:17:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 18, 2021 at 06:57:28PM -0700, David Aguilar wrote:\n\n> +\t/* Remove trailing slashes when $TMPDIR ends in '/'. */\n> +\twhile (tmpdir.len > 0 && tmpdir.buf[tmpdir.len - 1] == '/') {\n> +\t\tstrbuf_setlen(&tmpdir, tmpdir.len - 1);\n> +\t}\n> +\tstrbuf_addstr(&tmpdir, \"/git-difftool.XXXXXX\");\n\nIt sounds like this should remove any directory separator (so on\nWindows, it should also drop backslashes). You can use is_dir_sep() for\nthat, but better still, you can then do:\n\n  strbuf_trim_trailing_dir_sep(&tmpdir);\n\n-Peff\n"},{"id":"436346","messageId":"20210919022110.7543-1-davvid@gmail.com","threadId":"56536","inReplyTo":"20210919020235.98799-1-davvid@gmail.com","subject":"[PATCH v3 4/4] difftool: fix symlink-file writing in dir-diff mode","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-19T02:21:10Z","receivedAt":"2021-09-19T02:21:16Z","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---\nSorry for the patch noise. This supersedes v2 as well.\n\nChanges since v1:\n- Removed a spurious newline after the new test.\n- Updated test comments to fix a \"branch-init\" typo.\n- Updated test comments to clarify the purpose of the \"cat\" tools.\n- No functional changes.\n\n builtin/difftool.c  |  2 ++\n t/t7800-difftool.sh | 69 +++++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 69 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 2014a2bb9e..4cf454eca4 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -562,11 +562,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 3863afcaac..f58114b900 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -683,7 +683,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@@ -719,7 +718,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@@ -732,6 +730,73 @@ 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+\ttest_when_finished rm -rf 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 syminks 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\tprintf \"%s\\n\" 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\tprintf \"%s\\n\" 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.30.1 (Apple Git-130)\n\n"},{"id":"436347","messageId":"20210919023329.23356-1-davvid@gmail.com","threadId":"56536","inReplyTo":"YUaduDIYoW8Fo+5s@coredump.intra.peff.net","subject":"[PATCH v2 3/4] difftool: use a strbuf to create the tmpdir path","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-19T02:33:29Z","receivedAt":"2021-09-19T02:33:41Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Use a strbuf to create the buffer used for the dir-diff tmpdir.\nStrip trailing slashes \"/\" from the value read from TMPDIR to avoid\ndouble-slashes in the calculated paths.\n\nAdd a unit test to ensure that double-slashes are not present.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nChanges since v1:\n- Updated to use strbuf_trim_trailing_dir_sep().\n\nThanks Peff!\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 4d2e772031..0554ae5fb5 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@@ -612,7 +613,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@@ -642,11 +643,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@@ -658,6 +659,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 a923f193da..3863afcaac 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -454,6 +454,13 @@ run_dir_diff_test 'difftool --dir-diff' '\n \tgrep \"^file$\" output\n '\n \n+run_dir_diff_test 'difftool --dir-diff avoids double-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.721.ga252b5a140\n\n"},{"id":"436364","messageId":"8dfd0683-e019-a45c-30a4-d49ea627e36d@kdbg.org","threadId":"56536","inReplyTo":"20210919015729.98323-3-davvid@gmail.com","subject":"Re: [PATCH 3/4] difftool: use a strbuf to create the tmpdir path","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-09-19T09:00:07Z","receivedAt":"2021-09-19T09:00:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 19.09.21 um 03:57 schrieb David Aguilar:\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> double-slashes in the calculated paths.\n> \n> Add a unit test to ensure that double-slashes are not present.\n\nI wonder why it is necessary to strip trailing slashes? You even go so\nfar as to add a test case, but then bury the change in a commit with a\ntitle that is about a completely different topic.\n\nSo, which one of the two changes is the \"while at it, do that, too\" change?\n\n> @@ -360,11 +359,17 @@ 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> +\t/* Remove trailing slashes when $TMPDIR ends in '/'. */\n> +\twhile (tmpdir.len > 0 && tmpdir.buf[tmpdir.len - 1] == '/') {\n\nThis should most likely be is_dir_sep(tmpdir.buf[tmpdir.len - 1]).\n\n-- Hannes\n"},{"id":"436367","messageId":"875yuwd7a8.fsf@evledraar.gmail.com","threadId":"56536","inReplyTo":"20210919015729.98323-1-davvid@gmail.com","subject":"Re: [PATCH 1/4] t7800-difftool: cleanup temporary repositories used by tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-19T13:25:24Z","receivedAt":"2021-09-19T13:28:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Sep 18 2021, David Aguilar wrote:\n\n> The \"dirlinks\" and \"growing\" repositories should not outlive the\n> tests that use them.\n\nWhy not? The pattern of not bothering to clean up the work scratch are\nis fine as long as it doesn't leave crap around for other tests. See the\nreferenec to \"repo\" at[1].\n\nI.e. is this needed for later tests that change in this series, or just\ncleanup in case a future test ever cares that there's an untracked\n\"dirlinks\" and \"growing\" directory at the top-level?\n\n1. https://lore.kernel.org/git/87y27veeyj.fsf@evledraar.gmail.com/\n"},{"id":"436375","messageId":"CAJDDKr6AM0YZ-5GmXHz=2VbRKOusjQ-OAP2ng8GpBDb0x1pPRg@mail.gmail.com","threadId":"56536","inReplyTo":"8dfd0683-e019-a45c-30a4-d49ea627e36d@kdbg.org","subject":"Re: [PATCH 3/4] difftool: use a strbuf to create the tmpdir path","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-19T18:13:24Z","receivedAt":"2021-09-19T18:14:05Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Sep 19, 2021 at 2:00 AM Johannes Sixt <j6t@kdbg.org> wrote:\n>\n> Am 19.09.21 um 03:57 schrieb David Aguilar:\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> > double-slashes in the calculated paths.\n> >\n> > Add a unit test to ensure that double-slashes are not present.\n>\n> I wonder why it is necessary to strip trailing slashes? You even go so\n> far as to add a test case, but then bury the change in a commit with a\n> title that is about a completely different topic.\n>\n> So, which one of the two changes is the \"while at it, do that, too\" change?\n\nA better title might be:\n\n    difftool: use a strbuf to generate a tmpdir path without double-slashes\n\nThe double-slashes are the point. This patch is a minor cleanup that I\nfound on the path towards fixing the data loss bug in patch 4.\n\nThanks for the heads-up about the confusion ~ I'll reword in the next\nsubmission to make this point clearer.\n\nÆvar (cc'd) also asked why we have a patch that deletes the temporary\nrepositories used by the tests. It sounds like it's best to leave\nthose in place so the next submission will also drop that patch and\nwill adjust patch 4/4 (now 3/3) so that it also does not remove the\ntemporary repo used by its new test.\n\n\n> > @@ -360,11 +359,17 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> >\n> >       /* Setup temp directories */\n> >       tmp = getenv(\"TMPDIR\");\n> > -     xsnprintf(tmpdir, sizeof(tmpdir), \"%s/git-difftool.XXXXXX\", tmp ? tmp : \"/tmp\");\n> > -     if (!mkdtemp(tmpdir))\n> > -             return error(\"could not create '%s'\", tmpdir);\n> > -     strbuf_addf(&ldir, \"%s/left/\", tmpdir);\n> > -     strbuf_addf(&rdir, \"%s/right/\", tmpdir);\n> > +     strbuf_add_absolute_path(&tmpdir, tmp ? tmp : \"/tmp\");\n> > +     /* Remove trailing slashes when $TMPDIR ends in '/'. */\n> > +     while (tmpdir.len > 0 && tmpdir.buf[tmpdir.len - 1] == '/') {\n>\n> This should most likely be is_dir_sep(tmpdir.buf[tmpdir.len - 1]).\n\nIndeed. Peff also suggested strbuf_strip_trailing_dir_sep(&tmpdir)\nwhich is what we have in patch v2. That uses is_dir_sep().\n\nThis commit will be reworded, patch 1/4 \"t7800-difftool: cleanup\ntemporary repositories\" will be dropped, and the final patch will be\nadjusted to not cleanup its temporary test repository.\n\nI'll resend all 3 patches later with these suggestions as \"v4\".\n\ncheers,\n-- \nDavid\n"},{"id":"436376","messageId":"d882e3a7-d459-5063-eca7-e77e1addfed5@kdbg.org","threadId":"56536","inReplyTo":"CAJDDKr6AM0YZ-5GmXHz=2VbRKOusjQ-OAP2ng8GpBDb0x1pPRg@mail.gmail.com","subject":"Re: [PATCH 3/4] difftool: use a strbuf to create the tmpdir path","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-09-19T18:46:15Z","receivedAt":"2021-09-19T18:46:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 19.09.21 um 20:13 schrieb David Aguilar:\n> A better title might be:\n> \n>     difftool: use a strbuf to generate a tmpdir path without double-slashes\n> \n> The double-slashes are the point. This patch is a minor cleanup that I\n> found on the path towards fixing the data loss bug in patch 4.\n> \n> Thanks for the heads-up about the confusion ~ I'll reword in the next\n> submission to make this point clearer.\n\nThanks. My primary concern with the patch was actually that it looks\nlike mere code churn that introduces an error by not using is_dir_sep().\nThis is now settled.\n\nBut still, without a justification why the slashes should be cleaned up,\nthe patch looks like code churn. You can ignore me if it is obvious for\nothers why we need the patch.\n\n-- Hannes\n"},{"id":"436378","messageId":"CAJDDKr7+GnXX=sPfUvyxZhWOXCe8P_aOYtY_XdGS6t=cRNm8wA@mail.gmail.com","threadId":"56536","inReplyTo":"d882e3a7-d459-5063-eca7-e77e1addfed5@kdbg.org","subject":"Re: [PATCH 3/4] difftool: use a strbuf to create the tmpdir path","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-19T19:28:17Z","receivedAt":"2021-09-19T19:28:57Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Sep 19, 2021 at 11:46 AM Johannes Sixt <j6t@kdbg.org> wrote:\n>\n> Am 19.09.21 um 20:13 schrieb David Aguilar:\n> > A better title might be:\n> >\n> >     difftool: use a strbuf to generate a tmpdir path without double-slashes\n> >\n> > The double-slashes are the point. This patch is a minor cleanup that I\n> > found on the path towards fixing the data loss bug in patch 4.\n> >\n> > Thanks for the heads-up about the confusion ~ I'll reword in the next\n> > submission to make this point clearer.\n>\n> Thanks. My primary concern with the patch was actually that it looks\n> like mere code churn that introduces an error by not using is_dir_sep().\n> This is now settled.\n>\n> But still, without a justification why the slashes should be cleaned up,\n> the patch looks like code churn. You can ignore me if it is obvious for\n> others why we need the patch.\n\nNah, that's a good point. Thanks for clarifying. I'll hold off on\nresending until we've reached consensus on this patch.\n\nThe final bugfix patch is the most important one in the series so\nperhaps I should reorder the series so that the bugfix comes first,\nand these cosmetic improvements and typo fixes are the later ones in\nthe series? That'll make it so that the bugfix is easier to backport\nand not entangled with these prep fixups.\n\nI see double-slashes when using the dir-diff feature and they just\nlook wrong to me, but \"striving for perfection\" is a mere subjective\njustification.\n\nThe double-slashes fixup is cosmetic from a technical perspective, but\nsince the paths are exposed to the diff tools it's a cosmetic blemish\nthat users will see front and center.\n\nThe test environment where I observed TMPDIR containing a trailing\nslash is macOS, so it's a fairly common setup. One justification is\nthat we should not expose such blemishes to users -- that's what I've\nwritten below but perhaps there's a better way to express it?\n\nWhat do you and others think about the proposed message below and\nwhether or not this patch is worth applying?\n\n\"\"\"\ndifftool: use a strbuf to create a tmpdir path without repeated slashes\n\nThe 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\n\"\"\"\n-- \nDavid\n"},{"id":"436379","messageId":"b348baca-d9a8-8e71-b40f-0aace9d15ae5@kdbg.org","threadId":"56536","inReplyTo":"CAJDDKr7+GnXX=sPfUvyxZhWOXCe8P_aOYtY_XdGS6t=cRNm8wA@mail.gmail.com","subject":"Re: [PATCH 3/4] difftool: use a strbuf to create the tmpdir path","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-09-19T19:50:38Z","receivedAt":"2021-09-19T19:50:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 19.09.21 um 21:28 schrieb David Aguilar:\n> On Sun, Sep 19, 2021 at 11:46 AM Johannes Sixt <j6t@kdbg.org> wrote:\n>>\n>> Am 19.09.21 um 20:13 schrieb David Aguilar:\n>>> A better title might be:\n>>>\n>>>     difftool: use a strbuf to generate a tmpdir path without double-slashes\n>>>\n>>> The double-slashes are the point. This patch is a minor cleanup that I\n>>> found on the path towards fixing the data loss bug in patch 4.\n>>>\n>>> Thanks for the heads-up about the confusion ~ I'll reword in the next\n>>> submission to make this point clearer.\n>>\n>> Thanks. My primary concern with the patch was actually that it looks\n>> like mere code churn that introduces an error by not using is_dir_sep().\n>> This is now settled.\n>>\n>> But still, without a justification why the slashes should be cleaned up,\n>> the patch looks like code churn. You can ignore me if it is obvious for\n>> others why we need the patch.\n> \n> Nah, that's a good point. Thanks for clarifying. I'll hold off on\n> resending until we've reached consensus on this patch.\n> \n> The final bugfix patch is the most important one in the series so\n> perhaps I should reorder the series so that the bugfix comes first,\n> and these cosmetic improvements and typo fixes are the later ones in\n> the series? That'll make it so that the bugfix is easier to backport\n> and not entangled with these prep fixups.\n> \n> I see double-slashes when using the dir-diff feature and they just\n> look wrong to me, but \"striving for perfection\" is a mere subjective\n> justification.\n> \n> The double-slashes fixup is cosmetic from a technical perspective, but\n> since the paths are exposed to the diff tools it's a cosmetic blemish\n> that users will see front and center.\n> \n> The test environment where I observed TMPDIR containing a trailing\n> slash is macOS, so it's a fairly common setup. One justification is\n> that we should not expose such blemishes to users -- that's what I've\n> written below but perhaps there's a better way to express it?\n> \n> What do you and others think about the proposed message below and\n> whether or not this patch is worth applying?\n> \n> \"\"\"\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> \n> Add a unit test to ensure that repeated slashes are not present.\n> \n> \"\"\"\n\nWith the justification that the path is user-visible you have my full\nconsent to this patch. It shows a careful eye for the detail.\n\n-- Hannes\n"},{"id":"436448","messageId":"xmqqv92vayq2.fsf@gitster.g","threadId":"56536","inReplyTo":"20210919015729.98323-1-davvid@gmail.com","subject":"Re: [PATCH 1/4] t7800-difftool: cleanup temporary repositories used by tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-20T18:28:05Z","receivedAt":"2021-09-20T18:30:27Z","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> The \"dirlinks\" and \"growing\" repositories should not outlive the\n> tests that use them.\n>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n>  t/t7800-difftool.sh | 2 ++\n>  1 file changed, 2 insertions(+)\n>\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index a173f564bc..a923f193da 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -414,6 +414,7 @@ test_expect_success 'setup change in subdirectory' '\n>  test_expect_success 'difftool -d with growing paths' '\n>  \ta=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n>  \tgit init growing &&\n> +\ttest_when_finished rm -rf growing &&\n\nIf \"git init\" fails after it created the directory, it will be left\nbehind because test_when_finished hasn't been called yet.  The same\nproblem exists in the other hunk.  Moving it above \"git init\" may\ntrigger \"rm -rf X\" where X does not exist yet, but that is what you\nare giving the 'f'orce option there for.\n\nNot a huge deal and no need to resend only to fix them alone,\nthough.\n\n>  \t(\n>  \t\tcd growing &&\n>  \t\techo \"test -f \\\"\\$2/b\\\"\" | write_script .git/test-for-b.sh &&\n> @@ -646,6 +647,7 @@ test_expect_success 'difftool properly honors gitlink and core.worktree' '\n>  test_expect_success SYMLINKS 'difftool --dir-diff symlinked directories' '\n>  \ttest_when_finished git reset --hard &&\n>  \tgit init dirlinks &&\n> +\ttest_when_finished rm -rf dirlinks &&\n>  \t(\n>  \t\tcd dirlinks &&\n>  \t\tgit config diff.tool checktrees &&\n"},{"id":"436449","messageId":"xmqqr1djayec.fsf@gitster.g","threadId":"56536","inReplyTo":"20210919023329.23356-1-davvid@gmail.com","subject":"Re: [PATCH v2 3/4] difftool: use a strbuf to create the tmpdir path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-20T18:35:07Z","receivedAt":"2021-09-20T18:37:20Z","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> 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> double-slashes in the calculated paths.\n>\n> Add a unit test to ensure that double-slashes are not present.\n>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n> Changes since v1:\n> - Updated to use strbuf_trim_trailing_dir_sep().\n>\n> Thanks Peff!\n\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\nThis is taken hostage by the topic 'ab/retire-option-argument',\nwhich is not yet in 'master' and can never be merged to 'maint',\nwhich is not something you may want to do if this topic is meant as\na bugfix.\n\nI may queue this topic on top of a merge of 'ab/retire-option-argument'\ninto 'master' for now.\n\nThanks.\n"}]}