[PATCH 0/2] restore fast-export full filecopy detection

[PATCH 0/2] restore fast-export full filecopy detection

[Mark Nauwelaerts]

From: Mark Nauwelaerts <mnauw@users.sourceforge.net>

When using fast-export/fast-import to interface/bridge with another VCS
that explicitly tracks copy/rename as metadata, fast-export's ability
to report filecopy/filerename is quite useful (if not essential).

There has been a fix in this area recently with as side-effect that
in some scenarios a filecopy is no longer reported as such.
These few patches provide an alternative fix for the original problem
while still retaining previous fast-export's filecopy reporting.

See patches for additional details and commit references.


Mark Nauwelaerts (2):
  fast-export: ensure proper order of modify, copy and rename entries
  fast-export: remove now obsolete filtering of modified files

 builtin/fast-export.c  | 60 +++++++++++++++++++++++---------------------------
 t/t9350-fast-export.sh |  2 +-
 2 files changed, 28 insertions(+), 34 deletions(-)

-- 
2.7.4

[PATCH 1/2] fast-export: ensure proper order of modify, copy and rename entries

[Mark Nauwelaerts]

From: Mark Nauwelaerts <mnauw@users.sourceforge.net>

As b3e8ca8 ("fast-export: do not copy from modified file", 2017-09-20)
indicates, if a commit both modifies a file and uses it as a source for a
copy, then the specifications of git-fast-import require that the copy
should be reported first followed by the modification to the source file.

The commit above addressed the problem by never reporting the copy.  However,
the copy can still be reported if the entries are sorted properly.  That
can be achieved by adjusting the order of the sort that is performed anyway
for other reasons of consistency.  This is merely an extra order condition.

Furthermore, when using fast-export to export or bridge to another
version control system which explicitly tracks copies, then the 'C' commands
in the output are quite useful and necessary.

Signed-off-by: Mark Nauwelaerts <mnauw@users.sourceforge.net>
---
 builtin/fast-export.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 2fb60d6..1b3e250 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -262,6 +262,14 @@ static void export_blob(const struct object_id *oid)
 		free(buf);
 }
 
+static int order_status(const char status)
+{
+	if (status == 'R')
+		return 2;
+	else
+		return status == 'M' ? 1 : 0;
+}
+
 static int depth_first(const void *a_, const void *b_)
 {
 	const struct diff_filepair *a = *((const struct diff_filepair **)a_);
@@ -288,8 +296,12 @@ static int depth_first(const void *a_, const void *b_)
 	 * Move 'R'ename entries last so that all references of the file
 	 * appear in the output before it is renamed (e.g., when a file
 	 * was copied and renamed in the same commit).
+	 * Moreover, 'C' needs to go before 'M' if the file was copied
+	 * and then modified a bit, as it should be done that way as well
+	 * at import time (also recall that 'C' is calculated on the
+	 * original content).
 	 */
-	return (a->status == 'R') - (b->status == 'R');
+	return order_status(a->status) - order_status(b->status);
 }
 
 static void print_path_1(const char *path)
-- 
2.7.4

[PATCH 2/2] fast-export: remove now obsolete filtering of modified files

[Mark Nauwelaerts]

From: Mark Nauwelaerts <mnauw@users.sourceforge.net>

Revert b3e8ca8 ("fast-export: do not copy from modified file", 2017-09-20)
partially, since it is not necessary to filter out 'C' commands if the 'M'
and 'C' commands are in correct order.

The unit test that was added is kept around though, and it still passes
with this new approach.

Signed-off-by: Mark Nauwelaerts <mnauw@users.sourceforge.net>
---
 builtin/fast-export.c  | 46 ++++++++++++++--------------------------------
 t/t9350-fast-export.sh |  2 +-
 2 files changed, 15 insertions(+), 33 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 1b3e250..6aa4073 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -356,7 +356,6 @@ 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
@@ -372,31 +371,20 @@ 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:
-			/*
-			 * 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;
-			}
+			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;
 			/* fallthrough */
 
 		case DIFF_STATUS_TYPE_CHANGED:
@@ -417,7 +405,6 @@ 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;
 
@@ -553,8 +540,7 @@ 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,
-			  struct string_list *paths_of_changed_objects)
+static void handle_commit(struct commit *commit, struct rev_info *rev)
 {
 	int saved_output_format = rev->diffopt.output_format;
 	const char *commit_buffer;
@@ -641,7 +627,6 @@ 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");
@@ -657,15 +642,14 @@ 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,
-			struct string_list *paths_of_changed_objects)
+static void handle_tail(struct object_array *commits, struct rev_info *revs)
 {
 	struct commit *commit;
 	while (commits->nr) {
 		commit = (struct commit *)object_array_pop(commits);
 		if (has_unshown_parent(commit))
 			return;
-		handle_commit(commit, revs, paths_of_changed_objects);
+		handle_commit(commit, revs);
 	}
 }
 
@@ -1004,7 +988,6 @@ 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 <n> objects")),
@@ -1077,15 +1060,14 @@ 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, &paths_of_changed_objects);
-			handle_tail(&commits, &revs, &paths_of_changed_objects);
+			handle_commit(commit, &revs);
+			handle_tail(&commits, &revs);
 		}
 	}
 
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 866ddf6..06c66a1 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -522,7 +522,7 @@ 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_expect_success 'when using -C, correctly order copy and modify of source of copy' '
 	test_create_repo src &&
 	echo a_line >src/file.txt &&
 	git -C src add file.txt &&
-- 
2.7.4

Re: [PATCH 0/2] restore fast-export full filecopy detection

[Mark Nauwelaerts]

On 24/12/17 11:52, Mark Nauwelaerts wrote:
> From: Mark Nauwelaerts <mnauw@users.sourceforge.net>
>
> When using fast-export/fast-import to interface/bridge with another VCS
> that explicitly tracks copy/rename as metadata, fast-export's ability
> to report filecopy/filerename is quite useful (if not essential).
>
> There has been a fix in this area recently with as side-effect that
> in some scenarios a filecopy is no longer reported as such.
> These few patches provide an alternative fix for the original problem
> while still retaining previous fast-export's filecopy reporting.
>
> See patches for additional details and commit references.
Any comments or opinions on these patches?  Could they be applied and included?

Thanks,
Mark.