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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 31, 2024, 09:58 UTC
Message-ID: <3c081d3c-3f6f-45ff-b254-09f1cd6b7de5@gmail.com>
In-Reply-To: <CAPSxiM-X3gk4tsVSJ_dFP2EGWX_dvFnqRp0rTYvhPxqYsUeijg@mail.gmail.com>

Hi Usman

On 30/10/2024 16:19, Usman Akinyemi wrote:

Show 28 quoted lines

> On Wed, Oct 30, 2024 at 3:20 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 21/10/2024 13:20, Patrick Steinhardt wrote:
>>> On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
>>>> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
>>> These are a bit curious. As your test demonstrates, we retrieve the
>>> values from the "gitattributes" file. And given that the file tends to be
>>> checked into the repository, you can now basically break somebody elses
>>> commands by having an invalid value in there.
>>>
>>> That makes me think that we likely shouldn't die here. We may print a
>>> warning, but other than that we should likely continue and use the
>>> DEFAULT_CONFLICT_MARKER_SIZE.
>>
>> I think using a warning here is a good idea, we should probably fix the
>> whitespace attributes to do the same. If you have
>>
>>       * whitespace=indent-with-non-tab,tab-in-indent
>>
>> in .gitattributes then "git diff" dies with
>>
>>       fatal: cannot enforce both tab-in-indent and indent-with-non-tab
>>
>> Anyway that's not really related to this series but I thought I'd add it
>> as #leftoverbits for future reference.
>>
>> Thanks for working on this Usman, what is queued in next looks good to me.
> 
> I just checked it. I will be glad to work on it.

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.

> Also, when should someone redirect a warning/failure into a file then
> use test_grep or just used test_must_fail ?

You must use test_must_fail if you expect a git command to fail, if you expect the command to print a warning but exit successfully you should not use test_must_fail. So if you expect a command to fail and print an error or warning then you'd do

     test_must_fail git my failing command 2>err &&
     test_grep "error message" err

test_must_fail checks that the command fails, but reports an error if the command is killed by a signal such as SIGSEV.

Best Wishes

Phillip

Show 12 quoted lines

> Thank you
> Usman Akinyemi
>>
>> Best Wishes
>>
>> Phillip
>>
>>
>>> Patrick
>>>
>>
>