{"thread":{"id":"51879","subject":"[PATCH v2] merge-recursive: symlink's descendants not in way","startedAt":"2019-09-18T20:27:43Z","lastAt":"2019-09-20T20:50:38Z","messageCount":6,"participants":["Jonathan Tan","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"382572","messageId":"20190918202738.57273-1-jonathantanmy@google.com","threadId":"51879","inReplyTo":"https://public-inbox.org/git/20190917215040.132503-1-jonathantanmy@google.com/","subject":"[PATCH v2] merge-recursive: symlink's descendants not in way","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-09-18T20:27:38Z","receivedAt":"2019-09-18T20:27:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When the working tree has:\n - bar (directory)\n - bar/file (file)\n - foo (symlink to .)\n\n(note that lstat() for \"foo/bar\" would tell us that it is a directory)\n\nand the user merges a commit that deletes the foo symlink and instead\ncontains:\n - bar (directory, as above)\n - bar/file (file, as above)\n - foo (directory)\n - foo/bar (file)\n\nthe merge should happen without requiring user intervention. However,\nthis does not happen.\n\nThis is because dir_in_way(), when checking the working tree, thinks\nthat \"foo/bar\" is a directory. But a symlink should be treated much the\nsame as a file: since dir_in_way() is only checking to see if there is a\ndirectory in the way, we don't want symlinks in leading paths to\nsometimes cause dir_in_way() to return true.\n\nTeach dir_in_way() to also check for symlinks in leading paths before\nreporting whether a directory is in the way.\n\nHelped-by: Elijah Newren <newren@gmail.com>\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nChanges from v1:\n\n- Used has_symlink_leading_path(). This drastically shortens the diff.\n- Updated commit message following suggestions from Junio, Szeder Gábor,\n  and Elijah Newren.\n- Updated test to add prereq and verification that the working tree\n  contains what we want.\n---\n merge-recursive.c          |  3 ++-\n t/t3030-merge-recursive.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 6b812d67e3..22a12cfeba 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -764,7 +764,8 @@ static int dir_in_way(struct index_state *istate, const char *path,\n \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+\t\t!(empty_ok && is_empty_dir(path)) &&\n+\t\t!has_symlink_leading_path(path, strlen(path));\n }\n \n /*\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex ff641b348a..faa8892741 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -452,6 +452,34 @@ test_expect_success 'merge-recursive d/f conflict result' '\n \n '\n \n+test_expect_success SYMLINKS '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\t>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\t>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\tgit cherry-pick master &&\n+\t\ttest_path_is_dir foo &&\n+\t\ttest_path_is_file foo/bar\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":"382576","messageId":"CABPp-BGN9x5HAdfm1Pn14uswCBomczSYNHeqkNmuUhWnuQD++Q@mail.gmail.com","threadId":"51879","inReplyTo":"20190918202738.57273-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] merge-recursive: symlink's descendants not in way","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-18T21:54:32Z","receivedAt":"2019-09-18T21:54:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Sep 18, 2019 at 1:27 PM Jonathan Tan <jonathantanmy@google.com> wrote:\n>\n> When the working tree has:\n>  - bar (directory)\n>  - bar/file (file)\n>  - foo (symlink to .)\n>\n> (note that lstat() for \"foo/bar\" would tell us that it is a directory)\n>\n> and the user merges a commit that deletes the foo symlink and instead\n> contains:\n>  - bar (directory, as above)\n>  - bar/file (file, as above)\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> This is because dir_in_way(), when checking the working tree, thinks\n> that \"foo/bar\" is a directory. But a symlink should be treated much the\n> same as a file: since dir_in_way() is only checking to see if there is a\n> directory in the way, we don't want symlinks in leading paths to\n> sometimes cause dir_in_way() to return true.\n>\n> Teach dir_in_way() to also check for symlinks in leading paths before\n> reporting whether a directory is in the way.\n>\n> Helped-by: Elijah Newren <newren@gmail.com>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Changes from v1:\n>\n> - Used has_symlink_leading_path(). This drastically shortens the diff.\n> - Updated commit message following suggestions from Junio, Szeder Gábor,\n>   and Elijah Newren.\n> - Updated test to add prereq and verification that the working tree\n>   contains what we want.\n> ---\n>  merge-recursive.c          |  3 ++-\n>  t/t3030-merge-recursive.sh | 28 ++++++++++++++++++++++++++++\n>  2 files changed, 30 insertions(+), 1 deletion(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 6b812d67e3..22a12cfeba 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -764,7 +764,8 @@ static int dir_in_way(struct index_state *istate, const char *path,\n>\n>         strbuf_release(&dirpath);\n>         return check_working_copy && !lstat(path, &st) && S_ISDIR(st.st_mode) &&\n> -               !(empty_ok && is_empty_dir(path));\n> +               !(empty_ok && is_empty_dir(path)) &&\n> +               !has_symlink_leading_path(path, strlen(path));\n>  }\n>\n>  /*\n> diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\n> index ff641b348a..faa8892741 100755\n> --- a/t/t3030-merge-recursive.sh\n> +++ b/t/t3030-merge-recursive.sh\n> @@ -452,6 +452,34 @@ test_expect_success 'merge-recursive d/f conflict result' '\n>\n>  '\n>\n> +test_expect_success SYMLINKS 'dir in working tree with symlink ancestor does not produce d/f conflict' '\n> +       git init sym &&\n> +       (\n> +               cd sym &&\n> +               ln -s . foo &&\n> +               mkdir bar &&\n> +               >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> +               >foo/bar &&\n> +               git add foo/bar &&\n> +               git commit -m \"replace foo symlink with real foo dir and foo/bar file\" &&\n> +\n> +               git checkout branch1 &&\n> +\n> +               git cherry-pick master &&\n> +               test_path_is_dir foo &&\n> +               test_path_is_file foo/bar\n> +       )\n> +'\n> +\n>  test_expect_success 'reset and 3-way merge' '\n>\n>         git reset --hard \"$c2\" &&\n> --\n\nLooks good to me; nice how much it has simplified.  Thanks for working on this.\n\n> 2.23.0.237.gc6a4ce50a0-goog\n\nA total tangent, but what do you use the \"-goog\" suffix for?\n"},{"id":"382685","messageId":"xmqqzhiyvqhe.fsf@gitster-ct.c.googlers.com","threadId":"51879","inReplyTo":"20190918202738.57273-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] merge-recursive: symlink's descendants not in way","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-20T17:15:25Z","receivedAt":"2019-09-20T17:15:31Z","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>  - bar (directory)\n>  - bar/file (file)\n>  - foo (symlink to .)\n>\n> (note that lstat() for \"foo/bar\" would tell us that it is a directory)\n>\n> and the user merges a commit that deletes the foo symlink and instead\n> contains:\n>  - bar (directory, as above)\n>  - bar/file (file, as above)\n>  - foo (directory)\n>  - foo/bar (file)\n>\n> the merge should happen without requiring user intervention.\n\nThanks.  That clears my previous confusion.  It is clear that the\ndesired outcome is that bar/file will be merged with itself, foo\nitself will resolve to \"deleted\", and foo/bar will be created.\n\n> However, this does not happen.\n\nOK.  We notice that we need to newly create foo/bar but we\nincorrectly find that there is \"foo/bar\" already because of the\ncareless use of bare lstat(2) makes \"bar\" visible as if it were also\n\"foo/bar\".  I wonder if the current code would be confused the same\nway if the side branch added \"foo/bar/file\", or the confusion would\nbe even worse---it is not dir_in_way() and a different codepath\nwould be affected, no?\n\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 6b812d67e3..22a12cfeba 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -764,7 +764,8 @@ static int dir_in_way(struct index_state *istate, const char *path,\n>  \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> +\t\t!(empty_ok && is_empty_dir(path)) &&\n> +\t\t!has_symlink_leading_path(path, strlen(path));\n\nAs the new call is fairly heavyweight compared to everything else we\nare doing, I think it is very sensible to have this at the end, like\nthis patch does.\n\nNicely done.  Thanks, will queue.\n"},{"id":"382705","messageId":"20190920202506.74644-1-jonathantanmy@google.com","threadId":"51879","inReplyTo":"xmqqzhiyvqhe.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] merge-recursive: symlink's descendants not in way","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-09-20T20:25:06Z","receivedAt":"2019-09-20T20:25:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> OK.  We notice that we need to newly create foo/bar but we\n> incorrectly find that there is \"foo/bar\" already because of the\n> careless use of bare lstat(2) makes \"bar\" visible as if it were also\n> \"foo/bar\".  I wonder if the current code would be confused the same\n> way if the side branch added \"foo/bar/file\", or the confusion would\n> be even worse---it is not dir_in_way() and a different codepath\n> would be affected, no?\n\nI don't think there is a different codepath to be affected - as far as I\ncan tell, dir_in_way() is the only cause (at least of this particular\nerror). I've tested this case [1] and the current code actually works -\nas you said, dir_in_way() will not report anything in the way (since\nfoo/bar/file doesn't exist in the working tree), so the merge will\nhappen as expected.\n\n> Nicely done.  Thanks, will queue.\n\nThanks.\n\n[1]\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex faa8892741..f284aeb173 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -468,15 +468,16 @@ test_expect_success SYMLINKS 'dir in working tree with symlink ancestor does not\n                git checkout master &&\n                git rm foo &&\n                mkdir foo &&\n-               >foo/bar &&\n-               git add foo/bar &&\n-               git commit -m \"replace foo symlink with real foo dir and foo/bar file\" &&\n+               mkdir foo/bar &&\n+               >foo/bar/baz &&\n+               git add foo/bar/baz &&\n+               git commit -m \"replace foo symlink with real foo dir and foo/bar/baz file\" &&\n \n                git checkout branch1 &&\n \n                git cherry-pick master &&\n                test_path_is_dir foo &&\n-               test_path_is_file foo/bar\n+               test_path_is_file foo/bar/baz\n        )\n '\n\n"},{"id":"382708","messageId":"xmqq8sqiu2in.fsf@gitster-ct.c.googlers.com","threadId":"51879","inReplyTo":"20190920202506.74644-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] merge-recursive: symlink's descendants not in way","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-20T20:38:24Z","receivedAt":"2019-09-20T20:38:32Z","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>> OK.  We notice that we need to newly create foo/bar but we\n>> incorrectly find that there is \"foo/bar\" already because of the\n>> careless use of bare lstat(2) makes \"bar\" visible as if it were also\n>> \"foo/bar\".  I wonder if the current code would be confused the same\n>> way if the side branch added \"foo/bar/file\", or the confusion would\n>> be even worse---it is not dir_in_way() and a different codepath\n>> would be affected, no?\n>\n> I don't think there is a different codepath to be affected - as far as I\n> can tell, dir_in_way() is the only cause (at least of this particular\n> error).\n\nOK, so existing code already realizes that \"foo/bar/file\" added in\nthe side branch is the one that must survive, and the \"bar/file\" in\nthe current branch does not fool it into thinking that \"foo/bar/file\"\nis also on our end, and needs to be merged as an add-add conflict.\nIt was only the dir-in-the-way logic that was not careful enough?\n\nIn that case, thanks for a very good news and for a careful analysis.\n"},{"id":"382710","messageId":"20190920205032.84796-1-jonathantanmy@google.com","threadId":"51879","inReplyTo":"xmqq8sqiu2in.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] merge-recursive: symlink's descendants not in way","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-09-20T20:50:32Z","receivedAt":"2019-09-20T20:50:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> >> OK.  We notice that we need to newly create foo/bar but we\n> >> incorrectly find that there is \"foo/bar\" already because of the\n> >> careless use of bare lstat(2) makes \"bar\" visible as if it were also\n> >> \"foo/bar\".  I wonder if the current code would be confused the same\n> >> way if the side branch added \"foo/bar/file\", or the confusion would\n> >> be even worse---it is not dir_in_way() and a different codepath\n> >> would be affected, no?\n> >\n> > I don't think there is a different codepath to be affected - as far as I\n> > can tell, dir_in_way() is the only cause (at least of this particular\n> > error).\n> \n> OK, so existing code already realizes that \"foo/bar/file\" added in\n> the side branch is the one that must survive, and the \"bar/file\" in\n> the current branch does not fool it into thinking that \"foo/bar/file\"\n> is also on our end, and needs to be merged as an add-add conflict.\n> It was only the dir-in-the-way logic that was not careful enough?\n\nYes, that's correct. (I wrote foo/bar/baz in my other email but replaced\n\"baz\" with \"file\", and it still works before and after my patch.)\n\n> In that case, thanks for a very good news and for a careful analysis.\n\nYou're welcome! The careful analysis should be credited to Elijah Newren\n[1].\n\n[1] https://public-inbox.org/git/CABPp-BHpXWF+1hKUTfn8s-y4MJZXz+jUVS_K10eKyD6PGwo=gg@mail.gmail.com/\n"}]}