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

[Jonathan Nieder <jrnieder@xxxxxxxxx>]

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