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

[Usman Akinyemi <usmanakinyemi202@gmail.com>]

On Thu, Oct 31, 2024 at 9:58 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Usman
>
> On 30/10/2024 16:19, Usman Akinyemi wrote:
> > 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.
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.

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;
    }
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.

what do you think ?

Thank you.
Usman


>
> > 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.
Thanks for the explanation. I understand it well now.
>
> Best Wishes
>
> Phillip
>
> > Thank you
> > Usman Akinyemi
> >>
> >> Best Wishes
> >>
> >> Phillip
> >>
> >>
> >>> Patrick
> >>>
> >>
> >
>