On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
<felipeg.assis@xxxxxxxxx> 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...
> Signed-off-by: Felipe Gonçalves Assis <felipegassis@xxxxxxxxx>
> ---
> 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.
> + 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.
> + } &&
> +
> + 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.
> + rename_detected a &&
> + rename_detected b &&
> + rename_detected c &&
> + rename_undetected d
> + } &&
> +
> + check_find_renames_75 () {
> + check_common
Ditto.
> + rename_detected a &&
> + rename_detected b &&
> + rename_undetected c &&
> + rename_undetected d
> + } &&
> +
> + check_find_renames_100 () {
> + check_common
Ditto.
> + rename_detected a &&
> + rename_undetected b &&
> + rename_undetected c &&
> + rename_undetected d
> + } &&
> +
> + check_no_renames () {
> + check_common
Ditto.
> + 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.
> + 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/ ?
> + 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.
> +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
--
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