Re: [PATCH v2 3/3] difftool: handle modified symlinks in dir-diff mode

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 15, 2017, 18:54 UTC
Message-ID: <xmqq8to6fk15.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170315093130.30110-1-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 11 quoted lines

> Detect the null object ID for symlinks in dir-diff so that difftool can
> detect when symlinks are modified in the worktree.
>
> Previously, a null symlink object ID would crash difftool.
> Handle null object IDs as unknown content that must be read from
> the worktree.
>
> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> Only 3/3 was re-sent; the rest are the same.

OK, so in short you did the get_symlink() thing that was brought up in the previous round of review and everything else fell out as a natural consequence? That is wonderful ;-)

Show 7 quoted lines

> @@ -397,7 +438,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>  				return error("could not write '%s'", src_path);
>  		}
>  
> -		if (rmode) {
> +		if (rmode && !S_ISLNK(rmode)) {
>  			struct working_tree_entry *entry;

It still makes me wonder why the new !S_ISLNK() is done only for the right hand side, though. In fact, the processing done for the left hand side and the right hand side are vastly different even without this patch.

I suspect this is probably because the code is not prepared to drive the underlying "diff" when given the "-R" option (if I am reading the code correctly, argv[] that came from the end-user is appended to "diff --raw --no-abbrev -z", so the user could ask "difftool --dir-diff -R ..."), in which case you would see the working tree files as the left hand side of the diff. In the dir-diff mode, because you want to make only the working-tree side writable (and reflect whatever edit the user made back to the working-tree side), the choices you have to fix it would either be forbid "-R" (which is less preferrable as it is a more brittle solution between the two) or read the "diff --raw" output and swap the sides when you notice that LHS has 0{40} with non 0 mode, which is a sign that that side represents the working tree.

Having said all that, let's focus on the "symlink" stuff in this series.

Thanks.