Web lists-archives.com

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

David Aguilar <davvid@xxxxxxxxx> writes:

> 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@xxxxxx>
> Signed-off-by: David Aguilar <davvid@xxxxxxxxx>
> ---
> 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 ;-)

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