Hi Martin, No problem at all. Thanks for the super quick turnaround. :-) Isaac -----Original Message----- From: Martin Ågren [mailto:martin.agren@xxxxxxxxx] Sent: Friday, April 20, 2018 2:13 PM To: git@xxxxxxxxxxxxxxx; Isaac Chou <Isaac.Chou@xxxxxxxxxxxxxx> Cc: Junio C Hamano <gitster@xxxxxxxxx>; Johannes Schindelin <johannes.schindelin@xxxxxx>; Jonathan Tan <jonathantanmy@xxxxxxxxxx> Subject: [PATCH] fast-export: fix regression skipping some merge-commits 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@xxxxxxxxxxxxxx> Analyzed-by: Isaac Chou <Isaac.Chou@xxxxxxxxxxxxxx> Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx> --- 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
