RE: [BUG] Git fast-export with import marks file omits merge commits

RE: [BUG] Git fast-export with import marks file omits merge commits

[Isaac Chou]

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?

Thanks,

Isaac

-----Original Message-----
From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of Isaac Chou
Sent: Monday, April 16, 2018 3:58 PM
To: git@vger.kernel.org
Subject: Git fast-export with import marks file omits merge commits

Hello,

I 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:

0> git --version
git version 2.16.2.windows.1

1> git init
Initialized empty Git repository in c:/./.git/

2> echo 1111 >> file.txt

3> git add file.txt

4> git commit -m "first commit"
[master (root-commit) 711d4d5] first commit
1 file changed, 1 insertion(+)
create mode 100644 file.txt

5> git checkout -b test
Switched to a new branch 'test'

6> echo 2222 >> file.txt

7> git add file.txt

8> git commit -m "commit on test branch"
[test 76d231c] commit on test branch
1 file changed, 1 insertion(+)

9> git checkout master
Switched to branch 'master'

10> echo 3333 >> file.txt

11> git add file.txt

12> git commit -m "commit on master branch"
[master 61c55fd] commit on master branch
1 file changed, 1 insertion(+)

13> git merge test
Auto-merging file.txt
CONFLICT (content): Merge conflict in file.txt Automatic merge failed; fix conflicts and then commit the result.

14> notepad file.txt

15> git add file.txt

16> git commit -m "merged with test branch"
[master 1442e0e] merged with test branch

17> git log
commit 1442e0ee728c831e74550329e39d27d4188b4422 (HEAD -> master)
Merge: 61c55fd 76d231c
Author: isaac <...>
Date:   Mon Apr 16 15:08:39 2018 -0400

    merged with test branch

commit 61c55fdb883fc403e63c91b49bc11bdade62b3e8
Author: isaac <...>
Date:   Mon Apr 16 15:07:41 2018 -0400

    commit on master branch

commit 76d231cdb12eb84f45abdebede06a56f912613d3 (test)
Author: isaac <...>
Date:   Mon Apr 16 15:07:07 2018 -0400

    commit on test branch

commit 711d4d5781df41924421f8629af040e7c91a8d2e
Author: isaac <...>
Date:   Mon Apr 16 15:06:07 2018 -0400

    first commit

18> echo :1 711d4d5781df41924421f8629af040e7c91a8d2e > git-marks

19> cat git-marks
:1 711d4d5781df41924421f8629af040e7c91a8d2e

20> git fast-export --use-done-feature --import-marks=git-marks refs/heads/master --
feature done
blob
mark :2
data 12
1111
2222

commit refs/heads/master
mark :3
author isaac <...> 1523905627 -0400
committer isaac <...> 1523905627 -0400
data 22
commit on test branch
from :1
M 100644 :2 file.txt

blob
mark :4
data 12
1111
3333

commit refs/heads/master
mark :5
author isaac <...> 1523905661 -0400
committer isaac <...> 1523905661 -0400
data 24
commit on master branch
from :1
M 100644 :4 file.txt

done

Thanks,

Isaac

Re: [BUG] Git fast-export with import marks file omits merge commits

[Elijah Newren]

Hi Isaac,

On Thu, Apr 19, 2018 at 2:46 PM, Isaac Chou <Isaac.Chou@microfocus.com> wrote:
> 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?

Careful now, fast-export was also the location of my first patch.
It's easy to get addicted to contributing changes to git.  :-)

Inside the git.git repository, there are two files that explain the
basic process -- Documentation/SubmittingPatches and
Documentation/CodingGuidelines.  If they don't cover the process well,
that's probably a bug itself, but feel free to ask on the list if you
still run into questions.  Those documents can be slighly overwhelming
at first glance, but they've got good information.

Elijah

Re: [BUG] Git fast-export with import marks file omits merge commits

[Junio C Hamano]

Isaac Chou <Isaac.Chou@microfocus.com> writes:

> 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?
>
> Thanks,
>
> Isaac
>
> -----Original Message-----
> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of Isaac Chou
> Sent: Monday, April 16, 2018 3:58 PM
> To: git@vger.kernel.org
> Subject: Git fast-export with import marks file omits merge commits
>
> Hello,
>
> I 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.
> ...

There indeed are some differences between v2.14 and v2.15 around the
code that returns early when has_unshown_parent() says "yes" [*1*],
but the decision to return early when the function says "yes" hasn't
changed between that timeperiod---it dates back to f2dc849e ("Add
'git fast-export', the sister of 'git fast-import'", 2007-12-02),
i.e. the very beginning of the program's life.

I'll CC those who wrote the original and b3e8ca89 ("fast-export: do
not copy from modified file", 2017-09-20) and 71992039
("object_array: add and use `object_array_pop()`", 2017-09-23),
which are the only two commits that touch the surrounding area
during that timeperiod, to ask if something jumps at them.

Thanks.


[Footnotes]

*1* An excerpt from 'git diff v2.14.0 v2.15.0 builtin/fast-export.c'
    reads like so:

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d412c0a8f3..2fb60d6d48 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
...
@@ -630,15 +645,15 @@ 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)
+static void handle_tail(struct object_array *commits, struct rev_info *revs,
+			struct string_list *paths_of_changed_objects)
 {
 	struct commit *commit;
 	while (commits->nr) {
-		commit = (struct commit *)commits->objects[commits->nr - 1].item;
+		commit = (struct commit *)object_array_pop(commits);
 		if (has_unshown_parent(commit))
 			return;
-		handle_commit(commit, revs);
-		commits->nr--;
+		handle_commit(commit, revs, paths_of_changed_objects);
 	}
 }

Re: [BUG] Git fast-export with import marks file omits merge commits

[Martin Ågren]

On 20 April 2018 at 00:48, Junio C Hamano <gitster@pobox.com> wrote:
> Isaac Chou <Isaac.Chou@microfocus.com> writes:
>
>> 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?
>
> There indeed are some differences between v2.14 and v2.15 around the
> code that returns early when has_unshown_parent() says "yes" [*1*],
> but the decision to return early when the function says "yes" hasn't
> changed between that timeperiod---it dates back to f2dc849e ("Add
> 'git fast-export', the sister of 'git fast-import'", 2007-12-02),
> i.e. the very beginning of the program's life.
>
> I'll CC those who wrote the original and b3e8ca89 ("fast-export: do
> not copy from modified file", 2017-09-20) and 71992039
> ("object_array: add and use `object_array_pop()`", 2017-09-23),
> which are the only two commits that touch the surrounding area
> during that timeperiod, to ask if something jumps at them.
>
> Thanks.
>
>
> [Footnotes]
>
> *1* An excerpt from 'git diff v2.14.0 v2.15.0 builtin/fast-export.c'
>     reads like so:
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index d412c0a8f3..2fb60d6d48 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> ...
> @@ -630,15 +645,15 @@ 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)
> +static void handle_tail(struct object_array *commits, struct rev_info *revs,
> +                       struct string_list *paths_of_changed_objects)
>  {
>         struct commit *commit;
>         while (commits->nr) {
> -               commit = (struct commit *)commits->objects[commits->nr - 1].item;
> +               commit = (struct commit *)object_array_pop(commits);
>                 if (has_unshown_parent(commit))
>                         return;
> -               handle_commit(commit, revs);
> -               commits->nr--;
> +               handle_commit(commit, revs, paths_of_changed_objects);
>         }
>  }

Indeed. This looks wrong and the guilty person would be me.

If my 71992039 ("object_array: add and use `object_array_pop()`",
2017-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.

Isaac, 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.

Thanks for diagnosing this.

Martin

RE: [BUG] Git fast-export with import marks file omits merge commits

[Isaac Chou]

Hi Martin,

No 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.

Isaac

-----Original Message-----
From: Martin Ågren [mailto:martin.agren@gmail.com] 
Sent: Friday, April 20, 2018 1:08 AM
To: Junio C Hamano <gitster@pobox.com>
Cc: Isaac Chou <Isaac.Chou@microfocus.com>; Johannes Schindelin <johannes.schindelin@gmx.de>; Jonathan Tan <jonathantanmy@google.com>; git@vger.kernel.org
Subject: Re: [BUG] Git fast-export with import marks file omits merge commits

On 20 April 2018 at 00:48, Junio C Hamano <gitster@pobox.com> wrote:
> Isaac Chou <Isaac.Chou@microfocus.com> writes:
>
>> 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?
>
> There indeed are some differences between v2.14 and v2.15 around the 
> code that returns early when has_unshown_parent() says "yes" [*1*], 
> but the decision to return early when the function says "yes" hasn't 
> changed between that timeperiod---it dates back to f2dc849e ("Add 'git 
> fast-export', the sister of 'git fast-import'", 2007-12-02), i.e. the 
> very beginning of the program's life.
>
> I'll CC those who wrote the original and b3e8ca89 ("fast-export: do 
> not copy from modified file", 2017-09-20) and 71992039
> ("object_array: add and use `object_array_pop()`", 2017-09-23), which 
> are the only two commits that touch the surrounding area during that 
> timeperiod, to ask if something jumps at them.
>
> Thanks.
>
>
> [Footnotes]
>
> *1* An excerpt from 'git diff v2.14.0 v2.15.0 builtin/fast-export.c'
>     reads like so:
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 
> d412c0a8f3..2fb60d6d48 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> ...
> @@ -630,15 +645,15 @@ 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)
> +static void handle_tail(struct object_array *commits, struct rev_info *revs,
> +                       struct string_list *paths_of_changed_objects)
>  {
>         struct commit *commit;
>         while (commits->nr) {
> -               commit = (struct commit *)commits->objects[commits->nr - 1].item;
> +               commit = (struct commit *)object_array_pop(commits);
>                 if (has_unshown_parent(commit))
>                         return;
> -               handle_commit(commit, revs);
> -               commits->nr--;
> +               handle_commit(commit, revs, paths_of_changed_objects);
>         }
>  }

Indeed. This looks wrong and the guilty person would be me.

If my 71992039 ("object_array: add and use `object_array_pop()`",
2017-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.

Isaac, 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.

Thanks for diagnosing this.

Martin

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

[Martin Ågren]

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

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

[Isaac Chou]

Hi Martin,

No problem at all.  Thanks for the super quick turnaround.  :-)

Isaac

-----Original Message-----
From: Martin Ågren [mailto:martin.agren@gmail.com] 
Sent: Friday, April 20, 2018 2:13 PM
To: git@vger.kernel.org; Isaac Chou <Isaac.Chou@microfocus.com>
Cc: Junio C Hamano <gitster@pobox.com>; Johannes Schindelin <johannes.schindelin@gmx.de>; Jonathan Tan <jonathantanmy@google.com>
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@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

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

[Martin Ågren]

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 7227 bytes --]

Hi Martin,

On Fri, 20 Apr 2018, Martin Ågren wrote:

> 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.

Excellent explanation.

> 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.

Instead of using _peek() and _pop() and having to reason about the
correctness, maybe we should simply re-push? See below for my suggested
alternative.

> 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.

The only way to fail is by doing something. You did something. That is
much better than not doing anything. So please do not be sorry about
introducing a breakage. You did fix it, which makes you double awesome.

> 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!

It is as easy as

	git clone --depth=1 https://github.com/git-for-windows/git-sdk-64

(downloading half a gigabyte of objects, but then you have almost
everything except for the Git source and one support repository for Git
for Windows), then starting git-bash.exe in its toplevel directory and
calling

	sdk build git

in there. The `sdk` helper is in its infancy, so I could imagine that a
really neat thing would be to be able to build custom branches and bundle
them in a portable Git. Something like `sdk build portable-git --patch
https://public-inbox.org/git/20180420181248.2015922-1-martin.agren@gmail.com/`.

In the meantime, it should still be doable by calling

	sdk cd git
	sdk init build-extra
	/usr/src/build-extra/apply-from-public-inbox.sh https://public-inbox.org/git/20180420181248.2015922-1-martin.agren@gmail.com/
	make -j15 && make -j15 strip && make -j15 install
	sdk build installer

and then running that installer.

You could also build a portable Git instead by replacing the last line
with

	/usr/src/build-extra/portable/release.sh 0-test

Or if you want to avoid building a portable Git installer, and instead
copy the files directly, you could try this sequence:

	pacman -S --noconfirm rsync
	mkdir ~/my-test-git
	(cd / && rsync -Rau $(ARCH=x86_64 BITNESS=64 \
		usr/src/build-extra/make-file-list.sh) ~/my-test-git/)

Please let me know how it is going, I am always eager to make the Git for
Windows SDK easier to use, as it will ultimately save me time.

> 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 &&

I see that you copied the style of the latest test case, but I have to
admit that I would find it much easier to read if it said:

	(
		cd merging &&
		test_commit initial &&
		git checkout -b topic &&
		test_commit on-branch &&
		git checkout master &&
		test_commit on-master &&
		test_tick &&
		git merge --no-ff -m Yeah topic &&

		echo ":1 $(git rev-parse HEAD^^)" >marks &&
		git fast-export --import-marks=marks master >out &&
		grep Yeah out
	)

i.e. using the subshell where you cd into merging/ first thing, and then
making extensive use of `test_commit`.

> +
> +	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)

I looked, and this would be the first use of `inline` without `static`...

> +{
> +	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);
>  	}
>  }

As I stated above, I think we can make this a bit easier to reason about
(and less easy to break by future additions) if we avoided the _peek()
function altogether, like this:

 {
 	struct commit *commit;
 	while (commits->nr) {
 		commit = (struct commit *)object_array_pop(commits);
-		if (has_unshown_parent(commit))
+		if (has_unshown_parent(commit)) {
+			/* Queue again, to be handled later */
+			add_object_array(commits, NULL, commit);
 			return;
+		}
 		handle_commit(commit, revs, paths_of_changed_objects);
 	}
 }

(I did not test this, and I was honestly surprised that there is no
object_array_push() counterpart to _pop() ;-) So this might be all wrong.)

Ciao,
Johannes

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

[Martin Ågren]

Hi Johannes,

On 20 April 2018 at 21:07, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Fri, 20 Apr 2018, Martin Ågren wrote:
>
>> 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.
>
> Instead of using _peek() and _pop() and having to reason about the
> correctness, maybe we should simply re-push? See below for my suggested
> alternative.

>> 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 &&
>
> I see that you copied the style of the latest test case, but I have to
> admit that I would find it much easier to read if it said:
>
>         (
>                 cd merging &&
>                 test_commit initial &&
>                 git checkout -b topic &&
>                 test_commit on-branch &&
>                 git checkout master &&
>                 test_commit on-master &&
>                 test_tick &&
>                 git merge --no-ff -m Yeah topic &&
>
>                 echo ":1 $(git rev-parse HEAD^^)" >marks &&
>                 git fast-export --import-marks=marks master >out &&
>                 grep Yeah out
>         )

This looks much more succinct and much less noisy. I was perhaps too
focused on "subshells are bad" and less on "let's make a decent
trade-off here".

>> +/*
>> + * 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)
>
> I looked, and this would be the first use of `inline` without `static`...

Hm.

>> +{
>> +     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);
>>       }
>>  }
>
> As I stated above, I think we can make this a bit easier to reason about
> (and less easy to break by future additions) if we avoided the _peek()
> function altogether, like this:
>
>  {
>         struct commit *commit;
>         while (commits->nr) {
>                 commit = (struct commit *)object_array_pop(commits);
> -               if (has_unshown_parent(commit))
> +               if (has_unshown_parent(commit)) {
> +                       /* Queue again, to be handled later */
> +                       add_object_array(commits, NULL, commit);
>                         return;
> +               }
>                 handle_commit(commit, revs, paths_of_changed_objects);
>         }
>  }
>
> (I did not test this, and I was honestly surprised that there is no
> object_array_push() counterpart to _pop() ;-) So this might be all wrong.)

I was initially torn 50-50 between these two approaches, but now that I
see it, sure it's more verbose and a bit more "back and forth", but most
likely it's less error-prone going forwards.

Thanks a lot for your comments. I will give this some testing, check
that your proposed test fails and succeeds as it should, and so on, then
try to wrap this up. Between you cleaning up the test and providing a
different implementation, there's not much left for me to take credit
for. Can I forge your From: and Signed-off-by: on this?

Thanks a lot for the review.

Martin

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

Hi Martin,

On Fri, 20 Apr 2018, Martin Ågren wrote:

> Thanks a lot for your comments. I will give this some testing, check
> that your proposed test fails and succeeds as it should, and so on, then
> try to wrap this up.

Thank you so much!

> Between you cleaning up the test and providing a different
> implementation, there's not much left for me to take credit for. Can I
> forge your From: and Signed-off-by: on this?

I disagree, all I did was to play a variation of your tune. You are the
composer of this patch, you performed all the hard work (analysis,
implementation & testing), and you deserve the credit.

It would please my ego a bit, of course, if you could add a "Helped-by:
Dscho" line... ;-)

Ciao,
Dscho

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

[Martin Ågren]

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 without 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.

Re-add a commit when it is not yet time to handle it. An alternative
that was considered was to peek-then-pop. That carries some risk with it
since the peeking and poping need to act on the same object, in a
concerted fashion.

Add a test that would have caught this.

Reported-by: Isaac Chou <Isaac.Chou@microfocus.com>
Analyzed-by: Isaac Chou <Isaac.Chou@microfocus.com>
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
This v3 is similar in spirit to v1/v2, but with a reworked test and a
different fix approach, both based on Dscho's suggestions.

>> Between you cleaning up the test and providing a different
>> implementation, there's not much left for me to take credit for. Can I
>> forge your From: and Signed-off-by: on this?
>
> I disagree, all I did was to play a variation of your tune. You are the
> composer of this patch, you performed all the hard work (analysis,
> implementation & testing), and you deserve the credit.

Ok.

> It would please my ego a bit, of course, if you could add a "Helped-by:
> Dscho" line... ;-)

That's a given! Again, thanks for really helpful suggestions.

Martin

 t/t9350-fast-export.sh | 18 ++++++++++++++++++
 builtin/fast-export.c  |  5 ++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 866ddf6058..c699c88d00 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -540,4 +540,22 @@ 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 &&
+	(
+		cd merging &&
+		test_commit initial &&
+		git checkout -b topic &&
+		test_commit on-topic &&
+		git checkout master &&
+		test_commit on-master &&
+		test_tick &&
+		git merge --no-ff -m Yeah topic &&
+
+		echo ":1 $(git rev-parse HEAD^^)" >marks &&
+		git fast-export --import-marks=marks master >out &&
+		grep Yeah out
+	)
+'
+
 test_done
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 27b2cc138e..7b8dfc5af1 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -651,8 +651,11 @@ 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))
+		if (has_unshown_parent(commit)) {
+			/* Queue again, to be handled later */
+			add_object_array(&commit->object, NULL, commits);
 			return;
+		}
 		handle_commit(commit, revs, paths_of_changed_objects);
 	}
 }
-- 
2.17.0

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

[Eric Sunshine]

On Fri, Apr 20, 2018 at 6:12 PM, Martin Ågren <martin.agren@gmail.com> wrote:
> 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 without 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.
>
> Re-add a commit when it is not yet time to handle it. An alternative
> that was considered was to peek-then-pop. That carries some risk with it
> since the peeking and poping need to act on the same object, in a

s/poping/popping/

> concerted fashion.
>
> Add a test that would have caught this.
>
> Reported-by: Isaac Chou <Isaac.Chou@microfocus.com>
> Analyzed-by: Isaac Chou <Isaac.Chou@microfocus.com>
> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>

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

[Martin Ågren]

On 21 April 2018 at 00:16, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Apr 20, 2018 at 6:12 PM, Martin Ågren <martin.agren@gmail.com> wrote:
>> Re-add a commit when it is not yet time to handle it. An alternative
>> that was considered was to peek-then-pop. That carries some risk with it
>> since the peeking and poping need to act on the same object, in a
>
> s/poping/popping/

Thanks. I remember looking at that and going "hmmm". Apparently I left
it at that, since I see now that my spell-checker would have complained
about it.

Martin

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

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> +test_expect_success 'merge commit gets exported with --import-marks' '
> +	test_create_repo merging &&
> +	(
> +		cd merging &&
> +		test_commit initial &&
> +		git checkout -b topic &&
> +		test_commit on-topic &&
> +		git checkout master &&
> +		test_commit on-master &&
> +		test_tick &&
> +		git merge --no-ff -m Yeah topic &&
> +
> +		echo ":1 $(git rev-parse HEAD^^)" >marks &&
> +		git fast-export --import-marks=marks master >out &&
> +		grep Yeah out
> +	)
> +'

This test looks much better than the one in the earlier iteration,
but I do not think the updated "fix" below is better.  It might be
just aesthetics and I suspect I won't find it as disturbing if we
could push with

	object_array_push(commits, (struct object *)commit);

or something that is more clearly symmetric to object_array_pop().
The "Queue again" comment is needed only because use of "add"
highlights the lack of symmetry.

With add_object_array(), it looks somewhat more odd than your
previous

	peek it to check;
	if (it should not be molested)
		return;
	pop to mark it consumed;
	consume it;

sequence, in which peek() and pop() were more obviously related
operations on the same "array" object.

And I do not think it is a good idea to introduce _push() only for
symmetry (it would merely be a less capable version of add whose
name is spelled differently).  Hence my preference for peek-check-pop
over pop-oops-push-again-but-push-spelled-as-add.

Not worth a reroll, though.  I just wanted to spread better design
sense to contributors ;-)

>  test_done
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 27b2cc138e..7b8dfc5af1 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -651,8 +651,11 @@ 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))
> +		if (has_unshown_parent(commit)) {
> +			/* Queue again, to be handled later */
> +			add_object_array(&commit->object, NULL, commits);
>  			return;
> +		}
>  		handle_commit(commit, revs, paths_of_changed_objects);
>  	}
>  }

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

[Martin Ågren]

On 21 April 2018 at 05:43, Junio C Hamano <gitster@pobox.com> wrote:
> but I do not think the updated "fix" below is better.  It might be
> just aesthetics and I suspect I won't find it as disturbing if we
> could push with
>
>         object_array_push(commits, (struct object *)commit);
>
> or something that is more clearly symmetric to object_array_pop().
> The "Queue again" comment is needed only because use of "add"
> highlights the lack of symmetry.
>
> With add_object_array(), it looks somewhat more odd than your
> previous
>
>         peek it to check;
>         if (it should not be molested)
>                 return;
>         pop to mark it consumed;
>         consume it;
>
> sequence, in which peek() and pop() were more obviously related
> operations on the same "array" object.
>
> And I do not think it is a good idea to introduce _push() only for
> symmetry (it would merely be a less capable version of add whose
> name is spelled differently).  Hence my preference for peek-check-pop
> over pop-oops-push-again-but-push-spelled-as-add.
>
> Not worth a reroll, though.  I just wanted to spread better design
> sense to contributors ;-)

Thanks 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.

Thanks for sharing your thoughts.

Martin

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

[Isaac Chou]

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?

Thanks,

Isaac

-----Original Message-----
From: Martin Ågren [mailto:martin.agren@gmail.com] 
Sent: Saturday, April 21, 2018 3:00 AM
To: Junio C Hamano <gitster@pobox.com>
Cc: Git Mailing List <git@vger.kernel.org>; Johannes Schindelin <johannes.schindelin@gmx.de>; Isaac Chou <Isaac.Chou@microfocus.com>; Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v3] fast-export: fix regression skipping some merge-commits

On 21 April 2018 at 05:43, Junio C Hamano <gitster@pobox.com> wrote:
> but I do not think the updated "fix" below is better.  It might be 
> just aesthetics and I suspect I won't find it as disturbing if we 
> could push with
>
>         object_array_push(commits, (struct object *)commit);
>
> or something that is more clearly symmetric to object_array_pop().
> The "Queue again" comment is needed only because use of "add"
> highlights the lack of symmetry.
>
> With add_object_array(), it looks somewhat more odd than your previous
>
>         peek it to check;
>         if (it should not be molested)
>                 return;
>         pop to mark it consumed;
>         consume it;
>
> sequence, in which peek() and pop() were more obviously related 
> operations on the same "array" object.
>
> And I do not think it is a good idea to introduce _push() only for 
> symmetry (it would merely be a less capable version of add whose name 
> is spelled differently).  Hence my preference for peek-check-pop over 
> pop-oops-push-again-but-push-spelled-as-add.
>
> Not worth a reroll, though.  I just wanted to spread better design 
> sense to contributors ;-)

Thanks 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.

Thanks for sharing your thoughts.

Martin

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

[Duy Nguyen]

On Fri, Jun 1, 2018 at 9:41 PM, Isaac Chou <Isaac.Chou@microfocus.com> wrote:
> 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?

It's in 2.18.0-rc0, so it should be in the next  2.18.0 release
(unless something horrible happens but very unlikely).
-- 
Duy