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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 21, 2018, 03:43 UTC
Message-ID: <xmqqh8o5b4ww.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180420221231.4131611-1-martin.agren@gmail.com>

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

Show 17 quoted lines

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

Show 18 quoted lines

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