[PATCH] fast-export: fix regression skipping some merge-commits

["Martin Ågren" <martin.agren@gmail.com>]

7199203937 (object_array: add and use `object_array_pop()`, 2017-09-23)
noted that the pattern `object = array.objects[--array.nr].item` could
be abstracted as `object = object_array_pop(&array)`.

Unfortunately, one of the conversions was horribly wrong. Between
grabbing the last object (i.e., peeking at it) and decreasing the object
count, the original code would sometimes return early. The updated code
on the other hand, will always pop the last element, then maybe do the
early return before doing anything with the object.

The end result is that merge commits where all the parents have still
not been exported will simply be dropped, meaning that they will be
completely missing from the exported data.

Reintroduce the pattern of first grabbing the last object (using a new
function `object_array_peek()`), then later poping it. Using
`..._peek()` and `..._pop()` makes it clear that we are referring to the
same item, i.e., we do not grab one element, then remove another one.

Add a test that would have caught this.

Reported-by: Isaac Chou <Isaac.Chou@microfocus.com>
Analyzed-by: Isaac Chou <Isaac.Chou@microfocus.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
Based on maint, but applies equally well on master.

My sincerest apologies for the stupid train-wreck that the original
conversion was. Weird interactions between different components can make
for fun bugs, but this one is just embarassing.

Isaac, this should solve the problem you are seeing. Unfortunately, I do
not have any experience with building Git for Windows [1]. I really hope
that this bug did not take up too much of your time. Or eat your data!

Martin

[1] The least I can do is provide a link:
https://github.com/git-for-windows/git/wiki/Building-Git

 t/t9350-fast-export.sh | 22 ++++++++++++++++++++++
 object.h               |  9 +++++++++
 builtin/fast-export.c  |  3 ++-
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 866ddf6058..2b46a83a49 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -540,4 +540,26 @@ test_expect_success 'when using -C, do not declare copy when source of copy is a
 	test_cmp expected actual
 '
 
+test_expect_success 'todo' '
+	test_create_repo merging &&
+	git -C merging commit --allow-empty -m initial &&
+
+	git -C merging checkout -b topic &&
+	>merging/topic-file &&
+	git -C merging add topic-file &&
+	git -C merging commit -m topic-file &&
+
+	git -C merging checkout master &&
+	>merging/master-file &&
+	git -C merging add master-file &&
+	git -C merging commit -m master-file &&
+
+	git -C merging merge --no-ff topic -m "merge the topic" &&
+
+	oid=$(git -C merging rev-parse HEAD^^) &&
+	echo :1 $oid >merging/git-marks &&
+	git -C merging fast-export --import-marks=git-marks refs/heads/master >out &&
+	grep "merge the topic" out
+'
+
 test_done
diff --git a/object.h b/object.h
index f13f85b2a9..4d8ce280d9 100644
--- a/object.h
+++ b/object.h
@@ -129,6 +129,15 @@ void add_object_array_with_path(struct object *obj, const char *name, struct obj
  */
 struct object *object_array_pop(struct object_array *array);
 
+/*
+ * Returns NULL if the array is empty. Otherwise, returns the last object.
+ * That is, the returned value is what `object_array_pop()` would have returned.
+ */
+inline struct object *object_array_peek(const struct object_array *array)
+{
+	return array->nr ? array->objects[array->nr - 1].item : NULL;
+}
+
 typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);
 
 /*
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 27b2cc138e..8377d27b46 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -650,9 +650,10 @@ 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);
+		commit = (struct commit *)object_array_peek(commits);
 		if (has_unshown_parent(commit))
 			return;
+		(void)object_array_pop(commits);
 		handle_commit(commit, revs, paths_of_changed_objects);
 	}
 }
-- 
2.17.0