On 21 February 2016 at 15:40, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > 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. > Fair enough. As I said, I ordered the three test commits so that the first one could be applied soon after the commit introducing "rename-thresholds", the second soon after the commit introducing "no-renames" and the third one soon after the fixup for the commit introducing "find-renames" (which would ideally be correct from the start), but then this is probably more aesthetic than practical. I am currently working on the following order, which follows your constraints. 1/5: fix typo (I don't like typos) 2/5: tests involving --find-renames 3/5: tests involving --no-renames 4/5: tests involving --rename-threshold (this represents what would be reverted if the feature was discontinued) 5/5: fix --find-renames + test > More below... > > >> +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. > But it already is the first test involving "rename-threshold". The preceding tests verify the rename detection functionality with the recommended interface. Then we have tests for the deprecated option. This tail is exactly what we would remove if it was discontinued. What did you mean? >> 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