git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
To: git@vger.kernel.org
Cc: "Stefan Beller" <sbeller@google.com>,
	"Brandon Williams" <bmwill@google.com>,
	"Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
	"Johannes Sixt" <j6t@kdbg.org>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Subject: [PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()
Date: Fri, 12 Jan 2018 16:56:07 +0700	[thread overview]
Message-ID: <20180112095607.18293-5-pclouds@gmail.com> (raw)
In-Reply-To: <20180112095607.18293-1-pclouds@gmail.com>

The previous concatenate_env() is kinda dumb. It receives a env delta
in child_process and blindly follows it. If the run_command() user
"adds" more vars of the same value, or deletes vars that do not exist
in parent env in the first place (*), then why bother to print them?

This patch checks child_process.env against parent environment and
only print the actual differences. The unset syntax (and later on cwd)
follows closely shell syntax for easy reading/guessing and copy/paste.

There is an interesting thing with this change. In the previous patch,
we unconditionally print new vars. With submodule code we may have
something like this

    trace: ... GIT_DIR='.git' git 'status' '--porcelain=2'

but since parent's GIT_DIR usually has the same value '.git' too, this
change suppress that, now we can't see that the above command runs in
a separate repo. This is the run for printing cwd. Now we have

    trace: ... cd foo; git 'status' '--porcelain=2'

Another equally interesting thing is, some caller can do "unset GIT_DIR;
set GIT_DIR=.git". Since parent env can have the same value '.git', we
don't re-print GIT_DIR=.git. In that case must NOT print "unset GIT_DIR"
or the user will be misled to think the new command has no GIT_DIR.

A note about performance. Yes we're burning a lot more cycles for
displaying something this nice. But because it's protected by
$GIT_TRACE, and run_command() should not happen often anyway, it should
not feel any slower than before.

(*) this is the case with submodule code where all local_repo_env[]
vars are to be deleted even though many of them do not exist in the
first place. Printing all these is too noisy and that leads to
ignoring variable deletion in the previous patch.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 trace.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 69 insertions(+), 8 deletions(-)

diff --git a/trace.c b/trace.c
index e499074d39..a1f871e720 100644
--- a/trace.c
+++ b/trace.c
@@ -272,23 +272,78 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,
 
 #endif /* HAVE_VARIADIC_MACROS */
 
+static void sort_deltaenv(struct string_list *envs,
+			  const char *const *deltaenv)
+{
+	struct strbuf key = STRBUF_INIT;
+	const char *const *e;
+
+	for (e = deltaenv; e && *e; e++) {
+		char *equals = strchr(*e, '=');
+
+		if (equals) {
+			strbuf_reset(&key);
+			strbuf_add(&key, *e, equals - *e);
+			string_list_append(envs, key.buf)->util = equals + 1;
+		} else {
+			string_list_append(envs, *e)->util = NULL;
+		}
+	}
+	string_list_sort(envs);
+	strbuf_release(&key);
+}
 
-static void concatenate_env(struct strbuf *dst, const char *const *envs)
+
+static void concatenate_env(struct strbuf *dst, const char *const *deltaenv)
 {
+	struct string_list envs = STRING_LIST_INIT_DUP;
 	int i;
 
-	for (i = 0; envs[i]; i++) {
-		const char *env = envs[i];
-		const char *p = strchr(env, '=');
+	/*
+	 * Construct a sorted string list consisting of the delta
+	 * env. We need this to detect the case when the same var is
+	 * deleted first, then added again.
+	 */
+	sort_deltaenv(&envs, deltaenv);
+
+	/*
+	 * variable deletion first because it's printed like separate
+	 * shell commands
+	 */
+	for (i = 0; i < envs.nr; i++) {
+		const char *env = envs.items[i].string;
+		const char *p = envs.items[i].util;
+
+		if (p || !getenv(env))
+			continue;
 
-		if (!p) /* ignore var deletion for now */
+		/*
+		 * Do we have a sequence of "unset GIT_DIR; GIT_DIR=foo"?
+		 * Then don't bother with the unset thing.
+		 */
+		if (i + 1 < envs.nr &&
+		    !strcmp(env, envs.items[i + 1].string))
 			continue;
-		p++;
 
-		strbuf_addch(dst, ' ');
-		strbuf_add(dst, env, p - env);
+		strbuf_addf(dst, " unset %s;", env);
+	}
+
+	for (i = 0; i < envs.nr; i++) {
+		const char *env = envs.items[i].string;
+		const char *p = envs.items[i].util;
+		const char *old_value;
+
+		if (!p)
+			continue;
+
+		old_value = getenv(env);
+		if (old_value && !strcmp(old_value, p))
+			continue;
+
+		strbuf_addf(dst, " %s=", env);
 		sq_quote_buf(dst, p);
 	}
+	string_list_clear(&envs, 0);
 }
 
 void trace_run_command(const struct child_process *cp)
@@ -303,6 +358,12 @@ void trace_run_command(const struct child_process *cp)
 
 	strbuf_addf(&buf, "trace: run_command:");
 
+	if (cp->dir) {
+		strbuf_addstr(&buf, " cd ");
+		sq_quote_buf(&buf, cp->dir);
+		strbuf_addch(&buf, ';');
+	}
+
 	/*
 	 * The caller is responsible for initializing cp->env from
 	 * cp->env_array if needed. We only check one place.
-- 
2.15.1.600.g899a5f85c6


  parent reply	other threads:[~2018-01-12  9:57 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     ` Nguyễn Thái Ngọc Duy [this message]
2018-01-12 13:33       ` [PATCH v3 4/4] trace.c: be smart about what env to print " 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
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=20180112095607.18293-5-pclouds@gmail.com \
    --to=pclouds@gmail.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=j6t@kdbg.org \
    --cc=peff@peff.net \
    --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).