Re: [PATCH v5 2/7] replace: add test for --graft

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

Christian Couder <christian.couder@gmail.com> writes:

> On Wed, Jul 2, 2014 at 10:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Christian Couder <chriscool@tuxfamily.org> writes:
>>
>>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>> ---
>>>  t/t6050-replace.sh | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
>>> index 68b3cb2..ca45a84 100755
>>> --- a/t/t6050-replace.sh
>>> +++ b/t/t6050-replace.sh
>>> @@ -351,4 +351,16 @@ test_expect_success 'replace ref cleanup' '
>>>       test -z "$(git replace)"
>>>  '
>>>
>>> +test_expect_success '--graft with and without already replaced object' '
>>> +     test $(git log --oneline | wc -l) = 7 &&
>>> +     git replace --graft $HASH5 &&
>>> +     test $(git log --oneline | wc -l) = 3 &&
>>> +     git cat-file -p $HASH5 | test_must_fail grep parent &&
>>
>> Please do not run non-git command under test_must_fail.
>
> Ok, I think I will just use the following then:
>
> test_must_fail git rev-parse $HASH5^1
> ...

See below.

>>> +     test_must_fail git replace --graft $HASH5 $HASH4 $HASH3 &&
>>> +     git replace --force -g $HASH5 $HASH4 $HASH3 &&
>>> +     git cat-file -p $HASH5 | grep "parent $HASH4" &&
>>> +     git cat-file -p $HASH5 | grep "parent $HASH3" &&
>>> +     git replace -d $HASH5
>>> +'
>>> +
>>>  test_done
>>
>> For all these "git cat-file -p $commit | grep ...", I would think
>> you should add "commit_has_parents" helper function to allow a bit
>> more careful testing.  As written, the test will mistake a commit
>> with string "parent $HASHx" anywhere, not in the header part, and
>> the test does not ensure that $HASH{3,4} is the {first,second}
>> parent.
>>
>>         commit_has_parents $HASH5 $HASH4 $HASH3
>>
>> would then may be implemented like:
>>
>>         commit_has_parents () {
>>                 git cat-file commit "$1" >payload &&
>>                 sed -n -e '/^$/q' -e 's/^parent //p' <payload >actual &&
>>                 shift &&
>>                 printf 'parent %s\n' "$@" >expect &&
>>                 test_cmp expect actual
>>         }
>
> I think I'll rather use something like:
>
> test $(git rev-parse $HASH5^1) = "$HASH4" &&
> test $(git rev-parse $HASH5^2) = "$HASH3" &&
> ...

Even in that case, I'd suggest using the same "commit_has_parents"
abstraction, which you will also be using to check the "replaced to
be a new root" case in the earlier part of this test.

In case you do not get what I mean by "in that case", you are saying
that you will now be testing a different thing.  You can test what
your new code produces at the bit level by directly obtaining the
resulting object via "cat-file" and that lets you not to depend on
the rest of the system (i.e. the part that allows you to pretend an
existing object you have a corresponding replace ref for has contents
of a totally different object) working correctly.  Or you can choose
to test the system as a whole (i.e. not just the "git replace" produced
a new object with contents you planned to fabricate, but also by
having a replace ref, you can let the rest of the system use th
contents of that object when asked for the replaced object).

The implementation suggested in my previous message was in line with
the former, because your use of "cat-file" seemed to indicate that
you wanted to avoid depending on the rest of the system to test this
new feature in your new tests.  You seem to be saying that you now
want to take the other approach of testing both at the same time.

I am fine with either approach, but I want to make sure that you are
aware of the distinction.  The last thing I want to see is to flip
the approach you take to test not because "testing as a whole is
better than testing one thing without getting interfered by
potential breakage in other parts" but because "testing as a whole
is easier."

The implementation of commit_has_parents that tests the system as a
whole may look like

        commit=$1 parent_number=1
        shift
        for parent
        do
                test $(git rev-parse --verify "$commit^$parent_number") = "$parent" ||
		return 1
		parent_number=$(( $parent_number + 1 ))
	done
        test_must_fail git rev-parse $commit^$parent_number

and you would still use it like this:

  commit_has_parents $HASH5 ;# must have no parent
  commit_has_parents $HASH5 $HASH4 $HASH3 ;# must have these parents

Thanks.