On Mon, Sep 16, 2019 at 5:09 PM Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: > > Jonathan Tan wrote: > > > This was raised by a coworker at $DAYJOB. I run the following script: > [reproduction recipe from (*) snipped] > > The cherry-pick must be manually resolved, when I would expect it to > > happen without needing user intervention. > > > > You can see that at the point of the cherry-pick, in the working > > directory, ./foo is a symlink and ./foo/bar is a directory. I traced the > > code that ran during the cherry-pick to process_entry() in > > merge-recursive.c. When processing "foo/bar", control flow correctly > > reaches "Case B: Added in one", but the dir_in_way() invocation returns > > true, since lstat() indeed reveals that "foo/bar" is a directory. > > Gábor covered the "what happened", so let me say a little more about > the motivation. > > The "foo" symlink is being replaced by a "foo" directory containing a > "bar" file. We're pretty far along now: we want to write actual files > to disk. Using the index we know where we were going from and to, but > not everything in the world is tracked in the index: there could be > build outputs under "foo/bar" blocking the operation from moving > forward. > > So we check whether there's a directory there. Once we are writing > things out, there won't be, but the symlink confuses us. Nice find. Yep. > > [...] > > Is this use case something that Git should be able to handle, and if > > yes, is the correct solution to teach dir_in_way() that dirs reachable > > from symlinks are not really in the way (presumably the implementation > > would climb directories until we reach the root or we reach a filesystem > > boundary, similar to what we do when we search for the .git directory)? > > The crucial detail here is that "foo" is going to be removed before we > write "foo/bar". We should be able to notice that and skip the > dir_in_way check. I know what you're getting at from a high level view, but this view is incompatible with the machinery's internals. In particular, merge-recursive's design provides no way to "notice" that something is "*going* to be removed" (every path is updated on the fly as it processes it); the dir_in_way() check is there precisely to determine if it's safe to write to the given path -- which basically means if there is no directory in the way (a "foo/bar/" directory, in this case). So we definitely do not want to skip the dir_in_way() check, we want to modify it to be aware that a leading path being a symlink doesn't count as in the way (much as a the existence of a file on disk corresponding to one of our leading paths doesn't count as in the way for our purposes).
