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

From: Felipe Gonçalves Assis <felipeg.assis@gmail.com>
Date: Feb 21, 2016, 18:55 UTC
Message-ID: <CALMa68qhCBJ5MnT7i6UT989OKKzzOF_bTqo38thSPMwq0mbj6g@mail.gmail.com>
In-Reply-To: <CAPig+cTpngfi6EF+MbD-fQ1nM_9TTeVZJ_fBviD5=4Q9uN-8aA@mail.gmail.com>

On 21 February 2016 at 14:52, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 30 quoted lines

> On Sun, Feb 21, 2016 at 10:09 AM, Felipe Gonçalves Assis
> <felipeg.assis@gmail.com> wrote:
>> merge-recursive: test rename threshold option
>
>> +       git read-tree --reset -u HEAD &&
>> +       test_must_fail git merge-recursive HEAD^ -- HEAD master &&
>> +       check_find_renames_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 &&
>> +       check_find_renames_25
>> [...]
>> +       check_find_renames_75
>> [...]
>> +       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 &&
>> +       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.

Show 10 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

Thanks for the detailed review. Already working on the other comments.