Re: [PATCH v4 1/3] difftool: fix symlink-file writing in dir-diff mode
flat view
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-09-20 08:03:11
On Sun, Sep 19 2021, David Aguilar wrote:
The difftool dir-diff mode handles symlinks by replacing them with their readlink(2) values. This allows diff tools to see changes to symlinks as if they were regular text diffs with the old and new path values. This is analogous to what "git diff" displays when symlinks change. The temporary diff directories that are created initially contain symlinks because they get checked-out using a temporary index that retains the original symlinks as checked-in to the repository. A bug was introduced when difftool was rewritten in C that made difftool write the readlink(2) contents into the pointed-to file rather than the symlink itself. The write was going through the symlink and writing to its target rather than writing to the symlink path itself. Replace symlinks with raw text files by unlinking the symlink path before writing the readlink(2) content into them. When 18ec800512eb0634a0bf5e86b36ed156fbee73f3 added handling for
Nit: Please format commits like this: 18ec800512e (difftool: handle modified symlinks in dir-diff mode, 2017-03-15) See "git show -s --pretty=reference" in Documentation/SubmittingPatches.
quoted hunk ↗ jump to hunk
modified symlinks this bug got recorded in the test suite. The tests included the pointed-to symlink target paths. These paths were being reported because difftool was erroneously writing to them, but they should have never been reported nor written. Correct the modified-symlinks test cases by removing the target files from the expected output. Add a test to ensure that symlinks are written with the readlink(2) values and that the target files contain their original content. Reported-by: Alan Blotz <redacted> Helped-by: Đoàn Trần Công Danh [off-list ref] Signed-off-by: David Aguilar <redacted> --- builtin/difftool.c | 2 ++ t/t7800-difftool.sh | 68 +++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 68 insertions(+), 2 deletions(-)diff --git a/builtin/difftool.c b/builtin/difftool.c index bb9fe7245a..21e055d13a 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c@@ -557,11 +557,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, if (*entry->left) { add_path(&ldir, ldir_len, entry->path); ensure_leading_directories(ldir.buf); + unlink(ldir.buf); write_file(ldir.buf, "%s", entry->left); } if (*entry->right) { add_path(&rdir, rdir_len, entry->path); ensure_leading_directories(rdir.buf); + unlink(rdir.buf); write_file(rdir.buf, "%s", entry->right); } }diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index a173f564bc..07a52fb8e1 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh@@ -674,7 +674,6 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' ' rm c && ln -s d c && cat >expect <<-EOF && - b c c@@ -710,7 +709,6 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' ' # Deleted symlinks rm -f c && cat >expect <<-EOF && - b c EOF@@ -723,6 +721,72 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' ' test_cmp expect actual ' +test_expect_success SYMLINKS 'difftool --dir-diff writes symlinks as raw text' ' + # Start out on a branch called "branch-init". + git init -b branch-init symlink-files && + ( + cd ./symlink-files &&
The "./" isn't needed Neither of those small comments needs a re-roll I think, just reading through & noting things I spotted... I've omitted things that Eric mentioned in [ref].