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

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