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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 14, 2017, 04:13 UTC
Message-ID: <xmqq1su0lcm3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqlgs9rprt.fsf@gitster.mtv.corp.google.com>

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

Show 30 quoted lines

>> +	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.