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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 24, 2016, 00:50 UTC
Message-ID: <CAPig+cQ+MF7Nsvp7jASXPtPjNWT_g865ot45Y1Q347Y1iCDeYA@mail.gmail.com>
In-Reply-To: <1456271292-4652-2-git-send-email-felipegassis@gmail.com>

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