Re: [PATCH 1/3] t3034: add rename threshold tests

From: Felipe Gonçalves Assis <felipeg.assis@gmail.com>
Date: Feb 24, 2016, 01:15 UTC
Message-ID: <CALMa68oN-d1t67QZ8+7KetApbLgFXoZkJV93-QOeviT_JVPGdw@mail.gmail.com>
In-Reply-To: <CAPig+cQ+MF7Nsvp7jASXPtPjNWT_g865ot45Y1Q347Y1iCDeYA@mail.gmail.com>

On 23 February 2016 at 21:50, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 43 quoted lines

> On Tue, Feb 23, 2016 at 6:48 PM, Felipe Gonçalves Assis
> <felipeg.assis@gmail.com> wrote:
>> 10ae752 (merge-recursive: option to specify rename threshold,
>> 2010-09-27) introduced this feature but did not include any tests.
>>
>> The tests use the new option --find-renames, which replaces the then
>> introduced and now deprecated option --rename-threshold.
>>
>> Also update name and description of t3032 for consistency:
>> "merge-recursive options" -> "merge-recursive space options"
>
> A few superficial comments below...
>
>> +       cat <<-\EOF >3-old &&
>> +       33a
>> +       33b
>> +       33c
>> +       33d
>> +       EOF
>> +       sed s/33/22/ <3-old >2-old &&
>> +       sed s/33/11/ <3-old >1-old &&
>> +       sed s/33/00/ <3-old >0-old &&
>> +       git add [0-3]-old &&
>> +       git commit -m base &&
>> +       git rm [0-3]-old &&
>> +       git commit -m delete &&
>> +       git checkout -b rename HEAD^ &&
>> +       cp 3-old 3-new &&
>> +       sed 1,1s/./x/ <2-old >2-new &&
>> +       sed 1,2s/./x/ <1-old >1-new &&
>> +       sed 1,3s/./x/ <0-old >0-new &&
>> +       git add [0-3]-new &&
>> +       git rm [0-3]-old &&
>> +       git commit -m rename &&
>> +       get_expected_stages 0 &&
>> +       get_expected_stages 1 &&
>> +       get_expected_stages 2 &&
>> +       get_expected_stages 3 &&
>> +       check_50="false" &&
>
> Why isn't this assignment done in setup 2/2 where all the other
> assignments to 'check_50' are done?
>

Oh, it might be a minor thing, but I would like to make tests using check_50 fail if setup 2/2 is skipped, instead of succeeding without any actual checks.

Show 13 quoted lines

>> +       th0=$(sed -n "s/R\(...\)        0-old   0-new/\1/p" <diff-output) &&
>> +       th1=$(sed -n "s/R\(...\)        1-old   1-new/\1/p" <diff-output) &&
>> +       th2=$(sed -n "s/R\(...\)        2-old   2-new/\1/p" <diff-output) &&
>> +       th3=$(sed -n "s/R\(...\)        3-old   3-new/\1/p" <diff-output) &&
>> +       test "$th0" -lt "$th1" &&
>> +       test "$th1" -lt "$th2" &&
>> +       test "$th2" -lt "$th3" &&
>> +       test "$th3" = 100 &&
>
> It's very slightly odd to see '=' rather than '-eq' among all these
> other algebraic operators ('-lt', '-le'), but not so odd that I'd
> mention it (unless I just did), and not necessarily worth changing.
>

Here I favoured the stricter test. If the string is not exactly "100", then a lot should be reviewed.

Once more, thanks for the review. Your other comments should be included in the next version of the patches.