Re: [PATCH v5 1/1] contrib/subtree: Add a test for subtree rebase that loses commits

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 19, 2016, 17:41 UTC
Message-ID: <xmqqsi1tbh68.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <3eb25268597083cdb10303e3d5790302e719a803.1453172369.git.greened@obbligato.org>

David Greene <greened@obbligato.org> writes:

Show 19 quoted lines

> From: David A. Greene <greened@obbligato.org>
>
> This test merges an external tree in as a subtree, makes some commits
> on top of it and splits it back out.  In the process the added commits
> are lost or the rebase aborts with an internal error.  The tests are
> marked to expect failure so that we don't forget to fix it.
>
> Signed-off-by: David A. Greene <greened@obbligato.org>
> ---
>
> Notes:
>     Change History:
>     
>     v1 - Initial version
>     v2 - Additional tests and code cleanup
>     v3 - Remove check_equal, mark comments on failure and remove
>          test_debug statements
>     v4 - Send correct v3 test (botched v3)
>     v5 - Fix use of verbose

Thanks, both.  Will queue.

I have a couple of questions and comments, though.

Show 7 quoted lines

> +test_expect_success 'setup' '
> +	test_commit README &&
> +...
> +	tree=$(git write-tree) &&
> +	head=$(git rev-parse HEAD) &&
> +	rev=$(git rev-parse --verify files-master^0) &&
> +	commit=$(git commit-tree -p $head -p $rev -m "Add subproject master" $tree) &&

I think at this point, your index and working tree states match that of $commit. So the next command ...

> +	git reset $commit &&

... made me wonder what its significance was. I think you are doing this solely to move the HEAD pointer to point at $commit, but then it would be much better and more readable to use update-ref, i.e. making this line to:

	git update-ref HEAD $commit &&

instead, as "write-tree && commit-tree && update-ref" is a familiar pattern to reimplement "git commit" using the plumbing. Ending that three-command sequence with "reset" breaks the pattern.

Show 5 quoted lines

> +	(
> +		cd files_subtree &&
> +		test_commit master4
> +	) &&
> +	test_commit files_subtree/master5

I understand that you are creating these two commits both in the top-level repository (the one the history initially created in "files" repository gets merged into), but you are creating them slightly differently. Is that significant? I am not complaining about the style of writing tests, but I am wondering if having these two commits created differently has any effect on the bug you observed, which may be a good starting point for anybody who wants to fix it to start digging from. IOW, would the resulting history different if you did this instead?

	test_commit files_subtree/master4 &&
	test_commit files_subtree/master5

I also notice that files_subtree/master4 does not appear in any of the verification in the three tests that use the history being prepared here, i.e. if master4 is silently dropped while master5 is kept, such a bug won't be caught by them.