Re: [PATCH v4 1/3] difftool: fix symlink-file writing in dir-diff mode

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.

> 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 <work@blotz.org>
> Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  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
<CAPig+cTBfP5_czsPiALcF3tODJmNfXvNkTjqVFRbHCS535d-ng@mail.gmail.com>.