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

Commit Message

Usman Akinyemi Oct. 18, 2024, 1:52 p.m. UTC

From: Usman Akinyemi <[e-mail]>

Replaced atoi() with strtol_i() for parsing conflict-marker-size to
improve error handling. Invalid values, such as those containing letters
now trigger a clear error message.
Updated the test to verify invalid input handling.

Signed-off-by: Usman Akinyemi <[e-mail]>
---
 merge-ll.c            | 6 ++++--
 t/t6406-merge-attr.sh | 7 +++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

Comments

Patrick Steinhardt Oct. 21, 2024, 12:20 p.m. UTC | #1

On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <[e-mail]>
> 
> Replaced atoi() with strtol_i() for parsing conflict-marker-size to
> improve error handling. Invalid values, such as those containing letters
> now trigger a clear error message.
> Updated the test to verify invalid input handling.

When starting a new paragraph we typically have an empty line between
the paragraphs. We also tend to write commit messages as if instructing
the code to change. So instead of "Replaced atoi() with..." you'd say
"Replace atoi() with", and instead of "Updated the test...", you'd say
"Update the test ...".

The same applies to your other commits, as well.

> 
> diff --git a/merge-ll.c b/merge-ll.c
> index 8e63071922b..52870226816 100644
> --- a/merge-ll.c
> +++ b/merge-ll.c
> @@ -427,7 +427,8 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
>  	git_check_attr(istate, path, check);
>  	ll_driver_name = check->items[0].value;
>  	if (check->items[1].value) {
> -		marker_size = atoi(check->items[1].value);
> +		if (strtol_i(check->items[1].value, 10, &marker_size))
> +			die("invalid marker-size '%s', expecting an integer", check->items[1].value);
>  		if (marker_size <= 0)
>  			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
>  	}
> @@ -454,7 +455,8 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)
>  		check = attr_check_initl("conflict-marker-size", NULL);
>  	git_check_attr(istate, path, check);
>  	if (check->items[0].value) {
> -		marker_size = atoi(check->items[0].value);
> +		if (strtol_i(check->items[0].value, 10, &marker_size))
> +			die("invalid marker-size '%s', expecting an integer", check->items[0].value);
>  		if (marker_size <= 0)
>  			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
>  	}

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.

Patrick

Usman Akinyemi Oct. 21, 2024, 2:24 p.m. UTC | #2

On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <[e-mail]> wrote:
>
> On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > From: Usman Akinyemi <[e-mail]>
> >
> > Replaced atoi() with strtol_i() for parsing conflict-marker-size to
> > improve error handling. Invalid values, such as those containing letters
> > now trigger a clear error message.
> > Updated the test to verify invalid input handling.
>
> When starting a new paragraph we typically have an empty line between
> the paragraphs. We also tend to write commit messages as if instructing
> the code to change. So instead of "Replaced atoi() with..." you'd say
> "Replace atoi() with", and instead of "Updated the test...", you'd say
> "Update the test ...".
>
> The same applies to your other commits, as well.
>
> >
> > diff --git a/merge-ll.c b/merge-ll.c
> > index 8e63071922b..52870226816 100644
> > --- a/merge-ll.c
> > +++ b/merge-ll.c
> > @@ -427,7 +427,8 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
> >       git_check_attr(istate, path, check);
> >       ll_driver_name = check->items[0].value;
> >       if (check->items[1].value) {
> > -             marker_size = atoi(check->items[1].value);
> > +             if (strtol_i(check->items[1].value, 10, &marker_size))
> > +                     die("invalid marker-size '%s', expecting an integer", check->items[1].value);
> >               if (marker_size <= 0)
> >                       marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
> >       }
> > @@ -454,7 +455,8 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)
> >               check = attr_check_initl("conflict-marker-size", NULL);
> >       git_check_attr(istate, path, check);
> >       if (check->items[0].value) {
> > -             marker_size = atoi(check->items[0].value);
> > +             if (strtol_i(check->items[0].value, 10, &marker_size))
> > +                     die("invalid marker-size '%s', expecting an integer", check->items[0].value);
> >               if (marker_size <= 0)
> >                       marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
> >       }
>
> 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.
>
Ohh, I understand. Philip suggested this. For the warning, will I just
use printf statement or what function to print the statement ?
Also, how do I test the print warning statement ?

Thank you.
Usman Akinyemi.
> Patrick

Taylor Blau Oct. 21, 2024, 4:34 p.m. UTC | #3

On Mon, Oct 21, 2024 at 02:24:38PM +0000, Usman Akinyemi wrote:
> On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <[e-mail]> wrote:
> >
> > On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > From: Usman Akinyemi <[e-mail]>
> > >
> > > Replaced atoi() with strtol_i() for parsing conflict-marker-size to
> > > improve error handling. Invalid values, such as those containing letters
> > > now trigger a clear error message.
> > > Updated the test to verify invalid input handling.
> >
> > When starting a new paragraph we typically have an empty line between
> > the paragraphs. We also tend to write commit messages as if instructing
> > the code to change. So instead of "Replaced atoi() with..." you'd say
> > "Replace atoi() with", and instead of "Updated the test...", you'd say
> > "Update the test ...".
> >
> > The same applies to your other commits, as well.

Thanks for noting, Patrick.

> > 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.
> >
>
> Ohh, I understand. Philip suggested this. For the warning, will I just
> use printf statement or what function to print the statement ?
> Also, how do I test the print warning statement ?

You can use warning() instead of die(), which will also print the
message to stderr. You can redirect stderr to a separate file in your
test, and then grep or test_grep that to ensure that you see the warning
message.

These messages should also be marked for translation (with `_()`), so
the result will look something like:

    if (strtol_i(check->items[0].value, 10, &marker_size))
            warning(_("invalid marker-size '%s', expecting an integer"),
                    check->items[0].value);

Thanks,
Taylor

Usman Akinyemi Oct. 21, 2024, 4:39 p.m. UTC | #4

On Mon, Oct 21, 2024 at 4:34 PM Taylor Blau <[e-mail]> wrote:
>
> On Mon, Oct 21, 2024 at 02:24:38PM +0000, Usman Akinyemi wrote:
> > On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <[e-mail]> wrote:
> > >
> > > On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > From: Usman Akinyemi <[e-mail]>
> > > >
> > > > Replaced atoi() with strtol_i() for parsing conflict-marker-size to
> > > > improve error handling. Invalid values, such as those containing letters
> > > > now trigger a clear error message.
> > > > Updated the test to verify invalid input handling.
> > >
> > > When starting a new paragraph we typically have an empty line between
> > > the paragraphs. We also tend to write commit messages as if instructing
> > > the code to change. So instead of "Replaced atoi() with..." you'd say
> > > "Replace atoi() with", and instead of "Updated the test...", you'd say
> > > "Update the test ...".
> > >
> > > The same applies to your other commits, as well.
>
> Thanks for noting, Patrick.
>
> > > 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.
> > >
> >
> > Ohh, I understand. Philip suggested this. For the warning, will I just
> > use printf statement or what function to print the statement ?
> > Also, how do I test the print warning statement ?
>
> You can use warning() instead of die(), which will also print the
> message to stderr. You can redirect stderr to a separate file in your
> test, and then grep or test_grep that to ensure that you see the warning
> message.
>
> These messages should also be marked for translation (with `_()`), so
> the result will look something like:
>
>     if (strtol_i(check->items[0].value, 10, &marker_size))
>             warning(_("invalid marker-size '%s', expecting an integer"),
>                     check->items[0].value);
>
> Thanks,
> Taylor
Thank you, I will make the changes.

Usman Akinyemi Oct. 21, 2024, 6 p.m. UTC | #5

On Mon, Oct 21, 2024 at 4:34 PM Taylor Blau <[e-mail]> wrote:
>
> On Mon, Oct 21, 2024 at 02:24:38PM +0000, Usman Akinyemi wrote:
> > On Mon, Oct 21, 2024 at 2:01 PM Patrick Steinhardt <[e-mail]> wrote:
> > >
> > > On Fri, Oct 18, 2024 at 01:52:59PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > From: Usman Akinyemi <[e-mail]>
> > > >
> > > > Replaced atoi() with strtol_i() for parsing conflict-marker-size to
> > > > improve error handling. Invalid values, such as those containing letters
> > > > now trigger a clear error message.
> > > > Updated the test to verify invalid input handling.
> > >
> > > When starting a new paragraph we typically have an empty line between
> > > the paragraphs. We also tend to write commit messages as if instructing
> > > the code to change. So instead of "Replaced atoi() with..." you'd say
> > > "Replace atoi() with", and instead of "Updated the test...", you'd say
> > > "Update the test ...".
> > >
> > > The same applies to your other commits, as well.
>
> Thanks for noting, Patrick.
>
> > > 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.
> > >
> >
> > Ohh, I understand. Philip suggested this. For the warning, will I just
> > use printf statement or what function to print the statement ?
> > Also, how do I test the print warning statement ?
>
> You can use warning() instead of die(), which will also print the
> message to stderr. You can redirect stderr to a separate file in your
> test, and then grep or test_grep that to ensure that you see the warning
> message.
>
> These messages should also be marked for translation (with `_()`), so
> the result will look something like:
>
>     if (strtol_i(check->items[0].value, 10, &marker_size))
>             warning(_("invalid marker-size '%s', expecting an integer"),
>                     check->items[0].value);
Hi Taylor, when I try to use this warning(_, I was getting some error
In the editor
warning(_("invalid marker-size '%s', expecting an integer"),
check->items[1].value); Incompatible integer to pointer conversion
passing 'int' to parameter of type 'const char *'
while I tried run make

erge-ll.c: In function ‘ll_merge’:
merge-ll.c:432:33: error: implicit declaration of function ‘_’
[-Wimplicit-function-declaration]
  432 |                         warning(_("invalid marker-size '%s',
expecting an integer"), check->items[1].value);
      |                                 ^
merge-ll.c:432:33: error: passing argument 1 of ‘warning’ makes
pointer from integer without a cast [-Wint-conversion]
  432 |                         warning(_("invalid marker-size '%s',
expecting an integer"), check->items[1].value);
      |
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                 |
      |                                 int
In file included from merge-ll.c:9:
git-compat-util.h:691:26: note: expected ‘const char *’ but argument
is of type ‘int’
  691 | void warning(const char *err, ...) __attribute__((format
(printf, 1, 2)));
      |              ~~~~~~~~~~~~^~~


>
> Thanks,
> Taylor

Taylor Blau Oct. 21, 2024, 7:56 p.m. UTC | #6

On Mon, Oct 21, 2024 at 06:00:55PM +0000, Usman Akinyemi wrote:
> Hi Taylor, when I try to use this warning(_, I was getting some error
> In the editor

Let's see...

> erge-ll.c: In function ‘ll_merge’:
> merge-ll.c:432:33: error: implicit declaration of function ‘_’
> [-Wimplicit-function-declaration]
>   432 |                         warning(_("invalid marker-size '%s',
> expecting an integer"), check->items[1].value);
>       |                                 ^

Your compiler is correctly indicating that the error is that the
function '_' is undefined, likely because this file does not include
"gettext.h", which is what defines that function.

Thanks,
Taylor

Phillip Wood Oct. 30, 2024, 3:20 p.m. UTC | #7

Hi Patrick and Usman

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 <[e-mail]>
> 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.

Best Wishes

Phillip


> Patrick
>

Usman Akinyemi Oct. 30, 2024, 4:19 p.m. UTC | #8

On Wed, Oct 30, 2024 at 3:20 PM Phillip Wood <[e-mail]> wrote:
>
> Hi Patrick and Usman
>
> 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 <[e-mail]>
> > 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.
Hi Philip,

I just checked it. I will be glad to work on it.

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 ?

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

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

Phillip Wood Oct. 31, 2024, 9:58 a.m. UTC | #9

Hi Usman

On 30/10/2024 16:19, Usman Akinyemi wrote:
> On Wed, Oct 30, 2024 at 3:20 PM Phillip Wood <[e-mail]> 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 <[e-mail]>
>>> 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

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

Usman Akinyemi Oct. 31, 2024, 12:21 p.m. UTC | #10

On Thu, Oct 31, 2024 at 9:58 AM Phillip Wood <[e-mail]> wrote:
>
> Hi Usman
>
> On 30/10/2024 16:19, Usman Akinyemi wrote:
> > On Wed, Oct 30, 2024 at 3:20 PM Phillip Wood <[e-mail]> 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 <[e-mail]>
> >>> 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
> >>>
> >>
> >
>

Usman Akinyemi Nov. 6, 2024, 6:05 a.m. UTC | #11

On Thu, Oct 31, 2024 at 12:21 PM Usman Akinyemi
<[e-mail]> wrote:
>
> On Thu, Oct 31, 2024 at 9:58 AM Phillip Wood <[e-mail]> wrote:
> >
> > Hi Usman
> >
> > On 30/10/2024 16:19, Usman Akinyemi wrote:
> > > On Wed, Oct 30, 2024 at 3:20 PM Phillip Wood <[e-mail]> 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 <[e-mail]>
> > >>> 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
Hello,

Bringing attention to this.
>
>
> >
> > > 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
> > >>>
> > >>
> > >
> >

Phillip Wood Nov. 6, 2024, 4:03 p.m. UTC | #12

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 <[e-mail]> 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

Patch

diff --git a/merge-ll.c b/merge-ll.c
index 8e63071922b..52870226816 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -427,7 +427,8 @@  enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
 	git_check_attr(istate, path, check);
 	ll_driver_name = check->items[0].value;
 	if (check->items[1].value) {
-		marker_size = atoi(check->items[1].value);
+		if (strtol_i(check->items[1].value, 10, &marker_size))
+			die("invalid marker-size '%s', expecting an integer", check->items[1].value);
 		if (marker_size <= 0)
 			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 	}
@@ -454,7 +455,8 @@  int ll_merge_marker_size(struct index_state *istate, const char *path)
 		check = attr_check_initl("conflict-marker-size", NULL);
 	git_check_attr(istate, path, check);
 	if (check->items[0].value) {
-		marker_size = atoi(check->items[0].value);
+		if (strtol_i(check->items[0].value, 10, &marker_size))
+			die("invalid marker-size '%s', expecting an integer", check->items[0].value);
 		if (marker_size <= 0)
 			marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 	}
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 9bf95249347..1299b30aeb1 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -118,6 +118,13 @@  test_expect_success 'retry the merge with longer context' '
 	grep "<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<" actual
 '
 
+test_expect_success 'invalid conflict-marker-size 3a' '
+    echo "text conflict-marker-size=3a" >>.gitattributes &&
+    test_must_fail git checkout -m text 2>actual_error &&
+    test_write_lines "fatal: invalid marker-size '\''3a'\'', expecting an integer" >expected &&
+    test_cmp actual_error expected
+'
+
 test_expect_success 'custom merge backend' '
 
 	echo "* merge=union" >.gitattributes &&