Re: [PATCH v5 2/4] Change GIT_ALLOC_LIMIT check to use git_parse_ulong()

> On Sun, Aug 24, 2014 at 06:07:44PM +0200, Steffen Prohaska wrote:
> 
>> diff --git a/wrapper.c b/wrapper.c
>> index bc1bfb8..69d1c9b 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -11,14 +11,18 @@ static void (*try_to_free_routine)(size_t size) = do_nothing;
>> 
>> static void memory_limit_check(size_t size)
>> {
>> -	static int limit = -1;
>> -	if (limit == -1) {
>> -		const char *env = getenv("GIT_ALLOC_LIMIT");
>> -		limit = env ? atoi(env) * 1024 : 0;
>> +	static size_t limit = SIZE_MAX;
>> +	if (limit == SIZE_MAX) {
> 
> You use SIZE_MAX as the sentinel for "not set", and 0 as the sentinel
> for "no limit". That seems kind of backwards.
> 
> I guess you are inheriting this from the existing code, which lets
> GIT_ALLOC_LIMIT=0 mean "no limit". I'm not sure if we want to keep that
> or not (it would be backwards incompatible to change it, but we are
> already breaking compatibility here by assuming bytes rather than
> kilobytes; I think that's OK because this is not a documented feature,
> or one intended to be used externally).

>> +		const char *var = "GIT_ALLOC_LIMIT";
>> +		unsigned long val = 0;
>> +		const char *env = getenv(var);
>> +		if (env && !git_parse_ulong(env, &val))
>> +			die("Failed to parse %s", var);
>> +		limit = val;
>> 	}
> 
> This and the next patch both look OK to me, but I notice this part is
> largely duplicated between the two. We already have git_env_bool to do a
> similar thing for boolean environment variables. Should we do something
> similar like:
> 
> diff --git a/config.c b/config.c
> index 058505c..11919eb 100644
> --- a/config.c
> +++ b/config.c
> @@ -1122,6 +1122,14 @@ int git_env_bool(const char *k, int def)
> 	return v ? git_config_bool(k, v) : def;
> }
> 
> +unsigned long git_env_ulong(const char *k, unsigned long val)
> +{
> +	const char *v = getenv(k);
> +	if (v && !git_parse_ulong(k, &val))
> +		die("failed to parse %s", k);
> +	return val;
> +}
> +
> int git_config_system(void)
> {
> 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
> 
> It's not a lot of code, but I think the callers end up being much easier
> to read:
> 
>  if (limit == SIZE_MAX)
> 	limit = git_env_ulong("GIT_ALLOC_LIMIT", 0);