On Sun, Feb 21, 2016 at 2:55 PM, Felipe Gonçalves Assis <felipeg.assis@xxxxxxxxx> wrote: > On 21 February 2016 at 15:40, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: >> 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. [...] > > 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 Presumably 2/5 also tests --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 Sounds reasonable. >>> +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? It's probably just a minor viewpoint issue, as I interpreted the patch as primarily testing that --find-renames= and --rename-threshold= are aliases, in which case checking that condition would be done first. However, that's effectively moot given the proposed revised patch ordering since this test would be in patch 4/5, and the two above it would belong in 3/5. -- 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