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@xxxxxxxxx>
---
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
