{"thread":{"id":"44059","subject":"[PATCH] Do not record unstaged deleted file upon recursive merge if file was moved outside of working tree with enabled sparse-checkout.","startedAt":"2016-09-11T20:32:33Z","lastAt":"2016-09-12T11:18:11Z","messageCount":4,"participants":["Mikhail Filippov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"301668","messageId":"1473625505-7641-1-git-send-email-mikhail@filippov.me","threadId":"44059","inReplyTo":null,"subject":"[PATCH] Do not record unstaged deleted file upon recursive merge if file was moved outside of working tree with enabled sparse-checkout.","fromName":"Mikhail Filippov","fromEmail":"mikhail@filippov.me","sentAt":"2016-09-11T20:25:05Z","receivedAt":"2016-09-11T20:32:33Z","isPatch":true,"sender":{"key":"mikhail@filippov.me","avatar":"https://gravatar.com/avatar/1ad23fcc70633ccce28c11d221f70a4318667506fb9aefaba21a91dadd055630?d=mp&s=160"},"body":"---\n merge-recursive.c                    |  9 +++++---\n t/t6042-merge-rename-corner-cases.sh | 42 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex e349126..25dc701 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1724,9 +1724,12 @@ static int merge_content(struct merge_options *o,\n \t\t */\n \t\tpath_renamed_outside_HEAD = !path2 || !strcmp(path, path2);\n \t\tif (!path_renamed_outside_HEAD) {\n-\t\t\tadd_cacheinfo(o, mfi.mode, &mfi.oid, path,\n-\t\t\t\t      0, (!o->call_depth), 0);\n-\t\t\treturn mfi.clean;\n+\t\t\tstruct stat st;\n+\t\t\tif (lstat(path, &st) == 0) {\n+\t\t\t\tadd_cacheinfo(o, mfi.mode, &mfi.oid, path,\n+\t\t\t\t\t      0, (!o->call_depth), 0);\n+\t\t\t\treturn mfi.clean;\n+\t\t\t}\n \t\t}\n \t} else\n \t\toutput(o, 2, _(\"Auto-merging %s\"), path);\ndiff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh\nindex 411550d..2073e49 100755\n--- a/t/t6042-merge-rename-corner-cases.sh\n+++ b/t/t6042-merge-rename-corner-cases.sh\n@@ -575,4 +575,46 @@ test_expect_success 'rename/rename/add-dest merge still knows about conflicting\n \ttest ! -f c\n '\n \n+test_expect_success 'move file/sparse-checkout/merge should not delete moved file' '\n+\tgit rm -rf . &&\n+\tgit clean -fdqx &&\n+\trm -rf .git &&\n+\tgit init &&\n+\n+\techo output >.gitignore &&\n+\techo .gitignore >>.gitignore &&\n+\n+\techo b1 >b1 &&\n+\tgit add b1 &&\n+\tgit commit -m b1 &&\n+\n+\tmkdir excluded &&\n+\techo problem >excluded/to-be-moved.txt &&\n+\tgit add excluded/to-be-moved.txt &&\n+\tgit commit -m to-be-moved &&\n+\tgit tag split_point &&\n+\n+\techo b2 >b2 &&\n+\tgit add b2 &&\n+\tgit commit -m b2 &&\n+\tgit tag b2 &&\n+\n+\tgit reset --hard split_point &&\n+\n+\tgit mv excluded/to-be-moved.txt excluded/moved.txt &&\n+\tgit commit -m move &&\n+\tgit tag b1 &&\n+\n+\tgit config core.sparsecheckout true &&\n+\techo \"/*\" >.git/info/sparse-checkout &&\n+\techo \"!excluded/\" >>.git/info/sparse-checkout &&\n+\tgit read-tree -mu HEAD &&\n+\n+\tgit merge -m merge b2 &&\n+\n+\tgit status >output &&\n+\tcp output /tmp/a &&\n+\ttest_i18ngrep \"nothing to commit\" output\n+'\n+\n test_done\n-- \n2.7.4 (Apple Git-66)\n\n"},{"id":"301676","messageId":"xmqqeg4q6mw4.fsf@gitster.mtv.corp.google.com","threadId":"44059","inReplyTo":"1473625505-7641-1-git-send-email-mikhail@filippov.me","subject":"Re: [PATCH] Do not record unstaged deleted file upon recursive merge if file was moved outside of working tree with enabled sparse-checkout.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-11T23:24:11Z","receivedAt":"2016-09-11T23:24:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mikhail Filippov <mikhail@filippov.me> writes:\n\n> ---\n\nYou'd need a lot more explanation on why this is needed\n(i.e. without it what behaviour you would get, and why that\nbehaviour is wrong).\n\n>  merge-recursive.c                    |  9 +++++---\n>  t/t6042-merge-rename-corner-cases.sh | 42 ++++++++++++++++++++++++++++++++++++\n>  2 files changed, 48 insertions(+), 3 deletions(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index e349126..25dc701 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1724,9 +1724,12 @@ static int merge_content(struct merge_options *o,\n>  \t\t */\n>  \t\tpath_renamed_outside_HEAD = !path2 || !strcmp(path, path2);\n>  \t\tif (!path_renamed_outside_HEAD) {\n> -\t\t\tadd_cacheinfo(o, mfi.mode, &mfi.oid, path,\n> -\t\t\t\t      0, (!o->call_depth), 0);\n> -\t\t\treturn mfi.clean;\n> +\t\t\tstruct stat st;\n> +\t\t\tif (lstat(path, &st) == 0) {\n> +\t\t\t\tadd_cacheinfo(o, mfi.mode, &mfi.oid, path,\n> +\t\t\t\t\t      0, (!o->call_depth), 0);\n> +\t\t\t\treturn mfi.clean;\n> +\t\t\t}\n\nI cannot guess what you are trying to achieve without explanation in\nthe proposed log message, but I can say that this unconditional\nchecking of a working tree file cannot be correct (there may or may\nnot be other things that are wrong with this change, which cannot be\njudged without more information).\n\nImagine your o->call_depth is not zero, i.e. we are making a virtual\ncommon ancestor with this merge, in which case any of the three\ntrees involved may have nothing to do with the current working tree\nfiles.\n\n> +test_expect_success 'move file/sparse-checkout/merge should not delete moved file' '\n> +\tgit rm -rf . &&\n> +\tgit clean -fdqx &&\n> +\trm -rf .git &&\n> +\tgit init &&\n\nYuck.  This is inherited from existing tests but I think they need\nto be cleaned up.  It is not your fault, and it is not in the scope\nof this change.\n\n> +\tgit status >output &&\n> +\tcp output /tmp/a &&\n\nHuh?\n\n> +\ttest_i18ngrep \"nothing to commit\" output\n> +'\n> +\n>  test_done\n"},{"id":"301695","messageId":"798801473669480@web11o.yandex.ru","threadId":"44059","inReplyTo":"xmqqeg4q6mw4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Do not record unstaged deleted file upon recursive merge if file was moved outside of working tree with enabled sparse-checkout.","fromName":"Mikhail Filippov","fromEmail":"mikhail@filippov.me","sentAt":"2016-09-12T08:38:00Z","receivedAt":"2016-09-12T08:45:16Z","isPatch":true,"sender":{"key":"mikhail@filippov.me","avatar":"https://gravatar.com/avatar/1ad23fcc70633ccce28c11d221f70a4318667506fb9aefaba21a91dadd055630?d=mp&s=160"},"body":"It's a very unexpected behaviour when a user sees a deleted file after a merge with enabled sparse-checkout. Moreover, when the user resolves merge conflicts and commits the changes with the command \"git commit -am xxx\", a repository can be broken because all the moved files will be deleted. Finally, it's really hard to find a user who deleted these files because \"git log file\" doesn't show any merge commits by default. I'm not sure that my fix is correct but I checked all tests and I didn't find a better way to prevent files deleting.\n\n12.09.2016, 02:24, \"Junio C Hamano\" <gitster@pobox.com>:\n>    Mikhail Filippov <mikhail@filippov.me> writes:\n>\n>>     ---\n>\n>    You'd need a lot more explanation on why this is needed\n>    (i.e. without it what behaviour you would get, and why that\n>    behaviour is wrong).\n>\n>>      merge-recursive.c | 9 +++++---\n>>      t/t6042-merge-rename-corner-cases.sh | 42 ++++++++++++++++++++++++++++++++++++\n>>      2 files changed, 48 insertions(+), 3 deletions(-)\n>>\n>>     diff --git a/merge-recursive.c b/merge-recursive.c\n>>     index e349126..25dc701 100644\n>>     --- a/merge-recursive.c\n>>     +++ b/merge-recursive.c\n>>     @@ -1724,9 +1724,12 @@ static int merge_content(struct merge_options *o,\n>>                       */\n>>                      path_renamed_outside_HEAD = !path2 || !strcmp(path, path2);\n>>                      if (!path_renamed_outside_HEAD) {\n>>     - add_cacheinfo(o, mfi.mode, &mfi.oid, path,\n>>     - 0, (!o->call_depth), 0);\n>>     - return mfi.clean;\n>>     + struct stat st;\n>>     + if (lstat(path, &st) == 0) {\n>>     + add_cacheinfo(o, mfi.mode, &mfi.oid, path,\n>>     + 0, (!o->call_depth), 0);\n>>     + return mfi.clean;\n>>     + }\n>\n>    I cannot guess what you are trying to achieve without explanation in\n>    the proposed log message, but I can say that this unconditional\n>    checking of a working tree file cannot be correct (there may or may\n>    not be other things that are wrong with this change, which cannot be\n>    judged without more information).\n>\n>    Imagine your o->call_depth is not zero, i.e. we are making a virtual\n>    common ancestor with this merge, in which case any of the three\n>    trees involved may have nothing to do with the current working tree\n>    files.\n>\n>>     +test_expect_success 'move file/sparse-checkout/merge should not delete moved file' '\n>>     + git rm -rf . &&\n>>     + git clean -fdqx &&\n>>     + rm -rf .git &&\n>>     + git init &&\n>\n>    Yuck. This is inherited from existing tests but I think they need\n>    to be cleaned up. It is not your fault, and it is not in the scope\n>    of this change.\n>\n>>     + git status >output &&\n>>     + cp output /tmp/a &&\n>\n>    Huh?\n>\n>>     + test_i18ngrep \"nothing to commit\" output\n>>     +'\n>>     +\n>>      test_done\n\n\n\n\n-- \nMikhail Filippov \nSoftware Developer\nJetBrains\nhttp://jetbrains.com\n“The Drive To Develop\"\n"},{"id":"301699","messageId":"1843881473678425@web17m.yandex.ru","threadId":"44059","inReplyTo":"xmqqeg4q6mw4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Do not record unstaged deleted file upon recursive merge if file was moved outside of working tree with enabled sparse-checkout.","fromName":"Mikhail Filippov","fromEmail":"mikhail@filippov.me","sentAt":"2016-09-12T11:07:05Z","receivedAt":"2016-09-12T11:18:11Z","isPatch":true,"sender":{"key":"mikhail@filippov.me","avatar":"https://gravatar.com/avatar/1ad23fcc70633ccce28c11d221f70a4318667506fb9aefaba21a91dadd055630?d=mp&s=160"},"body":">>  +test_expect_success 'move file/sparse-checkout/merge should not delete moved file' '\n>>  + git rm -rf . &&\n>>  + git clean -fdqx &&\n>>  + rm -rf .git &&\n>>  + git init &&\n>\n> Yuck. This is inherited from existing tests but I think they need\n> to be cleaned up. It is not your fault, and it is not in the scope\n> of this change.\nIt's part need for cleanup repository from the previous case.\n"}]}