On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
<felipeg.assis@xxxxxxxxx> wrote:
> merge-recursive: test more consistent interface
The real meat of this patch (it seems) is that you're adding tests to
verify that --find-renames= and --rename-threshold= are aliases, so it
might make sense for the summary line to state that.
t3034: test that --find-renames= and --rename-threshold= are aliases
> Update basic tests to use the new option find-renames instead of
> rename-threshold. Add tests to verify that rename-threshold=<n> behaves
> as a synonym for find-renames=<n>. Test that find-renames resets
> threshold.
Likewise, the order of these sentences seems wrong. The important bit
should be mentioned first, which is that the one is an alias for the
other.
(In fact, if you take advice given below in the actual patch content,
then this paragraph can probably be dropped altogether since the other
two bits don't really belong in this patch.)
> Signed-off-by: Felipe Gonçalves Assis <felipegassis@xxxxxxxxx>
> ---
> diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh
> @@ -115,25 +115,25 @@ test_expect_success 'the default similarity index is 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 &&
> + test_must_fail git merge-recursive --find-renames=25 HEAD^ -- HEAD master &&
Since you're building this series atop 10ae752 (merge-recursive:
option to specify rename threshold, 2010-09-27) in 'next', the
--find-renames= option already exists, so these tests, which were
added in 3/5, can instead use --find-renames= from the start, thus
making this patch (5/5) much less noisy since this change and several
below will disappear altogether.
Taking the above and review comments from earlier patches into
account, it might make sense to re-order the series as follows:
1/5: add --find-renames & --find-renames= tests (including "last wins")
2/5: add --find-renames= / --rename-threshold= aliases tests
3/5: add --no-renames tests (including "last wins")
4/5: fix --find-renames to reset threshold to default (including test)
5/5: fix merge-strategies.txt typo
The position of the typo fix patch isn't significant; I just
arbitrarily plopped it at the end. Also, the order of patches 2 & 3 is
arbitrary.
More below...
> check_find_renames_25
> '
>
> test_expect_success 'high rename threshold' '
> git read-tree --reset -u HEAD &&
> - test_must_fail git merge-recursive --rename-threshold=75 HEAD^ -- HEAD master &&
> + test_must_fail git merge-recursive --find-renames=75 HEAD^ -- HEAD master &&
> check_find_renames_75
> '
>
> test_expect_success 'exact renames only' '
> git read-tree --reset -u HEAD &&
> - test_must_fail git merge-recursive --rename-threshold=100% HEAD^ -- HEAD master &&
> + test_must_fail git merge-recursive --find-renames=100% HEAD^ -- HEAD master &&
> 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 &&
> + test_must_fail git merge-recursive --find-renames=200% HEAD^ -- HEAD master &&
> check_find_renames_100
> '
>
> @@ -143,12 +143,36 @@ test_expect_success 'disabled rename detection' '
> check_no_renames
> '
>
> -test_expect_success 'last wins in --rename-threshold=<m> --rename-threshold=<n>' '
> +test_expect_success 'last wins in --find-renames=<m> --find-renames=<n>' '
> git read-tree --reset -u HEAD &&
> - test_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&
> + test_must_fail git merge-recursive --find-renames=25 --find-renames=75 HEAD^ -- HEAD master &&
> check_find_renames_75
> '
>
> +test_expect_success '--find-renames resets threshold' '
> + git read-tree --reset -u HEAD &&
> + test_must_fail git merge-recursive --find-renames=25 --find-renames HEAD^ -- HEAD master &&
> + check_find_renames_50
> +'
As mentioned in my review of patch 1/5, this test really ought to be
bundled with that patch.
> +test_expect_success 'last wins in --no-renames --find-renames' '
> + git read-tree --reset -u HEAD &&
> + test_must_fail git merge-recursive --no-renames --find-renames HEAD^ -- HEAD master &&
> + check_find_renames_50
> +'
> +
> +test_expect_success 'last wins in --find-renames --no-renames' '
> + git read-tree --reset -u HEAD &&
> + git merge-recursive --find-renames --no-renames HEAD^ -- HEAD master &&
> + check_no_renames
> +'
> +
> +test_expect_success 'rename-threshold=<n> is a synonym for find-renames=<n>' '
> + git read-tree --reset -u HEAD &&
> + test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD master &&
> + check_find_renames_25
> +'
I rather expected to see this test come first, as all the others are
rather subordinate to it.
> test_expect_success 'last wins in --no-renames --rename-threshold=<n>' '
> git read-tree --reset -u HEAD &&
> test_must_fail git merge-recursive --no-renames --rename-threshold=25 HEAD^ -- HEAD master &&
> @@ -161,4 +185,16 @@ test_expect_success 'last wins in --rename-threshold=<n> --no-renames' '
> check_no_renames
> '
>
> +test_expect_success 'last wins in --rename-threshold=<n> --find-renames' '
> + git read-tree --reset -u HEAD &&
> + test_must_fail git merge-recursive --rename-threshold=25 --find-renames HEAD^ -- HEAD master &&
> + check_find_renames_50
> +'
> +
> +test_expect_success 'last wins in --find-renames --rename-threshold=<n>' '
> + git read-tree --reset -u HEAD &&
> + test_must_fail git merge-recursive --find-renames --rename-threshold=25 HEAD^ -- HEAD master &&
> + check_find_renames_25
> +'
> +
> 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