{"thread":{"id":"56569","subject":"[PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches","startedAt":"2021-09-23T04:12:58Z","lastAt":"2021-09-30T17:05:51Z","messageCount":9,"participants":["David Aguilar","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":5,"patchTotal":3},"messages":[{"id":"436825","messageId":"20210923041252.52596-1-davvid@gmail.com","threadId":"56569","inReplyTo":null,"subject":"[PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-23T04:12:49Z","receivedAt":"2021-09-23T04:12:58Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"This is a resend of previously submitted patches.\n\nLinks to previous discussions:\nhttps://public-inbox.org/git/20210919203832.91207-1-davvid@gmail.com/\nhttps://public-inbox.org/git/20210920220855.18637-1-davvid@gmail.com/\nhttps://public-inbox.org/git/CAPig+cTBfP5_czsPiALcF3tODJmNfXvNkTjqVFRbHCS535d-ng@mail.gmail.com/\n\nPatches 2/3 and 3/3 have been rebased against \"next\" to resolve\nthe clash with ab/retire-option-argument.\n\nPatch 1/3 is maint-worthy and can be applied there without conflicts.\nThe commit message now uses the commit message ref format when referring to\nother commits. Its test was adjusted to do \"cd xyz\" instead of \"cd ./xyz\".\n\nPatch 2/3 has the strbuf leak avoidance brought up in the v4 round.\n\nPatch 3/3 is trivial and unchanged.\n\nDavid Aguilar (3):\n  difftool: fix symlink-file writing in dir-diff mode\n  difftool: create a tmpdir path without repeated slashes\n  difftool: add a missing space to the run_dir_diff() comments\n\n builtin/difftool.c  | 52 +++++++++++++++----------------\n t/t7800-difftool.sh | 75 +++++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 99 insertions(+), 28 deletions(-)\n\n-- \n2.33.0.720.g59ef144b50\n\n"},{"id":"436826","messageId":"20210923041252.52596-2-davvid@gmail.com","threadId":"56569","inReplyTo":"20210923041252.52596-1-davvid@gmail.com","subject":"[PATCH v5 1/3] difftool: fix symlink-file writing in dir-diff mode","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-23T04:12:50Z","receivedAt":"2021-09-23T04:13:00Z","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 18ec800512 (difftool: handle modified symlinks in dir-diff mode,\n2017-03-15) added handling for modified symlinks this bug got recorded\nin the test suite. The tests included the pointed-to symlink target\npaths. These paths were being reported because difftool was erroneously\nwriting to them, but they should 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 | 67 +++++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 67 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..528e0dabf0 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,71 @@ 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+\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.g59ef144b50\n\n"},{"id":"436827","messageId":"20210923041252.52596-3-davvid@gmail.com","threadId":"56569","inReplyTo":"20210923041252.52596-1-davvid@gmail.com","subject":"[PATCH v5 2/3] difftool: create a tmpdir path without repeated slashes","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-23T04:12:51Z","receivedAt":"2021-09-23T04:13:04Z","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 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 21e055d13a..861a4ec5fa 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 528e0dabf0..096456292c 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.g59ef144b50\n\n"},{"id":"436828","messageId":"20210923041252.52596-4-davvid@gmail.com","threadId":"56569","inReplyTo":"20210923041252.52596-1-davvid@gmail.com","subject":"[PATCH v5 3/3] difftool: add a missing space to the run_dir_diff() comments","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-23T04:12:52Z","receivedAt":"2021-09-23T04:13:06Z","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 861a4ec5fa..a972431f6d 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -542,7 +542,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.g59ef144b50\n\n"},{"id":"436888","messageId":"xmqqh7ebywu2.fsf@gitster.g","threadId":"56569","inReplyTo":"20210923041252.52596-1-davvid@gmail.com","subject":"Re: [PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-23T18:24:05Z","receivedAt":"2021-09-23T18:24:13Z","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 is a resend of previously submitted patches.\n\nVery much appreciated.\nWill queue.\n"},{"id":"436900","messageId":"xmqqv92rx8wa.fsf@gitster.g","threadId":"56569","inReplyTo":"20210923041252.52596-2-davvid@gmail.com","subject":"Re: [PATCH v5 1/3] difftool: fix symlink-file writing in dir-diff mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-23T21:46:29Z","receivedAt":"2021-09-23T21:46:35Z","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> 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\nCuriously, this pattern repeats twice in the vicinity of the code.\nWe cannot see it because it is out of pre-context, but the above is\na body of a loop that iterates over \"symlinks2\" hashmap.  There is\nanother identical loop that iterates over \"submodules\", and we are\nnot protecting ourselves from following a stray/leftover symbolic\nlink in the loop.\n\nI wonder if we should do the same to be defensive?  I also wondered\nif write_file() should be the one that may want to be doing the\nunlink(), but I ran out of time before I finished reading all the\ncallers to see if that is even a correct thing to do (meaning: some\ncaller may want to truly overwrite an existing file, and follow\nsymlinks if there already is, and I didn't audit all callers to see\nif there is no such caller).\n\nThe two identical looking loops also look like an accident waiting\nto happen---a patch like this that wants to touch only one of them\nwould risk application to the other, wrong, loop if the patch gets\nold enough and patch offset grows larger ;-).\n\nThanks.\n\n"},{"id":"436940","messageId":"871r5e45jw.fsf@evledraar.gmail.com","threadId":"56569","inReplyTo":"20210923041252.52596-3-davvid@gmail.com","subject":"Re: [PATCH v5 2/3] difftool: create a tmpdir path without repeated slashes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-24T10:35:52Z","receivedAt":"2021-09-24T10:43:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 22 2021, David Aguilar wrote:\n\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\nJust on this part: There was already a logic error in the pre-image\nwhere we'd return error() return values up to cmd_main() et al. This\nmeans that (on POSIX) we return 255, not 1, per the C standard doing\nthis is \"implementation defined\".\n\nI think you want the \"rc\" variable back, and either in another commit or\nin some other cleanup this on top or ahead of this patch:\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex a867c49067c..6605d17e6bc 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -345,6 +345,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tconst char *helper_argv[] = { \"difftool--helper\", NULL, NULL, NULL };\n \tstruct hashmap wt_modified, tmp_modified;\n \tint indices_loaded = 0;\n+\tint rc;\n \n \tworkdir = get_git_work_tree();\n \n@@ -574,7 +575,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-\tret = run_command_v_opt(helper_argv, flags);\n+\trc = run_command_v_opt(helper_argv, flags);\n \n \t/* TODO: audit for interaction with sparse-index. */\n \tensure_full_index(&wtindex);\n@@ -660,7 +661,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstrbuf_release(&tmpdir);\n \tUNLEAK(working_tree_dups);\n \n-\treturn ret;\n+\treturn ret < 0 ? 1 : rc;\n }\n \n static int run_file_diff(int prompt, const char *prefix,\n"},{"id":"437561","messageId":"CAJDDKr5QkUgHJVrij8bN-o0eNWr=eg7yHhHK0J8MxTALV0wr3Q@mail.gmail.com","threadId":"56569","inReplyTo":"xmqqv92rx8wa.fsf@gitster.g","subject":"Re: [PATCH v5 1/3] difftool: fix symlink-file writing in dir-diff mode","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-30T17:03:27Z","receivedAt":"2021-09-30T17:04:08Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Sep 23, 2021 at 2:46 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> David Aguilar <davvid@gmail.com> writes:\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> >               if (*entry->left) {\n> >                       add_path(&ldir, ldir_len, entry->path);\n> >                       ensure_leading_directories(ldir.buf);\n> > +                     unlink(ldir.buf);\n> >                       write_file(ldir.buf, \"%s\", entry->left);\n> >               }\n> >               if (*entry->right) {\n> >                       add_path(&rdir, rdir_len, entry->path);\n> >                       ensure_leading_directories(rdir.buf);\n> > +                     unlink(rdir.buf);\n> >                       write_file(rdir.buf, \"%s\", entry->right);\n> >               }\n> >       }\n>\n> Curiously, this pattern repeats twice in the vicinity of the code.\n> We cannot see it because it is out of pre-context, but the above is\n> a body of a loop that iterates over \"symlinks2\" hashmap.  There is\n> another identical loop that iterates over \"submodules\", and we are\n> not protecting ourselves from following a stray/leftover symbolic\n> link in the loop.\n\nI don't think the submodules loop ever runs into a scenario where the\nunlink would be relevant but it certainly wouldn't hurt from a defensive\nperspective.\n\n>\n> I wonder if we should do the same to be defensive?  I also wondered\n> if write_file() should be the one that may want to be doing the\n> unlink(), but I ran out of time before I finished reading all the\n> callers to see if that is even a correct thing to do (meaning: some\n> caller may want to truly overwrite an existing file, and follow\n> symlinks if there already is, and I didn't audit all callers to see\n> if there is no such caller).\n\nFrom my reading of write_file() usage it seems like we're better\noff dealing with this just in difftool only. We'd be doing a wasteful\nunlink() in most situations if we handled the unlinks in write_file().\n\n\n> The two identical looking loops also look like an accident waiting\n> to happen---a patch like this that wants to touch only one of them\n> would risk application to the other, wrong, loop if the patch gets\n> old enough and patch offset grows larger ;-).\n\nIndeed. Lifting this pattern out into a common helper would\nhelp reduce this risk here.\n\nI have a follow-up patch that addresses this and the edge cases\nthat Ævar pointed out about the exit codes that was just submitted.\n\nThey are incremental patches on top of these patches but I resent the\nentire series for convenience.\n--\nDavid\n"},{"id":"437562","messageId":"CAJDDKr6MqDhGXcPzCW8f96C3XEAp8w+imc9Jzrw+sUEasqXeng@mail.gmail.com","threadId":"56569","inReplyTo":"871r5e45jw.fsf@evledraar.gmail.com","subject":"Re: [PATCH v5 2/3] difftool: create a tmpdir path without repeated slashes","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-30T17:05:12Z","receivedAt":"2021-09-30T17:05:51Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Sep 24, 2021 at 3:43 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Wed, Sep 22 2021, David Aguilar wrote:\n>\n> > @@ -580,7 +574,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> >               flags = 0;\n> >       } else\n> >               setenv(\"GIT_DIFFTOOL_DIRDIFF\", \"true\", 1);\n> > -     rc = run_command_v_opt(helper_argv, flags);\n> > +     ret = run_command_v_opt(helper_argv, flags);\n> >\n> >       /* TODO: audit for interaction with sparse-index. */\n> >       ensure_full_index(&wtindex);\n>\n> Just on this part: There was already a logic error in the pre-image\n> where we'd return error() return values up to cmd_main() et al. This\n> means that (on POSIX) we return 255, not 1, per the C standard doing\n> this is \"implementation defined\".\n>\n> I think you want the \"rc\" variable back, and either in another commit or\n> in some other cleanup this on top or ahead of this patch:\n\nGood catch. I submitted an incremental follow-up patch that addresses\nthis issue.\n\n\n> diff --git a/builtin/difftool.c b/builtin/difftool.c\n> index a867c49067c..6605d17e6bc 100644\n> --- a/builtin/difftool.c\n> +++ b/builtin/difftool.c\n> @@ -345,6 +345,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n>         const char *helper_argv[] = { \"difftool--helper\", NULL, NULL, NULL };\n>         struct hashmap wt_modified, tmp_modified;\n>         int indices_loaded = 0;\n> +       int rc;\n>\n>         workdir = get_git_work_tree();\n>\n> @@ -574,7 +575,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n>                 flags = 0;\n>         } else\n>                 setenv(\"GIT_DIFFTOOL_DIRDIFF\", \"true\", 1);\n> -       ret = run_command_v_opt(helper_argv, flags);\n> +       rc = run_command_v_opt(helper_argv, flags);\n>\n>         /* TODO: audit for interaction with sparse-index. */\n>         ensure_full_index(&wtindex);\n> @@ -660,7 +661,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n>         strbuf_release(&tmpdir);\n>         UNLEAK(working_tree_dups);\n>\n> -       return ret;\n> +       return ret < 0 ? 1 : rc;\n>  }\n\n\n\"return ret < 0 ? 1 : rc;\" seems rather subtle.\n\nIn the submitted patch we're able to keep this as \"return ret;\" by\nnever assigning -1 in the first place.\n\nMy rationale is that it's better to not have to apply this fixup after the fact.\n\nThanks for reviewing.\n--\nDavid\n"}]}