Re: [PATCH 5/5] merge-recursive: test more consistent interface

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 21, 2016, 20:32 UTC
Message-ID: <CAPig+cRgBCH7-EqEQ8q=kJwSf0nNNWUGRL6PA4Lp0KSE2agLSA@mail.gmail.com>
In-Reply-To: <CALMa68qBtw+MdhDsOFofEDoySuTU5BJ2tQvEKrynQxcXKWkhGw@mail.gmail.com>

On Sun, Feb 21, 2016 at 2:55 PM, Felipe Gonçalves Assis <felipeg.assis@gmail.com> wrote:

Show 19 quoted lines

> On 21 February 2016 at 15:40, Eric Sunshine <sunshine@sunshineco.com> 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.

Show 26 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.
>
> 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.