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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 21, 2016, 23:15 UTC
Message-ID: <xmqq37slac4x.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAPig+cQ5h4hjOwO6-xXii6QQP0J8B_bHYUwWniR6HdQN2dB_1A@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 22 quoted lines

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

This is just us not caring enough about illogical input by crazy users who ignore the fact that the rename scores are supposed to be between 0 and max.

We could have added code to reject out-of-range values when we added support for input with '%', but we instead just made sure we clip the result within the range (probably because it was easier).

I personally do not think it would break too many people if we changed the parser to error out, but at the same time, I do not think it is worth the code churn to do so, and it probably is not worth to document the clipping either.

[Footnote]

*1* With the original "give us an integer, and we'll take it as a ratio of that number out of the next power of ten", there was no need to worry about out-of-range values.