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

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

David Aguilar <davvid@gmail.com> writes:

> Detect the null object ID for symlinks in dir-diff so that difftool can
> prepare temporary files that matches how git handles symlinks.
>
> Previously, a null object ID would crash difftool.  We now detect null
> object IDs and write the symlink's content into the temporary symlink
> stand-in file.
>
> Original-patch-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---

I would have appreciated (and I suspect other reviewers would, too)
a bit of back-story wrt how "Original-patch-by" resulted in this
patch after the three-dashes line.  It is perfectly fine if you two
coordinated privately; I mostly wanted to hear something like "Dscho
has been working on this and I asked him if it is OK to take over
his WIP to produce a quick-fix we can ship on the maint branch, here
is the result of that collaboration."  IOW, the person who is named
as the original author is fine to be named like so (I care only
because I do not think we saw the "original" here on the list).

> +static int create_symlink_file(struct cache_entry* ce, struct checkout* state)

Asterisk sticks to variable, not type.

> +{
> +	/*
> +	 * Dereference a worktree symlink and writes its contents

s/writes/write/

> +	 * into the checkout state's path.
> +	 */
> +	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?