Re: [PATCHv4 1/6] environment: static list of repo-local env vars

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Feb 24, 2010, 19:20 UTC
Message-ID: <cb7bb73a1002241120i4bbbce0bl50460c76db0c6529@mail.gmail.com>
In-Reply-To: <7vhbp64odi.fsf@alter.siamese.dyndns.org>

On Wed, Feb 24, 2010 at 4:58 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 24 quoted lines

> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> +/* Repository-local GIT_* environment variables */
>> +const char *const local_repo_env[] = {
>> +     ALTERNATE_DB_ENVIRONMENT,
>> +     CONFIG_ENVIRONMENT,
>> +     DB_ENVIRONMENT,
>> +     GIT_DIR_ENVIRONMENT,
>> +     GIT_WORK_TREE_ENVIRONMENT,
>> +     GRAFT_ENVIRONMENT,
>> +     INDEX_ENVIRONMENT,
>> +     NO_REPLACE_OBJECTS_ENVIRONMENT,
>> +     NULL
>> +};
>> +
>> +const unsigned int local_repo_env_size = ARRAY_SIZE(local_repo_env);
>
> This does not look very useful for two reasons.
>
>  - It counts the NULL at the tail, so the number is one-off; you cannot
>   say
>
>        for (i = 0; i < local_repo_env_size; i++)
>                do_something_to(local_repo_env[i]);

This is obviously very easy to fix by decrementing the size by 1 in the definition.

Show 6 quoted lines

>  - local_repo_env_size is not a compile time constant, so you cannot do:
>
>        const char *env[local_repo_env_size + 20];
>        memcpy(env, local_repo_env, sizeof(env[0]) * local_repo_env_size);
>        for (i = local_repo_env_size; i < ARRAY_SIZE(env); i++)
>                add_more_to(env + i, ...);

Indeed, I was just discussing this on the other thread. I personally have no objection to using this C99 feature.

I guess #defining local_repo_env_size in cache.h, and keeping it up-to-date when local_repo_env is updated is the best alternative. This can be done in such a way that if it the array needs expansion, forgetting to update its size is catched at compile time, but not for contraction. Of course we don't expect that it will contract. The disadvantage of this is that a change in cache.h requires an almost complete recompile because almost everything depends on it. It's a minor disadvantage, when compared to other approaches (double-walking the list or other stuff).

(Of course, I still wonder what's wrong with C99 VLAs, but anyway).

-- 
Giuseppe "Oblomov" Bilotta