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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 13, 2017, 18:32 UTC
Message-ID: <xmqqlgs9rprt.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170313175640.14106-1-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 10 quoted lines

> 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/

Show 12 quoted lines

> +	 * 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?