{"thread":{"id":"48329","subject":"RE: [BUG] Git fast-export with import marks file omits merge commits","startedAt":"2018-04-19T21:46:44Z","lastAt":"2018-06-02T06:49:21Z","messageCount":18,"participants":["Isaac Chou","Elijah Newren","Junio C Hamano","Martin Ågren","Johannes Schindelin","Eric Sunshine","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"345170","messageId":"MW2PR18MB2284E3091991A3DB228CEBBEE5B50@MW2PR18MB2284.namprd18.prod.outlook.com","threadId":"48329","inReplyTo":null,"subject":"RE: [BUG] Git fast-export with import marks file omits merge commits","fromName":"Isaac Chou","fromEmail":"isaac.chou@microfocus.com","sentAt":"2018-04-19T21:46:38Z","receivedAt":"2018-04-19T21:46:44Z","isPatch":false,"sender":{"key":"isaac.chou@microfocus.com","avatar":null},"body":"I inspected the source code (builtin/fast-export.c) for the fast-export issue I encountered, and it looks like the merge commit is discarded too early by the call to object_array_pop() after only one of the two UNSHOWN parents is processed in the method handle_tail(). The poped merge commit still has one UNSHOWN parent, therefore it is not processed and is lost in the output. Can someone advise me on how to submit a code change or bug report in order to get the fix into the code base?\n\nThanks,\n\nIsaac\n\n-----Original Message-----\nFrom: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of Isaac Chou\nSent: Monday, April 16, 2018 3:58 PM\nTo: git@vger.kernel.org\nSubject: Git fast-export with import marks file omits merge commits\n\nHello,\n\nI came across a change of behavior with Git version 2.15 and later where the fast-export command would omit the merge commits.  The same use case works correctly with Git version 2.14 and older.  Here is the detail of the use case:\n\n0> git --version\ngit version 2.16.2.windows.1\n\n1> git init\nInitialized empty Git repository in c:/./.git/\n\n2> echo 1111 >> file.txt\n\n3> git add file.txt\n\n4> git commit -m \"first commit\"\n[master (root-commit) 711d4d5] first commit\n1 file changed, 1 insertion(+)\ncreate mode 100644 file.txt\n\n5> git checkout -b test\nSwitched to a new branch 'test'\n\n6> echo 2222 >> file.txt\n\n7> git add file.txt\n\n8> git commit -m \"commit on test branch\"\n[test 76d231c] commit on test branch\n1 file changed, 1 insertion(+)\n\n9> git checkout master\nSwitched to branch 'master'\n\n10> echo 3333 >> file.txt\n\n11> git add file.txt\n\n12> git commit -m \"commit on master branch\"\n[master 61c55fd] commit on master branch\n1 file changed, 1 insertion(+)\n\n13> git merge test\nAuto-merging file.txt\nCONFLICT (content): Merge conflict in file.txt Automatic merge failed; fix conflicts and then commit the result.\n\n14> notepad file.txt\n\n15> git add file.txt\n\n16> git commit -m \"merged with test branch\"\n[master 1442e0e] merged with test branch\n\n17> git log\ncommit 1442e0ee728c831e74550329e39d27d4188b4422 (HEAD -> master)\nMerge: 61c55fd 76d231c\nAuthor: isaac <...>\nDate:   Mon Apr 16 15:08:39 2018 -0400\n\n    merged with test branch\n\ncommit 61c55fdb883fc403e63c91b49bc11bdade62b3e8\nAuthor: isaac <...>\nDate:   Mon Apr 16 15:07:41 2018 -0400\n\n    commit on master branch\n\ncommit 76d231cdb12eb84f45abdebede06a56f912613d3 (test)\nAuthor: isaac <...>\nDate:   Mon Apr 16 15:07:07 2018 -0400\n\n    commit on test branch\n\ncommit 711d4d5781df41924421f8629af040e7c91a8d2e\nAuthor: isaac <...>\nDate:   Mon Apr 16 15:06:07 2018 -0400\n\n    first commit\n\n18> echo :1 711d4d5781df41924421f8629af040e7c91a8d2e > git-marks\n\n19> cat git-marks\n:1 711d4d5781df41924421f8629af040e7c91a8d2e\n\n20> git fast-export --use-done-feature --import-marks=git-marks refs/heads/master --\nfeature done\nblob\nmark :2\ndata 12\n1111\n2222\n\ncommit refs/heads/master\nmark :3\nauthor isaac <...> 1523905627 -0400\ncommitter isaac <...> 1523905627 -0400\ndata 22\ncommit on test branch\nfrom :1\nM 100644 :2 file.txt\n\nblob\nmark :4\ndata 12\n1111\n3333\n\ncommit refs/heads/master\nmark :5\nauthor isaac <...> 1523905661 -0400\ncommitter isaac <...> 1523905661 -0400\ndata 24\ncommit on master branch\nfrom :1\nM 100644 :4 file.txt\n\ndone\n\nThanks,\n\nIsaac\n\n"},{"id":"345172","messageId":"CABPp-BHv=jZZfbZnfSt81oxJ0QAxJp7W2rOsfFq6ThVEYK5z7Q@mail.gmail.com","threadId":"48329","inReplyTo":"MW2PR18MB2284E3091991A3DB228CEBBEE5B50@MW2PR18MB2284.namprd18.prod.outlook.com","subject":"Re: [BUG] Git fast-export with import marks file omits merge commits","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-19T22:26:45Z","receivedAt":"2018-04-19T22:26:50Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Isaac,\n\nOn Thu, Apr 19, 2018 at 2:46 PM, Isaac Chou wrote:\n> I inspected the source code (builtin/fast-export.c) for the fast-export issue I encountered, and it looks like the merge commit is discarded too early by the call to object_array_pop() after only one of the two UNSHOWN parents is processed in the method handle_tail(). The poped merge commit still has one UNSHOWN parent, therefore it is not processed and is lost in the output. Can someone advise me on how to submit a code change or bug report in order to get the fix into the code base?\n\nCareful now, fast-export was also the location of my first patch.\nIt's easy to get addicted to contributing changes to git. :-)\n\nInside the git.git repository, there are two files that explain the\nbasic process -- Documentation/SubmittingPatches and\nDocumentation/CodingGuidelines. If they don't cover the process well,\nthat's probably a bug itself, but feel free to ask on the list if you\nstill run into questions. Those documents can be slighly overwhelming\nat first glance, but they've got good information.\n\nElijah\n"},{"id":"345173","messageId":"xmqq604mertl.fsf@gitster-ct.c.googlers.com","threadId":"48329","inReplyTo":"MW2PR18MB2284E3091991A3DB228CEBBEE5B50@MW2PR18MB2284.namprd18.prod.outlook.com","subject":"Re: [BUG] Git fast-export with import marks file omits merge commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-19T22:48:06Z","receivedAt":"2018-04-19T22:48:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Isaac Chou writes:\n\n> I inspected the source code (builtin/fast-export.c) for the\n> fast-export issue I encountered, and it looks like the merge\n> commit is discarded too early by the call to object_array_pop()\n> after only one of the two UNSHOWN parents is processed in the\n> method handle_tail(). The poped merge commit still has one\n> UNSHOWN parent, therefore it is not processed and is lost in the\n> output. Can someone advise me on how to submit a code change or\n> bug report in order to get the fix into the code base?\n>\n> Thanks,\n>\n> Isaac\n>\n> -----Original Message-----\n> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of Isaac Chou\n> Sent: Monday, April 16, 2018 3:58 PM\n> To: git@vger.kernel.org\n> Subject: Git fast-export with import marks file omits merge commits\n>\n> Hello,\n>\n> I came across a change of behavior with Git version 2.15 and later\n> where the fast-export command would omit the merge commits. The\n> same use case works correctly with Git version 2.14 and older.\n> ...\n\nThere indeed are some differences between v2.14 and v2.15 around the\ncode that returns early when has_unshown_parent() says \"yes\" [*1*],\nbut the decision to return early when the function says \"yes\" hasn't\nchanged between that timeperiod---it dates back to f2dc849e (\"Add\n'git fast-export', the sister of 'git fast-import'\", 2007-12-02),\ni.e. the very beginning of the program's life.\n\nI'll CC those who wrote the original and b3e8ca89 (\"fast-export: do\nnot copy from modified file\", 2017-09-20) and 71992039\n(\"object_array: add and use `object_array_pop()`\", 2017-09-23),\nwhich are the only two commits that touch the surrounding area\nduring that timeperiod, to ask if something jumps at them.\n\nThanks.\n\n\n[Footnotes]\n\n*1* An excerpt from 'git diff v2.14.0 v2.15.0 builtin/fast-export.c'\n reads like so:\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex d412c0a8f3..2fb60d6d48 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n...\n@@ -630,15 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len)\n \treturn strbuf_detach(&out, len);\n }\n \n-static void handle_tail(struct object_array *commits, struct rev_info *revs)\n+static void handle_tail(struct object_array *commits, struct rev_info *revs,\n+\t\t\tstruct string_list *paths_of_changed_objects)\n {\n \tstruct commit *commit;\n \twhile (commits->nr) {\n-\t\tcommit = (struct commit *)commits->objects[commits->nr - 1].item;\n+\t\tcommit = (struct commit *)object_array_pop(commits);\n \t\tif (has_unshown_parent(commit))\n \t\t\treturn;\n-\t\thandle_commit(commit, revs);\n-\t\tcommits->nr--;\n+\t\thandle_commit(commit, revs, paths_of_changed_objects);\n \t}\n }\n"},{"id":"345186","messageId":"CAN0heSo9bdvNNzwX5-7nHXxg9_oZrsDixzwQmx6gnEtwny2NOA@mail.gmail.com","threadId":"48329","inReplyTo":"xmqq604mertl.fsf@gitster-ct.c.googlers.com","subject":"Re: [BUG] Git fast-export with import marks file omits merge commits","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-04-20T05:07:57Z","receivedAt":"2018-04-20T05:08:01Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 20 April 2018 at 00:48, Junio C Hamano wrote:\n> Isaac Chou writes:\n>\n>> I inspected the source code (builtin/fast-export.c) for the\n>> fast-export issue I encountered, and it looks like the merge\n>> commit is discarded too early by the call to object_array_pop()\n>> after only one of the two UNSHOWN parents is processed in the\n>> method handle_tail(). The poped merge commit still has one\n>> UNSHOWN parent, therefore it is not processed and is lost in the\n>> output. Can someone advise me on how to submit a code change or\n>> bug report in order to get the fix into the code base?\n>\n> There indeed are some differences between v2.14 and v2.15 around the\n> code that returns early when has_unshown_parent() says \"yes\" [*1*],\n> but the decision to return early when the function says \"yes\" hasn't\n> changed between that timeperiod---it dates back to f2dc849e (\"Add\n> 'git fast-export', the sister of 'git fast-import'\", 2007-12-02),\n> i.e. the very beginning of the program's life.\n>\n> I'll CC those who wrote the original and b3e8ca89 (\"fast-export: do\n> not copy from modified file\", 2017-09-20) and 71992039\n> (\"object_array: add and use `object_array_pop()`\", 2017-09-23),\n> which are the only two commits that touch the surrounding area\n> during that timeperiod, to ask if something jumps at them.\n>\n> Thanks.\n>\n>\n> [Footnotes]\n>\n> *1* An excerpt from 'git diff v2.14.0 v2.15.0 builtin/fast-export.c'\n> reads like so:\n>\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index d412c0a8f3..2fb60d6d48 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> ...\n> @@ -630,15 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len)\n> return strbuf_detach(&out, len);\n> }\n>\n> -static void handle_tail(struct object_array *commits, struct rev_info *revs)\n> +static void handle_tail(struct object_array *commits, struct rev_info *revs,\n> + struct string_list *paths_of_changed_objects)\n> {\n> struct commit *commit;\n> while (commits->nr) {\n> - commit = (struct commit *)commits->objects[commits->nr - 1].item;\n> + commit = (struct commit *)object_array_pop(commits);\n> if (has_unshown_parent(commit))\n> return;\n> - handle_commit(commit, revs);\n> - commits->nr--;\n> + handle_commit(commit, revs, paths_of_changed_objects);\n> }\n> }\n\nIndeed. This looks wrong and the guilty person would be me.\n\nIf my 71992039 (\"object_array: add and use `object_array_pop()`\",\n2017-09-23) would instead have done something like\ns/commits->nr--/(void)object_array_pop(commits)/ it would not have\nscrewed up as much. It could also use a peek+pop-pattern.\n\nIsaac, are you up for submitting a patch? Just let me know if you\nencounter any issues. Otherwise, I can submit a patch shortly since I\nwas the one who dropped the ball originally.\n\nThanks for diagnosing this.\n\nMartin\n"},{"id":"345225","messageId":"MW2PR18MB228432C95C18DE786957DE70E5B40@MW2PR18MB2284.namprd18.prod.outlook.com","threadId":"48329","inReplyTo":"CAN0heSo9bdvNNzwX5-7nHXxg9_oZrsDixzwQmx6gnEtwny2NOA@mail.gmail.com","subject":"RE: [BUG] Git fast-export with import marks file omits merge commits","fromName":"Isaac Chou","fromEmail":"isaac.chou@microfocus.com","sentAt":"2018-04-20T13:53:27Z","receivedAt":"2018-04-20T13:53:38Z","isPatch":false,"sender":{"key":"isaac.chou@microfocus.com","avatar":null},"body":"Hi Martin,\n\nNo problem. I was thinking of the peek/pop pattern as well. :) If you don't mind, can you please go ahead and submit a patch for this? Thanks so much.\n\nIsaac\n\n-----Original Message-----\nFrom: Martin Ågren [mailto:martin.agren@gmail.com] \nSent: Friday, April 20, 2018 1:08 AM\nTo: Junio C Hamano \nCc: Isaac Chou ; Johannes Schindelin ; Jonathan Tan ; git@vger.kernel.org\nSubject: Re: [BUG] Git fast-export with import marks file omits merge commits\n\nOn 20 April 2018 at 00:48, Junio C Hamano wrote:\n> Isaac Chou writes:\n>\n>> I inspected the source code (builtin/fast-export.c) for the \n>> fast-export issue I encountered, and it looks like the merge commit \n>> is discarded too early by the call to object_array_pop() after only \n>> one of the two UNSHOWN parents is processed in the method \n>> handle_tail(). The poped merge commit still has one UNSHOWN parent, \n>> therefore it is not processed and is lost in the output. Can someone \n>> advise me on how to submit a code change or bug report in order to \n>> get the fix into the code base?\n>\n> There indeed are some differences between v2.14 and v2.15 around the \n> code that returns early when has_unshown_parent() says \"yes\" [*1*], \n> but the decision to return early when the function says \"yes\" hasn't \n> changed between that timeperiod---it dates back to f2dc849e (\"Add 'git \n> fast-export', the sister of 'git fast-import'\", 2007-12-02), i.e. the \n> very beginning of the program's life.\n>\n> I'll CC those who wrote the original and b3e8ca89 (\"fast-export: do \n> not copy from modified file\", 2017-09-20) and 71992039\n> (\"object_array: add and use `object_array_pop()`\", 2017-09-23), which \n> are the only two commits that touch the surrounding area during that \n> timeperiod, to ask if something jumps at them.\n>\n> Thanks.\n>\n>\n> [Footnotes]\n>\n> *1* An excerpt from 'git diff v2.14.0 v2.15.0 builtin/fast-export.c'\n> reads like so:\n>\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c index \n> d412c0a8f3..2fb60d6d48 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> ...\n> @@ -630,15 +645,15 @@ static void *anonymize_tag(const void *old, size_t *len)\n> return strbuf_detach(&out, len); }\n>\n> -static void handle_tail(struct object_array *commits, struct rev_info \n> *revs)\n> +static void handle_tail(struct object_array *commits, struct rev_info *revs,\n> + struct string_list *paths_of_changed_objects)\n> {\n> struct commit *commit;\n> while (commits->nr) {\n> - commit = (struct commit *)commits->objects[commits->nr - 1].item;\n> + commit = (struct commit *)object_array_pop(commits);\n> if (has_unshown_parent(commit))\n> return;\n> - handle_commit(commit, revs);\n> - commits->nr--;\n> + handle_commit(commit, revs, paths_of_changed_objects);\n> }\n> }\n\nIndeed. This looks wrong and the guilty person would be me.\n\nIf my 71992039 (\"object_array: add and use `object_array_pop()`\",\n2017-09-23) would instead have done something like s/commits->nr--/(void)object_array_pop(commits)/ it would not have screwed up as much. It could also use a peek+pop-pattern.\n\nIsaac, are you up for submitting a patch? Just let me know if you encounter any issues. Otherwise, I can submit a patch shortly since I was the one who dropped the ball originally.\n\nThanks for diagnosing this.\n\nMartin\n"},{"id":"345244","messageId":"20180420181248.2015922-1-martin.agren@gmail.com","threadId":"48329","inReplyTo":"MW2PR18MB228432C95C18DE786957DE70E5B40@MW2PR18MB2284.namprd18.prod.outlook.com","subject":"[PATCH] fast-export: fix regression skipping some merge-commits","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-04-20T18:12:48Z","receivedAt":"2018-04-20T18:13:48Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"7199203937 (object_array: add and use `object_array_pop()`, 2017-09-23)\nnoted that the pattern `object = array.objects[--array.nr].item` could\nbe abstracted as `object = object_array_pop(&array)`.\n\nUnfortunately, one of the conversions was horribly wrong. Between\ngrabbing the last object (i.e., peeking at it) and decreasing the object\ncount, the original code would sometimes return early. The updated code\non the other hand, will always pop the last element, then maybe do the\nearly return before doing anything with the object.\n\nThe end result is that merge commits where all the parents have still\nnot been exported will simply be dropped, meaning that they will be\ncompletely missing from the exported data.\n\nReintroduce the pattern of first grabbing the last object (using a new\nfunction `object_array_peek()`), then later poping it. Using\n`..._peek()` and `..._pop()` makes it clear that we are referring to the\nsame item, i.e., we do not grab one element, then remove another one.\n\nAdd a test that would have caught this.\n\nReported-by: Isaac Chou \nAnalyzed-by: Isaac Chou \nSigned-off-by: Martin Ågren \n---\nBased on maint, but applies equally well on master.\n\nMy sincerest apologies for the stupid train-wreck that the original\nconversion was. Weird interactions between different components can make\nfor fun bugs, but this one is just embarassing.\n\nIsaac, this should solve the problem you are seeing. Unfortunately, I do\nnot have any experience with building Git for Windows [1]. I really hope\nthat this bug did not take up too much of your time. Or eat your data!\n\nMartin\n\n[1] The least I can do is provide a link:\nhttps://github.com/git-for-windows/git/wiki/Building-Git\n\n t/t9350-fast-export.sh | 22 ++++++++++++++++++++++\n object.h | 9 +++++++++\n builtin/fast-export.c | 3 ++-\n 3 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 866ddf6058..2b46a83a49 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -540,4 +540,26 @@ test_expect_success 'when using -C, do not declare copy when source of copy is a\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'todo' '\n+\ttest_create_repo merging &&\n+\tgit -C merging commit --allow-empty -m initial &&\n+\n+\tgit -C merging checkout -b topic &&\n+\t>merging/topic-file &&\n+\tgit -C merging add topic-file &&\n+\tgit -C merging commit -m topic-file &&\n+\n+\tgit -C merging checkout master &&\n+\t>merging/master-file &&\n+\tgit -C merging add master-file &&\n+\tgit -C merging commit -m master-file &&\n+\n+\tgit -C merging merge --no-ff topic -m \"merge the topic\" &&\n+\n+\toid=$(git -C merging rev-parse HEAD^^) &&\n+\techo :1 $oid >merging/git-marks &&\n+\tgit -C merging fast-export --import-marks=git-marks refs/heads/master >out &&\n+\tgrep \"merge the topic\" out\n+'\n+\n test_done\ndiff --git a/object.h b/object.h\nindex f13f85b2a9..4d8ce280d9 100644\n--- a/object.h\n+++ b/object.h\n@@ -129,6 +129,15 @@ void add_object_array_with_path(struct object *obj, const char *name, struct obj\n */\n struct object *object_array_pop(struct object_array *array);\n \n+/*\n+ * Returns NULL if the array is empty. Otherwise, returns the last object.\n+ * That is, the returned value is what `object_array_pop()` would have returned.\n+ */\n+inline struct object *object_array_peek(const struct object_array *array)\n+{\n+\treturn array->nr ? array->objects[array->nr - 1].item : NULL;\n+}\n+\n typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);\n \n /*\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 27b2cc138e..8377d27b46 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -650,9 +650,10 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs,\n {\n \tstruct commit *commit;\n \twhile (commits->nr) {\n-\t\tcommit = (struct commit *)object_array_pop(commits);\n+\t\tcommit = (struct commit *)object_array_peek(commits);\n \t\tif (has_unshown_parent(commit))\n \t\t\treturn;\n+\t\t(void)object_array_pop(commits);\n \t\thandle_commit(commit, revs, paths_of_changed_objects);\n \t}\n }\n-- \n2.17.0\n\n"},{"id":"345249","messageId":"MW2PR18MB22841823133D9C96D4428CDDE5B40@MW2PR18MB2284.namprd18.prod.outlook.com","threadId":"48329","inReplyTo":"20180420181248.2015922-1-martin.agren@gmail.com","subject":"RE: [PATCH] fast-export: fix regression skipping some merge-commits","fromName":"Isaac Chou","fromEmail":"isaac.chou@microfocus.com","sentAt":"2018-04-20T18:57:32Z","receivedAt":"2018-04-20T18:57:38Z","isPatch":true,"sender":{"key":"isaac.chou@microfocus.com","avatar":null},"body":"Hi Martin,\n\nNo problem at all. Thanks for the super quick turnaround. :-)\n\nIsaac\n\n-----Original Message-----\nFrom: Martin Ågren [mailto:martin.agren@gmail.com] \nSent: Friday, April 20, 2018 2:13 PM\nTo: git@vger.kernel.org; Isaac Chou \nCc: Junio C Hamano ; Johannes Schindelin ; Jonathan Tan \nSubject: [PATCH] fast-export: fix regression skipping some merge-commits\n\n7199203937 (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)`.\n\nUnfortunately, 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.\n\nThe 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.\n\nReintroduce 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.\n\nAdd a test that would have caught this.\n\nReported-by: Isaac Chou \nAnalyzed-by: Isaac Chou \nSigned-off-by: Martin Ågren \n---\nBased on maint, but applies equally well on master.\n\nMy 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.\n\nIsaac, 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!\n\nMartin\n\n[1] The least I can do is provide a link:\nhttps://github.com/git-for-windows/git/wiki/Building-Git\n\n t/t9350-fast-export.sh | 22 ++++++++++++++++++++++\n object.h | 9 +++++++++\n builtin/fast-export.c | 3 ++-\n 3 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh index 866ddf6058..2b46a83a49 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -540,4 +540,26 @@ test_expect_success 'when using -C, do not declare copy when source of copy is a\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'todo' '\n+\ttest_create_repo merging &&\n+\tgit -C merging commit --allow-empty -m initial &&\n+\n+\tgit -C merging checkout -b topic &&\n+\t>merging/topic-file &&\n+\tgit -C merging add topic-file &&\n+\tgit -C merging commit -m topic-file &&\n+\n+\tgit -C merging checkout master &&\n+\t>merging/master-file &&\n+\tgit -C merging add master-file &&\n+\tgit -C merging commit -m master-file &&\n+\n+\tgit -C merging merge --no-ff topic -m \"merge the topic\" &&\n+\n+\toid=$(git -C merging rev-parse HEAD^^) &&\n+\techo :1 $oid >merging/git-marks &&\n+\tgit -C merging fast-export --import-marks=git-marks refs/heads/master >out &&\n+\tgrep \"merge the topic\" out\n+'\n+\n test_done\ndiff --git a/object.h b/object.h\nindex f13f85b2a9..4d8ce280d9 100644\n--- a/object.h\n+++ b/object.h\n@@ -129,6 +129,15 @@ void add_object_array_with_path(struct object *obj, const char *name, struct obj\n */\n struct object *object_array_pop(struct object_array *array);\n \n+/*\n+ * Returns NULL if the array is empty. Otherwise, returns the last object.\n+ * That is, the returned value is what `object_array_pop()` would have returned.\n+ */\n+inline struct object *object_array_peek(const struct object_array \n+*array) {\n+\treturn array->nr ? array->objects[array->nr - 1].item : NULL; }\n+\n typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);\n \n /*\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c index 27b2cc138e..8377d27b46 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -650,9 +650,10 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs, {\n \tstruct commit *commit;\n \twhile (commits->nr) {\n-\t\tcommit = (struct commit *)object_array_pop(commits);\n+\t\tcommit = (struct commit *)object_array_peek(commits);\n \t\tif (has_unshown_parent(commit))\n \t\t\treturn;\n+\t\t(void)object_array_pop(commits);\n \t\thandle_commit(commit, revs, paths_of_changed_objects);\n \t}\n }\n--\n2.17.0\n\n"},{"id":"345250","messageId":"nycvar.QRO.7.76.6.1804202041400.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48329","inReplyTo":"20180420181248.2015922-1-martin.agren@gmail.com","subject":"Re: [PATCH] fast-export: fix regression skipping some merge-commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-20T19:07:32Z","receivedAt":"2018-04-20T19:08:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Martin,\n\nOn Fri, 20 Apr 2018, Martin Ågren wrote:\n\n> 7199203937 (object_array: add and use `object_array_pop()`, 2017-09-23)\n> noted that the pattern `object = array.objects[--array.nr].item` could\n> be abstracted as `object = object_array_pop(&array)`.\n> \n> Unfortunately, one of the conversions was horribly wrong. Between\n> grabbing the last object (i.e., peeking at it) and decreasing the object\n> count, the original code would sometimes return early. The updated code\n> on the other hand, will always pop the last element, then maybe do the\n> early return before doing anything with the object.\n> \n> The end result is that merge commits where all the parents have still\n> not been exported will simply be dropped, meaning that they will be\n> completely missing from the exported data.\n\nExcellent explanation.\n\n> Reintroduce the pattern of first grabbing the last object (using a new\n> function `object_array_peek()`), then later poping it. Using\n> `..._peek()` and `..._pop()` makes it clear that we are referring to the\n> same item, i.e., we do not grab one element, then remove another one.\n\nInstead of using _peek() and _pop() and having to reason about the\ncorrectness, maybe we should simply re-push? See below for my suggested\nalternative.\n\n> My sincerest apologies for the stupid train-wreck that the original\n> conversion was. Weird interactions between different components can make\n> for fun bugs, but this one is just embarassing.\n\nThe only way to fail is by doing something. You did something. That is\nmuch better than not doing anything. So please do not be sorry about\nintroducing a breakage. You did fix it, which makes you double awesome.\n\n> Isaac, this should solve the problem you are seeing. Unfortunately, I do\n> not have any experience with building Git for Windows [1]. I really hope\n> that this bug did not take up too much of your time. Or eat your data!\n\nIt is as easy as\n\n\tgit clone --depth=1 https://github.com/git-for-windows/git-sdk-64\n\n(downloading half a gigabyte of objects, but then you have almost\neverything except for the Git source and one support repository for Git\nfor Windows), then starting git-bash.exe in its toplevel directory and\ncalling\n\n\tsdk build git\n\nin there. The `sdk` helper is in its infancy, so I could imagine that a\nreally neat thing would be to be able to build custom branches and bundle\nthem in a portable Git. Something like `sdk build portable-git --patch\nhttps://public-inbox.org/git/20180420181248.2015922-1-martin.agren@gmail.com/`.\n\nIn the meantime, it should still be doable by calling\n\n\tsdk cd git\n\tsdk init build-extra\n\t/usr/src/build-extra/apply-from-public-inbox.sh https://public-inbox.org/git/20180420181248.2015922-1-martin.agren@gmail.com/\n\tmake -j15 && make -j15 strip && make -j15 install\n\tsdk build installer\n\nand then running that installer.\n\nYou could also build a portable Git instead by replacing the last line\nwith\n\n\t/usr/src/build-extra/portable/release.sh 0-test\n\nOr if you want to avoid building a portable Git installer, and instead\ncopy the files directly, you could try this sequence:\n\n\tpacman -S --noconfirm rsync\n\tmkdir ~/my-test-git\n\t(cd / && rsync -Rau $(ARCH=x86_64 BITNESS=64 \\\n\t\tusr/src/build-extra/make-file-list.sh) ~/my-test-git/)\n\nPlease let me know how it is going, I am always eager to make the Git for\nWindows SDK easier to use, as it will ultimately save me time.\n\n> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\n> index 866ddf6058..2b46a83a49 100755\n> --- a/t/t9350-fast-export.sh\n> +++ b/t/t9350-fast-export.sh\n> @@ -540,4 +540,26 @@ test_expect_success 'when using -C, do not declare copy when source of copy is a\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success 'todo' '\n> +\ttest_create_repo merging &&\n> +\tgit -C merging commit --allow-empty -m initial &&\n\nI see that you copied the style of the latest test case, but I have to\nadmit that I would find it much easier to read if it said:\n\n\t(\n\t\tcd merging &&\n\t\ttest_commit initial &&\n\t\tgit checkout -b topic &&\n\t\ttest_commit on-branch &&\n\t\tgit checkout master &&\n\t\ttest_commit on-master &&\n\t\ttest_tick &&\n\t\tgit merge --no-ff -m Yeah topic &&\n\n\t\techo \":1 $(git rev-parse HEAD^^)\" >marks &&\n\t\tgit fast-export --import-marks=marks master >out &&\n\t\tgrep Yeah out\n\t)\n\ni.e. using the subshell where you cd into merging/ first thing, and then\nmaking extensive use of `test_commit`.\n\n> +\n> +\tgit -C merging checkout -b topic &&\n> +\t>merging/topic-file &&\n> +\tgit -C merging add topic-file &&\n> +\tgit -C merging commit -m topic-file &&\n> +\n> +\tgit -C merging checkout master &&\n> +\t>merging/master-file &&\n> +\tgit -C merging add master-file &&\n> +\tgit -C merging commit -m master-file &&\n> +\n> +\tgit -C merging merge --no-ff topic -m \"merge the topic\" &&\n> +\n> +\toid=$(git -C merging rev-parse HEAD^^) &&\n> +\techo :1 $oid >merging/git-marks &&\n> +\tgit -C merging fast-export --import-marks=git-marks refs/heads/master >out &&\n> +\tgrep \"merge the topic\" out\n> +'\n> +\n> test_done\n> diff --git a/object.h b/object.h\n> index f13f85b2a9..4d8ce280d9 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -129,6 +129,15 @@ void add_object_array_with_path(struct object *obj, const char *name, struct obj\n> */\n> struct object *object_array_pop(struct object_array *array);\n> \n> +/*\n> + * Returns NULL if the array is empty. Otherwise, returns the last object.\n> + * That is, the returned value is what `object_array_pop()` would have returned.\n> + */\n> +inline struct object *object_array_peek(const struct object_array *array)\n\nI looked, and this would be the first use of `inline` without `static`...\n\n> +{\n> +\treturn array->nr ? array->objects[array->nr - 1].item : NULL;\n> +}\n> +\n> typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);\n> \n> /*\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 27b2cc138e..8377d27b46 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -650,9 +650,10 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs,\n> {\n> \tstruct commit *commit;\n> \twhile (commits->nr) {\n> -\t\tcommit = (struct commit *)object_array_pop(commits);\n> +\t\tcommit = (struct commit *)object_array_peek(commits);\n> \t\tif (has_unshown_parent(commit))\n> \t\t\treturn;\n> +\t\t(void)object_array_pop(commits);\n> \t\thandle_commit(commit, revs, paths_of_changed_objects);\n> \t}\n> }\n\nAs I stated above, I think we can make this a bit easier to reason about\n(and less easy to break by future additions) if we avoided the _peek()\nfunction altogether, like this:\n\n {\n \tstruct commit *commit;\n \twhile (commits->nr) {\n \t\tcommit = (struct commit *)object_array_pop(commits);\n-\t\tif (has_unshown_parent(commit))\n+\t\tif (has_unshown_parent(commit)) {\n+\t\t\t/* Queue again, to be handled later */\n+\t\t\tadd_object_array(commits, NULL, commit);\n \t\t\treturn;\n+\t\t}\n \t\thandle_commit(commit, revs, paths_of_changed_objects);\n \t}\n }\n\n(I did not test this, and I was honestly surprised that there is no\nobject_array_push() counterpart to _pop() ;-) So this might be all wrong.)\n\nCiao,\nJohannes"},{"id":"345251","messageId":"20180420190829.2433702-1-martin.agren@gmail.com","threadId":"48329","inReplyTo":"MW2PR18MB22841823133D9C96D4428CDDE5B40@MW2PR18MB2284.namprd18.prod.outlook.com","subject":"[PATCH v2] fast-export: fix regression skipping some merge-commits","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-04-20T19:08:29Z","receivedAt":"2018-04-20T19:08:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"7199203937 (object_array: add and use `object_array_pop()`, 2017-09-23)\nnoted that the pattern `object = array.objects[--array.nr].item` could\nbe abstracted as `object = object_array_pop(&array)`.\n\nUnfortunately, one of the conversions was horribly wrong. Between\ngrabbing the last object (i.e., peeking at it) and decreasing the object\ncount, the original code would sometimes return early. The updated code\non the other hand, will always pop the last element, then maybe do the\nearly return before doing anything with the object.\n\nThe end result is that merge commits where all the parents have still\nnot been exported will simply be dropped, meaning that they will be\ncompletely missing from the exported data.\n\nReintroduce the pattern of first grabbing the last object (using a new\nfunction `object_array_peek()`), then later poping it. Using\n`..._peek()` and `..._pop()` makes it clear that we are referring to the\nsame item, i.e., we do not grab one element, then remove another one.\n\nAdd a test that would have caught this.\n\nReported-by: Isaac Chou \nAnalyzed-by: Isaac Chou \nSigned-off-by: Martin Ågren \n---\nHmph. Version 1 described the test as \"todo\". This version uses a better\ndescription. No other changes.\n\n t/t9350-fast-export.sh | 22 ++++++++++++++++++++++\n object.h | 9 +++++++++\n builtin/fast-export.c | 3 ++-\n 3 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 866ddf6058..194782b05b 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -540,4 +540,26 @@ test_expect_success 'when using -C, do not declare copy when source of copy is a\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'merge commit gets exported with --import-marks' '\n+\ttest_create_repo merging &&\n+\tgit -C merging commit --allow-empty -m initial &&\n+\n+\tgit -C merging checkout -b topic &&\n+\t>merging/topic-file &&\n+\tgit -C merging add topic-file &&\n+\tgit -C merging commit -m topic-file &&\n+\n+\tgit -C merging checkout master &&\n+\t>merging/master-file &&\n+\tgit -C merging add master-file &&\n+\tgit -C merging commit -m master-file &&\n+\n+\tgit -C merging merge --no-ff topic -m \"merge the topic\" &&\n+\n+\toid=$(git -C merging rev-parse HEAD^^) &&\n+\techo :1 $oid >merging/git-marks &&\n+\tgit -C merging fast-export --import-marks=git-marks refs/heads/master >out &&\n+\tgrep \"merge the topic\" out\n+'\n+\n test_done\ndiff --git a/object.h b/object.h\nindex f13f85b2a9..4d8ce280d9 100644\n--- a/object.h\n+++ b/object.h\n@@ -129,6 +129,15 @@ void add_object_array_with_path(struct object *obj, const char *name, struct obj\n */\n struct object *object_array_pop(struct object_array *array);\n \n+/*\n+ * Returns NULL if the array is empty. Otherwise, returns the last object.\n+ * That is, the returned value is what `object_array_pop()` would have returned.\n+ */\n+inline struct object *object_array_peek(const struct object_array *array)\n+{\n+\treturn array->nr ? array->objects[array->nr - 1].item : NULL;\n+}\n+\n typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);\n \n /*\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 27b2cc138e..8377d27b46 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -650,9 +650,10 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs,\n {\n \tstruct commit *commit;\n \twhile (commits->nr) {\n-\t\tcommit = (struct commit *)object_array_pop(commits);\n+\t\tcommit = (struct commit *)object_array_peek(commits);\n \t\tif (has_unshown_parent(commit))\n \t\t\treturn;\n+\t\t(void)object_array_pop(commits);\n \t\thandle_commit(commit, revs, paths_of_changed_objects);\n \t}\n }\n-- \n2.17.0\n\n"},{"id":"345255","messageId":"CAN0heSreFmd_0QnTjgYdpOZ8y7iNc3J+dYcpo5oKJBF2ff5-jQ@mail.gmail.com","threadId":"48329","inReplyTo":"nycvar.QRO.7.76.6.1804202041400.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH] fast-export: fix regression skipping some merge-commits","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-04-20T19:32:10Z","receivedAt":"2018-04-20T19:32:14Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Johannes,\n\nOn 20 April 2018 at 21:07, Johannes Schindelin\n wrote:\n> On Fri, 20 Apr 2018, Martin Ågren wrote:\n>\n>> Reintroduce the pattern of first grabbing the last object (using a new\n>> function `object_array_peek()`), then later poping it. Using\n>> `..._peek()` and `..._pop()` makes it clear that we are referring to the\n>> same item, i.e., we do not grab one element, then remove another one.\n>\n> Instead of using _peek() and _pop() and having to reason about the\n> correctness, maybe we should simply re-push? See below for my suggested\n> alternative.\n\n>> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\n>> index 866ddf6058..2b46a83a49 100755\n>> --- a/t/t9350-fast-export.sh\n>> +++ b/t/t9350-fast-export.sh\n>> @@ -540,4 +540,26 @@ test_expect_success 'when using -C, do not declare copy when source of copy is a\n>> test_cmp expected actual\n>> '\n>>\n>> +test_expect_success 'todo' '\n>> + test_create_repo merging &&\n>> + git -C merging commit --allow-empty -m initial &&\n>\n> I see that you copied the style of the latest test case, but I have to\n> admit that I would find it much easier to read if it said:\n>\n> (\n> cd merging &&\n> test_commit initial &&\n> git checkout -b topic &&\n> test_commit on-branch &&\n> git checkout master &&\n> test_commit on-master &&\n> test_tick &&\n> git merge --no-ff -m Yeah topic &&\n>\n> echo \":1 $(git rev-parse HEAD^^)\" >marks &&\n> git fast-export --import-marks=marks master >out &&\n> grep Yeah out\n> )\n\nThis looks much more succinct and much less noisy. I was perhaps too\nfocused on \"subshells are bad\" and less on \"let's make a decent\ntrade-off here\".\n\n>> +/*\n>> + * Returns NULL if the array is empty. Otherwise, returns the last object.\n>> + * That is, the returned value is what `object_array_pop()` would have returned.\n>> + */\n>> +inline struct object *object_array_peek(const struct object_array *array)\n>\n> I looked, and this would be the first use of `inline` without `static`...\n\nHm.\n\n>> +{\n>> + return array->nr ? array->objects[array->nr - 1].item : NULL;\n>> +}\n>> +\n>> typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);\n>>\n>> /*\n>> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n>> index 27b2cc138e..8377d27b46 100644\n>> --- a/builtin/fast-export.c\n>> +++ b/builtin/fast-export.c\n>> @@ -650,9 +650,10 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs,\n>> {\n>> struct commit *commit;\n>> while (commits->nr) {\n>> - commit = (struct commit *)object_array_pop(commits);\n>> + commit = (struct commit *)object_array_peek(commits);\n>> if (has_unshown_parent(commit))\n>> return;\n>> + (void)object_array_pop(commits);\n>> handle_commit(commit, revs, paths_of_changed_objects);\n>> }\n>> }\n>\n> As I stated above, I think we can make this a bit easier to reason about\n> (and less easy to break by future additions) if we avoided the _peek()\n> function altogether, like this:\n>\n> {\n> struct commit *commit;\n> while (commits->nr) {\n> commit = (struct commit *)object_array_pop(commits);\n> - if (has_unshown_parent(commit))\n> + if (has_unshown_parent(commit)) {\n> + /* Queue again, to be handled later */\n> + add_object_array(commits, NULL, commit);\n> return;\n> + }\n> handle_commit(commit, revs, paths_of_changed_objects);\n> }\n> }\n>\n> (I did not test this, and I was honestly surprised that there is no\n> object_array_push() counterpart to _pop() ;-) So this might be all wrong.)\n\nI was initially torn 50-50 between these two approaches, but now that I\nsee it, sure it's more verbose and a bit more \"back and forth\", but most\nlikely it's less error-prone going forwards.\n\nThanks a lot for your comments. I will give this some testing, check\nthat your proposed test fails and succeeds as it should, and so on, then\ntry to wrap this up. Between you cleaning up the test and providing a\ndifferent implementation, there's not much left for me to take credit\nfor. Can I forge your From: and Signed-off-by: on this?\n\nThanks a lot for the review.\n\nMartin\n"},{"id":"345264","messageId":"nycvar.QRO.7.76.6.1804202258071.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48329","inReplyTo":"CAN0heSreFmd_0QnTjgYdpOZ8y7iNc3J+dYcpo5oKJBF2ff5-jQ@mail.gmail.com","subject":"Re: [PATCH] fast-export: fix regression skipping some merge-commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-20T21:00:36Z","receivedAt":"2018-04-20T21:01:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Martin,\n\nOn Fri, 20 Apr 2018, Martin Ågren wrote:\n\n> Thanks a lot for your comments. I will give this some testing, check\n> that your proposed test fails and succeeds as it should, and so on, then\n> try to wrap this up.\n\nThank you so much!\n\n> Between you cleaning up the test and providing a different\n> implementation, there's not much left for me to take credit for. Can I\n> forge your From: and Signed-off-by: on this?\n\nI disagree, all I did was to play a variation of your tune. You are the\ncomposer of this patch, you performed all the hard work (analysis,\nimplementation & testing), and you deserve the credit.\n\nIt would please my ego a bit, of course, if you could add a \"Helped-by:\nDscho\" line... ;-)\n\nCiao,\nDscho"},{"id":"345275","messageId":"20180420221231.4131611-1-martin.agren@gmail.com","threadId":"48329","inReplyTo":"nycvar.QRO.7.76.6.1804202258071.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"[PATCH v3] fast-export: fix regression skipping some merge-commits","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-04-20T22:12:31Z","receivedAt":"2018-04-20T22:13:42Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"7199203937 (object_array: add and use `object_array_pop()`, 2017-09-23)\nnoted that the pattern `object = array.objects[--array.nr].item` could\nbe abstracted as `object = object_array_pop(&array)`.\n\nUnfortunately, one of the conversions was horribly wrong. Between\ngrabbing the last object (i.e., peeking at it) and decreasing the object\ncount, the original code would sometimes return early. The updated code\non the other hand, will always pop the last element, then maybe do the\nearly return without doing anything with the object.\n\nThe end result is that merge commits where all the parents have still\nnot been exported will simply be dropped, meaning that they will be\ncompletely missing from the exported data.\n\nRe-add a commit when it is not yet time to handle it. An alternative\nthat was considered was to peek-then-pop. That carries some risk with it\nsince the peeking and poping need to act on the same object, in a\nconcerted fashion.\n\nAdd a test that would have caught this.\n\nReported-by: Isaac Chou \nAnalyzed-by: Isaac Chou \nHelped-by: Johannes Schindelin \nSigned-off-by: Martin Ågren \n---\nThis v3 is similar in spirit to v1/v2, but with a reworked test and a\ndifferent fix approach, both based on Dscho's suggestions.\n\n>> Between you cleaning up the test and providing a different\n>> implementation, there's not much left for me to take credit for. Can I\n>> forge your From: and Signed-off-by: on this?\n>\n> I disagree, all I did was to play a variation of your tune. You are the\n> composer of this patch, you performed all the hard work (analysis,\n> implementation & testing), and you deserve the credit.\n\nOk.\n\n> It would please my ego a bit, of course, if you could add a \"Helped-by:\n> Dscho\" line... ;-)\n\nThat's a given! Again, thanks for really helpful suggestions.\n\nMartin\n\n t/t9350-fast-export.sh | 18 ++++++++++++++++++\n builtin/fast-export.c | 5 ++++-\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 866ddf6058..c699c88d00 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -540,4 +540,22 @@ test_expect_success 'when using -C, do not declare copy when source of copy is a\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'merge commit gets exported with --import-marks' '\n+\ttest_create_repo merging &&\n+\t(\n+\t\tcd merging &&\n+\t\ttest_commit initial &&\n+\t\tgit checkout -b topic &&\n+\t\ttest_commit on-topic &&\n+\t\tgit checkout master &&\n+\t\ttest_commit on-master &&\n+\t\ttest_tick &&\n+\t\tgit merge --no-ff -m Yeah topic &&\n+\n+\t\techo \":1 $(git rev-parse HEAD^^)\" >marks &&\n+\t\tgit fast-export --import-marks=marks master >out &&\n+\t\tgrep Yeah out\n+\t)\n+'\n+\n test_done\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 27b2cc138e..7b8dfc5af1 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -651,8 +651,11 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs,\n \tstruct commit *commit;\n \twhile (commits->nr) {\n \t\tcommit = (struct commit *)object_array_pop(commits);\n-\t\tif (has_unshown_parent(commit))\n+\t\tif (has_unshown_parent(commit)) {\n+\t\t\t/* Queue again, to be handled later */\n+\t\t\tadd_object_array(&commit->object, NULL, commits);\n \t\t\treturn;\n+\t\t}\n \t\thandle_commit(commit, revs, paths_of_changed_objects);\n \t}\n }\n-- \n2.17.0\n\n"},{"id":"345276","messageId":"CAPig+cSKBAq3h4CAe4phFoG+APDV_qzApJgCpYK6AZrjW-+xxw@mail.gmail.com","threadId":"48329","inReplyTo":"20180420221231.4131611-1-martin.agren@gmail.com","subject":"Re: [PATCH v3] fast-export: fix regression skipping some merge-commits","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-04-20T22:16:05Z","receivedAt":"2018-04-20T22:16:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 20, 2018 at 6:12 PM, Martin Ågren wrote:\n> 7199203937 (object_array: add and use `object_array_pop()`, 2017-09-23)\n> noted that the pattern `object = array.objects[--array.nr].item` could\n> be abstracted as `object = object_array_pop(&array)`.\n>\n> Unfortunately, one of the conversions was horribly wrong. Between\n> grabbing the last object (i.e., peeking at it) and decreasing the object\n> count, the original code would sometimes return early. The updated code\n> on the other hand, will always pop the last element, then maybe do the\n> early return without doing anything with the object.\n>\n> The end result is that merge commits where all the parents have still\n> not been exported will simply be dropped, meaning that they will be\n> completely missing from the exported data.\n>\n> Re-add a commit when it is not yet time to handle it. An alternative\n> that was considered was to peek-then-pop. That carries some risk with it\n> since the peeking and poping need to act on the same object, in a\n\ns/poping/popping/\n\n> concerted fashion.\n>\n> Add a test that would have caught this.\n>\n> Reported-by: Isaac Chou \n> Analyzed-by: Isaac Chou \n> Helped-by: Johannes Schindelin \n> Signed-off-by: Martin Ågren \n"},{"id":"345296","messageId":"xmqqh8o5b4ww.fsf@gitster-ct.c.googlers.com","threadId":"48329","inReplyTo":"20180420221231.4131611-1-martin.agren@gmail.com","subject":"Re: [PATCH v3] fast-export: fix regression skipping some merge-commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-21T03:43:27Z","receivedAt":"2018-04-21T03:44:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> +test_expect_success 'merge commit gets exported with --import-marks' '\n> +\ttest_create_repo merging &&\n> +\t(\n> +\t\tcd merging &&\n> +\t\ttest_commit initial &&\n> +\t\tgit checkout -b topic &&\n> +\t\ttest_commit on-topic &&\n> +\t\tgit checkout master &&\n> +\t\ttest_commit on-master &&\n> +\t\ttest_tick &&\n> +\t\tgit merge --no-ff -m Yeah topic &&\n> +\n> +\t\techo \":1 $(git rev-parse HEAD^^)\" >marks &&\n> +\t\tgit fast-export --import-marks=marks master >out &&\n> +\t\tgrep Yeah out\n> +\t)\n> +'\n\nThis test looks much better than the one in the earlier iteration,\nbut I do not think the updated \"fix\" below is better. It might be\njust aesthetics and I suspect I won't find it as disturbing if we\ncould push with\n\n\tobject_array_push(commits, (struct object *)commit);\n\nor something that is more clearly symmetric to object_array_pop().\nThe \"Queue again\" comment is needed only because use of \"add\"\nhighlights the lack of symmetry.\n\nWith add_object_array(), it looks somewhat more odd than your\nprevious\n\n\tpeek it to check;\n\tif (it should not be molested)\n\t\treturn;\n\tpop to mark it consumed;\n\tconsume it;\n\nsequence, in which peek() and pop() were more obviously related\noperations on the same \"array\" object.\n\nAnd I do not think it is a good idea to introduce _push() only for\nsymmetry (it would merely be a less capable version of add whose\nname is spelled differently). Hence my preference for peek-check-pop\nover pop-oops-push-again-but-push-spelled-as-add.\n\nNot worth a reroll, though. I just wanted to spread better design\nsense to contributors ;-)\n\n> test_done\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 27b2cc138e..7b8dfc5af1 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -651,8 +651,11 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs,\n> \tstruct commit *commit;\n> \twhile (commits->nr) {\n> \t\tcommit = (struct commit *)object_array_pop(commits);\n> -\t\tif (has_unshown_parent(commit))\n> +\t\tif (has_unshown_parent(commit)) {\n> +\t\t\t/* Queue again, to be handled later */\n> +\t\t\tadd_object_array(&commit->object, NULL, commits);\n> \t\t\treturn;\n> +\t\t}\n> \t\thandle_commit(commit, revs, paths_of_changed_objects);\n> \t}\n> }\n"},{"id":"345314","messageId":"CAN0heSpu0sJkpr1DJCYv_Yu7zsZ2ewO-z_ERYbX29yf3y-Rn9w@mail.gmail.com","threadId":"48329","inReplyTo":"CAPig+cSKBAq3h4CAe4phFoG+APDV_qzApJgCpYK6AZrjW-+xxw@mail.gmail.com","subject":"Re: [PATCH v3] fast-export: fix regression skipping some merge-commits","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-04-21T06:58:33Z","receivedAt":"2018-04-21T06:58:37Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 21 April 2018 at 00:16, Eric Sunshine wrote:\n> On Fri, Apr 20, 2018 at 6:12 PM, Martin Ågren wrote:\n>> Re-add a commit when it is not yet time to handle it. An alternative\n>> that was considered was to peek-then-pop. That carries some risk with it\n>> since the peeking and poping need to act on the same object, in a\n>\n> s/poping/popping/\n\nThanks. I remember looking at that and going \"hmmm\". Apparently I left\nit at that, since I see now that my spell-checker would have complained\nabout it.\n\nMartin\n"},{"id":"345315","messageId":"CAN0heSqQckJT-4mHbwoP_XjbE2-UH8+k2nG6mnN76_t3nzO_xw@mail.gmail.com","threadId":"48329","inReplyTo":"xmqqh8o5b4ww.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] fast-export: fix regression skipping some merge-commits","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-04-21T07:00:23Z","receivedAt":"2018-04-21T07:00:26Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 21 April 2018 at 05:43, Junio C Hamano wrote:\n> but I do not think the updated \"fix\" below is better. It might be\n> just aesthetics and I suspect I won't find it as disturbing if we\n> could push with\n>\n> object_array_push(commits, (struct object *)commit);\n>\n> or something that is more clearly symmetric to object_array_pop().\n> The \"Queue again\" comment is needed only because use of \"add\"\n> highlights the lack of symmetry.\n>\n> With add_object_array(), it looks somewhat more odd than your\n> previous\n>\n> peek it to check;\n> if (it should not be molested)\n> return;\n> pop to mark it consumed;\n> consume it;\n>\n> sequence, in which peek() and pop() were more obviously related\n> operations on the same \"array\" object.\n>\n> And I do not think it is a good idea to introduce _push() only for\n> symmetry (it would merely be a less capable version of add whose\n> name is spelled differently). Hence my preference for peek-check-pop\n> over pop-oops-push-again-but-push-spelled-as-add.\n>\n> Not worth a reroll, though. I just wanted to spread better design\n> sense to contributors ;-)\n\nThanks for your wise words. :-) One thing that just occurred to me is\nthat if the original site where we `add_object_array()` all objects\nstarts adding a non-NULL `name` for some reason, then we need to\nremember to do the same with this new caller. I suspect that at that\ntime, at the latest, we will be switching to peek-check-pop.\n\nThanks for sharing your thoughts.\n\nMartin\n"},{"id":"349015","messageId":"MW2PR18MB22845EE9631BB6586BA4E8E8E5620@MW2PR18MB2284.namprd18.prod.outlook.com","threadId":"48329","inReplyTo":"CAN0heSqQckJT-4mHbwoP_XjbE2-UH8+k2nG6mnN76_t3nzO_xw@mail.gmail.com","subject":"RE: [PATCH v3] fast-export: fix regression skipping some merge-commits","fromName":"Isaac Chou","fromEmail":"isaac.chou@microfocus.com","sentAt":"2018-06-01T19:41:33Z","receivedAt":"2018-06-01T19:56:38Z","isPatch":true,"sender":{"key":"isaac.chou@microfocus.com","avatar":null},"body":"Hello, I need help on this topic again. I need to inform our customers what release this issue will be addressed in. I checked the 2.17.1 binary release recently and found that the fix is not included. Can someone help me with that information or point me to a document that I can use to determine it myself?\n\nThanks,\n\nIsaac\n\n-----Original Message-----\nFrom: Martin Ågren [mailto:martin.agren@gmail.com] \nSent: Saturday, April 21, 2018 3:00 AM\nTo: Junio C Hamano \nCc: Git Mailing List ; Johannes Schindelin ; Isaac Chou ; Jonathan Tan \nSubject: Re: [PATCH v3] fast-export: fix regression skipping some merge-commits\n\nOn 21 April 2018 at 05:43, Junio C Hamano wrote:\n> but I do not think the updated \"fix\" below is better. It might be \n> just aesthetics and I suspect I won't find it as disturbing if we \n> could push with\n>\n> object_array_push(commits, (struct object *)commit);\n>\n> or something that is more clearly symmetric to object_array_pop().\n> The \"Queue again\" comment is needed only because use of \"add\"\n> highlights the lack of symmetry.\n>\n> With add_object_array(), it looks somewhat more odd than your previous\n>\n> peek it to check;\n> if (it should not be molested)\n> return;\n> pop to mark it consumed;\n> consume it;\n>\n> sequence, in which peek() and pop() were more obviously related \n> operations on the same \"array\" object.\n>\n> And I do not think it is a good idea to introduce _push() only for \n> symmetry (it would merely be a less capable version of add whose name \n> is spelled differently). Hence my preference for peek-check-pop over \n> pop-oops-push-again-but-push-spelled-as-add.\n>\n> Not worth a reroll, though. I just wanted to spread better design \n> sense to contributors ;-)\n\nThanks for your wise words. :-) One thing that just occurred to me is that if the original site where we `add_object_array()` all objects starts adding a non-NULL `name` for some reason, then we need to remember to do the same with this new caller. I suspect that at that time, at the latest, we will be switching to peek-check-pop.\n\nThanks for sharing your thoughts.\n\nMartin\n"},{"id":"349073","messageId":"CACsJy8DCu116xp9buwm8ujdCF9jOzGgG0jO5zmKufa8uwaBcEQ@mail.gmail.com","threadId":"48329","inReplyTo":"MW2PR18MB22845EE9631BB6586BA4E8E8E5620@MW2PR18MB2284.namprd18.prod.outlook.com","subject":"Re: [PATCH v3] fast-export: fix regression skipping some merge-commits","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-06-02T06:48:46Z","receivedAt":"2018-06-02T06:49:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jun 1, 2018 at 9:41 PM, Isaac Chou wrote:\n> Hello, I need help on this topic again. I need to inform our customers what release this issue will be addressed in. I checked the 2.17.1 binary release recently and found that the fix is not included. Can someone help me with that information or point me to a document that I can use to determine it myself?\n\nIt's in 2.18.0-rc0, so it should be in the next 2.18.0 release\n(unless something horrible happens but very unlikely).\n-- \nDuy\n"}]}