Re: [PATCH] difftool: handle changing symlinks in dir-diff mode

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

>> +	struct strbuf path = STRBUF_INIT;
>> +	struct strbuf link = STRBUF_INIT;
>> +
>> +	int ok = 0;
>> +
>> +	if (strbuf_readlink(&link, ce->name, ce_namelen(ce)) == 0) {
>> +		strbuf_add(&path, state->base_dir, state->base_dir_len);
>> +		strbuf_add(&path, ce->name, ce_namelen(ce));
>> +
>> +		write_file_buf(path.buf, link.buf, link.len);
>
> This does "write content into symlink stand-in file", but why?  A
> symbolic link that is not changed on the right-hand side of the
> comparison (i.e. S_ISLNK(rmode) && !is_null_oid(&roid)) would go to
> checkout_entry() which
>
>  - creates a symbolic link, on a filesystem that supports symlink; or
>  - writes the stand-in file on a filesystem that does not.
>
> which is the right thing.  It seems that the other side of "if (!use_wt_file())"
> if/elseif/... cascade also does the right thing manually.
>
> Shouldn't this helper also do the same (i.e. create symlink and fall
> back to stand-in)?
>
> Also, I am not sure if strbuf_readlink() can unconditionally used
> here.  On a filesystem without symbolic link, the working tree
> entity that corresponds to the ce that represents a symlink is a
> stand-in regular file, so shouldn't we be opening it as a regular
> file and reading its contents in that case?

I _think_ I was mistaken (please correct me again if my reasoning
below is wrong) and unconditional writing of a plain regular file is
what this codepath wants to do, because we are preparing two temporary
directories, to be fed to a Git-unaware tool that knows how to show
a diff of two directories (e.g. "diff -r A B").  Because the tool is
Git-unaware, if a symbolic link appears in either of these temporary
directories, it will try to dereference and show the difference of
the target of the symbolic link, which is not what we want, as the
goal of the dir-diff mode is to produce an output that is logically
equivalent to what "git diff" produces---most importantly, we want
to get textual comparison of the result of the readlink(2).  And
write_file_buf() used here is exactly that---we write the contents
of symlink to a regular file to force the external tool to compare
the readlink(2) result as if it is a text.  Even on a filesystem
that is capable of doing a symbolic link.

The strbuf_readlink() that read the contents of symlink, however,
is wrong on a filesystem that is not capable of a symbolic link; if
core.symlinks is false, we do need to read them as a regular file.