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

From: Max Kirillov <max@max630.net>
Date: Sep 16, 2015, 04:20 UTC
Message-ID: <20150916042037.GB7806@wheezy.local>
In-Reply-To: <vpq4miwfa78.fsf@anie.imag.fr>

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.

Show 18 quoted lines

> 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