[PATCH v2] 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>
---
Hmph. Version 1 described the test as "todo". This version uses a better
description. No other changes.

 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..194782b05b 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 'merge commit gets exported with --import-marks' '
+	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