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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 3, 2014, 18:45 UTC
Message-ID: <xmqq8uoaffhq.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAP8UFD2L8wV=7dyW6ChA2Y1UddQnLMZ=b4eUvGYGQY65ndLgHA@mail.gmail.com>

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

Show 29 quoted lines

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

Show 33 quoted lines

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