# git fast-export/import bug with -M -C 4 messages from 2017-09-14 to 2017-09-21. Participants: Juraj Oršulić, Jonathan Tan. Thread: https://gitlist.dev/t/46752 ## Juraj Oršulić, 2017-09-14 22:01 Subject: git fast-export/import bug with -M -C Message-ID: URL: https://gitlist.dev/e/CAEPqvoyVJFe2EOvhnZD4vdF%3D1-VuoZrMP92TeGJ2WAE0X%2BB5Tw%40mail.gmail.com ``` The commands should be self explanatory. 0.2.0~20 is the first commit where the reconstructed repository diverges, that commit had a simultaneous copy and edit of one file. It seems that copy/rename detection, enabled with -M -C is confused by this. I reproduced it with git 2.14 next @ 8fa685d. git clone https://github.com/googlecartographer/cartographer_ros mkdir copy && cd copy && git init (cd ../cartographer_ros; git fast-export --all --date-order -M -C) | git fast-import git rev-parse 0.2.0~20 #2237e1d0a974977fbcb0737dd1fb5876a2b8e29d git rev-parse 0.2.0~21 #cd1276a99ccffcc491d0b2e50296ec04347ba5f2 cd ../cartographer_ros git rev-parse 0.2.0~20 #9d5b221ed41783b15c84bc90b71527194b8d9a49 git rev-parse 0.2.0~21 #cd1276a99ccffcc491d0b2e50296ec04347ba5f2 ``` ## Juraj Oršulić, 2017-09-20 16:55 Subject: Re: git fast-export/import bug with -M -C Message-ID: URL: https://gitlist.dev/e/CAEPqvowWEdFdJoyqsmBt7KRU8xWyuLnFwV7CJ7zH2QRa2AQCtg%40mail.gmail.com In-Reply-To: ``` Hi, did anyone manage to take a look at this bug? Friendly ping. Juraj On Fri, Sep 15, 2017 at 12:01 AM, Juraj Oršulić wrote: > The commands should be self explanatory. 0.2.0~20 is the first commit > where the reconstructed repository diverges, that commit had a > simultaneous copy and edit of one file. It seems that copy/rename > detection, enabled with -M -C is confused by this. I reproduced it > with git 2.14 next @ 8fa685d. > > git clone https://github.com/googlecartographer/cartographer_ros > mkdir copy && cd copy && git init > (cd ../cartographer_ros; git fast-export --all --date-order -M -C) | > git fast-import > git rev-parse 0.2.0~20 #2237e1d0a974977fbcb0737dd1fb5876a2b8e29d > git rev-parse 0.2.0~21 #cd1276a99ccffcc491d0b2e50296ec04347ba5f2 > cd ../cartographer_ros > git rev-parse 0.2.0~20 #9d5b221ed41783b15c84bc90b71527194b8d9a49 > git rev-parse 0.2.0~21 #cd1276a99ccffcc491d0b2e50296ec04347ba5f2 ``` ## Jonathan Tan, 2017-09-20 23:55 Subject: [PATCH] fast-export: do not copy from modified file Message-ID: <20170920235502.6214-1-jonathantanmy@google.com> URL: https://gitlist.dev/e/20170920235502.6214-1-jonathantanmy%40google.com In-Reply-To: ``` When run with the "-C" option, fast-export writes 'C' commands in its output whenever the internal diff mechanism detects a file copy, indicating that fast-import should copy the given existing file to the given new filename. However, the diff mechanism works against the prior version of the file, whereas fast-import uses whatever is current. This causes issues when a commit both modifies a file and uses it as the source for a copy. Therefore, teach fast-export to refrain from writing 'C' when it has already written a modification command for a file. An existing test in t9350-fast-export is also fixed in this patch. The existing line "C file6 file7" copies the wrong version of file6, but it has coincidentally worked because file7 was subsequently overridden. Reported-by: Juraj Oršulić Signed-off-by: Jonathan Tan --- I tested this with the reproduction commands given by Juraj Oršulić and it works. I also have a version that prints the redundant 'C' (and does not require the change to t9350), omitting it only if it would cause a wrong result. That seems imprecise to me, but I can send that out if the redundant 'C' is preferred. --- builtin/fast-export.c | 46 ++++++++++++++++++++++++++++++++-------------- t/t9350-fast-export.sh | 20 +++++++++++++++++++- 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/builtin/fast-export.c b/builtin/fast-export.c index d412c0a8f..da42ee5e6 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -344,6 +344,7 @@ static void show_filemodify(struct diff_queue_struct *q, struct diff_options *options, void *data) { int i; + struct string_list *changed = data; /* * Handle files below a directory first, in case they are all deleted @@ -359,20 +360,31 @@ static void show_filemodify(struct diff_queue_struct *q, case DIFF_STATUS_DELETED: printf("D "); print_path(spec->path); + string_list_insert(changed, spec->path); putchar('\n'); break; case DIFF_STATUS_COPIED: case DIFF_STATUS_RENAMED: - printf("%c ", q->queue[i]->status); - print_path(ospec->path); - putchar(' '); - print_path(spec->path); - putchar('\n'); - - if (!oidcmp(&ospec->oid, &spec->oid) && - ospec->mode == spec->mode) - break; + /* + * If a change in the file corresponding to ospec->path + * has been observed, we cannot trust its contents + * because the diff is calculated based on the prior + * contents, not the current contents. So, declare a + * copy or rename only if there was no change observed. + */ + if (!string_list_has_string(changed, ospec->path)) { + printf("%c ", q->queue[i]->status); + print_path(ospec->path); + putchar(' '); + print_path(spec->path); + string_list_insert(changed, spec->path); + putchar('\n'); + + if (!oidcmp(&ospec->oid, &spec->oid) && + ospec->mode == spec->mode) + break; + } /* fallthrough */ case DIFF_STATUS_TYPE_CHANGED: @@ -393,6 +405,7 @@ static void show_filemodify(struct diff_queue_struct *q, get_object_mark(object)); } print_path(spec->path); + string_list_insert(changed, spec->path); putchar('\n'); break; @@ -528,7 +541,8 @@ static void anonymize_ident_line(const char **beg, const char **end) *end = out->buf + out->len; } -static void handle_commit(struct commit *commit, struct rev_info *rev) +static void handle_commit(struct commit *commit, struct rev_info *rev, + struct string_list *paths_of_changed_objects) { int saved_output_format = rev->diffopt.output_format; const char *commit_buffer; @@ -615,6 +629,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev) if (full_tree) printf("deleteall\n"); log_tree_diff_flush(rev); + string_list_clear(paths_of_changed_objects, 0); rev->diffopt.output_format = saved_output_format; printf("\n"); @@ -630,14 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len) return strbuf_detach(&out, len); } -static void handle_tail(struct object_array *commits, struct rev_info *revs) +static void handle_tail(struct object_array *commits, struct rev_info *revs, + struct string_list *paths_of_changed_objects) { struct commit *commit; while (commits->nr) { commit = (struct commit *)commits->objects[commits->nr - 1].item; if (has_unshown_parent(commit)) return; - handle_commit(commit, revs); + handle_commit(commit, revs, paths_of_changed_objects); commits->nr--; } } @@ -977,6 +993,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix) char *export_filename = NULL, *import_filename = NULL; uint32_t lastimportid; struct string_list refspecs_list = STRING_LIST_INIT_NODUP; + struct string_list paths_of_changed_objects = STRING_LIST_INIT_DUP; struct option options[] = { OPT_INTEGER(0, "progress", &progress, N_("show progress after objects")), @@ -1049,14 +1066,15 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix) if (prepare_revision_walk(&revs)) die("revision walk setup failed"); revs.diffopt.format_callback = show_filemodify; + revs.diffopt.format_callback_data = &paths_of_changed_objects; DIFF_OPT_SET(&revs.diffopt, RECURSIVE); while ((commit = get_revision(&revs))) { if (has_unshown_parent(commit)) { add_object_array(&commit->object, NULL, &commits); } else { - handle_commit(commit, &revs); - handle_tail(&commits, &revs); + handle_commit(commit, &revs, &paths_of_changed_objects); + handle_tail(&commits, &revs, &paths_of_changed_objects); } } diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh index 8dcb05c4a..866ddf605 100755 --- a/t/t9350-fast-export.sh +++ b/t/t9350-fast-export.sh @@ -234,7 +234,7 @@ test_expect_success 'fast-export -C -C | fast-import' ' mkdir new && git --git-dir=new/.git init && git fast-export -C -C --signed-tags=strip --all > output && - grep "^C file6 file7\$" output && + grep "^C file2 file4\$" output && cat output | (cd new && git fast-import && @@ -522,4 +522,22 @@ test_expect_success 'delete refspec' ' test_cmp expected actual ' +test_expect_success 'when using -C, do not declare copy when source of copy is also modified' ' + test_create_repo src && + echo a_line >src/file.txt && + git -C src add file.txt && + git -C src commit -m 1st_commit && + + cp src/file.txt src/file2.txt && + echo another_line >>src/file.txt && + git -C src add file.txt file2.txt && + git -C src commit -m 2nd_commit && + + test_create_repo dst && + git -C src fast-export --all -C | git -C dst fast-import && + git -C src show >expected && + git -C dst show >actual && + test_cmp expected actual +' + test_done -- 2.14.1.821.g8fa685d3b7-goog ``` ## Juraj Oršulić, 2017-09-21 09:07 Subject: Re: [PATCH] fast-export: do not copy from modified file Message-ID: URL: https://gitlist.dev/e/CAEPqvox03p3jVSy4e9q%2Be_rE%2BoEudE%3DY3FtX%2BAXahaYs9hYX9A%40mail.gmail.com In-Reply-To: <20170920235502.6214-1-jonathantanmy@google.com> ``` Thank you, Jonathan! On Thu, Sep 21, 2017 at 1:55 AM, Jonathan Tan wrote: > When run with the "-C" option, fast-export writes 'C' commands in its > output whenever the internal diff mechanism detects a file copy, > indicating that fast-import should copy the given existing file to the > given new filename. However, the diff mechanism works against the > prior version of the file, whereas fast-import uses whatever is current. > This causes issues when a commit both modifies a file and uses it as the > source for a copy. > > Therefore, teach fast-export to refrain from writing 'C' when it has > already written a modification command for a file. > > An existing test in t9350-fast-export is also fixed in this patch. The > existing line "C file6 file7" copies the wrong version of file6, but it > has coincidentally worked because file7 was subsequently overridden. > > Reported-by: Juraj Oršulić > Signed-off-by: Jonathan Tan > --- > I tested this with the reproduction commands given by Juraj Oršulić and > it works. > > I also have a version that prints the redundant 'C' (and does not > require the change to t9350), omitting it only if it would cause a wrong > result. That seems imprecise to me, but I can send that out if the > redundant 'C' is preferred. > --- > builtin/fast-export.c | 46 ++++++++++++++++++++++++++++++++-------------- > t/t9350-fast-export.sh | 20 +++++++++++++++++++- > 2 files changed, 51 insertions(+), 15 deletions(-) > > diff --git a/builtin/fast-export.c b/builtin/fast-export.c > index d412c0a8f..da42ee5e6 100644 > --- a/builtin/fast-export.c > +++ b/builtin/fast-export.c > @@ -344,6 +344,7 @@ static void show_filemodify(struct diff_queue_struct *q, > struct diff_options *options, void *data) > { > int i; > + struct string_list *changed = data; > > /* > * Handle files below a directory first, in case they are all deleted > @@ -359,20 +360,31 @@ static void show_filemodify(struct diff_queue_struct *q, > case DIFF_STATUS_DELETED: > printf("D "); > print_path(spec->path); > + string_list_insert(changed, spec->path); > putchar('\n'); > break; > > case DIFF_STATUS_COPIED: > case DIFF_STATUS_RENAMED: > - printf("%c ", q->queue[i]->status); > - print_path(ospec->path); > - putchar(' '); > - print_path(spec->path); > - putchar('\n'); > - > - if (!oidcmp(&ospec->oid, &spec->oid) && > - ospec->mode == spec->mode) > - break; > + /* > + * If a change in the file corresponding to ospec->path > + * has been observed, we cannot trust its contents > + * because the diff is calculated based on the prior > + * contents, not the current contents. So, declare a > + * copy or rename only if there was no change observed. > + */ > + if (!string_list_has_string(changed, ospec->path)) { > + printf("%c ", q->queue[i]->status); > + print_path(ospec->path); > + putchar(' '); > + print_path(spec->path); > + string_list_insert(changed, spec->path); > + putchar('\n'); > + > + if (!oidcmp(&ospec->oid, &spec->oid) && > + ospec->mode == spec->mode) > + break; > + } > /* fallthrough */ > > case DIFF_STATUS_TYPE_CHANGED: > @@ -393,6 +405,7 @@ static void show_filemodify(struct diff_queue_struct *q, > get_object_mark(object)); > } > print_path(spec->path); > + string_list_insert(changed, spec->path); > putchar('\n'); > break; > > @@ -528,7 +541,8 @@ static void anonymize_ident_line(const char **beg, const char **end) > *end = out->buf + out->len; > } > > -static void handle_commit(struct commit *commit, struct rev_info *rev) > +static void handle_commit(struct commit *commit, struct rev_info *rev, > + struct string_list *paths_of_changed_objects) > { > int saved_output_format = rev->diffopt.output_format; > const char *commit_buffer; > @@ -615,6 +629,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev) > if (full_tree) > printf("deleteall\n"); > log_tree_diff_flush(rev); > + string_list_clear(paths_of_changed_objects, 0); > rev->diffopt.output_format = saved_output_format; > > printf("\n"); > @@ -630,14 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len) > return strbuf_detach(&out, len); > } > > -static void handle_tail(struct object_array *commits, struct rev_info *revs) > +static void handle_tail(struct object_array *commits, struct rev_info *revs, > + struct string_list *paths_of_changed_objects) > { > struct commit *commit; > while (commits->nr) { > commit = (struct commit *)commits->objects[commits->nr - 1].item; > if (has_unshown_parent(commit)) > return; > - handle_commit(commit, revs); > + handle_commit(commit, revs, paths_of_changed_objects); > commits->nr--; > } > } > @@ -977,6 +993,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix) > char *export_filename = NULL, *import_filename = NULL; > uint32_t lastimportid; > struct string_list refspecs_list = STRING_LIST_INIT_NODUP; > + struct string_list paths_of_changed_objects = STRING_LIST_INIT_DUP; > struct option options[] = { > OPT_INTEGER(0, "progress", &progress, > N_("show progress after objects")), > @@ -1049,14 +1066,15 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix) > if (prepare_revision_walk(&revs)) > die("revision walk setup failed"); > revs.diffopt.format_callback = show_filemodify; > + revs.diffopt.format_callback_data = &paths_of_changed_objects; > DIFF_OPT_SET(&revs.diffopt, RECURSIVE); > while ((commit = get_revision(&revs))) { > if (has_unshown_parent(commit)) { > add_object_array(&commit->object, NULL, &commits); > } > else { > - handle_commit(commit, &revs); > - handle_tail(&commits, &revs); > + handle_commit(commit, &revs, &paths_of_changed_objects); > + handle_tail(&commits, &revs, &paths_of_changed_objects); > } > } > > diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh > index 8dcb05c4a..866ddf605 100755 > --- a/t/t9350-fast-export.sh > +++ b/t/t9350-fast-export.sh > @@ -234,7 +234,7 @@ test_expect_success 'fast-export -C -C | fast-import' ' > mkdir new && > git --git-dir=new/.git init && > git fast-export -C -C --signed-tags=strip --all > output && > - grep "^C file6 file7\$" output && > + grep "^C file2 file4\$" output && > cat output | > (cd new && > git fast-import && > @@ -522,4 +522,22 @@ test_expect_success 'delete refspec' ' > test_cmp expected actual > ' > > +test_expect_success 'when using -C, do not declare copy when source of copy is also modified' ' > + test_create_repo src && > + echo a_line >src/file.txt && > + git -C src add file.txt && > + git -C src commit -m 1st_commit && > + > + cp src/file.txt src/file2.txt && > + echo another_line >>src/file.txt && > + git -C src add file.txt file2.txt && > + git -C src commit -m 2nd_commit && > + > + test_create_repo dst && > + git -C src fast-export --all -C | git -C dst fast-import && > + git -C src show >expected && > + git -C dst show >actual && > + test_cmp expected actual > +' > + > test_done > -- > 2.14.1.821.g8fa685d3b7-goog > ```