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

From: Martin Ågren <martin.agren@gmail.com>
Date: Apr 20, 2018, 19:32 UTC
Message-ID: <CAN0heSreFmd_0QnTjgYdpOZ8y7iNc3J+dYcpo5oKJBF2ff5-jQ@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1804202041400.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>

Hi Johannes,

On 20 April 2018 at 21:07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 10 quoted lines

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

Show 29 quoted lines

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

Show 7 quoted lines

>> +/*
>> + * 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.

Show 44 quoted lines

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