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

2015-09-16 Thread Matthieu Moy
Max Kirillov writes: > On Tue, Sep 15, 2015 at 08:50:03AM +0200, Matthieu Moy wrote: >> --- 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; >> >>

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

2015-09-15 Thread Max Kirillov
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

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

2015-09-15 Thread Junio C Hamano
Matthieu Moy writes: > Not just the return type (which is the error status), but also the type > of the result argument indeed. It's not clear to me whether this is > intentional (09f2825 (git-grep: don't use sscanf, 2007-03-12) introduced > it, the commit message

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

2015-09-15 Thread Matthieu Moy
[ Cc-ing Michael Haggerty who wrote the numparse module ] Max Kirillov writes: > On Mon, Sep 14, 2015 at 08:30:54AM +0200, Matthieu Moy wrote: >>> Fix it by changing the last check to trigger earlier, as soon as it >>> becomes bigger than INT_MAX. >> >> What if the value is

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

2015-09-14 Thread Max Kirillov
On Mon, Sep 14, 2015 at 08:30:54AM +0200, Matthieu Moy wrote: >> Fix it by changing the last check to trigger earlier, as soon as it >> becomes bigger than INT_MAX. > > What if the value is actually greater than INT_MAX? The function is > returning an unsigned long (64 bits on 64bits

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

2015-09-14 Thread Matthieu Moy
Max Kirillov writes: > If s == "-1" and CPU is i386, then none of the checks is triggered, including > the last "(unsigned int) ul != ul", because ul == 2**32 - 1, which fits into > "unsigned int". Thanks for noticing and reporting. > Fix it by changing the last check to

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

2015-09-13 Thread Max Kirillov
If s == "-1" and CPU is i386, then none of the checks is triggered, including the last "(unsigned int) ul != ul", because ul == 2**32 - 1, which fits into "unsigned int". Fix it by changing the last check to trigger earlier, as soon as it becomes bigger than INT_MAX. Signed-off-by: Max Kirillov