Re: [PATCH 3/5] merge-recursive: test rename threshold option

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 21, 2016, 17:52 UTC
Message-ID: <CAPig+cTpngfi6EF+MbD-fQ1nM_9TTeVZJ_fBviD5=4Q9uN-8aA@mail.gmail.com>
In-Reply-To: <1456067358-19781-4-git-send-email-felipegassis@gmail.com>

On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis <felipeg.assis@gmail.com> wrote:

> merge-recursive: test rename threshold option

Maybe rephrase this as:

    t3034: add rename threshold tests

> Commit 10ae7526bebb505ddddba01f76ec97d5f7b5e0e5 introduced this feature,

In commit messages, it is helpful to provide more context about a commit by quoting its "subject", like this:

    10ae752 (merge-recursive: option to specify rename
    threshold, 2010-09-27) introduced this feature...

> but did not include any tests. This commit fixes this.

"This commit fixes this." is redundant with the subject and could be dropped.

The above are superficial observation; see below for a bit more meaty stuff...

Show 21 quoted lines

> Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
> ---
> diff --git a/t/t3034-merge-recursive-rename-threshold.sh b/t/t3034-merge-recursive-rename-threshold.sh
> @@ -0,0 +1,146 @@
> +#!/bin/sh
> +
> +test_description='merge-recursive rename threshold option
> +
> +Test rename detection by examining rename/delete conflicts.
> +
> +Similarity index:
> +R100 a-old a-new
> +R075 b-old b-new
> +R050 c-old c-new
> +R025 d-old d-new
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +       get_expected_stages () {

There is no particularly good reason to define these shell functions within the 'setup' test, and doing so makes the test itself more difficult to read. More typical would be to define the functions outside any test.

> +               git checkout rename -- $1-new &&
> +               git ls-files --stage $1-new > expected-stages-undetected-$1

Broken &&-chain.

Style: Omit space after redirection operators (here and elsewhere in
the script). <inputfile >outputfile

> +               sed "s/ 0       / 2     /
> +               " < expected-stages-undetected-$1 > expected-stages-detected-$1

This is difficult to read due to the way this is formatted with the newline inside the quoted string. How about formatting it in a more typical fashion, like this?

    sed "s/ 0       / 2     /" <expected-stages-undetected-$1 \
        >expected-stages-detected-$1

Also, broken &&-chain.

Show 16 quoted lines

> +               git read-tree -u --reset HEAD
> +       } &&
> +
> +       rename_detected () {
> +               git ls-files --stage $1-old $1-new > stages-actual-$1 &&
> +               test_cmp expected-stages-detected-$1 stages-actual-$1
> +       } &&
> +
> +       rename_undetected () {
> +               git ls-files --stage $1-old $1-new > stages-actual-$1 &&
> +               test_cmp expected-stages-undetected-$1 stages-actual-$1
> +       } &&
> +
> +       check_common () {
> +               git ls-files --stage > stages-actual &&
> +               test $(wc -l < stages-actual) -eq 4

Perhaps use test_line_count() instead.

Show 12 quoted lines

> +       } &&
> +
> +       check_find_renames_25 () {
> +               check_common &&
> +               rename_detected a &&
> +               rename_detected b &&
> +               rename_detected c &&
> +               rename_detected d
> +       } &&
> +
> +       check_find_renames_50 () {
> +               check_common

Broken &&-chain.

Show 8 quoted lines

> +               rename_detected a &&
> +               rename_detected b &&
> +               rename_detected c &&
> +               rename_undetected d
> +       } &&
> +
> +       check_find_renames_75 () {
> +               check_common

Ditto.

Show 8 quoted lines

> +               rename_detected a &&
> +               rename_detected b &&
> +               rename_undetected c &&
> +               rename_undetected d
> +       } &&
> +
> +       check_find_renames_100 () {
> +               check_common

Ditto.

Show 8 quoted lines

> +               rename_detected a &&
> +               rename_undetected b &&
> +               rename_undetected c &&
> +               rename_undetected d
> +       } &&
> +
> +       check_no_renames () {
> +               check_common

Ditto.

Show 17 quoted lines

> +               rename_undetected a &&
> +               rename_undetected b &&
> +               rename_undetected c &&
> +               rename_undetected d
> +       } &&
> +
> +       cat <<-\EOF > a-old &&
> +       aa1
> +       aa2
> +       aa3
> +       aa4
> +       EOF
> +       sed s/aa/bb/ < a-old > b-old &&
> +       sed s/aa/cc/ < a-old > c-old &&
> +       sed s/aa/dd/ < a-old > d-old &&
> +       git add [a-d]-old &&
> +       test_tick &&

Nothing in these tests depend upon these test_tick() invocations, so having them here distracts the reader by making him wonder if something unusual is going on to require them.

Show 20 quoted lines

> +       git commit -m base &&
> +       git rm [a-d]-old &&
> +       test_tick &&
> +       git commit -m delete &&
> +       git checkout -b rename HEAD^ &&
> +       cp a-old a-new &&
> +       sed 1,1s/./x/ < b-old > b-new &&
> +       sed 1,2s/./x/ < c-old > c-new &&
> +       sed 1,3s/./x/ < d-old > d-new &&
> +       git add [a-d]-new &&
> +       git rm [a-d]-old &&
> +       test_tick &&
> +       git commit -m rename &&
> +       get_expected_stages a &&
> +       get_expected_stages b &&
> +       get_expected_stages c &&
> +       get_expected_stages d
> +'
> +
> +test_expect_success 'the default similarity index is 50%' '

s/the//

Also, do you mean s/index/threshold/ ?

Show 20 quoted lines

> +       git read-tree --reset -u HEAD &&
> +       test_must_fail git merge-recursive HEAD^ -- HEAD master &&
> +       check_find_renames_50
> +'
> +
> +test_expect_success 'low rename threshold' '
> +       git read-tree --reset -u HEAD &&
> +       test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD master &&
> +       check_find_renames_25
> [...]
> +       check_find_renames_75
> [...]
> +       check_find_renames_100
> +'
> +
> +test_expect_success 'rename threshold is truncated' '
> +       git read-tree --reset -u HEAD &&
> +       test_must_fail git merge-recursive --rename-threshold=200% HEAD^ -- HEAD master &&
> +       check_find_renames_100
> +'

Is this truncation documented behavior or is it just a detail of the current implementation. (Genuine question; I haven't checked the documentation or source.) If just an implementation detail, then it might not be desirable to formalize it via a test.

Show 5 quoted lines

> +test_expect_success 'last wins in --rename-threshold=<m> --rename-threshold=<n>' '
> +       git read-tree --reset -u HEAD &&
> +       test_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&
> +       check_find_renames_75
> +'

Would it make sense to add tests checking that invalid --rename-threshold= arguments, such as negative and non-numbers, correctly error out?

> +test_done