Re: [PATCH 1/3] t3034: add rename threshold tests
On Tue, Feb 23, 2016 at 6:48 PM, Felipe Gonçalves Assis <felipeg.assis@gmail.com> wrote:
Show 8 quoted lines
> 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...
Show 5 quoted lines
> Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
> ---
> diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh
> @@ -0,0 +1,235 @@
> +test_expect_success 'setup 1/2: basic setup' '
If someone ever adds a third setup test, it's likely that that person will forget to update this title to say "1/3" (ditto regarding "setup 2/2" below). Perhaps just call this "setup repo" and 2/2 "setup thresholds" or something.
Show 26 quoted lines
> + 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?
Show 9 quoted lines
> + tail="HEAD^ -- HEAD master"
> +'
> +
> +test_expect_success 'setup 2/2: threshold array' '
> + git diff --name-status -M01 HEAD^ HEAD >diff-output &&
> + test_debug "cat diff-output" &&
> + test_line_count = 4 diff-output &&
> + grep "R[0-9]\{3\} \([0-3]\)-old \1-new" diff-output \
> + >grep-output &&I'd be slightly concerned about the use of \{3\} with older grep's such as on Solaris, and would just code it as "[0-9][0-9][0-9]" and be done with it, but perhaps it's not worth worrying about until someone complains.
> + test_cmp diff-output grep-output &&
So, this is asserting that you only see "renames" and nothing else. Okay.
Show 8 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.
Show 32 quoted lines
> + if [ 50 -le "$th0" ]; then
> + check_50=check_threshold_0
> + elif [ 50 -le "$th1" ]; then
> + check_50=check_threshold_1
> + elif [ 50 -le "$th2" ]; then
> + check_50=check_threshold_2
> + fi &&
> + th0="$th0%" &&
> + th1="$th1%" &&
> + th2="$th2%" &&
> + th3="$th3%"
> +'
> +
> +test_expect_success 'assumption for tests: rename detection with diff' '
> + git diff --name-status -M$th0 --diff-filter=R HEAD^ HEAD \
> + >diff-output-0 &&
> + git diff --name-status -M$th1 --diff-filter=R HEAD^ HEAD \
> + >diff-output-1 &&
> + git diff --name-status -M$th2 --diff-filter=R HEAD^ HEAD \
> + >diff-output-2 &&
> + git diff --name-status -M100% --diff-filter=R HEAD^ HEAD \
> + >diff-output-3 &&
> + test_line_count = 4 diff-output-0 &&
> + test_line_count = 3 diff-output-1 &&
> + test_line_count = 2 diff-output-2 &&
> + test_line_count = 1 diff-output-3
> +'
> +
> +test_expect_success 'default similarity threshold is 50%' '
> + git read-tree --reset -u HEAD &&
> + test_must_fail git merge-recursive $tail &&
> + $check_50
