Re: [PATCH] strtoul_ui: actually report error in case of negative input

[Max Kirillov <max@max630.net>]

On Tue, Sep 15, 2015 at 08:50:03AM +0200, Matthieu Moy wrote:
> I think it would be better to just return a long to avoid needless
> limitations, but changing the argument to "long" would interfer with
> in-flight topics. Not worth the trouble.

Sure.

> 
> One potential issue with your patch is that you're forbidding the
> interval [2^31, 2^32[ which was previously allowed, both on 32 and 64
> bits. I'm not sure whether we have a use for this in the codebase.

As far as I could see it was used only for file modes. Which
does not need that big numbers.

> This alternative patch is rather ugly to, but I think it is less
> limiting and does not have the "large negative wrapped to positive"
> issue:
> 
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -814,6 +814,9 @@ static inline int strtoul_ui(char const *s, int base, unsigned int *result)
>         char *p;
>  
>         errno = 0;
> +       /* negative values would be accepted by strtoul */
> +       if (strchr(s, '-'))
> +               return -1;
>         ul = strtoul(s, &p, base);
>         if (errno || *p || p == s || (unsigned int) ul != ul)
>                 return -1;
> 
> What do you think?

Explicit rejection of '-' is of course useful addition.

I still find "(unsigned int) ul != ul" bad. As far as I
understand it makes no sense for i386. And even for 64-bit
it's too obscure. In form of "(ul & 0xffffffffL) == 0" it
would be more clear. Or just make explicit comparison with
intended limit, like I did.

Well, actually I don't have strong preferences as long as
"make -C t" does not alarm me with things I did not break.
Maybe somebody else will comment more.

-- 
Max