Re: [PATCH 5/5] merge-recursive: test more consistent interface
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Feb 21, 2016, 18:40 UTC
- Message-ID
- <CAPig+cSAWjqD7weNLyk4MrAU1Q7+R6z16GndKkByHo2Uc4yWqg@mail.gmail.com>
- In-Reply-To
- <1456067358-19781-6-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 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.)
Show 9 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 > @@ -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...
Show 41 quoted lines
> 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.
Show 17 quoted lines
> +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.
Show 20 quoted lines
> 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