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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html