git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Cc: git@vger.kernel.org, Stefan Beller <sbeller@google.com>,
	Brandon Williams <bmwill@google.com>,
	Junio C Hamano <gitster@pobox.com>, Johannes Sixt <j6t@kdbg.org>
Subject: Re: [PATCH v4 6/7] trace.c: print env vars in trace_run_command()
Date: Sat, 13 Jan 2018 02:25:24 -0500	[thread overview]
Message-ID: <20180113072524.GB27251@sigill.intra.peff.net> (raw)
In-Reply-To: <20180113064949.6043-7-pclouds@gmail.com>

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

  reply	other threads:[~2018-01-13  7:25 UTC|newest]

Thread overview: 74+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-10 10:48 [PATCH] run-command.c: print env vars when GIT_TRACE is set Nguyễn Thái Ngọc Duy
2018-01-10 18:09 ` Brandon Williams
2018-01-10 19:14   ` Stefan Beller
2018-01-10 19:26     ` Brandon Williams
2018-01-10 19:35       ` Stefan Beller
2018-01-11  9:47 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
2018-01-11 11:25   ` Duy Nguyen
2018-01-11 17:53   ` Brandon Williams
2018-01-11 18:20     ` Stefan Beller
2018-01-11 19:27   ` Junio C Hamano
2018-01-12  9:23     ` Duy Nguyen
2018-01-12  9:56   ` [PATCH v3 0/4] " Nguyễn Thái Ngọc Duy
2018-01-12  9:56     ` [PATCH v3 1/4] trace.c: introduce trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-12 13:05       ` Jeff King
2018-01-12 13:11         ` Jeff King
2018-01-12  9:56     ` [PATCH v3 2/4] trace.c: print program 'git' when child_process.git_cmd is set Nguyễn Thái Ngọc Duy
2018-01-12 13:05       ` Jeff King
2018-01-12  9:56     ` [PATCH v3 3/4] trace.c: print env vars in trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-12 13:13       ` Jeff King
2018-01-12  9:56     ` [PATCH v3 4/4] trace.c: be smart about what env to print " Nguyễn Thái Ngọc Duy
2018-01-12 13:33       ` Jeff King
2018-01-12 18:24         ` Junio C Hamano
2018-01-12 18:45           ` Jeff King
2018-01-12 19:19             ` Junio C Hamano
2018-01-12 19:23               ` Jeff King
2018-01-12 20:28                 ` Brandon Williams
2018-01-12 22:54         ` Junio C Hamano
2018-01-13  4:54           ` Duy Nguyen
2018-01-13  6:47             ` Duy Nguyen
2018-01-12 13:36     ` [PATCH v3 0/4] run-command.c: print env vars when GIT_TRACE is set Jeff King
2018-01-12 13:38       ` [PATCH 5/4] sq_quote_argv: drop maxlen parameter Jeff King
2018-01-12 23:20         ` Junio C Hamano
2018-01-12 13:39       ` [PATCH 6/4] trace: avoid unnecessary quoting Jeff King
2018-01-12 18:06       ` [PATCH v3 0/4] run-command.c: print env vars when GIT_TRACE is set Stefan Beller
2018-01-12 17:19     ` Stefan Beller
2018-01-13  6:54       ` Duy Nguyen
2018-01-13  6:49     ` [PATCH v4 0/7] Trace env variables in run_command() Nguyễn Thái Ngọc Duy
2018-01-13  6:49       ` [PATCH v4 1/7] sq_quote_argv: drop maxlen parameter Nguyễn Thái Ngọc Duy
2018-01-13  6:49       ` [PATCH v4 2/7] trace: avoid unnecessary quoting Nguyễn Thái Ngọc Duy
2018-01-13  6:49       ` [PATCH v4 3/7] trace.c: move strbuf_release() out of print_trace_line() Nguyễn Thái Ngọc Duy
2018-01-13  7:14         ` Jeff King
2018-01-13  6:49       ` [PATCH v4 4/7] trace.c: introduce trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-13  6:49       ` [PATCH v4 5/7] trace.c: print program 'git' when child_process.git_cmd is set Nguyễn Thái Ngọc Duy
2018-01-13  6:49       ` [PATCH v4 6/7] trace.c: print env vars in trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-13  7:25         ` Jeff King [this message]
2018-01-16 22:13         ` Junio C Hamano
2018-01-16 22:20           ` Stefan Beller
2018-01-17  1:32             ` Junio C Hamano
2018-01-13  6:49       ` [PATCH v4 7/7] trace.c: print new cwd " Nguyễn Thái Ngọc Duy
2018-01-13  7:26       ` [PATCH v4 0/7] Trace env variables in run_command() Jeff King
2018-01-15 10:59       ` [PATCH v5 " Nguyễn Thái Ngọc Duy
2018-01-15 10:59         ` [PATCH v5 1/7] sq_quote_argv: drop maxlen parameter Nguyễn Thái Ngọc Duy
2018-01-15 10:59         ` [PATCH v5 2/7] trace: avoid unnecessary quoting Nguyễn Thái Ngọc Duy
2018-01-15 10:59         ` [PATCH v5 3/7] trace.c: move strbuf_release() out of print_trace_line() Nguyễn Thái Ngọc Duy
2018-01-16 18:24           ` Brandon Williams
2018-01-15 10:59         ` [PATCH v5 4/7] trace.c: introduce trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-17 22:21           ` Jeff King
2018-01-17 22:41             ` Junio C Hamano
2018-01-15 10:59         ` [PATCH v5 5/7] trace.c: print program 'git' when child_process.git_cmd is set Nguyễn Thái Ngọc Duy
2018-01-15 10:59         ` [PATCH v5 6/7] trace.c: print env vars in trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-16 18:32           ` Brandon Williams
2018-01-15 10:59         ` [PATCH v5 7/7] trace.c: print new cwd " Nguyễn Thái Ngọc Duy
2018-01-18  9:45         ` [PATCH v6 0/7] Trace env variables in run_command() Nguyễn Thái Ngọc Duy
2018-01-18  9:45           ` [PATCH v6 1/7] sq_quote_argv: drop maxlen parameter Nguyễn Thái Ngọc Duy
2018-01-18  9:45           ` [PATCH v6 2/7] trace: avoid unnecessary quoting Nguyễn Thái Ngọc Duy
2018-01-18  9:45           ` [PATCH v6 3/7] trace.c: move strbuf_release() out of print_trace_line() Nguyễn Thái Ngọc Duy
2018-01-18  9:45           ` [PATCH v6 4/7] run-command.c: introduce trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-18  9:45           ` [PATCH v6 5/7] run-command.c: print program 'git' when tracing git_cmd mode Nguyễn Thái Ngọc Duy
2018-01-18  9:45           ` [PATCH v6 6/7] run-command.c: print env vars in trace_run_command() Nguyễn Thái Ngọc Duy
2018-01-18  9:45           ` [PATCH v6 7/7] run-command.c: print new cwd " Nguyễn Thái Ngọc Duy
2018-01-19 21:39           ` [PATCH v6 0/7] Trace env variables in run_command() Jeff King
2018-01-11 10:07 ` [PATCH] run-command.c: print env vars when GIT_TRACE is set Jeff King
2018-01-11 11:13   ` Duy Nguyen
2018-01-11 18:21   ` Johannes Sixt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180113072524.GB27251@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=j6t@kdbg.org \
    --cc=pclouds@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).