Re: [PATCH 4/5] run-command: prepare child environment before forking

[Jonathan Nieder]

Brandon Williams wrote:

> In order to avoid allocation between 'fork()' and 'exec()' prepare the
> environment to be used in the child process prior to forking.

If using something like posix_spawn(), this would be needed anyway, so
I'll review it.

[...]
> +++ b/run-command.c
[...]
> +static char **prep_childenv(const char *const *deltaenv)
> +{
> +     char **childenv;
> +     int childenv_nr = 0, childenv_alloc = 0;
> +     int i;
> +
> +     for (i = 0; environ[i]; i++)
> +             childenv_nr++;
> +     for (i = 0; deltaenv && deltaenv[i]; i++)
> +             childenv_alloc++;
> +     /* Add one for the NULL termination */
> +     childenv_alloc += childenv_nr + 1;
> +
> +     childenv = xcalloc(childenv_alloc, sizeof(char *));
> +     memcpy(childenv, environ, childenv_nr * sizeof(char *));
> +
> +     /* merge in deltaenv */
> +     for (i = 0; deltaenv && deltaenv[i]; i++)
> +             childenv_nr = do_putenv(childenv, childenv_nr, deltaenv[i]);
> +
> +     return childenv;
> +}

This potentially copies around most of 'environ' several times as it
adjusts for each deltaenv item. Can it be simplified? E.g.

        struct argv_array result = ARGV_ARRAY_INIT;
        struct string_list mods = STRING_LIST_INIT_DUP;
        struct strbuf key = STRBUF_INIT;
        const char **p;

        for (p = cmd_env; *p; p++) {
                const char *equals = strchr(*p, '=');
                if (equals) {
                        strbuf_reset(&key);
                        strbuf_add(&key, *p, equals - *p);
                        string_list_append(&mods, key.buf)->util = *p;
                } else {
                        string_list_append(&mods, *p);
                }
        }
        string_list_sort(&mods);

        for (p = environ; *p; p++) {
                struct string_list_item *item;
                const char *equals = strchr(*p, '=');
                if (!equals)
                        continue;
                strbuf_reset(&key);
                strbuf_add(&key, *p, equals - *p);
                item = string_list_lookup(&mods, key.buf);

                if (!item) /* no change */
                        argv_array_push(&result, *p);
                else if (!item->util) /* unsetenv */
                        ; /* skip */
                else /* setenv */
                        argv_array_push(&result, item->util);
        }

        strbuf_release(&key);
        string_list_clear(&mods);
        return argv_array_detach(&result);

If the string_list API provided a lookup function taking a buffer and
length as argument, the environ loop could be simplified further to
use *p instead of a copy.

Thanks and hope that helps,
Jonathan