Re: [PATCH v2 2/3] merge: replace atoi() with strtol_i() for marker size validation

[phillip.wood123@gmail.com]

Hi Usman

Sorry for the slow response

On 31/10/2024 12:21, Usman Akinyemi wrote:
> On Thu, Oct 31, 2024 at 9:58 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 30/10/2024 16:19, Usman Akinyemi wrote:
>>
>> If you want to work on this that's great, but please don't feel any
>> obligation to do so.
>>
>>> I also noticed that the test used for testing used a different
>>> approach(test_must_fail) compared to the one I wrote which used
>>> test_grep. Should I change the test also ?
>>
>> I'm not sure which test you are looking at but I assume it is using
>> test_must_fail because the command being tested is expected to die. If
>> we change the code to print a warning instead then we'd need to capture
>> stderr and use test_grep or test_cmp. Note that we only want to print a
>> warning when parsing .gitattributes, the other callers of
>> parse_whitespace_rule() still want to die. Also we should decide what
>> value to use when the user provides both - neither indent-with-non-tab
>> or tab-in-indent are on by default so it's not clear exactly what we
>> should do.
> Hi Philip,
> 
> I understand, we will have to pick one if we are to use a warning in this case,
> indent-with-non-tab seems to be a good candidate as it is not excluded
> by default.

I'm not sure I understand I what you mean by "not excluded by default".

 > We can have something like this>
>      if (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB) {
>          warning(_("cannot enforce both tab-in-indent and
> indent-with-non-tab, removing tab-in-indent"));
>          rule &= ~WS_TAB_IN_INDENT;
>      }

That sounds reasonable for the cases where we want to warn rather than die.

> and this for default
> #define WS_DEFAULT_RULE (WS_TRAILING_SPACE | WS_SPACE_BEFORE_TAB |
> WS_INDENT_WITH_NON_TAB | 8)
> 
> or just leave the WS_DEFAULT_RULE as it is and remove WS_TAB_IN_INDENT
> in case both are set.

I don't think we want to change the default rule as it could cause 
problems for users who rely on it.

Best Wishes

Phillip