Martin Ågren <martin.ag...@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);
>       }
>  }

Reply via email to