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

[greened@obbligato.org (David A. Greene)]

Junio C Hamano <gitster@pobox.com> writes:

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

Ok, that makes sense.

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

That is a good question.  I originally created the test to see if making
these two commits differently would cause any problems with
git-subtree's split command.  Obviously, I didn't get that far.  :)

So I think it makes sense for me to at least test and see what happens.
I will add another test to this set if it makes a difference.

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

Ah, good catch.  I should add a test for that.

Let me do a re-roll of this since I think you bring up some excellent
points.  Might be a few days due to work obbligations.

                        -David