Re: [PATCH 2/3] config: require at least one digit when parsing numbers

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 25, 2022, 09:54 UTC
Message-ID: <cb034574-8753-d641-c166-ae2c34b1c589@dunelm.org.uk>
In-Reply-To: <xmqqwn8tjazb.fsf@gitster.g>

Hi Junio

On 21/10/2022 19:19, Junio C Hamano wrote:

Show 20 quoted lines

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> @@ -1167,6 +1167,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
>>   		val = strtoimax(value, &end, 0);
>>   		if (errno == ERANGE)
>>   			return 0;
>> +		if (end == value) {
>> +			errno = EINVAL;
>> +			return 0;
>> +		}
> 
> This means well, but doesn't strto*() family of functions silently
> ignore leading blanks, e.g.
> 
>      l = strtol("  432k", &end, 0);
> 	... l == 432, *end = k ...
> 
> If you really want to reject a string with no number before the
> optional unit, end at this point may not match value.  With " k" as
> input, value would point at the space at the beginning, and end
> would point at 'k'.

It only skips the space if it sees a digit, if it does not find anything to convert it sets *end = start. Using peff's trick for testing this patch we can see there is no change in behavior if there is leading whitespace

$ bin-wrappers/git config --int --default " m" some.key fatal: bad numeric config value ' m' for 'some.key': invalid unit

$ git config --int --default " m" some.key fatal: bad numeric config value ' m' for 'some.key': invalid unit

> It does not look _too_ bad if we just let such an empty string
> through and interpreted it as zero.  Is that a problem?  Who are we
> trying to help?

My reasoning was that a single units factor is likely to be the result of some kind of bad edit and defaulting to zero when the user thought they set a large value is not likely to be helpful. Having said that I'm not that wedded to this patch if you feel it would be better to drop it.

Best Wishes

Phillip