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

[Duy Nguyen]

On Fri, Jun 1, 2018 at 9:41 PM, Isaac Chou  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

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.ag...@gmail.com] 
Sent: Saturday, April 21, 2018 3:00 AM
To: Junio C Hamano 
Cc: Git Mailing List ; Johannes Schindelin 
; Isaac Chou ; Jonathan 
Tan 
Subject: Re: [PATCH v3] fast-export: fix regression skipping some merge-commits

On 21 April 2018 at 05:43, Junio C Hamano  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

[Martin Ågren]

On 21 April 2018 at 05:43, Junio C Hamano  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

[Martin Ågren]

On 21 April 2018 at 00:16, Eric Sunshine  wrote:
> On Fri, Apr 20, 2018 at 6:12 PM, Martin Ågren  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  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(>object, NULL, commits);
>   return;
> + }
>   handle_commit(commit, revs, paths_of_changed_objects);
>   }
>  }

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

[Eric Sunshine]

On Fri, Apr 20, 2018 at 6:12 PM, 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()`.
>
> 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 
> Analyzed-by: Isaac Chou 
> Helped-by: Johannes Schindelin 
> Signed-off-by: Martin Ågren 

[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()`.

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 
Analyzed-by: Isaac Chou 
Helped-by: Johannes Schindelin 
Signed-off-by: Martin Ågren 
---
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(>object, NULL, commits);
return;
+   }
handle_commit(commit, revs, paths_of_changed_objects);
}
 }
-- 
2.17.0