Re: [PATCH 3/5] merge-recursive: test rename threshold option

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 21, 2016, 20:45 UTC
Message-ID: <CAPig+cQ5h4hjOwO6-xXii6QQP0J8B_bHYUwWniR6HdQN2dB_1A@mail.gmail.com>
In-Reply-To: <CALMa68qhCBJ5MnT7i6UT989OKKzzOF_bTqo38thSPMwq0mbj6g@mail.gmail.com>

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

Show 16 quoted lines

> On 21 February 2016 at 14:52, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
>> <felipeg.assis@gmail.com> wrote:
>>> +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 &&
>>> +       check_find_renames_100
>>> +'
>>
>> Is this truncation documented behavior or is it just a detail of the
>> current implementation. (Genuine question; I haven't checked the
>> documentation or source.) If just an implementation detail, then it
>> might not be desirable to formalize it via a test.
>
> Not documented. I will remove this. If you prefer to have it
> documented and the test added back later, I can do that.

Looking at the code itself and its history, this seems to be a deliberate decision, so the test may be appropriate, however, I defer to Junio's judgment.

Show 17 quoted lines

>>> +test_expect_success 'last wins in --rename-threshold=<m> --rename-threshold=<n>' '
>>> +       git read-tree --reset -u HEAD &&
>>> +       test_must_fail git merge-recursive --rename-threshold=25 --rename-threshold=75 HEAD^ -- HEAD master &&
>>> +       check_find_renames_75
>>> +'
>>
>> Would it make sense to add tests checking that invalid
>> --rename-threshold= arguments, such as negative and non-numbers,
>> correctly error out?
>
> I guess so. Can I ask you for a suggestion on how to check this?
>
> Given that merges here usually fail anyway because of the conflicts,
> what is the best way of checking the effect of an argument rejection?
> 1. Check that the merge fails but the index is not changed
> 2. Check for a specific exit code
> 3. Use another setup so that the merges succeed

The last option would be most straightforward, however, looking at diff.c:parse_rename_score(), I see that it never actually errors out (by returning -1), so you may not (presently) be able to test these cases. It seems (presently) that merge-recursive.c:parse_merge_opt() can only catch a usage error if nothing follows the '='.