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

On 23 February 2016 at 21:50, Eric Sunshine [off-list ref] wrote:

On Tue, Feb 23, 2016 at 6:48 PM, Felipe Gonçalves Assis
[off-list ref] wrote:

quoted

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

quoted

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

quoted

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