{"thread":{"id":"46752","subject":"git fast-export/import bug with -M -C","startedAt":"2017-09-14T22:08:08Z","lastAt":"2017-09-21T09:08:38Z","messageCount":4,"participants":["Juraj Oršulić","Jonathan Tan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"328079","messageId":"CAEPqvoyVJFe2EOvhnZD4vdF=1-VuoZrMP92TeGJ2WAE0X+B5Tw@mail.gmail.com","threadId":"46752","inReplyTo":null,"subject":"git fast-export/import bug with -M -C","fromName":"Juraj Oršulić","fromEmail":"juraj.orsulic@fer.hr","sentAt":"2017-09-14T22:01:04Z","receivedAt":"2017-09-14T22:08:08Z","isPatch":false,"sender":{"key":"juraj.orsulic@fer.hr","avatar":"https://gravatar.com/avatar/1539f078c7cc6d853a33423b8059ece7a9b3b9c2c9eeb2269e7a8be647328ab1?d=mp&s=160"},"body":"The commands should be self explanatory. 0.2.0~20 is the first commit\nwhere the reconstructed repository diverges, that commit had a\nsimultaneous copy and edit of one file. It seems that copy/rename\ndetection, enabled with -M -C is confused by this. I reproduced it\nwith git 2.14 next @ 8fa685d.\n\ngit clone https://github.com/googlecartographer/cartographer_ros\nmkdir copy && cd copy && git init\n(cd ../cartographer_ros; git fast-export --all --date-order -M -C) |\ngit fast-import\ngit rev-parse 0.2.0~20 #2237e1d0a974977fbcb0737dd1fb5876a2b8e29d\ngit rev-parse 0.2.0~21 #cd1276a99ccffcc491d0b2e50296ec04347ba5f2\ncd ../cartographer_ros\ngit rev-parse 0.2.0~20 #9d5b221ed41783b15c84bc90b71527194b8d9a49\ngit rev-parse 0.2.0~21 #cd1276a99ccffcc491d0b2e50296ec04347ba5f2\n"},{"id":"328472","messageId":"CAEPqvowWEdFdJoyqsmBt7KRU8xWyuLnFwV7CJ7zH2QRa2AQCtg@mail.gmail.com","threadId":"46752","inReplyTo":"CAEPqvoyVJFe2EOvhnZD4vdF=1-VuoZrMP92TeGJ2WAE0X+B5Tw@mail.gmail.com","subject":"Re: git fast-export/import bug with -M -C","fromName":"Juraj Oršulić","fromEmail":"juraj.orsulic@fer.hr","sentAt":"2017-09-20T16:55:30Z","receivedAt":"2017-09-20T16:56:20Z","isPatch":false,"sender":{"key":"juraj.orsulic@fer.hr","avatar":"https://gravatar.com/avatar/1539f078c7cc6d853a33423b8059ece7a9b3b9c2c9eeb2269e7a8be647328ab1?d=mp&s=160"},"body":"Hi, did anyone manage to take a look at this bug? Friendly ping.\n\nJuraj\n\nOn Fri, Sep 15, 2017 at 12:01 AM, Juraj Oršulić <juraj.orsulic@fer.hr> wrote:\n> The commands should be self explanatory. 0.2.0~20 is the first commit\n> where the reconstructed repository diverges, that commit had a\n> simultaneous copy and edit of one file. It seems that copy/rename\n> detection, enabled with -M -C is confused by this. I reproduced it\n> with git 2.14 next @ 8fa685d.\n>\n> git clone https://github.com/googlecartographer/cartographer_ros\n> mkdir copy && cd copy && git init\n> (cd ../cartographer_ros; git fast-export --all --date-order -M -C) |\n> git fast-import\n> git rev-parse 0.2.0~20 #2237e1d0a974977fbcb0737dd1fb5876a2b8e29d\n> git rev-parse 0.2.0~21 #cd1276a99ccffcc491d0b2e50296ec04347ba5f2\n> cd ../cartographer_ros\n> git rev-parse 0.2.0~20 #9d5b221ed41783b15c84bc90b71527194b8d9a49\n> git rev-parse 0.2.0~21 #cd1276a99ccffcc491d0b2e50296ec04347ba5f2\n"},{"id":"328509","messageId":"20170920235502.6214-1-jonathantanmy@google.com","threadId":"46752","inReplyTo":"CAEPqvoyVJFe2EOvhnZD4vdF=1-VuoZrMP92TeGJ2WAE0X+B5Tw@mail.gmail.com","subject":"[PATCH] fast-export: do not copy from modified file","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-09-20T23:55:02Z","receivedAt":"2017-09-20T23:55:21Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When run with the \"-C\" option, fast-export writes 'C' commands in its\noutput whenever the internal diff mechanism detects a file copy,\nindicating that fast-import should copy the given existing file to the\ngiven new filename. However, the diff mechanism works against the\nprior version of the file, whereas fast-import uses whatever is current.\nThis causes issues when a commit both modifies a file and uses it as the\nsource for a copy.\n\nTherefore, teach fast-export to refrain from writing 'C' when it has\nalready written a modification command for a file.\n\nAn existing test in t9350-fast-export is also fixed in this patch. The\nexisting line \"C file6 file7\" copies the wrong version of file6, but it\nhas coincidentally worked because file7 was subsequently overridden.\n\nReported-by: Juraj Oršulić <juraj.orsulic@fer.hr>\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nI tested this with the reproduction commands given by Juraj Oršulić and\nit works.\n\nI also have a version that prints the redundant 'C' (and does not\nrequire the change to t9350), omitting it only if it would cause a wrong\nresult. That seems imprecise to me, but I can send that out if the\nredundant 'C' is preferred.\n---\n builtin/fast-export.c  | 46 ++++++++++++++++++++++++++++++++--------------\n t/t9350-fast-export.sh | 20 +++++++++++++++++++-\n 2 files changed, 51 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex d412c0a8f..da42ee5e6 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -344,6 +344,7 @@ static void show_filemodify(struct diff_queue_struct *q,\n \t\t\t    struct diff_options *options, void *data)\n {\n \tint i;\n+\tstruct string_list *changed = data;\n \n \t/*\n \t * Handle files below a directory first, in case they are all deleted\n@@ -359,20 +360,31 @@ static void show_filemodify(struct diff_queue_struct *q,\n \t\tcase DIFF_STATUS_DELETED:\n \t\t\tprintf(\"D \");\n \t\t\tprint_path(spec->path);\n+\t\t\tstring_list_insert(changed, spec->path);\n \t\t\tputchar('\\n');\n \t\t\tbreak;\n \n \t\tcase DIFF_STATUS_COPIED:\n \t\tcase DIFF_STATUS_RENAMED:\n-\t\t\tprintf(\"%c \", q->queue[i]->status);\n-\t\t\tprint_path(ospec->path);\n-\t\t\tputchar(' ');\n-\t\t\tprint_path(spec->path);\n-\t\t\tputchar('\\n');\n-\n-\t\t\tif (!oidcmp(&ospec->oid, &spec->oid) &&\n-\t\t\t    ospec->mode == spec->mode)\n-\t\t\t\tbreak;\n+\t\t\t/*\n+\t\t\t * If a change in the file corresponding to ospec->path\n+\t\t\t * has been observed, we cannot trust its contents\n+\t\t\t * because the diff is calculated based on the prior\n+\t\t\t * contents, not the current contents.  So, declare a\n+\t\t\t * copy or rename only if there was no change observed.\n+\t\t\t */\n+\t\t\tif (!string_list_has_string(changed, ospec->path)) {\n+\t\t\t\tprintf(\"%c \", q->queue[i]->status);\n+\t\t\t\tprint_path(ospec->path);\n+\t\t\t\tputchar(' ');\n+\t\t\t\tprint_path(spec->path);\n+\t\t\t\tstring_list_insert(changed, spec->path);\n+\t\t\t\tputchar('\\n');\n+\n+\t\t\t\tif (!oidcmp(&ospec->oid, &spec->oid) &&\n+\t\t\t\t    ospec->mode == spec->mode)\n+\t\t\t\t\tbreak;\n+\t\t\t}\n \t\t\t/* fallthrough */\n \n \t\tcase DIFF_STATUS_TYPE_CHANGED:\n@@ -393,6 +405,7 @@ static void show_filemodify(struct diff_queue_struct *q,\n \t\t\t\t       get_object_mark(object));\n \t\t\t}\n \t\t\tprint_path(spec->path);\n+\t\t\tstring_list_insert(changed, spec->path);\n \t\t\tputchar('\\n');\n \t\t\tbreak;\n \n@@ -528,7 +541,8 @@ static void anonymize_ident_line(const char **beg, const char **end)\n \t*end = out->buf + out->len;\n }\n \n-static void handle_commit(struct commit *commit, struct rev_info *rev)\n+static void handle_commit(struct commit *commit, struct rev_info *rev,\n+\t\t\t  struct string_list *paths_of_changed_objects)\n {\n \tint saved_output_format = rev->diffopt.output_format;\n \tconst char *commit_buffer;\n@@ -615,6 +629,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)\n \tif (full_tree)\n \t\tprintf(\"deleteall\\n\");\n \tlog_tree_diff_flush(rev);\n+\tstring_list_clear(paths_of_changed_objects, 0);\n \trev->diffopt.output_format = saved_output_format;\n \n \tprintf(\"\\n\");\n@@ -630,14 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len)\n \treturn strbuf_detach(&out, len);\n }\n \n-static void handle_tail(struct object_array *commits, struct rev_info *revs)\n+static void handle_tail(struct object_array *commits, struct rev_info *revs,\n+\t\t\tstruct string_list *paths_of_changed_objects)\n {\n \tstruct commit *commit;\n \twhile (commits->nr) {\n \t\tcommit = (struct commit *)commits->objects[commits->nr - 1].item;\n \t\tif (has_unshown_parent(commit))\n \t\t\treturn;\n-\t\thandle_commit(commit, revs);\n+\t\thandle_commit(commit, revs, paths_of_changed_objects);\n \t\tcommits->nr--;\n \t}\n }\n@@ -977,6 +993,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \tchar *export_filename = NULL, *import_filename = NULL;\n \tuint32_t lastimportid;\n \tstruct string_list refspecs_list = STRING_LIST_INIT_NODUP;\n+\tstruct string_list paths_of_changed_objects = STRING_LIST_INIT_DUP;\n \tstruct option options[] = {\n \t\tOPT_INTEGER(0, \"progress\", &progress,\n \t\t\t    N_(\"show progress after <n> objects\")),\n@@ -1049,14 +1066,15 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\n \trevs.diffopt.format_callback = show_filemodify;\n+\trevs.diffopt.format_callback_data = &paths_of_changed_objects;\n \tDIFF_OPT_SET(&revs.diffopt, RECURSIVE);\n \twhile ((commit = get_revision(&revs))) {\n \t\tif (has_unshown_parent(commit)) {\n \t\t\tadd_object_array(&commit->object, NULL, &commits);\n \t\t}\n \t\telse {\n-\t\t\thandle_commit(commit, &revs);\n-\t\t\thandle_tail(&commits, &revs);\n+\t\t\thandle_commit(commit, &revs, &paths_of_changed_objects);\n+\t\t\thandle_tail(&commits, &revs, &paths_of_changed_objects);\n \t\t}\n \t}\n \ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 8dcb05c4a..866ddf605 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -234,7 +234,7 @@ test_expect_success 'fast-export -C -C | fast-import' '\n \tmkdir new &&\n \tgit --git-dir=new/.git init &&\n \tgit fast-export -C -C --signed-tags=strip --all > output &&\n-\tgrep \"^C file6 file7\\$\" output &&\n+\tgrep \"^C file2 file4\\$\" output &&\n \tcat output |\n \t(cd new &&\n \t git fast-import &&\n@@ -522,4 +522,22 @@ test_expect_success 'delete refspec' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'when using -C, do not declare copy when source of copy is also modified' '\n+\ttest_create_repo src &&\n+\techo a_line >src/file.txt &&\n+\tgit -C src add file.txt &&\n+\tgit -C src commit -m 1st_commit &&\n+\n+\tcp src/file.txt src/file2.txt &&\n+\techo another_line >>src/file.txt &&\n+\tgit -C src add file.txt file2.txt &&\n+\tgit -C src commit -m 2nd_commit &&\n+\n+\ttest_create_repo dst &&\n+\tgit -C src fast-export --all -C | git -C dst fast-import &&\n+\tgit -C src show >expected &&\n+\tgit -C dst show >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.14.1.821.g8fa685d3b7-goog\n\n"},{"id":"328563","messageId":"CAEPqvox03p3jVSy4e9q+e_rE+oEudE=Y3FtX+AXahaYs9hYX9A@mail.gmail.com","threadId":"46752","inReplyTo":"20170920235502.6214-1-jonathantanmy@google.com","subject":"Re: [PATCH] fast-export: do not copy from modified file","fromName":"Juraj Oršulić","fromEmail":"juraj.orsulic@fer.hr","sentAt":"2017-09-21T09:07:48Z","receivedAt":"2017-09-21T09:08:38Z","isPatch":true,"sender":{"key":"juraj.orsulic@fer.hr","avatar":"https://gravatar.com/avatar/1539f078c7cc6d853a33423b8059ece7a9b3b9c2c9eeb2269e7a8be647328ab1?d=mp&s=160"},"body":"Thank you, Jonathan!\n\nOn Thu, Sep 21, 2017 at 1:55 AM, Jonathan Tan <jonathantanmy@google.com> wrote:\n> When run with the \"-C\" option, fast-export writes 'C' commands in its\n> output whenever the internal diff mechanism detects a file copy,\n> indicating that fast-import should copy the given existing file to the\n> given new filename. However, the diff mechanism works against the\n> prior version of the file, whereas fast-import uses whatever is current.\n> This causes issues when a commit both modifies a file and uses it as the\n> source for a copy.\n>\n> Therefore, teach fast-export to refrain from writing 'C' when it has\n> already written a modification command for a file.\n>\n> An existing test in t9350-fast-export is also fixed in this patch. The\n> existing line \"C file6 file7\" copies the wrong version of file6, but it\n> has coincidentally worked because file7 was subsequently overridden.\n>\n> Reported-by: Juraj Oršulić <juraj.orsulic@fer.hr>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> I tested this with the reproduction commands given by Juraj Oršulić and\n> it works.\n>\n> I also have a version that prints the redundant 'C' (and does not\n> require the change to t9350), omitting it only if it would cause a wrong\n> result. That seems imprecise to me, but I can send that out if the\n> redundant 'C' is preferred.\n> ---\n>  builtin/fast-export.c  | 46 ++++++++++++++++++++++++++++++++--------------\n>  t/t9350-fast-export.sh | 20 +++++++++++++++++++-\n>  2 files changed, 51 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index d412c0a8f..da42ee5e6 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -344,6 +344,7 @@ static void show_filemodify(struct diff_queue_struct *q,\n>                             struct diff_options *options, void *data)\n>  {\n>         int i;\n> +       struct string_list *changed = data;\n>\n>         /*\n>          * Handle files below a directory first, in case they are all deleted\n> @@ -359,20 +360,31 @@ static void show_filemodify(struct diff_queue_struct *q,\n>                 case DIFF_STATUS_DELETED:\n>                         printf(\"D \");\n>                         print_path(spec->path);\n> +                       string_list_insert(changed, spec->path);\n>                         putchar('\\n');\n>                         break;\n>\n>                 case DIFF_STATUS_COPIED:\n>                 case DIFF_STATUS_RENAMED:\n> -                       printf(\"%c \", q->queue[i]->status);\n> -                       print_path(ospec->path);\n> -                       putchar(' ');\n> -                       print_path(spec->path);\n> -                       putchar('\\n');\n> -\n> -                       if (!oidcmp(&ospec->oid, &spec->oid) &&\n> -                           ospec->mode == spec->mode)\n> -                               break;\n> +                       /*\n> +                        * If a change in the file corresponding to ospec->path\n> +                        * has been observed, we cannot trust its contents\n> +                        * because the diff is calculated based on the prior\n> +                        * contents, not the current contents.  So, declare a\n> +                        * copy or rename only if there was no change observed.\n> +                        */\n> +                       if (!string_list_has_string(changed, ospec->path)) {\n> +                               printf(\"%c \", q->queue[i]->status);\n> +                               print_path(ospec->path);\n> +                               putchar(' ');\n> +                               print_path(spec->path);\n> +                               string_list_insert(changed, spec->path);\n> +                               putchar('\\n');\n> +\n> +                               if (!oidcmp(&ospec->oid, &spec->oid) &&\n> +                                   ospec->mode == spec->mode)\n> +                                       break;\n> +                       }\n>                         /* fallthrough */\n>\n>                 case DIFF_STATUS_TYPE_CHANGED:\n> @@ -393,6 +405,7 @@ static void show_filemodify(struct diff_queue_struct *q,\n>                                        get_object_mark(object));\n>                         }\n>                         print_path(spec->path);\n> +                       string_list_insert(changed, spec->path);\n>                         putchar('\\n');\n>                         break;\n>\n> @@ -528,7 +541,8 @@ static void anonymize_ident_line(const char **beg, const char **end)\n>         *end = out->buf + out->len;\n>  }\n>\n> -static void handle_commit(struct commit *commit, struct rev_info *rev)\n> +static void handle_commit(struct commit *commit, struct rev_info *rev,\n> +                         struct string_list *paths_of_changed_objects)\n>  {\n>         int saved_output_format = rev->diffopt.output_format;\n>         const char *commit_buffer;\n> @@ -615,6 +629,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)\n>         if (full_tree)\n>                 printf(\"deleteall\\n\");\n>         log_tree_diff_flush(rev);\n> +       string_list_clear(paths_of_changed_objects, 0);\n>         rev->diffopt.output_format = saved_output_format;\n>\n>         printf(\"\\n\");\n> @@ -630,14 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len)\n>         return strbuf_detach(&out, len);\n>  }\n>\n> -static void handle_tail(struct object_array *commits, struct rev_info *revs)\n> +static void handle_tail(struct object_array *commits, struct rev_info *revs,\n> +                       struct string_list *paths_of_changed_objects)\n>  {\n>         struct commit *commit;\n>         while (commits->nr) {\n>                 commit = (struct commit *)commits->objects[commits->nr - 1].item;\n>                 if (has_unshown_parent(commit))\n>                         return;\n> -               handle_commit(commit, revs);\n> +               handle_commit(commit, revs, paths_of_changed_objects);\n>                 commits->nr--;\n>         }\n>  }\n> @@ -977,6 +993,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n>         char *export_filename = NULL, *import_filename = NULL;\n>         uint32_t lastimportid;\n>         struct string_list refspecs_list = STRING_LIST_INIT_NODUP;\n> +       struct string_list paths_of_changed_objects = STRING_LIST_INIT_DUP;\n>         struct option options[] = {\n>                 OPT_INTEGER(0, \"progress\", &progress,\n>                             N_(\"show progress after <n> objects\")),\n> @@ -1049,14 +1066,15 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n>         if (prepare_revision_walk(&revs))\n>                 die(\"revision walk setup failed\");\n>         revs.diffopt.format_callback = show_filemodify;\n> +       revs.diffopt.format_callback_data = &paths_of_changed_objects;\n>         DIFF_OPT_SET(&revs.diffopt, RECURSIVE);\n>         while ((commit = get_revision(&revs))) {\n>                 if (has_unshown_parent(commit)) {\n>                         add_object_array(&commit->object, NULL, &commits);\n>                 }\n>                 else {\n> -                       handle_commit(commit, &revs);\n> -                       handle_tail(&commits, &revs);\n> +                       handle_commit(commit, &revs, &paths_of_changed_objects);\n> +                       handle_tail(&commits, &revs, &paths_of_changed_objects);\n>                 }\n>         }\n>\n> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\n> index 8dcb05c4a..866ddf605 100755\n> --- a/t/t9350-fast-export.sh\n> +++ b/t/t9350-fast-export.sh\n> @@ -234,7 +234,7 @@ test_expect_success 'fast-export -C -C | fast-import' '\n>         mkdir new &&\n>         git --git-dir=new/.git init &&\n>         git fast-export -C -C --signed-tags=strip --all > output &&\n> -       grep \"^C file6 file7\\$\" output &&\n> +       grep \"^C file2 file4\\$\" output &&\n>         cat output |\n>         (cd new &&\n>          git fast-import &&\n> @@ -522,4 +522,22 @@ test_expect_success 'delete refspec' '\n>         test_cmp expected actual\n>  '\n>\n> +test_expect_success 'when using -C, do not declare copy when source of copy is also modified' '\n> +       test_create_repo src &&\n> +       echo a_line >src/file.txt &&\n> +       git -C src add file.txt &&\n> +       git -C src commit -m 1st_commit &&\n> +\n> +       cp src/file.txt src/file2.txt &&\n> +       echo another_line >>src/file.txt &&\n> +       git -C src add file.txt file2.txt &&\n> +       git -C src commit -m 2nd_commit &&\n> +\n> +       test_create_repo dst &&\n> +       git -C src fast-export --all -C | git -C dst fast-import &&\n> +       git -C src show >expected &&\n> +       git -C dst show >actual &&\n> +       test_cmp expected actual\n> +'\n> +\n>  test_done\n> --\n> 2.14.1.821.g8fa685d3b7-goog\n>\n"}]}