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

[Junio C Hamano <gitster@pobox.com>]

David Greene <greened@obbligato.org> writes:

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

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

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