On Sat, Jan 13, 2018 at 01:49:48PM +0700, Nguyễn Thái Ngọc Duy wrote:
> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index d24d157379..0ab71f14fb 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c
> @@ -56,6 +56,10 @@ int cmd_main(int argc, const char **argv)
>
> if (argc < 3)
> return 1;
> + while (!strcmp(argv[1], "env")) {
> + argv_array_push(&proc.env_array, argv[2]);
> + argv += 2;
> + }
> proc.argv = (const char **)argv + 2;
Probably not super important for a test command, but do we want to make
sure we don't walk past the end of the array in our loop? E.g.:
while (argv[1] && !strcmp(argv[1], "env")) {
if (!argv[2]))
die("env specifier without a value");
argv_array_push(&proc.env_array, argv[2]);
argv += 2;
argc -= 2;
}
/* make sure we still have 2 args left */
if (argc < 3)
return 1;
proc.argv = ...;
> diff --git a/trace.c b/trace.c
> index 7f43da211a..ffa1cf9b91 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -275,6 +275,62 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,
>
> #endif /* HAVE_VARIADIC_MACROS */
>
> +static void add_env(struct strbuf *dst, const char *const *deltaenv)
> +{
> + struct string_list envs = STRING_LIST_INIT_DUP;
> + const char *const *e;
> + int i;
> + int printed_unset = 0;
> +
> + /* Last one wins, see run-command.c:prep_childenv() for context */
> + for (e = deltaenv; e && *e; e++) {
> + struct strbuf key = STRBUF_INIT;
> + char *equals = strchr(*e, '=');
> + if (equals) {
> + strbuf_reset(&key);
> + strbuf_add(&key, *e, equals - *e);
This strbuf_reset() is unnecessary now, since the key strbuf is fresh in
each loop (alternatively, it could be moved out of the loop to avoid
repeated allocations, and move the corresponding release out of the
loop.
> + string_list_insert(&envs, key.buf)->util = equals + 1;
> + } else {
> + string_list_insert(&envs, *e)->util = NULL;
> + }
Note that this is quadratic in the size of deltaenv (inserting into a
sorted list). But it's the same technique that the actual
prep_childenv() uses, so it's probably OK (we simply don't add that
large a deltaenv in practice).
> + /* "unset X Y...;" */
> + for (i = 0; i < envs.nr; i++) {
> + const char *var = envs.items[i].string;
> + const char *val = envs.items[i].util;
> +
> + if (val || !getenv(var))
> + continue;
> +
> + if (!printed_unset) {
> + strbuf_addstr(dst, " unset");
> + printed_unset = 1;
> + }
> + strbuf_addf(dst, " %s", var);
> + }
> + if (printed_unset)
> + strbuf_addch(dst, ';');
Looks good.
> + /* ... followed by "A=B C=D ..." */
> + for (i = 0; i < envs.nr; i++) {
> + const char *var = envs.items[i].string;
> + const char *val = envs.items[i].util;
> + const char *oldval;
> +
> + if (!val)
> + continue;
> +
> + oldval = getenv(var);
> + if (oldval && !strcmp(val, oldval))
> + continue;
> +
> + strbuf_addf(dst, " %s=", var);
> + sq_quote_buf_pretty(dst, val);
> + }
> + string_list_clear(&envs, 0);
> +}
Looks good.
The loops in this one are much easier to follow, IMHO.
-Peff
