{"thread":{"id":"51860","subject":"merge-recursive thinks symlink's child dirs are \"real\"","startedAt":"2019-09-16T21:47:19Z","lastAt":"2019-09-18T00:35:43Z","messageCount":13,"participants":["Jonathan Tan","SZEDER Gábor","Jonathan Nieder","Elijah Newren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"382465","messageId":"20190916214707.190171-1-jonathantanmy@google.com","threadId":"51860","inReplyTo":null,"subject":"merge-recursive thinks symlink's child dirs are \"real\"","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-09-16T21:47:07Z","receivedAt":"2019-09-16T21:47:19Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This was raised by a coworker at $DAYJOB. I run the following script:\n\n  $GIT init test && cd test\n  ln -s . foo\n  mkdir bar && touch bar/file\n  $GIT add foo bar/file\n  $GIT commit -m \"foo symlink\"\n  \n  $GIT checkout -b branch1\n  $GIT commit --allow-empty -m \"empty commit\"\n  \n  $GIT checkout master\n  $GIT rm foo\n  mkdir foo\n  (cd foo; ln -s ../bar bar)\n  $GIT add foo/bar\n  $GIT commit -m \"replace foo symlink with real foo dir and foo/bar symlink\"\n  \n  $GIT checkout branch1\n  $GIT cherry-pick master\n\nThe cherry-pick must be manually resolved, when I would expect it to\nhappen without needing user intervention.\n\nYou can see that at the point of the cherry-pick, in the working\ndirectory, ./foo is a symlink and ./foo/bar is a directory. I traced the\ncode that ran during the cherry-pick to process_entry() in\nmerge-recursive.c. When processing \"foo/bar\", control flow correctly\nreaches \"Case B: Added in one\", but the dir_in_way() invocation returns\ntrue, since lstat() indeed reveals that \"foo/bar\" is a directory. If I\nhardcode dir_in_way() to return false, then the cherry-pick happens\nwithout needing user intervention. I checked with \"ls-tree -r\" and the\nresulting tree is as I expect (foo is a real dir, foo/bar is a symlink).\n\nIs this use case something that Git should be able to handle, and if\nyes, is the correct solution to teach dir_in_way() that dirs reachable\nfrom symlinks are not really in the way (presumably the implementation\nwould climb directories until we reach the root or we reach a filesystem\nboundary, similar to what we do when we search for the .git directory)?\nAlso, my proposed solution would work in the specific use case outlined\nin the script above, but can anyone think offhand of a case that it\nwould make worse?\n"},{"id":"382468","messageId":"20190916221501.GD6190@szeder.dev","threadId":"51860","inReplyTo":"20190916214707.190171-1-jonathantanmy@google.com","subject":"Re: merge-recursive thinks symlink's child dirs are \"real\"","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-16T22:15:01Z","receivedAt":"2019-09-16T22:15:07Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Sep 16, 2019 at 02:47:07PM -0700, Jonathan Tan wrote:\n> This was raised by a coworker at $DAYJOB. I run the following script:\n> \n>   $GIT init test && cd test\n>   ln -s . foo\n>   mkdir bar && touch bar/file\n>   $GIT add foo bar/file\n>   $GIT commit -m \"foo symlink\"\n>   \n>   $GIT checkout -b branch1\n>   $GIT commit --allow-empty -m \"empty commit\"\n>   \n>   $GIT checkout master\n>   $GIT rm foo\n>   mkdir foo\n>   (cd foo; ln -s ../bar bar)\n>   $GIT add foo/bar\n>   $GIT commit -m \"replace foo symlink with real foo dir and foo/bar symlink\"\n>   \n>   $GIT checkout branch1\n>   $GIT cherry-pick master\n> \n> The cherry-pick must be manually resolved, when I would expect it to\n> happen without needing user intervention.\n> \n> You can see that at the point of the cherry-pick, in the working\n> directory, ./foo is a symlink and ./foo/bar is a directory. I traced the\n> code that ran during the cherry-pick to process_entry() in\n> merge-recursive.c. When processing \"foo/bar\", control flow correctly\n> reaches \"Case B: Added in one\", but the dir_in_way() invocation returns\n> true, since lstat() indeed reveals that \"foo/bar\" is a directory. If I\n> hardcode dir_in_way() to return false, then the cherry-pick happens\n> without needing user intervention. I checked with \"ls-tree -r\" and the\n> resulting tree is as I expect (foo is a real dir, foo/bar is a symlink).\n> \n> Is this use case something that Git should be able to handle,\n\nFWIW, Git used to handle this case, but it broke with edd2faf52e\n(merge-recursive: Consolidate process_entry() and process_df_entry(),\n2011-08-11).\n\nCc-ing Elijah for insights...\n\n> and if\n> yes, is the correct solution to teach dir_in_way() that dirs reachable\n> from symlinks are not really in the way (presumably the implementation\n> would climb directories until we reach the root or we reach a filesystem\n> boundary, similar to what we do when we search for the .git directory)?\n> Also, my proposed solution would work in the specific use case outlined\n> in the script above, but can anyone think offhand of a case that it\n> would make worse?\n"},{"id":"382472","messageId":"20190917000902.GC67467@google.com","threadId":"51860","inReplyTo":"20190916214707.190171-1-jonathantanmy@google.com","subject":"Re: merge-recursive thinks symlink's child dirs are \"real\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-09-17T00:09:02Z","receivedAt":"2019-09-17T00:09:08Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> This was raised by a coworker at $DAYJOB. I run the following script:\n[reproduction recipe from (*) snipped]\n> The cherry-pick must be manually resolved, when I would expect it to\n> happen without needing user intervention.\n>\n> You can see that at the point of the cherry-pick, in the working\n> directory, ./foo is a symlink and ./foo/bar is a directory. I traced the\n> code that ran during the cherry-pick to process_entry() in\n> merge-recursive.c. When processing \"foo/bar\", control flow correctly\n> reaches \"Case B: Added in one\", but the dir_in_way() invocation returns\n> true, since lstat() indeed reveals that \"foo/bar\" is a directory.\n\nGábor covered the \"what happened\", so let me say a little more about\nthe motivation.\n\nThe \"foo\" symlink is being replaced by a \"foo\" directory containing a\n\"bar\" file.  We're pretty far along now: we want to write actual files\nto disk.  Using the index we know where we were going from and to, but\nnot everything in the world is tracked in the index: there could be\nbuild outputs under \"foo/bar\" blocking the operation from moving\nforward.\n\nSo we check whether there's a directory there.  Once we are writing\nthings out, there won't be, but the symlink confuses us.  Nice find.\n\n[...]\n> Is this use case something that Git should be able to handle, and if\n> yes, is the correct solution to teach dir_in_way() that dirs reachable\n> from symlinks are not really in the way (presumably the implementation\n> would climb directories until we reach the root or we reach a filesystem\n> boundary, similar to what we do when we search for the .git directory)?\n\nThe crucial detail here is that \"foo\" is going to be removed before we\nwrite \"foo/bar\".  We should be able to notice that and skip the\ndir_in_way check.\n\nJust my uninformed guess --- I haven't tried it. :)\n\nThoughts?\n\nThanks,\nJonathan\n\n(*) https://public-inbox.org/git/20190916214707.190171-1-jonathantanmy@google.com/\n"},{"id":"382492","messageId":"CABPp-BHpXWF+1hKUTfn8s-y4MJZXz+jUVS_K10eKyD6PGwo=gg@mail.gmail.com","threadId":"51860","inReplyTo":"20190916214707.190171-1-jonathantanmy@google.com","subject":"Re: merge-recursive thinks symlink's child dirs are \"real\"","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T15:48:40Z","receivedAt":"2019-09-17T15:48:57Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Sep 16, 2019 at 4:09 PM Jonathan Tan <jonathantanmy@google.com> wrote:\n>\n> This was raised by a coworker at $DAYJOB. I run the following script:\n>\n>   $GIT init test && cd test\n>   ln -s . foo\n>   mkdir bar && touch bar/file\n>   $GIT add foo bar/file\n>   $GIT commit -m \"foo symlink\"\n>\n>   $GIT checkout -b branch1\n>   $GIT commit --allow-empty -m \"empty commit\"\n>\n>   $GIT checkout master\n>   $GIT rm foo\n>   mkdir foo\n>   (cd foo; ln -s ../bar bar)\n>   $GIT add foo/bar\n>   $GIT commit -m \"replace foo symlink with real foo dir and foo/bar symlink\"\n>\n>   $GIT checkout branch1\n>   $GIT cherry-pick master\n>\n> The cherry-pick must be manually resolved, when I would expect it to\n> happen without needing user intervention.\n>\n> You can see that at the point of the cherry-pick, in the working\n> directory, ./foo is a symlink and ./foo/bar is a directory. I traced the\n> code that ran during the cherry-pick to process_entry() in\n> merge-recursive.c. When processing \"foo/bar\", control flow correctly\n> reaches \"Case B: Added in one\", but the dir_in_way() invocation returns\n> true, since lstat() indeed reveals that \"foo/bar\" is a directory. If I\n> hardcode dir_in_way() to return false, then the cherry-pick happens\n> without needing user intervention. I checked with \"ls-tree -r\" and the\n> resulting tree is as I expect (foo is a real dir, foo/bar is a symlink).\n\nNice catch, and good digging.\n\n> Is this use case something that Git should be able to handle, and if\n> yes, is the correct solution to teach dir_in_way() that dirs reachable\n> from symlinks are not really in the way\n\nYes, and mostly yes.  I'd phrase it instead as \"if any leading path\nwithin the repository of the given filename is a symlink, then it is\nnot in the way\".  More on that below.\n\n> (presumably the implementation\n> would climb directories until we reach the root or we reach a filesystem\n> boundary, similar to what we do when we search for the .git directory)?\n\nThe .git directory handling is a bit different because you don't have\na repository toplevel yet.  Here you are trying to add a path\n\"foo/bar\"; it could just as well have been named \"a/b/c/d/e/f\", but\nregardless, in all cases it's not an absolute path but a path relative\nto the toplevel of the repository.  We'll have to check all paths up\nto the repository toplevel and if any of the leading directories are a\nsymlink (just \"foo/\" in the first case, five checks for \"a/\" and\n\"a/b/\" and ... \"a/b/c/d/e\" in the second case), then have dir_in_way()\nreturn false.\n\nIt might be interesting to test out the performance afterward; we may\nneed to cache the check-paths-for-symlink status to avoid re-stating\nthe same leading directories repeatedly.  (But then again, we only\nneed to do this for files that unpack_trees() couldn't resolve and\nmarked as conflicted, so maybe it's not that big of an issue.)\n\n> Also, my proposed solution would work in the specific use case outlined\n> in the script above, but can anyone think offhand of a case that it\n> would make worse?\n\nWell, merge-recursive is hairy and slightly brittle based on its\nupdate-as-you-go-and-do-a-four-way-merge design, so there's a small\nchance this could introduce problems.  I've tried to come up with some\nand haven't been able to.  It is the right idea implementationally,\nthough.  Let me explain:\n\nIn merge_trees() we loop over the index entries (from entries->nr-1\ndown to 0) calling process_entry(), and process_entry() updates the\nindex and working copy as it goes.  The thing about this reverse order\nis that in the case of directory/file conflicts, directories will be\nhandled before the corresponding file, giving us a chance to check if\nthe directory has been removed (i.e. the resolution of the merge\nremoves that directory and there were no untracked files left in it)\nbefore we handle the file. Let's walk through how this works:\n\nAfter unpack_trees, we end up with a result that looks like:\n\nIndex:\n  * bar/file (not conflicted; same in all three commits)\n  * foo (deleted on one side -- conflict (could be part of rename),\nalso, part of D/F conflict)\n  * foo/bar (added on one side -- conflict (could be part of rename),\nalso, leading path part of D/F conflict)\n\nThen we check for renames.  There are none.  Then we process all the\nindex entries in reverse lexicographic order, skipping the ones that\nhave already been processed (i.e. which weren't marked as conflicted\nby unpack_trees).\n\nfoo/bar is first; it was added on one side, so we should keep it.  Is\nthere a foo/bar/ directory in the way in the index?  No.  Is there a\nfoo/bar/ directory in the way in the working copy?  The current code\ngot tricked, but as you point out the answer should be no because foo\nis a symlink.  Note that the code is already smart enough to say \"no\"\nif \"foo\" is present as a regular file.  In particular, we don't care\nthat there is a file in the way of a leading path at all.\ndir_in_way() simply says there is nothing _critical_ in the way (which\nis defined to only be subdirectories), and then make_room_for_path()\nwill nuke any leading files in the way of creating directories.\n\nOnce we've processed foo/bar, the next entry we process is \"foo\".  In\nthe case, it was unmodifed on one side and deleted on the other so we\ncan just delete it.  We delete it from the index, note it was already\nnuked from the working copy, and we're happy.  (By way of comparison,\nif you changed your empty commit to modify the location of the foo\nsymlink, then we would instead have a modify/delete conflict for foo.\nWhile foo will have been removed from the working directory, we can\nre-instate it at an alternate location.  For D/F conflicts, we make\nthe assumption that it's always better to move the file of the D/F\nconflict aside to a different path than to move the whole directory\naside.)\n\nSo, in short, all your idea does is treat symlinks like regular files\nin terms of removability, which is exactly what we want.\n"},{"id":"382493","messageId":"CABPp-BEjsfPb8BYjdDGwwJE2GEbRDsujV2xD=c-0zygJCv4F0g@mail.gmail.com","threadId":"51860","inReplyTo":"20190916221501.GD6190@szeder.dev","subject":"Re: merge-recursive thinks symlink's child dirs are \"real\"","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T15:54:52Z","receivedAt":"2019-09-17T15:55:10Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Sep 16, 2019 at 3:15 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n>\n> On Mon, Sep 16, 2019 at 02:47:07PM -0700, Jonathan Tan wrote:\n> > This was raised by a coworker at $DAYJOB. I run the following script:\n> >\n> >   $GIT init test && cd test\n> >   ln -s . foo\n> >   mkdir bar && touch bar/file\n> >   $GIT add foo bar/file\n> >   $GIT commit -m \"foo symlink\"\n> >\n> >   $GIT checkout -b branch1\n> >   $GIT commit --allow-empty -m \"empty commit\"\n> >\n> >   $GIT checkout master\n> >   $GIT rm foo\n> >   mkdir foo\n> >   (cd foo; ln -s ../bar bar)\n> >   $GIT add foo/bar\n> >   $GIT commit -m \"replace foo symlink with real foo dir and foo/bar symlink\"\n> >\n> >   $GIT checkout branch1\n> >   $GIT cherry-pick master\n> >\n> > The cherry-pick must be manually resolved, when I would expect it to\n> > happen without needing user intervention.\n> >\n> > You can see that at the point of the cherry-pick, in the working\n> > directory, ./foo is a symlink and ./foo/bar is a directory. I traced the\n> > code that ran during the cherry-pick to process_entry() in\n> > merge-recursive.c. When processing \"foo/bar\", control flow correctly\n> > reaches \"Case B: Added in one\", but the dir_in_way() invocation returns\n> > true, since lstat() indeed reveals that \"foo/bar\" is a directory. If I\n> > hardcode dir_in_way() to return false, then the cherry-pick happens\n> > without needing user intervention. I checked with \"ls-tree -r\" and the\n> > resulting tree is as I expect (foo is a real dir, foo/bar is a symlink).\n> >\n> > Is this use case something that Git should be able to handle,\n>\n> FWIW, Git used to handle this case, but it broke with edd2faf52e\n> (merge-recursive: Consolidate process_entry() and process_df_entry(),\n> 2011-08-11).\n>\n> Cc-ing Elijah for insights...\n\nThanks for the heads up.\n\nYeah, git used to get _extremely_ confused with D/F conflicts, even\nworse than here.  Interesting that it took 8 years to find this case.\nAnyway, the problem here is that a symlink involved in a D/F conflict\ncould be a symlink to a directory instead of to a file, and then make\nit look like we have some weird nested D/F conflicts in cases when we\ndon't (i.e. make it look like not only \"foo\" exists as both a file\n(symlink) and directory, but that \"foo/bar\" does too).\n\nAnyway, being more careful to treat symlinks in D/F conflicts more\nlike how we treat files in such conflicts, using the way Jonathan Tan\nsuggested, should fix this issue up.  I put more details in my email\nto him.\n"},{"id":"382494","messageId":"CABPp-BFqZS5WaR1b-MtQ0eMrsDJNSNjSrC0NjPpKk6+5WtNNWw@mail.gmail.com","threadId":"51860","inReplyTo":"20190917000902.GC67467@google.com","subject":"Re: merge-recursive thinks symlink's child dirs are \"real\"","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T16:02:07Z","receivedAt":"2019-09-17T16:02:21Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Sep 16, 2019 at 5:09 PM Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n> Jonathan Tan wrote:\n>\n> > This was raised by a coworker at $DAYJOB. I run the following script:\n> [reproduction recipe from (*) snipped]\n> > The cherry-pick must be manually resolved, when I would expect it to\n> > happen without needing user intervention.\n> >\n> > You can see that at the point of the cherry-pick, in the working\n> > directory, ./foo is a symlink and ./foo/bar is a directory. I traced the\n> > code that ran during the cherry-pick to process_entry() in\n> > merge-recursive.c. When processing \"foo/bar\", control flow correctly\n> > reaches \"Case B: Added in one\", but the dir_in_way() invocation returns\n> > true, since lstat() indeed reveals that \"foo/bar\" is a directory.\n>\n> Gábor covered the \"what happened\", so let me say a little more about\n> the motivation.\n>\n> The \"foo\" symlink is being replaced by a \"foo\" directory containing a\n> \"bar\" file.  We're pretty far along now: we want to write actual files\n> to disk.  Using the index we know where we were going from and to, but\n> not everything in the world is tracked in the index: there could be\n> build outputs under \"foo/bar\" blocking the operation from moving\n> forward.\n>\n> So we check whether there's a directory there.  Once we are writing\n> things out, there won't be, but the symlink confuses us.  Nice find.\n\nYep.\n\n>\n> [...]\n> > Is this use case something that Git should be able to handle, and if\n> > yes, is the correct solution to teach dir_in_way() that dirs reachable\n> > from symlinks are not really in the way (presumably the implementation\n> > would climb directories until we reach the root or we reach a filesystem\n> > boundary, similar to what we do when we search for the .git directory)?\n>\n> The crucial detail here is that \"foo\" is going to be removed before we\n> write \"foo/bar\".  We should be able to notice that and skip the\n> dir_in_way check.\n\nI know what you're getting at from a high level view, but this view is\nincompatible with the machinery's internals.  In particular,\nmerge-recursive's design provides no way to \"notice\" that something is\n\"*going* to be removed\" (every path is updated on the fly as it\nprocesses it); the dir_in_way() check is there precisely to determine\nif it's safe to write to the given path -- which basically means if\nthere is no directory in the way (a \"foo/bar/\" directory, in this\ncase).  So we definitely do not want to skip the dir_in_way() check,\nwe want to modify it to be aware that a leading path being a symlink\ndoesn't count as in the way (much as a the existence of a file on disk\ncorresponding to one of our leading paths doesn't count as in the way\nfor our purposes).\n"},{"id":"382523","messageId":"20190917215040.132503-1-jonathantanmy@google.com","threadId":"51860","inReplyTo":"CABPp-BHpXWF+1hKUTfn8s-y4MJZXz+jUVS_K10eKyD6PGwo=gg@mail.gmail.com","subject":"[RFC PATCH] merge-recursive: symlink's descendants not in way","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-09-17T21:50:40Z","receivedAt":"2019-09-17T21:50:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When the working tree has:\n - foo (symlink)\n - foo/bar (directory)\n\nand the user merges a commit that deletes the foo symlink and instead\ncontains:\n - foo (directory)\n - foo/bar (file)\n\nthe merge should happen without requiring user intervention. However,\nthis does not happen.\n\nIn merge_trees(), process_entry() will be invoked first for \"foo/bar\",\nthen \"foo\" (in reverse lexicographical order). process_entry() correctly\nreaches \"Case B: Added in one\", but dir_in_way() states that \"bar\" is\nalready present as a directory, causing a directory/file conflict at the\nwrong point.\n\nInstead, teach dir_in_way() that directories under symlinks are not \"in\nthe way\", so that symlinks are treated as regular files instead of\ndirectories containing other directories and files. Thus, the \"else\"\nbranch will be followed instead: \"foo/bar\" will be added to the working\ntree, make_room_for_path() being indirectly called to unlink the \"foo\"\nsymlink (just like if \"foo\" were a regular file instead). When\nprocess_entry() is subsequently invoked for \"foo\", process_entry() will\nreach \"Case A: Deleted in one\", and will handle it as \"Add/delete\" or\n\"Modify/delete\" appropriately (including reinstatement of the previously\nunlinked symlink with a new unique filename if necessary, again, just\nlike if \"foo\" were a regular file instead).\n\nHelped-by: Elijah Newren <newren@gmail.com>\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThanks to Elijah for his help. Some of the commit message is based on\nhis explanation [1].\n\nI'm finding this relatively complicated, so I'm sending this as RFC. My\nmain concern is that whether all callers of dir_in_way() are OK with its\nbehavior change, and if yes, how to explain it. I suspect that this is\ncorrect because dir_in_way() should behave consistently for all its\ncallers, but I might be wrong.\n\nThe other thing is whether the commit message is clear enough. In\nparticular, whether it needs more detail (I didn't mention the index,\nfor example), or whether it should be more concise (for example, I\nthought of just stating that we treat symlinks as regular files and this\nwould be backed up by the fact that I've updated the only part of\nmerge-recursive.c that calls lstat() and then S_ISDIR).\n\n[1] https://public-inbox.org/git/CABPp-BHpXWF+1hKUTfn8s-y4MJZXz+jUVS_K10eKyD6PGwo=gg@mail.gmail.com/\n---\n merge-recursive.c          | 40 +++++++++++++++++++++++++++++++++-----\n t/t3030-merge-recursive.sh | 27 +++++++++++++++++++++++++\n 2 files changed, 62 insertions(+), 5 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 6b812d67e3..a2029a4e94 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -747,7 +747,7 @@ static int dir_in_way(struct index_state *istate, const char *path,\n {\n \tint pos;\n \tstruct strbuf dirpath = STRBUF_INIT;\n-\tstruct stat st;\n+\tint ret = 0;\n \n \tstrbuf_addstr(&dirpath, path);\n \tstrbuf_addch(&dirpath, '/');\n@@ -758,13 +758,43 @@ static int dir_in_way(struct index_state *istate, const char *path,\n \t\tpos = -1 - pos;\n \tif (pos < istate->cache_nr &&\n \t    !strncmp(dirpath.buf, istate->cache[pos]->name, dirpath.len)) {\n-\t\tstrbuf_release(&dirpath);\n-\t\treturn 1;\n+\t\tret = 1;\n+\t\tgoto cleanup;\n \t}\n \n+\tif (check_working_copy) {\n+\t\tstruct stat st;\n+\n+\t\tstrbuf_trim_trailing_dir_sep(&dirpath);\n+\t\tif (lstat(dirpath.buf, &st))\n+\t\t\tgoto cleanup; /* doesn't exist, OK */\n+\t\tif (!S_ISDIR(st.st_mode))\n+\t\t\tgoto cleanup; /* not directory, OK */\n+\t\tif (empty_ok && is_empty_dir(dirpath.buf))\n+\t\t\tgoto cleanup;\n+\n+\t\t/*\n+\t\t * The given path is a directory, but this should not\n+\t\t * be treated as \"in the way\" if one of this\n+\t\t * directory's ancestors is a symlink. Check for this\n+\t\t * case.\n+\t\t */\n+\t\twhile (1) {\n+\t\t\tchar *slash = strrchr(dirpath.buf, '/');\n+\n+\t\t\tif (!slash) {\n+\t\t\t\tret = 1;\n+\t\t\t\tgoto cleanup;\n+\t\t\t}\n+\t\t\t*slash = '\\0';\n+\t\t\tif (!lstat(dirpath.buf, &st) && S_ISLNK(st.st_mode))\n+\t\t\t\tgoto cleanup;\n+\t\t}\n+\t}\n+\n+cleanup:\n \tstrbuf_release(&dirpath);\n-\treturn check_working_copy && !lstat(path, &st) && S_ISDIR(st.st_mode) &&\n-\t\t!(empty_ok && is_empty_dir(path));\n+\treturn ret;\n }\n \n /*\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex ff641b348a..dfd617a845 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -452,6 +452,33 @@ test_expect_success 'merge-recursive d/f conflict result' '\n \n '\n \n+test_expect_success 'dir in working tree with symlink ancestor does not produce d/f conflict' '\n+\tgit init sym &&\n+\t(\n+\t\tcd sym &&\n+\t\tln -s . foo &&\n+\t\tmkdir bar &&\n+\t\ttouch bar/file &&\n+\t\tgit add foo bar/file &&\n+\t\tgit commit -m \"foo symlink\" &&\n+\n+\t\tgit checkout -b branch1 &&\n+\t\tgit commit --allow-empty -m \"empty commit\" &&\n+\n+\t\tgit checkout master &&\n+\t\tgit rm foo &&\n+\t\tmkdir foo &&\n+\t\ttouch foo/bar &&\n+\t\tgit add foo/bar &&\n+\t\tgit commit -m \"replace foo symlink with real foo dir and foo/bar file\" &&\n+\n+\t\tgit checkout branch1 &&\n+\n+\t\t# Exercise to make sure that this works without errors\n+\t\tgit cherry-pick master\n+\t)\n+'\n+\n test_expect_success 'reset and 3-way merge' '\n \n \tgit reset --hard \"$c2\" &&\n-- \n2.23.0.237.gc6a4ce50a0-goog\n\n"},{"id":"382525","messageId":"xmqqh85a7eax.fsf@gitster-ct.c.googlers.com","threadId":"51860","inReplyTo":"20190917215040.132503-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] merge-recursive: symlink's descendants not in way","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-17T22:23:18Z","receivedAt":"2019-09-17T22:26:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When the working tree has:\n>  - foo (symlink)\n>  - foo/bar (directory)\n\nWhoa, wait.  I assume, since this is about merge, the assumption is\nthat the working tree is clean with respect to the index, so 'foo'\nis a symbolic link that is in the index.  Now, if foo is a symlink,\nhow can foo/bar (whether it is a directory or something else) exist,\nwhich requires foo to be a directory in the first place?\n\n> and the user merges a commit that deletes the foo symlink and instead\n> contains:\n>  - foo (directory)\n>  - foo/bar (file)\n\nThis side is possible.  If foo is a directory, then there can be\nfoo/bar.  But I do not get the initial setup you start with.\n\nIn any case, if the working tree has 'foo' as a symlink, Git should\nnot look at or get affected by what 'foo' points at.\n"},{"id":"382526","messageId":"20190917223236.149613-1-jonathantanmy@google.com","threadId":"51860","inReplyTo":"xmqqh85a7eax.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH] merge-recursive: symlink's descendants not in way","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-09-17T22:32:36Z","receivedAt":"2019-09-17T22:32:44Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> > When the working tree has:\n> >  - foo (symlink)\n> >  - foo/bar (directory)\n> \n> Whoa, wait.  I assume, since this is about merge, the assumption is\n> that the working tree is clean with respect to the index, so 'foo'\n> is a symbolic link that is in the index.  Now, if foo is a symlink,\n> how can foo/bar (whether it is a directory or something else) exist,\n> which requires foo to be a directory in the first place?\n\nI was trying to be concise but I guess I omitted too much detail. This is\nwhat's happening:\n\nWorking tree:\n - foo (symlink pointing to .)\n - bar (directory)\n - bar/file (file)\n\nAnd the new commit:\n - foo (directory)\n - foo/bar (file)\n - bar (directory)\n - bar/file (file)\n\nI'll update the commit message when I send out another version.\n\n> In any case, if the working tree has 'foo' as a symlink, Git should\n> not look at or get affected by what 'foo' points at.\n\nGit should not, but it does - there is a call in process_entry() that calls\nlstat() on \"foo/bar\", which indeed reports that \"foo/bar\" is a directory. This\npatch adds a check that none of its ancestors are symlinks.\n"},{"id":"382528","messageId":"xmqq8sqm7dmw.fsf@gitster-ct.c.googlers.com","threadId":"51860","inReplyTo":"20190917223236.149613-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] merge-recursive: symlink's descendants not in way","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-17T22:37:43Z","receivedAt":"2019-09-17T22:37:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> In any case, if the working tree has 'foo' as a symlink, Git should\n>> not look at or get affected by what 'foo' points at.\n>\n> Git should not, but it does - there is a call in process_entry() that calls\n> lstat() on \"foo/bar\", which indeed reports that \"foo/bar\" is a directory. This\n> patch adds a check that none of its ancestors are symlinks.\n\nYeah, I recall having to add has_symlink_leading_path() long time\nago in different codepaths (including \"apply\").  It is not surprising\nto see a similar glitch remaining in merge-recursive (it's a tricky\nissue, and it's a tricky code).\n\n"},{"id":"382530","messageId":"20190917224909.123111-1-jonathantanmy@google.com","threadId":"51860","inReplyTo":"xmqq8sqm7dmw.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH] merge-recursive: symlink's descendants not in way","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-09-17T22:49:08Z","receivedAt":"2019-09-17T22:49:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Yeah, I recall having to add has_symlink_leading_path() long time\n> ago in different codepaths (including \"apply\").  It is not surprising\n> to see a similar glitch remaining in merge-recursive (it's a tricky\n> issue, and it's a tricky code).\n\nThanks for the pointer to has_symlink_leading_path() - I tried it and it\nseems to work too. I'll wait for more replies and send an updated\nversion tomorrow.\n"},{"id":"382532","messageId":"20190917230224.GB29845@szeder.dev","threadId":"51860","inReplyTo":"20190917215040.132503-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] merge-recursive: symlink's descendants not in way","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-17T23:02:24Z","receivedAt":"2019-09-17T23:02:31Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Sep 17, 2019 at 02:50:40PM -0700, Jonathan Tan wrote:\n> diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\n> index ff641b348a..dfd617a845 100755\n> --- a/t/t3030-merge-recursive.sh\n> +++ b/t/t3030-merge-recursive.sh\n> @@ -452,6 +452,33 @@ test_expect_success 'merge-recursive d/f conflict result' '\n>  \n>  '\n>  \n> +test_expect_success 'dir in working tree with symlink ancestor does not produce d/f conflict' '\n\nThis test needs the SYMLINKS prereq.\n\n> +\tgit init sym &&\n> +\t(\n> +\t\tcd sym &&\n> +\t\tln -s . foo &&\n> +\t\tmkdir bar &&\n> +\t\ttouch bar/file &&\n\nNit: >bar/file (though because of the symlink this test won't be run\non the most fork()-challenged platform).\n\n> +\t\tgit add foo bar/file &&\n> +\t\tgit commit -m \"foo symlink\" &&\n> +\n> +\t\tgit checkout -b branch1 &&\n> +\t\tgit commit --allow-empty -m \"empty commit\" &&\n> +\n> +\t\tgit checkout master &&\n> +\t\tgit rm foo &&\n> +\t\tmkdir foo &&\n> +\t\ttouch foo/bar &&\n> +\t\tgit add foo/bar &&\n> +\t\tgit commit -m \"replace foo symlink with real foo dir and foo/bar file\" &&\n> +\n> +\t\tgit checkout branch1 &&\n> +\n> +\t\t# Exercise to make sure that this works without errors\n> +\t\tgit cherry-pick master\n\nWorking without errors is great, but I think this test should also\ncheck that the results are as expected, e.g. 'foo' is now a directory,\netc.\n\n> +\t)\n> +'\n> +\n>  test_expect_success 'reset and 3-way merge' '\n>  \n>  \tgit reset --hard \"$c2\" &&\n> -- \n> 2.23.0.237.gc6a4ce50a0-goog\n> \n"},{"id":"382534","messageId":"CABPp-BEXjZCOOuDS4b1bajYvE-T0NVw32YCRMi9Ks8TikPhORg@mail.gmail.com","threadId":"51860","inReplyTo":"20190917215040.132503-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] merge-recursive: symlink's descendants not in way","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-18T00:35:28Z","receivedAt":"2019-09-18T00:35:43Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Jonathan,\n\nOn Tue, Sep 17, 2019 at 2:50 PM Jonathan Tan <jonathantanmy@google.com> wrote:\n>\n> When the working tree has:\n>  - foo (symlink)\n>  - foo/bar (directory)\n>\n> and the user merges a commit that deletes the foo symlink and instead\n> contains:\n>  - foo (directory)\n>  - foo/bar (file)\n>\n> the merge should happen without requiring user intervention. However,\n> this does not happen.\n>\n> In merge_trees(), process_entry() will be invoked first for \"foo/bar\",\n> then \"foo\" (in reverse lexicographical order). process_entry() correctly\n> reaches \"Case B: Added in one\", but dir_in_way() states that \"bar\" is\n> already present as a directory, causing a directory/file conflict at the\n> wrong point.\n\nI don't think the notes about hitting the \"Case B: Added in one\"\ncodepath help; that's only one codepath that calls dir_in_way(), and\nI'm pretty sure with a little work we could trigger the same bug with\nthe other ones.\n\n> Instead, teach dir_in_way() that directories under symlinks are not \"in\n> the way\", so that symlinks are treated as regular files instead of\n> directories containing other directories and files. Thus, the \"else\"\n> branch will be followed instead: \"foo/bar\" will be added to the working\n> tree, make_room_for_path() being indirectly called to unlink the \"foo\"\n> symlink (just like if \"foo\" were a regular file instead). When\n> process_entry() is subsequently invoked for \"foo\", process_entry() will\n> reach \"Case A: Deleted in one\", and will handle it as \"Add/delete\" or\n> \"Modify/delete\" appropriately (including reinstatement of the previously\n> unlinked symlink with a new unique filename if necessary, again, just\n> like if \"foo\" were a regular file instead).\n\nI was trying to think of a way to summarize it a bit, and then Junio\nlater in the thread came in and provided a different and compatible\nway to view the issue that summarizes it quite nicely:\n\n\"In any case, if the working tree has 'foo' as a symlink, Git should\nnot look at or get affected by what 'foo' points at.\"\n\nWe can probably make the commit message pretty concise using that\nwording or something similar.  Maybe adding something like \"In\nparticular, the presence of a symlink should be treated much the same\nas the presence of a file; since dir_in_way() is only checking to see\nif there is a directory in the way, we don't want symlinks in leading\npaths to sometimes cause dir_in_way() to return true.\"\n\n>\n> Helped-by: Elijah Newren <newren@gmail.com>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Thanks to Elijah for his help. Some of the commit message is based on\n> his explanation [1].\n>\n> I'm finding this relatively complicated, so I'm sending this as RFC. My\n> main concern is that whether all callers of dir_in_way() are OK with its\n> behavior change, and if yes, how to explain it. I suspect that this is\n> correct because dir_in_way() should behave consistently for all its\n> callers, but I might be wrong.\n\nYes, we want all callers of dir_in_way() to get this change; if they\ndon't, I'm pretty sure with some work we could devise special\nscenarios that exhibit the same bug.\n\n\n\nThanks for working on this,\nElijah\n"}]}