Re: [PATCH v6 03/10] replace: add test for --graft

> 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 | 40 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>>
>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
>> index fb07ad2..d80a89e 100755
>> --- a/t/t6050-replace.sh
>> +++ b/t/t6050-replace.sh
>> @@ -18,6 +18,33 @@ add_and_commit_file()
>>      git commit --quiet -m "$_file: $_msg"
>>  }
>>  
>> +commit_buffer_contains_parents()
> 
> SP before (), perhaps?

>> +{
>> +    git cat-file commit "$1" >payload &&
>> +    sed -n -e '/^$/q' -e '/^parent /p' <payload >actual &&
>> +    shift &&
>> +    : >expected &&
>> +    for _parent
>> +    do
>> +	echo "parent $_parent" >>expected || return 1
>> +    done &&
> 
> You can redirect the entire loop to simplify the above 5 lines,
> which would read better, no?
> 
> 	for _parent
>         do
> 		echo "parent $_parent"
> 	done >expect

>> +    test_cmp actual expected
> 
> As "test_cmp" normally runs "diff", it is better to compare expect
> with actual, not the other way around; running the test verbosely,
> i.e. "t6050-replace.sh -v", shows how the actual output differs from
> what is expected when diagnosing breakage and would be more useful
> that way.

> If your goal is to test both the object contents with this code
> *and* the overall system behaviour with the "$child^$parent" below,
> perhaps they should be in the same "commit_has_parents" function,
> without forcing the caller to choose between the two or call both,
> no?