On Wed, Mar 15, 2017 at 11:54:14AM -0700, Junio C Hamano wrote: > David Aguilar <dav...@gmail.com> writes: > > > @@ -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.
Agreed. I can take a look at the reverse-diff question later this week separately from this issue. The symlink test case I just added can be used as a starting ground for adding reverse-diff tests. Thanks Junio and Dscho for the reviews, -- David