On 23 February 2016 at 21:50, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Tue, Feb 23, 2016 at 6:48 PM, Felipe Gonçalves Assis
> <felipeg.assis@xxxxxxxxx> 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.
>> + 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html