On Aug 25, 2014, at 1:38 PM, Jeff King <peff@xxxxxxxx> wrote:
> 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).
I think it's reasonable that GIT_ALLOC_LIMIT=0 means "no limit", so that
the limit can easily be disabled temporarily.
But I could change the sentinel and handle 0 like:
if (git_parse_ulong(env, &val)) {
if (!val) {
val = SIZE_MAX;
}
}
Maybe we should do this.
>> + 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);
I think you're right. I'll change it.
Steffen--
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