{"thread":{"id":"47584","subject":"[PATCH] run-command.c: print env vars when GIT_TRACE is set","startedAt":"2018-01-10T10:48:50Z","lastAt":"2018-01-19T21:39:51Z","messageCount":74,"participants":["Nguyễn Thái Ngọc Duy","Brandon Williams","Stefan Beller","Jeff King","Duy Nguyen","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"336372","messageId":"20180110104835.22905-1-pclouds@gmail.com","threadId":"47584","inReplyTo":null,"subject":"[PATCH] run-command.c: print env vars when GIT_TRACE is set","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-10T10:48:35Z","receivedAt":"2018-01-10T10:48:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Occasionally submodule code could execute new commands with GIT_DIR set\nto some submodule. GIT_TRACE prints just the command line which makes it\nhard to tell that it's not really executed on this repository.\n\nPrint env variables in this case. Note that the code deliberately ignore\nvariables unsetting because there are so many of them (to keep git\nenvironment clean for the next process) and really hard to read.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n A minor thing that I ignored in this patch is quoting the environment\n variables. For me it's meh. Perhaps I should just dumb the env\n without quoting at all.\n\n run-command.c | 3 ++-\n trace.c | 38 +++++++++++++++++++++++++++++++++++---\n trace.h | 18 +++++++++++++++---\n 3 files changed, 52 insertions(+), 7 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 31fc5ea86e..235367087d 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -624,7 +624,8 @@ int start_command(struct child_process *cmd)\n \t\tcmd->err = fderr[0];\n \t}\n \n-\ttrace_argv_printf(cmd->argv, \"trace: run_command:\");\n+\ttrace_env_argv_printf(cmd->env, cmd->argv, \"trace: run_command:\");\n+\n \tfflush(NULL);\n \n #ifndef GIT_WINDOWS_NATIVE\ndiff --git a/trace.c b/trace.c\nindex b7530b51a9..d8967b66e8 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -146,7 +146,26 @@ static void trace_vprintf_fl(const char *file, int line, struct trace_key *key,\n \tprint_trace_line(key, &buf);\n }\n \n+static void concatenate_env(struct strbuf *dst, const char *const *env)\n+{\n+\tint i;\n+\n+\t/* Copy into destination buffer. */\n+\tstrbuf_grow(dst, 255);\n+\tfor (i = 0; env[i]; ++i) {\n+\t\t/*\n+\t\t * the main interesting is setting new vars\n+\t\t * e.g. GIT_DIR, ignore the unsetting to reduce noise.\n+\t\t */\n+\t\tif (!strchr(env[i], '='))\n+\t\t\tcontinue;\n+\t\tstrbuf_addch(dst, ' ');\n+\t\tsq_quote_buf(dst, env[i]);\n+\t}\n+}\n+\n static void trace_argv_vprintf_fl(const char *file, int line,\n+\t\t\t\t const char *const *env,\n \t\t\t\t const char **argv, const char *format,\n \t\t\t\t va_list ap)\n {\n@@ -157,6 +176,9 @@ static void trace_argv_vprintf_fl(const char *file, int line,\n \n \tstrbuf_vaddf(&buf, format, ap);\n \n+\tif (env)\n+\t\tconcatenate_env(&buf, env);\n+\n \tsq_quote_argv(&buf, argv, 0);\n \tprint_trace_line(&trace_default_key, &buf);\n }\n@@ -214,7 +236,16 @@ void trace_argv_printf(const char **argv, const char *format, ...)\n {\n \tva_list ap;\n \tva_start(ap, format);\n-\ttrace_argv_vprintf_fl(NULL, 0, argv, format, ap);\n+\ttrace_argv_vprintf_fl(NULL, 0, NULL, argv, format, ap);\n+\tva_end(ap);\n+}\n+\n+void trace_env_argv_printf(const char *const *env, const char **argv,\n+\t\t\t const char *format, ...)\n+{\n+\tva_list ap;\n+\tva_start(ap, format);\n+\ttrace_argv_vprintf_fl(NULL, 0, env, argv, format, ap);\n \tva_end(ap);\n }\n \n@@ -251,12 +282,13 @@ void trace_printf_key_fl(const char *file, int line, struct trace_key *key,\n \tva_end(ap);\n }\n \n-void trace_argv_printf_fl(const char *file, int line, const char **argv,\n+void trace_argv_printf_fl(const char *file, int line,\n+\t\t\t const char *const *env, const char **argv,\n \t\t\t const char *format, ...)\n {\n \tva_list ap;\n \tva_start(ap, format);\n-\ttrace_argv_vprintf_fl(file, line, argv, format, ap);\n+\ttrace_argv_vprintf_fl(file, line, env, argv, format, ap);\n \tva_end(ap);\n }\n \ndiff --git a/trace.h b/trace.h\nindex 88055abef7..830d9dcd19 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -34,6 +34,10 @@ extern void trace_printf_key(struct trace_key *key, const char *format, ...);\n __attribute__((format (printf, 2, 3)))\n extern void trace_argv_printf(const char **argv, const char *format, ...);\n \n+__attribute__((format (printf, 3, 4)))\n+extern void trace_env_argv_printf(const char * const*env, const char **argv,\n+\t\t\t\t const char *format, ...);\n+\n extern void trace_strbuf(struct trace_key *key, const struct strbuf *data);\n \n /* Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled. */\n@@ -93,7 +97,14 @@ extern void trace_performance_since(uint64_t start, const char *format, ...);\n \tdo {\t\t\t\t\t\t\t\t \\\n \t\tif (trace_pass_fl(&trace_default_key))\t\t\t \\\n \t\t\ttrace_argv_printf_fl(TRACE_CONTEXT, __LINE__,\t \\\n-\t\t\t\t\t argv, __VA_ARGS__);\t\t \\\n+\t\t\t\t\t NULL, argv, __VA_ARGS__);\t \\\n+\t} while (0)\n+\n+#define trace_env_argv_printf(env, argv, ...)\t\t\t\t \\\n+\tdo {\t\t\t\t\t\t\t\t \\\n+\t\tif (trace_pass_fl(&trace_default_key))\t\t\t \\\n+\t\t\ttrace_argv_printf_fl(TRACE_CONTEXT, __LINE__, \t \\\n+\t\t\t\t\t env, argv, __VA_ARGS__); \t \\\n \t} while (0)\n \n #define trace_strbuf(key, data)\t\t\t\t\t\t \\\n@@ -121,8 +132,9 @@ extern void trace_performance_since(uint64_t start, const char *format, ...);\n __attribute__((format (printf, 4, 5)))\n extern void trace_printf_key_fl(const char *file, int line, struct trace_key *key,\n \t\t\t\tconst char *format, ...);\n-__attribute__((format (printf, 4, 5)))\n-extern void trace_argv_printf_fl(const char *file, int line, const char **argv,\n+__attribute__((format (printf, 5, 6)))\n+extern void trace_argv_printf_fl(const char *file, int line,\n+\t\t\t\t const char *const *env, const char **argv,\n \t\t\t\t const char *format, ...);\n extern void trace_strbuf_fl(const char *file, int line, struct trace_key *key,\n \t\t\t const struct strbuf *data);\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336401","messageId":"20180110180945.GA53941@google.com","threadId":"47584","inReplyTo":"20180110104835.22905-1-pclouds@gmail.com","subject":"Re: [PATCH] run-command.c: print env vars when GIT_TRACE is set","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-01-10T18:09:45Z","receivedAt":"2018-01-10T18:09:53Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 01/10, Nguyễn Thái Ngọc Duy wrote:\n> Occasionally submodule code could execute new commands with GIT_DIR set\n> to some submodule. GIT_TRACE prints just the command line which makes it\n> hard to tell that it's not really executed on this repository.\n> \n> Print env variables in this case. Note that the code deliberately ignore\n> variables unsetting because there are so many of them (to keep git\n> environment clean for the next process) and really hard to read.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> A minor thing that I ignored in this patch is quoting the environment\n> variables. For me it's meh. Perhaps I should just dumb the env\n> without quoting at all.\n\nA patch like this would have been very helpful to me on some previous\noccasions, so thanks for writing it up.\n\n> \n> run-command.c | 3 ++-\n> trace.c | 38 +++++++++++++++++++++++++++++++++++---\n> trace.h | 18 +++++++++++++++---\n> 3 files changed, 52 insertions(+), 7 deletions(-)\n> \n> diff --git a/run-command.c b/run-command.c\n> index 31fc5ea86e..235367087d 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -624,7 +624,8 @@ int start_command(struct child_process *cmd)\n> \t\tcmd->err = fderr[0];\n> \t}\n> \n> -\ttrace_argv_printf(cmd->argv, \"trace: run_command:\");\n> +\ttrace_env_argv_printf(cmd->env, cmd->argv, \"trace: run_command:\");\n> +\n> \tfflush(NULL);\n> \n> #ifndef GIT_WINDOWS_NATIVE\n> diff --git a/trace.c b/trace.c\n> index b7530b51a9..d8967b66e8 100644\n> --- a/trace.c\n> +++ b/trace.c\n> @@ -146,7 +146,26 @@ static void trace_vprintf_fl(const char *file, int line, struct trace_key *key,\n> \tprint_trace_line(key, &buf);\n> }\n> \n> +static void concatenate_env(struct strbuf *dst, const char *const *env)\n> +{\n> +\tint i;\n> +\n> +\t/* Copy into destination buffer. */\n> +\tstrbuf_grow(dst, 255);\n> +\tfor (i = 0; env[i]; ++i) {\n> +\t\t/*\n> +\t\t * the main interesting is setting new vars\n> +\t\t * e.g. GIT_DIR, ignore the unsetting to reduce noise.\n> +\t\t */\n\nI think you're missing a word, maybe:\n 'The main interesting part is setting new vars'\n\n> +\t\tif (!strchr(env[i], '='))\n> +\t\t\tcontinue;\n> +\t\tstrbuf_addch(dst, ' ');\n> +\t\tsq_quote_buf(dst, env[i]);\n> +\t}\n\nAt first when i read this I was under the impression that the whole\nenvironment was going to be printed out...but i now realize that this\ntracing will only print out the delta's or the additions to the\nenvironment that the child will see. Maybe this should be called out\nmore clearly in the commit message?\n\n> +}\n> +\n> static void trace_argv_vprintf_fl(const char *file, int line,\n> +\t\t\t\t const char *const *env,\n> \t\t\t\t const char **argv, const char *format,\n> \t\t\t\t va_list ap)\n> {\n> @@ -157,6 +176,9 @@ static void trace_argv_vprintf_fl(const char *file, int line,\n> \n> \tstrbuf_vaddf(&buf, format, ap);\n> \n> +\tif (env)\n> +\t\tconcatenate_env(&buf, env);\n> +\n> \tsq_quote_argv(&buf, argv, 0);\n> \tprint_trace_line(&trace_default_key, &buf);\n> }\n> @@ -214,7 +236,16 @@ void trace_argv_printf(const char **argv, const char *format, ...)\n> {\n> \tva_list ap;\n> \tva_start(ap, format);\n> -\ttrace_argv_vprintf_fl(NULL, 0, argv, format, ap);\n> +\ttrace_argv_vprintf_fl(NULL, 0, NULL, argv, format, ap);\n> +\tva_end(ap);\n> +}\n> +\n> +void trace_env_argv_printf(const char *const *env, const char **argv,\n> +\t\t\t const char *format, ...)\n> +{\n> +\tva_list ap;\n> +\tva_start(ap, format);\n> +\ttrace_argv_vprintf_fl(NULL, 0, env, argv, format, ap);\n> \tva_end(ap);\n> }\n> \n> @@ -251,12 +282,13 @@ void trace_printf_key_fl(const char *file, int line, struct trace_key *key,\n> \tva_end(ap);\n> }\n> \n> -void trace_argv_printf_fl(const char *file, int line, const char **argv,\n> +void trace_argv_printf_fl(const char *file, int line,\n> +\t\t\t const char *const *env, const char **argv,\n> \t\t\t const char *format, ...)\n> {\n> \tva_list ap;\n> \tva_start(ap, format);\n> -\ttrace_argv_vprintf_fl(file, line, argv, format, ap);\n> +\ttrace_argv_vprintf_fl(file, line, env, argv, format, ap);\n> \tva_end(ap);\n> }\n> \n> diff --git a/trace.h b/trace.h\n> index 88055abef7..830d9dcd19 100644\n> --- a/trace.h\n> +++ b/trace.h\n> @@ -34,6 +34,10 @@ extern void trace_printf_key(struct trace_key *key, const char *format, ...);\n> __attribute__((format (printf, 2, 3)))\n> extern void trace_argv_printf(const char **argv, const char *format, ...);\n> \n> +__attribute__((format (printf, 3, 4)))\n> +extern void trace_env_argv_printf(const char * const*env, const char **argv,\n> +\t\t\t\t const char *format, ...);\n> +\n> extern void trace_strbuf(struct trace_key *key, const struct strbuf *data);\n> \n> /* Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled. */\n> @@ -93,7 +97,14 @@ extern void trace_performance_since(uint64_t start, const char *format, ...);\n> \tdo {\t\t\t\t\t\t\t\t \\\n> \t\tif (trace_pass_fl(&trace_default_key))\t\t\t \\\n> \t\t\ttrace_argv_printf_fl(TRACE_CONTEXT, __LINE__,\t \\\n> -\t\t\t\t\t argv, __VA_ARGS__);\t\t \\\n> +\t\t\t\t\t NULL, argv, __VA_ARGS__);\t \\\n> +\t} while (0)\n> +\n> +#define trace_env_argv_printf(env, argv, ...)\t\t\t\t \\\n> +\tdo {\t\t\t\t\t\t\t\t \\\n> +\t\tif (trace_pass_fl(&trace_default_key))\t\t\t \\\n> +\t\t\ttrace_argv_printf_fl(TRACE_CONTEXT, __LINE__, \t \\\n> +\t\t\t\t\t env, argv, __VA_ARGS__); \t \\\n> \t} while (0)\n> \n> #define trace_strbuf(key, data)\t\t\t\t\t\t \\\n> @@ -121,8 +132,9 @@ extern void trace_performance_since(uint64_t start, const char *format, ...);\n> __attribute__((format (printf, 4, 5)))\n> extern void trace_printf_key_fl(const char *file, int line, struct trace_key *key,\n> \t\t\t\tconst char *format, ...);\n> -__attribute__((format (printf, 4, 5)))\n> -extern void trace_argv_printf_fl(const char *file, int line, const char **argv,\n> +__attribute__((format (printf, 5, 6)))\n> +extern void trace_argv_printf_fl(const char *file, int line,\n> +\t\t\t\t const char *const *env, const char **argv,\n> \t\t\t\t const char *format, ...);\n> extern void trace_strbuf_fl(const char *file, int line, struct trace_key *key,\n> \t\t\t const struct strbuf *data);\n> -- \n> 2.15.1.600.g899a5f85c6\n> \n\n-- \nBrandon Williams\n"},{"id":"336407","messageId":"CAGZ79ka0mhPy776fRYtvnaqd5P1dwvuaeONozEWpaJVZxSneiA@mail.gmail.com","threadId":"47584","inReplyTo":"20180110180945.GA53941@google.com","subject":"Re: [PATCH] run-command.c: print env vars when GIT_TRACE is set","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-10T19:14:32Z","receivedAt":"2018-01-10T19:14:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jan 10, 2018 at 10:09 AM, Brandon Williams wrote:\n> On 01/10, Nguyễn Thái Ngọc Duy wrote:\n>> Occasionally submodule code could execute new commands with GIT_DIR set\n>> to some submodule. GIT_TRACE prints just the command line which makes it\n>> hard to tell that it's not really executed on this repository.\n\nSpeaking of GIT_DIR, we may also want to print the dir that the command is\nexecuted in as the GIT_DIR for submodules usually is only \".git\" assuming the\nrun-command struct set the .dir to the submodules worktree. I think I had a\npatch for printing the cwd of a process on the list once upon a time, but\nI am unable to find it again. Maybe we'd want to include the cwd of a spawned\nprocess if it differs from the current process?\n\n>>\n>> Print env variables in this case. Note that the code deliberately ignore\n>> variables unsetting because there are so many of them (to keep git\n>> environment clean for the next process) and really hard to read.\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> A minor thing that I ignored in this patch is quoting the environment\n>> variables. For me it's meh. Perhaps I should just dumb the env\n>> without quoting at all.\n>\n> A patch like this would have been very helpful to me on some previous\n> occasions, so thanks for writing it up.\n\nThe patch I mentioned above (very similar though less powerful than this one)\nwas carried by locally for some time, so I definitely see value in this patch.\nThanks for writing it!\n\n>\n>>\n>> run-command.c | 3 ++-\n>> trace.c | 38 +++++++++++++++++++++++++++++++++++---\n>> trace.h | 18 +++++++++++++++---\n>> 3 files changed, 52 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/run-command.c b/run-command.c\n>> index 31fc5ea86e..235367087d 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -624,7 +624,8 @@ int start_command(struct child_process *cmd)\n>> cmd->err = fderr[0];\n>> }\n>>\n>> - trace_argv_printf(cmd->argv, \"trace: run_command:\");\n>> + trace_env_argv_printf(cmd->env, cmd->argv, \"trace: run_command:\");\n>> +\n>> fflush(NULL);\n>>\n>> #ifndef GIT_WINDOWS_NATIVE\n>> diff --git a/trace.c b/trace.c\n>> index b7530b51a9..d8967b66e8 100644\n>> --- a/trace.c\n>> +++ b/trace.c\n>> @@ -146,7 +146,26 @@ static void trace_vprintf_fl(const char *file, int line, struct trace_key *key,\n>> print_trace_line(key, &buf);\n>> }\n>>\n>> +static void concatenate_env(struct strbuf *dst, const char *const *env)\n>> +{\n>> + int i;\n>> +\n>> + /* Copy into destination buffer. */\n>> + strbuf_grow(dst, 255);\n>> + for (i = 0; env[i]; ++i) {\n>> + /*\n>> + * the main interesting is setting new vars\n>> + * e.g. GIT_DIR, ignore the unsetting to reduce noise.\n>> + */\n>\n> I think you're missing a word, maybe:\n> 'The main interesting part is setting new vars'\n>\n>> + if (!strchr(env[i], '='))\n>> + continue;\n>> + strbuf_addch(dst, ' ');\n>> + sq_quote_buf(dst, env[i]);\n>> + }\n>\n> At first when i read this I was under the impression that the whole\n> environment was going to be printed out...but i now realize that this\n> tracing will only print out the delta's or the additions to the\n> environment that the child will see. Maybe this should be called out\n> more clearly in the commit message?\n\nIt only adds newly set variables, I wonder why deletions are noisy?\nI could not find an example of a removal of environment variables\nspecific to submodules that would be noisy.\n\nStefan\n"},{"id":"336409","messageId":"20180110192606.GB53941@google.com","threadId":"47584","inReplyTo":"CAGZ79ka0mhPy776fRYtvnaqd5P1dwvuaeONozEWpaJVZxSneiA@mail.gmail.com","subject":"Re: [PATCH] run-command.c: print env vars when GIT_TRACE is set","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-01-10T19:26:06Z","receivedAt":"2018-01-10T19:28:01Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 01/10, Stefan Beller wrote:\n> On Wed, Jan 10, 2018 at 10:09 AM, Brandon Williams wrote:\n> > At first when i read this I was under the impression that the whole\n> > environment was going to be printed out...but i now realize that this\n> > tracing will only print out the delta's or the additions to the\n> > environment that the child will see. Maybe this should be called out\n> > more clearly in the commit message?\n> \n> It only adds newly set variables, I wonder why deletions are noisy?\n> I could not find an example of a removal of environment variables\n> specific to submodules that would be noisy.\n\nDeletions are noisy because we append local_repo_env anytime we kick\noff a child process (ok maybe not all the time, but a lot of the time)\nwhich is just a list of ~15 deletions.\n\n-- \nBrandon Williams\n"},{"id":"336410","messageId":"CAGZ79kayPNuCNBDe7Ba823APf+QLZU64j2WqFeMn3UjPU7jUGg@mail.gmail.com","threadId":"47584","inReplyTo":"20180110192606.GB53941@google.com","subject":"Re: [PATCH] run-command.c: print env vars when GIT_TRACE is set","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-10T19:35:18Z","receivedAt":"2018-01-10T19:35:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jan 10, 2018 at 11:26 AM, Brandon Williams wrote:\n> On 01/10, Stefan Beller wrote:\n>> On Wed, Jan 10, 2018 at 10:09 AM, Brandon Williams wrote:\n>> > At first when i read this I was under the impression that the whole\n>> > environment was going to be printed out...but i now realize that this\n>> > tracing will only print out the delta's or the additions to the\n>> > environment that the child will see. Maybe this should be called out\n>> > more clearly in the commit message?\n>>\n>> It only adds newly set variables, I wonder why deletions are noisy?\n>> I could not find an example of a removal of environment variables\n>> specific to submodules that would be noisy.\n>\n> Deletions are noisy because we append local_repo_env anytime we kick\n> off a child process (ok maybe not all the time, but a lot of the time)\n> which is just a list of ~15 deletions.\n\nOh, I see. I'm fine with that.\n\nThough to further the discussion, maybe we want to check if such a variable\nis set and if so actually show the deletion? I have the impression that most\nof the variables are not set, and yet we unconditionally delete them, \"just to\nbe sure\". Maybe we'd only want to delete them if we actually have them\nset in the superproject, and then we can show the diff in env vars\nmore faithful?\n"},{"id":"336449","messageId":"20180111094712.2551-1-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180110104835.22905-1-pclouds@gmail.com","subject":"[PATCH v2] run-command.c: print env vars when GIT_TRACE is set","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-11T09:47:12Z","receivedAt":"2018-01-11T09:47:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Occasionally submodule code could execute new commands with GIT_DIR set\nto some submodule. GIT_TRACE prints just the command line which makes it\nhard to tell that it's not really executed on this repository.\n\nPrint modified env variables (compared to parent environment) in this\ncase. Actually only modified or new variables are printed. Variable\ndeletion is suppressed because they are often used to clean up\nrepo-specific variables that git passes around between processes. When\nsubmodule code executes commands on another repo, it clears all these\nvariables, _many_ of these, that make the output really noisy.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n v2 fixes up commit message to clarify about \"env delta\" and why var\n deletion is not printed.\n\n It also keeps child_process tracing in one place/function, this\n should make it easier to trace more e.g. cwd and stuff.\n\n Though, Stefan, while i'm not opposed to trace every single setting\n in child_process, including variable deletion, cwd and even more, it\n may be not that often needed for a \"casual\" developer.\n \n I suggest we have something like $GIT_TRACE_EXEC instead that could\n be super verbose when we need it and leave $GIT_TRACE with a\n reasonable subset.\n\n run-command.c | 3 ++-\n trace.c | 39 +++++++++++++++++++++++++++++++++++++++\n trace.h | 3 +++\n 3 files changed, 44 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 31fc5ea86e..002074b128 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -624,7 +624,8 @@ int start_command(struct child_process *cmd)\n \t\tcmd->err = fderr[0];\n \t}\n \n-\ttrace_argv_printf(cmd->argv, \"trace: run_command:\");\n+\ttrace_run_command(cmd);\n+\n \tfflush(NULL);\n \n #ifndef GIT_WINDOWS_NATIVE\ndiff --git a/trace.c b/trace.c\nindex b7530b51a9..e5e46e2a09 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -23,6 +23,7 @@\n \n #include \"cache.h\"\n #include \"quote.h\"\n+#include \"run-command.h\"\n \n struct trace_key trace_default_key = { \"GIT_TRACE\", 0, 0, 0 };\n struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);\n@@ -272,6 +273,44 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,\n #endif /* HAVE_VARIADIC_MACROS */\n \n \n+static void concatenate_env(struct strbuf *dst, const char *const *env)\n+{\n+\tint i;\n+\n+\t/* Copy into destination buffer. */\n+\tstrbuf_grow(dst, 255);\n+\tfor (i = 0; env[i]; ++i) {\n+\t\t/*\n+\t\t * the main interesting is setting new vars\n+\t\t * e.g. GIT_DIR, ignore the unsetting to reduce noise.\n+\t\t */\n+\t\tif (!strchr(env[i], '='))\n+\t\t\tcontinue;\n+\t\tstrbuf_addch(dst, ' ');\n+\t\tsq_quote_buf(dst, env[i]);\n+\t}\n+}\n+\n+void trace_run_command(const struct child_process *cp)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tif (!prepare_trace_line(NULL, 0, &trace_default_key, &buf))\n+\t\treturn;\n+\n+\tstrbuf_addf(&buf, \"trace: run_command:\");\n+\n+\t/*\n+\t * caller is responsible for setting this if the main source\n+\t * is actually in cp->env_array\n+\t */\n+\tif (cp->env)\n+\t\tconcatenate_env(&buf, cp->env);\n+\n+\tsq_quote_argv(&buf, cp->argv, 0);\n+\tprint_trace_line(&trace_default_key, &buf);\n+}\n+\n static const char *quote_crnl(const char *path)\n {\n \tstatic struct strbuf new_path = STRBUF_INIT;\ndiff --git a/trace.h b/trace.h\nindex 88055abef7..e54c687f26 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -4,6 +4,8 @@\n #include \"git-compat-util.h\"\n #include \"strbuf.h\"\n \n+struct child_process;\n+\n struct trace_key {\n \tconst char * const key;\n \tint fd;\n@@ -17,6 +19,7 @@ extern struct trace_key trace_default_key;\n extern struct trace_key trace_perf_key;\n \n extern void trace_repo_setup(const char *prefix);\n+extern void trace_run_command(const struct child_process *cp);\n extern int trace_want(struct trace_key *key);\n extern void trace_disable(struct trace_key *key);\n extern uint64_t getnanotime(void);\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336450","messageId":"20180111100734.GA17274@sigill.intra.peff.net","threadId":"47584","inReplyTo":"20180110104835.22905-1-pclouds@gmail.com","subject":"Re: [PATCH] run-command.c: print env vars when GIT_TRACE is set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-11T10:07:35Z","receivedAt":"2018-01-11T10:07:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 10, 2018 at 05:48:35PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> Occasionally submodule code could execute new commands with GIT_DIR set\n> to some submodule. GIT_TRACE prints just the command line which makes it\n> hard to tell that it's not really executed on this repository.\n> \n> Print env variables in this case. Note that the code deliberately ignore\n> variables unsetting because there are so many of them (to keep git\n> environment clean for the next process) and really hard to read.\n\nI like this, and I'm pretty sure it would have helped me debug at least\nonce in the past. I did notice one funny thing, though I think it was\nlargely there before.\n\nThe output for a single command is pretty shell-like due to the quoting:\n\n $ GIT_TRACE=1 ./git upload-pack . >/dev/null\n [...]run_command: 'git-upload-pack' '.'\n\nYou could copy and paste that to a shell if you wanted. And with\nenvironment variables, that remains so:\n\n $ GIT_TRACE=1 ./git ls-remote https://github.com/git/git >/dev/null\n [...]run_command: 'GIT_DIR=.git' 'git-remote-https' 'https://[...]'\n\nBut if we're actually running a command via the shell, it all gets\nquoted as one argument:\n\n $ GIT_TRACE=1 GIT_PAGER='foo | bar' ./git log\n [...]run_command: 'LV=-c' 'foo | bar'\n\nwhich is kind of weird, as that's not something that can be run in a\nshell. This isn't introduced by your patch at all, but I noticed it more\nbecause of the shell-like environment variable output.\n\nWe actually used to print a separate:\n\n exec: /bin/sh -c 'foo | bar'\n\nline when we invoked a shell, which would arguably be the right place to\nshow the env variables for such a case. But that went away with\n3967e25be1 (run-command: prepare command before forking, 2017-04-19).\n\nI think it might be helpful if we added back in \"/bin/sh -c\" to the\nrun_command line when \"use_shell\" is in effect (and when we're not doing\nour \"skip the shell\" trickery). But that's totally orthogonal to your\npatch.\n\nAnd anyway, it's just tracing output, so I don't think it's incredibly\nimportant either way. It was just something I noticed while looking at\nyour patch's output.\n\n-Peff\n"},{"id":"336454","messageId":"CACsJy8Beq2gx8K8Hk7NMVj_1Cgn5v-tuN9eZtDRCuOxB_urcWw@mail.gmail.com","threadId":"47584","inReplyTo":"20180111100734.GA17274@sigill.intra.peff.net","subject":"Re: [PATCH] run-command.c: print env vars when GIT_TRACE is set","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-11T11:13:45Z","receivedAt":"2018-01-11T11:14:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jan 11, 2018 at 5:07 PM, Jeff King wrote:\n> On Wed, Jan 10, 2018 at 05:48:35PM +0700, Nguyễn Thái Ngọc Duy wrote:\n>\n>> Occasionally submodule code could execute new commands with GIT_DIR set\n>> to some submodule. GIT_TRACE prints just the command line which makes it\n>> hard to tell that it's not really executed on this repository.\n>>\n>> Print env variables in this case. Note that the code deliberately ignore\n>> variables unsetting because there are so many of them (to keep git\n>> environment clean for the next process) and really hard to read.\n>\n> I like this, and I'm pretty sure it would have helped me debug at least\n> once in the past. I did notice one funny thing, though I think it was\n> largely there before.\n>\n> The output for a single command is pretty shell-like due to the quoting:\n>\n> $ GIT_TRACE=1 ./git upload-pack . >/dev/null\n> [...]run_command: 'git-upload-pack' '.'\n>\n> You could copy and paste that to a shell if you wanted. And with\n> environment variables, that remains so:\n>\n> $ GIT_TRACE=1 ./git ls-remote https://github.com/git/git >/dev/null\n> [...]run_command: 'GIT_DIR=.git' 'git-remote-https' 'https://[...]'\n>\n> But if we're actually running a command via the shell, it all gets\n> quoted as one argument:\n>\n> $ GIT_TRACE=1 GIT_PAGER='foo | bar' ./git log\n> [...]run_command: 'LV=-c' 'foo | bar'\n>\n> which is kind of weird, as that's not something that can be run in a\n> shell. This isn't introduced by your patch at all, but I noticed it more\n> because of the shell-like environment variable output.\n\nI think you just found an argument to change my \"meh\" with regards to\nquoting to \"need to fix\" because I also often copy/paste these\ncommands. I thought about it and assumed shells would still recognize\n'name=value' assignments without actually testing it.\n\n> We actually used to print a separate:\n>\n> exec: /bin/sh -c 'foo | bar'\n>\n> line when we invoked a shell, which would arguably be the right place to\n> show the env variables for such a case. But that went away with\n> 3967e25be1 (run-command: prepare command before forking, 2017-04-19).\n>\n> I think it might be helpful if we added back in \"/bin/sh -c\" to the\n> run_command line when \"use_shell\" is in effect (and when we're not doing\n> our \"skip the shell\" trickery). But that's totally orthogonal to your\n> patch.\n>\n> And anyway, it's just tracing output, so I don't think it's incredibly\n> important either way. It was just something I noticed while looking at\n> your patch's output.\n\nNoted. I might do it if it's not super complex.\n-- \nDuy\n"},{"id":"336455","messageId":"CACsJy8BpFiNTQKLU2g=4Fat2gC1R0OcMteqcZx0ubg5ChNmKgw@mail.gmail.com","threadId":"47584","inReplyTo":"20180111094712.2551-1-pclouds@gmail.com","subject":"Re: [PATCH v2] run-command.c: print env vars when GIT_TRACE is set","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-11T11:25:07Z","receivedAt":"2018-01-11T11:25:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jan 11, 2018 at 4:47 PM, Nguyễn Thái Ngọc Duy wrote:\n> Though, Stefan, while i'm not opposed to trace every single setting\n> in child_process, including variable deletion, cwd and even more, it\n\nAnother thing I forgot to add, s/ and even more/, redirection&/. At\nsome point I think I was checking the git-pack-objects command from\nGIT_TRACE and didn't realize it was taking input from stdin (I was\nnaive :D). At least on linux we could even take advantage of\n/proc//fd to show path names and stuff in addition to plain file\ndescriptors.\n\n> may be not that often needed for a \"casual\" developer.\n>\n> I suggest we have something like $GIT_TRACE_EXEC instead that could\n> be super verbose when we need it and leave $GIT_TRACE with a\n> reasonable subset.\n-- \nDuy\n"},{"id":"336464","messageId":"20180111175316.GD91567@google.com","threadId":"47584","inReplyTo":"20180111094712.2551-1-pclouds@gmail.com","subject":"Re: [PATCH v2] run-command.c: print env vars when GIT_TRACE is set","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-01-11T17:53:16Z","receivedAt":"2018-01-11T17:53:24Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 01/11, Nguyễn Thái Ngọc Duy wrote:\n> Occasionally submodule code could execute new commands with GIT_DIR set\n> to some submodule. GIT_TRACE prints just the command line which makes it\n> hard to tell that it's not really executed on this repository.\n> \n> Print modified env variables (compared to parent environment) in this\n> case. Actually only modified or new variables are printed. Variable\n> deletion is suppressed because they are often used to clean up\n> repo-specific variables that git passes around between processes. When\n> submodule code executes commands on another repo, it clears all these\n> variables, _many_ of these, that make the output really noisy.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> v2 fixes up commit message to clarify about \"env delta\" and why var\n> deletion is not printed.\n> \n> It also keeps child_process tracing in one place/function, this\n> should make it easier to trace more e.g. cwd and stuff.\n> \n> Though, Stefan, while i'm not opposed to trace every single setting\n> in child_process, including variable deletion, cwd and even more, it\n> may be not that often needed for a \"casual\" developer.\n> \n> I suggest we have something like $GIT_TRACE_EXEC instead that could\n> be super verbose when we need it and leave $GIT_TRACE with a\n> reasonable subset.\n> \n> run-command.c | 3 ++-\n> trace.c | 39 +++++++++++++++++++++++++++++++++++++++\n> trace.h | 3 +++\n> 3 files changed, 44 insertions(+), 1 deletion(-)\n> \n> diff --git a/run-command.c b/run-command.c\n> index 31fc5ea86e..002074b128 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -624,7 +624,8 @@ int start_command(struct child_process *cmd)\n> \t\tcmd->err = fderr[0];\n> \t}\n> \n> -\ttrace_argv_printf(cmd->argv, \"trace: run_command:\");\n> +\ttrace_run_command(cmd);\n> +\n> \tfflush(NULL);\n> \n> #ifndef GIT_WINDOWS_NATIVE\n> diff --git a/trace.c b/trace.c\n> index b7530b51a9..e5e46e2a09 100644\n> --- a/trace.c\n> +++ b/trace.c\n> @@ -23,6 +23,7 @@\n> \n> #include \"cache.h\"\n> #include \"quote.h\"\n> +#include \"run-command.h\"\n> \n> struct trace_key trace_default_key = { \"GIT_TRACE\", 0, 0, 0 };\n> struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);\n> @@ -272,6 +273,44 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,\n> #endif /* HAVE_VARIADIC_MACROS */\n> \n> \n> +static void concatenate_env(struct strbuf *dst, const char *const *env)\n> +{\n> +\tint i;\n> +\n> +\t/* Copy into destination buffer. */\n> +\tstrbuf_grow(dst, 255);\n> +\tfor (i = 0; env[i]; ++i) {\n> +\t\t/*\n> +\t\t * the main interesting is setting new vars\n> +\t\t * e.g. GIT_DIR, ignore the unsetting to reduce noise.\n> +\t\t */\n\nPatch looks good to me! Only nit is this comment which reads funny which i\npointed out in v1.\n\n> +\t\tif (!strchr(env[i], '='))\n> +\t\t\tcontinue;\n> +\t\tstrbuf_addch(dst, ' ');\n> +\t\tsq_quote_buf(dst, env[i]);\n> +\t}\n> +}\n> +\n> +void trace_run_command(const struct child_process *cp)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\tif (!prepare_trace_line(NULL, 0, &trace_default_key, &buf))\n> +\t\treturn;\n> +\n> +\tstrbuf_addf(&buf, \"trace: run_command:\");\n> +\n> +\t/*\n> +\t * caller is responsible for setting this if the main source\n> +\t * is actually in cp->env_array\n> +\t */\n> +\tif (cp->env)\n> +\t\tconcatenate_env(&buf, cp->env);\n> +\n> +\tsq_quote_argv(&buf, cp->argv, 0);\n> +\tprint_trace_line(&trace_default_key, &buf);\n> +}\n> +\n> static const char *quote_crnl(const char *path)\n> {\n> \tstatic struct strbuf new_path = STRBUF_INIT;\n> diff --git a/trace.h b/trace.h\n> index 88055abef7..e54c687f26 100644\n> --- a/trace.h\n> +++ b/trace.h\n> @@ -4,6 +4,8 @@\n> #include \"git-compat-util.h\"\n> #include \"strbuf.h\"\n> \n> +struct child_process;\n> +\n> struct trace_key {\n> \tconst char * const key;\n> \tint fd;\n> @@ -17,6 +19,7 @@ extern struct trace_key trace_default_key;\n> extern struct trace_key trace_perf_key;\n> \n> extern void trace_repo_setup(const char *prefix);\n> +extern void trace_run_command(const struct child_process *cp);\n> extern int trace_want(struct trace_key *key);\n> extern void trace_disable(struct trace_key *key);\n> extern uint64_t getnanotime(void);\n> -- \n> 2.15.1.600.g899a5f85c6\n> \n\n-- \nBrandon Williams\n"},{"id":"336468","messageId":"CAGZ79kZDh6paRocJvWCtSEDZ=8+4+h0_QUStqBwkeLcvwqVCeg@mail.gmail.com","threadId":"47584","inReplyTo":"20180111175316.GD91567@google.com","subject":"Re: [PATCH v2] run-command.c: print env vars when GIT_TRACE is set","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-11T18:20:01Z","receivedAt":"2018-01-11T18:20:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 11, 2018 at 9:53 AM, Brandon Williams wrote:\n> On 01/11, Nguyễn Thái Ngọc Duy wrote:\n>> Occasionally submodule code could execute new commands with GIT_DIR set\n>> to some submodule. GIT_TRACE prints just the command line which makes it\n>> hard to tell that it's not really executed on this repository.\n>>\n>> Print modified env variables (compared to parent environment) in this\n>> case. Actually only modified or new variables are printed. Variable\n>> deletion is suppressed because they are often used to clean up\n>> repo-specific variables that git passes around between processes. When\n>> submodule code executes commands on another repo, it clears all these\n>> variables, _many_ of these, that make the output really noisy.\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> v2 fixes up commit message to clarify about \"env delta\" and why var\n>> deletion is not printed.\n>>\n>> It also keeps child_process tracing in one place/function, this\n>> should make it easier to trace more e.g. cwd and stuff.\n>>\n>> Though, Stefan, while i'm not opposed to trace every single setting\n>> in child_process, including variable deletion, cwd and even more, it\n>> may be not that often needed for a \"casual\" developer.\n>>\n>> I suggest we have something like $GIT_TRACE_EXEC instead that could\n>> be super verbose when we need it and leave $GIT_TRACE with a\n>> reasonable subset.\n\nMakes sense. Thanks for working on this!\nCode&commit msg look good to me. I agree with Brandon on the\ncomments grammar to have a missing piece.\n\nThanks,\nStefan\n"},{"id":"336469","messageId":"efacd948-0984-ef83-c249-b4139f61a519@kdbg.org","threadId":"47584","inReplyTo":"20180111100734.GA17274@sigill.intra.peff.net","subject":"Re: [PATCH] run-command.c: print env vars when GIT_TRACE is set","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-01-11T18:21:40Z","receivedAt":"2018-01-11T18:21:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.01.2018 um 11:07 schrieb Jeff King:\n> The output for a single command is pretty shell-like due to the quoting:\n> \n> $ GIT_TRACE=1 ./git upload-pack . >/dev/null\n> [...]run_command: 'git-upload-pack' '.'\n> \n> You could copy and paste that to a shell if you wanted. And with\n> environment variables, that remains so:\n> \n> $ GIT_TRACE=1 ./git ls-remote https://github.com/git/git >/dev/null\n> [...]run_command: 'GIT_DIR=.git' 'git-remote-https' 'https://[...]'\n\nNot quite, though. For variable assignments to be recognized as such, \nthe name and equal sign must not be quoted:\n\n GIT_DIR='.git' 'git-remote-https' 'https://[...]'\n\n-- Hannes\n"},{"id":"336473","messageId":"xmqq4lnsyz4l.fsf@gitster.mtv.corp.google.com","threadId":"47584","inReplyTo":"20180111094712.2551-1-pclouds@gmail.com","subject":"Re: [PATCH v2] run-command.c: print env vars when GIT_TRACE is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-11T19:27:38Z","receivedAt":"2018-01-11T19:27:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Occasionally submodule code could execute new commands with GIT_DIR set\n> to some submodule. GIT_TRACE prints just the command line which makes it\n> hard to tell that it's not really executed on this repository.\n>\n> Print modified env variables (compared to parent environment) in this\n> case. Actually only modified or new variables are printed. Variable\n> deletion is suppressed because ...\n\nWhen I read the above, I imagined that you are reducing noise from\nthe output by showing only modified or new variables, but it appears\nthat the implementation of concatenate_env() just blindly copies the\nvariables without checking if they are setting the same value.\n\nI wonder how common it would be to attempt exporting a variable with\nthe same value, and to attempt unsetting a variable that is not\nexported, which might help you reduce the clutter by hiding stuff\nthat truly do not matter, while keeping what matters (possibly\nincluding the \"unset\" part).\n\n> +static void concatenate_env(struct strbuf *dst, const char *const *env)\n> +{\n> +\tint i;\n> +\n> +\t/* Copy into destination buffer. */\n> +\tstrbuf_grow(dst, 255);\n> +\tfor (i = 0; env[i]; ++i) {\n> +\t\t/*\n> +\t\t * the main interesting is setting new vars\n\nI'll do s/interesting/& part/ while queuing.\n\n> +\t\t * e.g. GIT_DIR, ignore the unsetting to reduce noise.\n> +\t\t */\n> +\t\tif (!strchr(env[i], '='))\n> +\t\t\tcontinue;\n> +\t\tstrbuf_addch(dst, ' ');\n> +\t\tsq_quote_buf(dst, env[i]);\n\nI think you'd tweak the quoting around here in a later iteration,\nbased on the distinction between the two:\n\n\t$ 'GIT_DIR=.git' git foo\n\t$ GIT_DIR='.git' git foo\n\nthat was pointed out in a near-by message.\n\nThanks.\n"},{"id":"336489","messageId":"CACsJy8BrLJiwoKugJQhFj-TvUBDG5tcdTeOGnYPZ=H=6kSHdzA@mail.gmail.com","threadId":"47584","inReplyTo":"xmqq4lnsyz4l.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] run-command.c: print env vars when GIT_TRACE is set","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-12T09:23:12Z","receivedAt":"2018-01-12T09:23:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jan 12, 2018 at 2:27 AM, Junio C Hamano wrote:\n> I wonder how common it would be to attempt exporting a variable with\n> the same value, and to attempt unsetting a variable that is not\n> exported, which might help you reduce the clutter by hiding stuff\n> that truly do not matter, while keeping what matters (possibly\n> including the \"unset\" part).\n\nIt looks pretty good actually. This is with 'git status' and a repo\nwith submodule 'foo'.\n\ntrace: run_command: cd 'foo'; unset GIT_PREFIX; git 'status' '--porcelain=2'\n\nThanks for the suggestion.\n-- \nDuy\n"},{"id":"336490","messageId":"20180112095607.18293-1-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180111094712.2551-1-pclouds@gmail.com","subject":"[PATCH v3 0/4] run-command.c: print env vars when GIT_TRACE is set","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-12T09:56:03Z","receivedAt":"2018-01-12T09:57:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v3 turns a single patch into a series. Changes from v2\n\n- env var quoting is now done correctly (from shell syntax perspective)\n- the program name is prepended in git_cmd mode\n- cwd is now printed too (because I have too, see 4/4)\n- we don't blindly print the env delta anymore but print the actual\n differences compared to parent env\n- which means we also print \"unset XXX\" statements in $GIT_TRACE\n\nThe new output on git.git looks like this\n\n trace: run_command: cd 'sha1collisiondetection'; unset GIT_PREFIX; GIT_DIR='.git' git 'status' '--porcelain=2'\n\na bit longer than I would like, but that's because of\nsha1collisiondetection and it's not long enough for me to invent\n$GIT_TRACE_EXEC.\n\nI'm not adding \"sh -c\" back though because that looks like it should\nbe done in run_command.c itself, not just output tracing output.\n\nOh.. before anybody asks, I'm not adding git prefix to the \"cd\" part.\nYou're supposed to know that git moves back to worktree top dir when\nyou read $GIT_TRACE (or are welcome to improve the print out).\n\nNguyễn Thái Ngọc Duy (4):\n trace.c: introduce trace_run_command()\n trace.c: print program 'git' when child_process.git_cmd is set\n trace.c: print env vars in trace_run_command()\n trace.c: be smart about what env to print in trace_run_command()\n\n run-command.c | 3 +-\n trace.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n trace.h | 3 ++\n 3 files changed, 111 insertions(+), 1 deletion(-)\n\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336491","messageId":"20180112095607.18293-2-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180112095607.18293-1-pclouds@gmail.com","subject":"[PATCH v3 1/4] trace.c: introduce trace_run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-12T09:56:04Z","receivedAt":"2018-01-12T09:57:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This is the same as the old code that uses trace_argv_printf() in\nrun-command.c. This function will be improved in later patches to\nprint more information from struct child_process.\n\nA slight regression: the old code would print run-command.c:xxx as the\ntrace location site while the new code prints trace.c:xxx. This should\nbe fine until the second call site is added, then we might need a macro\nwrapper named trace_run_command() to capture the right source location.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n run-command.c | 3 ++-\n trace.c | 15 +++++++++++++++\n trace.h | 3 +++\n 3 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 31fc5ea86e..002074b128 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -624,7 +624,8 @@ int start_command(struct child_process *cmd)\n \t\tcmd->err = fderr[0];\n \t}\n \n-\ttrace_argv_printf(cmd->argv, \"trace: run_command:\");\n+\ttrace_run_command(cmd);\n+\n \tfflush(NULL);\n \n #ifndef GIT_WINDOWS_NATIVE\ndiff --git a/trace.c b/trace.c\nindex b7530b51a9..f08c673b51 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -23,6 +23,7 @@\n \n #include \"cache.h\"\n #include \"quote.h\"\n+#include \"run-command.h\"\n \n struct trace_key trace_default_key = { \"GIT_TRACE\", 0, 0, 0 };\n struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);\n@@ -272,6 +273,20 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,\n #endif /* HAVE_VARIADIC_MACROS */\n \n \n+void trace_run_command(const struct child_process *cp)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tif (!prepare_trace_line(__FILE__, __LINE__,\n+\t\t\t\t&trace_default_key, &buf))\n+\t\treturn;\n+\n+\tstrbuf_addf(&buf, \"trace: run_command:\");\n+\n+\tsq_quote_argv(&buf, cp->argv, 0);\n+\tprint_trace_line(&trace_default_key, &buf);\n+}\n+\n static const char *quote_crnl(const char *path)\n {\n \tstatic struct strbuf new_path = STRBUF_INIT;\ndiff --git a/trace.h b/trace.h\nindex 88055abef7..e54c687f26 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -4,6 +4,8 @@\n #include \"git-compat-util.h\"\n #include \"strbuf.h\"\n \n+struct child_process;\n+\n struct trace_key {\n \tconst char * const key;\n \tint fd;\n@@ -17,6 +19,7 @@ extern struct trace_key trace_default_key;\n extern struct trace_key trace_perf_key;\n \n extern void trace_repo_setup(const char *prefix);\n+extern void trace_run_command(const struct child_process *cp);\n extern int trace_want(struct trace_key *key);\n extern void trace_disable(struct trace_key *key);\n extern uint64_t getnanotime(void);\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336492","messageId":"20180112095607.18293-3-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180112095607.18293-1-pclouds@gmail.com","subject":"[PATCH v3 2/4] trace.c: print program 'git' when child_process.git_cmd is set","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-12T09:56:05Z","receivedAt":"2018-01-12T09:57:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"We normally print full command line, including the program and its\nargument. When git_cmd is set, we have a special code path to run the\nright \"git\" program and child_process.argv[0] will not contain the\nprogram name anymore. As a result, we print just the command\narguments.\n\nI thought it was a regression when the code was refactored and git_cmd\nadded, but apparently it's not. git_cmd mode was introduced before\ntracing was added in 8852f5d704 (run_command(): respect GIT_TRACE -\n2008-07-07) so it's more like an oversight in 8852f5d704.\n\nFix it, print the program name \"git\" in git_cmd mode. It's nice to have\nnow. But it will be more important later when we start to print env\nvariables too, in shell syntax. The lack of a program name would look\nconfusing then.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n trace.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/trace.c b/trace.c\nindex f08c673b51..ed1f728021 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -283,6 +283,9 @@ void trace_run_command(const struct child_process *cp)\n \n \tstrbuf_addf(&buf, \"trace: run_command:\");\n \n+\tif (cp->git_cmd)\n+\t\tstrbuf_addstr(&buf, \" git\");\n+\n \tsq_quote_argv(&buf, cp->argv, 0);\n \tprint_trace_line(&trace_default_key, &buf);\n }\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336493","messageId":"20180112095607.18293-4-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180112095607.18293-1-pclouds@gmail.com","subject":"[PATCH v3 3/4] trace.c: print env vars in trace_run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-12T09:56:06Z","receivedAt":"2018-01-12T09:57:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Occasionally submodule code could execute new commands with GIT_DIR set\nto some submodule. GIT_TRACE prints just the command line which makes it\nhard to tell that it's not really executed on this repository.\n\nPrint modified env variables (compared to parent environment) in this\ncase. Actually only modified or new variables (*) are printed. Variable\ndeletion is suppressed because they are often used to clean up\nrepo-specific variables that git passes around between processes. When\nsubmodule code executes commands on another repo, it clears all these\nvariables, _many_ of these, that make the output really noisy.\n\n(*) sort of. More on this in the next patch.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n trace.c | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/trace.c b/trace.c\nindex ed1f728021..e499074d39 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -273,6 +273,24 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,\n #endif /* HAVE_VARIADIC_MACROS */\n \n \n+static void concatenate_env(struct strbuf *dst, const char *const *envs)\n+{\n+\tint i;\n+\n+\tfor (i = 0; envs[i]; i++) {\n+\t\tconst char *env = envs[i];\n+\t\tconst char *p = strchr(env, '=');\n+\n+\t\tif (!p) /* ignore var deletion for now */\n+\t\t\tcontinue;\n+\t\tp++;\n+\n+\t\tstrbuf_addch(dst, ' ');\n+\t\tstrbuf_add(dst, env, p - env);\n+\t\tsq_quote_buf(dst, p);\n+\t}\n+}\n+\n void trace_run_command(const struct child_process *cp)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -281,8 +299,17 @@ void trace_run_command(const struct child_process *cp)\n \t\t\t\t&trace_default_key, &buf))\n \t\treturn;\n \n+\tstrbuf_grow(&buf, 255);\n+\n \tstrbuf_addf(&buf, \"trace: run_command:\");\n \n+\t/*\n+\t * The caller is responsible for initializing cp->env from\n+\t * cp->env_array if needed. We only check one place.\n+\t */\n+\tif (cp->env)\n+\t\tconcatenate_env(&buf, cp->env);\n+\n \tif (cp->git_cmd)\n \t\tstrbuf_addstr(&buf, \" git\");\n \n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336494","messageId":"20180112095607.18293-5-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180112095607.18293-1-pclouds@gmail.com","subject":"[PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-12T09:56:07Z","receivedAt":"2018-01-12T09:57:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The previous concatenate_env() is kinda dumb. It receives a env delta\nin child_process and blindly follows it. If the run_command() user\n\"adds\" more vars of the same value, or deletes vars that do not exist\nin parent env in the first place (*), then why bother to print them?\n\nThis patch checks child_process.env against parent environment and\nonly print the actual differences. The unset syntax (and later on cwd)\nfollows closely shell syntax for easy reading/guessing and copy/paste.\n\nThere is an interesting thing with this change. In the previous patch,\nwe unconditionally print new vars. With submodule code we may have\nsomething like this\n\n trace: ... GIT_DIR='.git' git 'status' '--porcelain=2'\n\nbut since parent's GIT_DIR usually has the same value '.git' too, this\nchange suppress that, now we can't see that the above command runs in\na separate repo. This is the run for printing cwd. Now we have\n\n trace: ... cd foo; git 'status' '--porcelain=2'\n\nAnother equally interesting thing is, some caller can do \"unset GIT_DIR;\nset GIT_DIR=.git\". Since parent env can have the same value '.git', we\ndon't re-print GIT_DIR=.git. In that case must NOT print \"unset GIT_DIR\"\nor the user will be misled to think the new command has no GIT_DIR.\n\nA note about performance. Yes we're burning a lot more cycles for\ndisplaying something this nice. But because it's protected by\n$GIT_TRACE, and run_command() should not happen often anyway, it should\nnot feel any slower than before.\n\n(*) this is the case with submodule code where all local_repo_env[]\nvars are to be deleted even though many of them do not exist in the\nfirst place. Printing all these is too noisy and that leads to\nignoring variable deletion in the previous patch.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n trace.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n 1 file changed, 69 insertions(+), 8 deletions(-)\n\ndiff --git a/trace.c b/trace.c\nindex e499074d39..a1f871e720 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -272,23 +272,78 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,\n \n #endif /* HAVE_VARIADIC_MACROS */\n \n+static void sort_deltaenv(struct string_list *envs,\n+\t\t\t const char *const *deltaenv)\n+{\n+\tstruct strbuf key = STRBUF_INIT;\n+\tconst char *const *e;\n+\n+\tfor (e = deltaenv; e && *e; e++) {\n+\t\tchar *equals = strchr(*e, '=');\n+\n+\t\tif (equals) {\n+\t\t\tstrbuf_reset(&key);\n+\t\t\tstrbuf_add(&key, *e, equals - *e);\n+\t\t\tstring_list_append(envs, key.buf)->util = equals + 1;\n+\t\t} else {\n+\t\t\tstring_list_append(envs, *e)->util = NULL;\n+\t\t}\n+\t}\n+\tstring_list_sort(envs);\n+\tstrbuf_release(&key);\n+}\n \n-static void concatenate_env(struct strbuf *dst, const char *const *envs)\n+\n+static void concatenate_env(struct strbuf *dst, const char *const *deltaenv)\n {\n+\tstruct string_list envs = STRING_LIST_INIT_DUP;\n \tint i;\n \n-\tfor (i = 0; envs[i]; i++) {\n-\t\tconst char *env = envs[i];\n-\t\tconst char *p = strchr(env, '=');\n+\t/*\n+\t * Construct a sorted string list consisting of the delta\n+\t * env. We need this to detect the case when the same var is\n+\t * deleted first, then added again.\n+\t */\n+\tsort_deltaenv(&envs, deltaenv);\n+\n+\t/*\n+\t * variable deletion first because it's printed like separate\n+\t * shell commands\n+\t */\n+\tfor (i = 0; i < envs.nr; i++) {\n+\t\tconst char *env = envs.items[i].string;\n+\t\tconst char *p = envs.items[i].util;\n+\n+\t\tif (p || !getenv(env))\n+\t\t\tcontinue;\n \n-\t\tif (!p) /* ignore var deletion for now */\n+\t\t/*\n+\t\t * Do we have a sequence of \"unset GIT_DIR; GIT_DIR=foo\"?\n+\t\t * Then don't bother with the unset thing.\n+\t\t */\n+\t\tif (i + 1 < envs.nr &&\n+\t\t !strcmp(env, envs.items[i + 1].string))\n \t\t\tcontinue;\n-\t\tp++;\n \n-\t\tstrbuf_addch(dst, ' ');\n-\t\tstrbuf_add(dst, env, p - env);\n+\t\tstrbuf_addf(dst, \" unset %s;\", env);\n+\t}\n+\n+\tfor (i = 0; i < envs.nr; i++) {\n+\t\tconst char *env = envs.items[i].string;\n+\t\tconst char *p = envs.items[i].util;\n+\t\tconst char *old_value;\n+\n+\t\tif (!p)\n+\t\t\tcontinue;\n+\n+\t\told_value = getenv(env);\n+\t\tif (old_value && !strcmp(old_value, p))\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_addf(dst, \" %s=\", env);\n \t\tsq_quote_buf(dst, p);\n \t}\n+\tstring_list_clear(&envs, 0);\n }\n \n void trace_run_command(const struct child_process *cp)\n@@ -303,6 +358,12 @@ void trace_run_command(const struct child_process *cp)\n \n \tstrbuf_addf(&buf, \"trace: run_command:\");\n \n+\tif (cp->dir) {\n+\t\tstrbuf_addstr(&buf, \" cd \");\n+\t\tsq_quote_buf(&buf, cp->dir);\n+\t\tstrbuf_addch(&buf, ';');\n+\t}\n+\n \t/*\n \t * The caller is responsible for initializing cp->env from\n \t * cp->env_array if needed. We only check one place.\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336500","messageId":"20180112130522.GB27499@sigill.intra.peff.net","threadId":"47584","inReplyTo":"20180112095607.18293-2-pclouds@gmail.com","subject":"Re: [PATCH v3 1/4] trace.c: introduce trace_run_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-12T13:05:22Z","receivedAt":"2018-01-12T13:05:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 12, 2018 at 04:56:04PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> This is the same as the old code that uses trace_argv_printf() in\n> run-command.c. This function will be improved in later patches to\n> print more information from struct child_process.\n> \n> A slight regression: the old code would print run-command.c:xxx as the\n> trace location site while the new code prints trace.c:xxx. This should\n> be fine until the second call site is added, then we might need a macro\n> wrapper named trace_run_command() to capture the right source location.\n\nI agree that's probably fine for now.\n\n> +void trace_run_command(const struct child_process *cp)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\tif (!prepare_trace_line(__FILE__, __LINE__,\n> +\t\t\t\t&trace_default_key, &buf))\n> +\t\treturn;\n> +\n> +\tstrbuf_addf(&buf, \"trace: run_command:\");\n> +\n> +\tsq_quote_argv(&buf, cp->argv, 0);\n> +\tprint_trace_line(&trace_default_key, &buf);\n> +}\n\nIt looks like this leaks \"buf\".\n\nIf prepare_trace_line() returns 0, I think it's safe to assume that\nnothing was allocated. So we'd just need a strbuf_release() at the end.\n\nLooking at the other trace functions, it looks like a bunch of them have\nthe same problem.\n\n-Peff\n"},{"id":"336501","messageId":"20180112130554.GC27499@sigill.intra.peff.net","threadId":"47584","inReplyTo":"20180112095607.18293-3-pclouds@gmail.com","subject":"Re: [PATCH v3 2/4] trace.c: print program 'git' when child_process.git_cmd is set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-12T13:05:54Z","receivedAt":"2018-01-12T13:06:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 12, 2018 at 04:56:05PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> We normally print full command line, including the program and its\n> argument. When git_cmd is set, we have a special code path to run the\n> right \"git\" program and child_process.argv[0] will not contain the\n> program name anymore. As a result, we print just the command\n> arguments.\n> \n> I thought it was a regression when the code was refactored and git_cmd\n> added, but apparently it's not. git_cmd mode was introduced before\n> tracing was added in 8852f5d704 (run_command(): respect GIT_TRACE -\n> 2008-07-07) so it's more like an oversight in 8852f5d704.\n> \n> Fix it, print the program name \"git\" in git_cmd mode. It's nice to have\n> now. But it will be more important later when we start to print env\n> variables too, in shell syntax. The lack of a program name would look\n> confusing then.\n\nMakes sense. Note that \"git\" isn't quoted here, but the rest of the\narguments are. More on that in a minute. :)\n\n-Peff\n"},{"id":"336502","messageId":"20180112131101.GA7880@sigill.intra.peff.net","threadId":"47584","inReplyTo":"20180112130522.GB27499@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] trace.c: introduce trace_run_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-12T13:11:02Z","receivedAt":"2018-01-12T13:11:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 12, 2018 at 08:05:22AM -0500, Jeff King wrote:\n\n> > +void trace_run_command(const struct child_process *cp)\n> > +{\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\n> > +\tif (!prepare_trace_line(__FILE__, __LINE__,\n> > +\t\t\t\t&trace_default_key, &buf))\n> > +\t\treturn;\n> > +\n> > +\tstrbuf_addf(&buf, \"trace: run_command:\");\n> > +\n> > +\tsq_quote_argv(&buf, cp->argv, 0);\n> > +\tprint_trace_line(&trace_default_key, &buf);\n> > +}\n> \n> It looks like this leaks \"buf\".\n> \n> If prepare_trace_line() returns 0, I think it's safe to assume that\n> nothing was allocated. So we'd just need a strbuf_release() at the end.\n> \n> Looking at the other trace functions, it looks like a bunch of them have\n> the same problem.\n\nNever mind, I got fooled by print_trace_line(), which does quite a bit\nmore than its name says. :)\n\nThis call (and the existing ones) are all fine.\n"},{"id":"336503","messageId":"20180112131301.GD27499@sigill.intra.peff.net","threadId":"47584","inReplyTo":"20180112095607.18293-4-pclouds@gmail.com","subject":"Re: [PATCH v3 3/4] trace.c: print env vars in trace_run_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-12T13:13:02Z","receivedAt":"2018-01-12T13:13:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 12, 2018 at 04:56:06PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> Occasionally submodule code could execute new commands with GIT_DIR set\n> to some submodule. GIT_TRACE prints just the command line which makes it\n> hard to tell that it's not really executed on this repository.\n> \n> Print modified env variables (compared to parent environment) in this\n> case. Actually only modified or new variables (*) are printed. Variable\n> deletion is suppressed because they are often used to clean up\n> repo-specific variables that git passes around between processes. When\n> submodule code executes commands on another repo, it clears all these\n> variables, _many_ of these, that make the output really noisy.\n> \n> (*) sort of. More on this in the next patch.\n\nOK. I think we could probably just squash patches 3 and 4, but I'm OK\nwith them separate, too.\n\nThe code looks fine, though I have one nit:\n\n> @@ -281,8 +299,17 @@ void trace_run_command(const struct child_process *cp)\n> \t\t\t\t&trace_default_key, &buf))\n> \t\treturn;\n> \n> +\tstrbuf_grow(&buf, 255);\n> +\n\nIMHO these magic-numbers grows detract from the readability (because you\nwonder if they're meaningful), and I doubt if they provide any real\nperformance gain in practice.\n\n-Peff\n"},{"id":"336505","messageId":"20180112133355.GE27499@sigill.intra.peff.net","threadId":"47584","inReplyTo":"20180112095607.18293-5-pclouds@gmail.com","subject":"Re: [PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-12T13:33:55Z","receivedAt":"2018-01-12T13:34:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 12, 2018 at 04:56:07PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> The previous concatenate_env() is kinda dumb. It receives a env delta\n> in child_process and blindly follows it. If the run_command() user\n> \"adds\" more vars of the same value, or deletes vars that do not exist\n> in parent env in the first place (*), then why bother to print them?\n> \n> This patch checks child_process.env against parent environment and\n> only print the actual differences. The unset syntax (and later on cwd)\n> follows closely shell syntax for easy reading/guessing and copy/paste.\n\nI like it.\n\n> There is an interesting thing with this change. In the previous patch,\n> we unconditionally print new vars. With submodule code we may have\n> something like this\n> \n> trace: ... GIT_DIR='.git' git 'status' '--porcelain=2'\n> \n> but since parent's GIT_DIR usually has the same value '.git' too, this\n> change suppress that, now we can't see that the above command runs in\n> a separate repo. This is the run for printing cwd. Now we have\n> \n> trace: ... cd foo; git 'status' '--porcelain=2'\n\nMakes sense (though s/run/reason/ in the last paragraph?).\n\n> Another equally interesting thing is, some caller can do \"unset GIT_DIR;\n> set GIT_DIR=.git\". Since parent env can have the same value '.git', we\n> don't re-print GIT_DIR=.git. In that case must NOT print \"unset GIT_DIR\"\n> or the user will be misled to think the new command has no GIT_DIR.\n\nInteresting. I wonder if any callers actually do that. It seems like\nkind of an odd thing, but maybe a caller sets GIT_DIR on top of the\nclearing of local_repo_env.\n\n> A note about performance. Yes we're burning a lot more cycles for\n> displaying something this nice. But because it's protected by\n> $GIT_TRACE, and run_command() should not happen often anyway, it should\n> not feel any slower than before.\n\nI'd agree that performance probably doesn't matter much here.\n\n> +\t\t/*\n> +\t\t * Do we have a sequence of \"unset GIT_DIR; GIT_DIR=foo\"?\n> +\t\t * Then don't bother with the unset thing.\n> +\t\t */\n> +\t\tif (i + 1 < envs.nr &&\n> +\t\t !strcmp(env, envs.items[i + 1].string))\n> \t\t\tcontinue;\n\nAre we guaranteed that \"FOO\" and \"FOO=bar\" appear next to each other in the\nsorted list? I think \"FOO123=bar\" could come between.\n\nI also think this is a special case of a more general problem. FOO could\nappear any number of times in the \"env\" array, as a deletion or with\nmultiple values. Our prep_childenv() would treat that as \"last one\nwins\", I think. Could we just do the same here?\n\n-Peff\n"},{"id":"336506","messageId":"20180112133613.GF27499@sigill.intra.peff.net","threadId":"47584","inReplyTo":"20180112095607.18293-1-pclouds@gmail.com","subject":"Re: [PATCH v3 0/4] run-command.c: print env vars when GIT_TRACE is set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-12T13:36:14Z","receivedAt":"2018-01-12T13:36:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 12, 2018 at 04:56:03PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> v3 turns a single patch into a series. Changes from v2\n> \n> - env var quoting is now done correctly (from shell syntax perspective)\n> - the program name is prepended in git_cmd mode\n> - cwd is now printed too (because I have too, see 4/4)\n> - we don't blindly print the env delta anymore but print the actual\n> differences compared to parent env\n> - which means we also print \"unset XXX\" statements in $GIT_TRACE\n\nOverall I like it, though I raised on corner case in 4/4.\n\n> The new output on git.git looks like this\n> \n> trace: run_command: cd 'sha1collisiondetection'; unset GIT_PREFIX; GIT_DIR='.git' git 'status' '--porcelain=2'\n> \n> a bit longer than I would like, but that's because of\n> sha1collisiondetection and it's not long enough for me to invent\n> $GIT_TRACE_EXEC.\n\nIMHO the unconditional single-quotes make this longer and uglier than it\nneeds to be. I've often been tempted to have them kick in only when\nnecessary. Here are some patches to do that (on top of yours).\n\n [5/4]: sq_quote_argv: drop maxlen parameter\n [6/4]: trace: avoid unnecessary quoting\n\n builtin/am.c | 2 +-\n builtin/rev-parse.c | 4 ++--\n quote.c | 30 +++++++++++++++++++++++++++---\n quote.h | 10 +++++++++-\n trace.c | 10 +++++-----\n 5 files changed, 44 insertions(+), 12 deletions(-)\n\n-Peff\n"},{"id":"336507","messageId":"20180112133810.GA8290@sigill.intra.peff.net","threadId":"47584","inReplyTo":"20180112133613.GF27499@sigill.intra.peff.net","subject":"[PATCH 5/4] sq_quote_argv: drop maxlen parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-12T13:38:10Z","receivedAt":"2018-01-12T13:38:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"No caller passes anything but \"0\" for this parameter, which\nrequests that the function ignore it completely. In fact, in\nall of history there was only one such caller, and it went\naway in 7f51f8bc2b (alias: use run_command api to execute\naliases, 2011-01-07).\n\nSigned-off-by: Jeff King \n---\nJust a cleanup, though it touches the caller you just added, too.\n\n builtin/am.c | 2 +-\n builtin/rev-parse.c | 4 ++--\n quote.c | 4 +---\n quote.h | 2 +-\n trace.c | 6 +++---\n 5 files changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex acfe9d3c8c..5bdd2d7578 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1061,7 +1061,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t}\n \twrite_state_text(state, \"scissors\", str);\n \n-\tsq_quote_argv(&sb, state->git_apply_opts.argv, 0);\n+\tsq_quote_argv(&sb, state->git_apply_opts.argv);\n \twrite_state_text(state, \"apply-opt\", sb.buf);\n \n \tif (state->rebasing)\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 74aa644cbb..96d06a5d01 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -516,7 +516,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_SHELL_EVAL);\n \n \tstrbuf_addstr(&parsed, \" --\");\n-\tsq_quote_argv(&parsed, argv, 0);\n+\tsq_quote_argv(&parsed, argv);\n \tputs(parsed.buf);\n \treturn 0;\n }\n@@ -526,7 +526,7 @@ static int cmd_sq_quote(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \n \tif (argc)\n-\t\tsq_quote_argv(&buf, argv, 0);\n+\t\tsq_quote_argv(&buf, argv);\n \tprintf(\"%s\\n\", buf.buf);\n \tstrbuf_release(&buf);\n \ndiff --git a/quote.c b/quote.c\nindex de2922ddd6..b2970da627 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -56,7 +56,7 @@ void sq_quotef(struct strbuf *dst, const char *fmt, ...)\n \tstrbuf_release(&src);\n }\n \n-void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)\n+void sq_quote_argv(struct strbuf *dst, const char **argv)\n {\n \tint i;\n \n@@ -65,8 +65,6 @@ void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)\n \tfor (i = 0; argv[i]; ++i) {\n \t\tstrbuf_addch(dst, ' ');\n \t\tsq_quote_buf(dst, argv[i]);\n-\t\tif (maxlen && dst->len > maxlen)\n-\t\t\tdie(\"Too many or long arguments\");\n \t}\n }\n \ndiff --git a/quote.h b/quote.h\nindex 66f5644aa2..48a75a416b 100644\n--- a/quote.h\n+++ b/quote.h\n@@ -30,7 +30,7 @@ struct strbuf;\n */\n \n extern void sq_quote_buf(struct strbuf *, const char *src);\n-extern void sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);\n+extern void sq_quote_argv(struct strbuf *, const char **argv);\n extern void sq_quotef(struct strbuf *, const char *fmt, ...);\n \n /* This unwraps what sq_quote() produces in place, but returns\ndiff --git a/trace.c b/trace.c\nindex a1f871e720..973e462a2c 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -158,7 +158,7 @@ static void trace_argv_vprintf_fl(const char *file, int line,\n \n \tstrbuf_vaddf(&buf, format, ap);\n \n-\tsq_quote_argv(&buf, argv, 0);\n+\tsq_quote_argv(&buf, argv);\n \tprint_trace_line(&trace_default_key, &buf);\n }\n \n@@ -374,7 +374,7 @@ void trace_run_command(const struct child_process *cp)\n \tif (cp->git_cmd)\n \t\tstrbuf_addstr(&buf, \" git\");\n \n-\tsq_quote_argv(&buf, cp->argv, 0);\n+\tsq_quote_argv(&buf, cp->argv);\n \tprint_trace_line(&trace_default_key, &buf);\n }\n \n@@ -532,6 +532,6 @@ void trace_command_performance(const char **argv)\n \t\tatexit(print_command_performance_atexit);\n \n \tstrbuf_reset(&command_line);\n-\tsq_quote_argv(&command_line, argv, 0);\n+\tsq_quote_argv(&command_line, argv);\n \tcommand_start_time = getnanotime();\n }\n-- \n2.16.0.rc1.459.g7b9f90244d\n\n"},{"id":"336508","messageId":"20180112133936.GB8290@sigill.intra.peff.net","threadId":"47584","inReplyTo":"20180112133613.GF27499@sigill.intra.peff.net","subject":"[PATCH 6/4] trace: avoid unnecessary quoting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-12T13:39:36Z","receivedAt":"2018-01-12T13:39:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Trace output which contains arbitrary strings (e.g., the\narguments to commands which we are running) is always passed\nthrough sq_quote_buf(). That function always adds\nsingle-quotes, even if the output consists of vanilla\ncharacters. This can make the output a bit hard to read.\n\nLet's avoid the quoting if there are no characters which a\nshell would interpret. Trace output doesn't necessarily need\nto be shell-compatible, but:\n\n - the shell language is a good ballpark for what humans\n consider readable (well, humans versed in command line\n tools)\n\n - the run_command bits can be cut-and-pasted to a shell,\n and we'll keep that property\n\n - it covers any cases which would make the output\n visually ambiguous (e.g., embedded whitespace or quotes)\n\nSigned-off-by: Jeff King \n---\n quote.c | 26 ++++++++++++++++++++++++++\n quote.h | 8 ++++++++\n trace.c | 10 +++++-----\n 3 files changed, 39 insertions(+), 5 deletions(-)\n\ndiff --git a/quote.c b/quote.c\nindex b2970da627..37d2686865 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -43,6 +43,22 @@ void sq_quote_buf(struct strbuf *dst, const char *src)\n \tfree(to_free);\n }\n \n+void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n+{\n+\tstatic const char ok_punct[] = \"+,-./:=@_^\";\n+\tconst char *p;\n+\n+\tfor (p = src; *p; p++) {\n+\t\tif (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {\n+\t\t\tsq_quote_buf(dst, src);\n+\t\t\treturn;\n+\t\t}\n+\t}\n+\n+\t/* if we get here, we did not need quoting */\n+\tstrbuf_addstr(dst, src);\n+}\n+\n void sq_quotef(struct strbuf *dst, const char *fmt, ...)\n {\n \tstruct strbuf src = STRBUF_INIT;\n@@ -68,6 +84,16 @@ void sq_quote_argv(struct strbuf *dst, const char **argv)\n \t}\n }\n \n+void sq_quote_argv_pretty(struct strbuf *dst, const char **argv)\n+{\n+\tint i;\n+\n+\tfor (i = 0; argv[i]; i++) {\n+\t\tstrbuf_addch(dst, ' ');\n+\t\tsq_quote_buf_pretty(dst, argv[i]);\n+\t}\n+}\n+\n static char *sq_dequote_step(char *arg, char **next)\n {\n \tchar *dst = arg;\ndiff --git a/quote.h b/quote.h\nindex 48a75a416b..ea992dcc91 100644\n--- a/quote.h\n+++ b/quote.h\n@@ -33,6 +33,14 @@ extern void sq_quote_buf(struct strbuf *, const char *src);\n extern void sq_quote_argv(struct strbuf *, const char **argv);\n extern void sq_quotef(struct strbuf *, const char *fmt, ...);\n \n+/*\n+ * These match their non-pretty variants, except that they avoid\n+ * quoting when there are no exotic characters. These should only be used for\n+ * human-readable output, as sq_dequote() is not smart enough to dequote it.\n+ */\n+void sq_quote_buf_pretty(struct strbuf *, const char *src);\n+void sq_quote_argv_pretty(struct strbuf *, const char **argv);\n+\n /* This unwraps what sq_quote() produces in place, but returns\n * NULL if the input does not look like what sq_quote would have\n * produced.\ndiff --git a/trace.c b/trace.c\nindex 973e462a2c..aba2825044 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -158,7 +158,7 @@ static void trace_argv_vprintf_fl(const char *file, int line,\n \n \tstrbuf_vaddf(&buf, format, ap);\n \n-\tsq_quote_argv(&buf, argv);\n+\tsq_quote_argv_pretty(&buf, argv);\n \tprint_trace_line(&trace_default_key, &buf);\n }\n \n@@ -341,7 +341,7 @@ static void concatenate_env(struct strbuf *dst, const char *const *deltaenv)\n \t\t\tcontinue;\n \n \t\tstrbuf_addf(dst, \" %s=\", env);\n-\t\tsq_quote_buf(dst, p);\n+\t\tsq_quote_buf_pretty(dst, p);\n \t}\n \tstring_list_clear(&envs, 0);\n }\n@@ -360,7 +360,7 @@ void trace_run_command(const struct child_process *cp)\n \n \tif (cp->dir) {\n \t\tstrbuf_addstr(&buf, \" cd \");\n-\t\tsq_quote_buf(&buf, cp->dir);\n+\t\tsq_quote_buf_pretty(&buf, cp->dir);\n \t\tstrbuf_addch(&buf, ';');\n \t}\n \n@@ -374,7 +374,7 @@ void trace_run_command(const struct child_process *cp)\n \tif (cp->git_cmd)\n \t\tstrbuf_addstr(&buf, \" git\");\n \n-\tsq_quote_argv(&buf, cp->argv);\n+\tsq_quote_argv_pretty(&buf, cp->argv);\n \tprint_trace_line(&trace_default_key, &buf);\n }\n \n@@ -532,6 +532,6 @@ void trace_command_performance(const char **argv)\n \t\tatexit(print_command_performance_atexit);\n \n \tstrbuf_reset(&command_line);\n-\tsq_quote_argv(&command_line, argv);\n+\tsq_quote_argv_pretty(&command_line, argv);\n \tcommand_start_time = getnanotime();\n }\n-- \n2.16.0.rc1.459.g7b9f90244d\n"},{"id":"336519","messageId":"CAGZ79kYUyugA_BhMgr_k1aWfxg9+cNbOcVO+LOZkZ1YAAu7_mQ@mail.gmail.com","threadId":"47584","inReplyTo":"20180112095607.18293-1-pclouds@gmail.com","subject":"Re: [PATCH v3 0/4] run-command.c: print env vars when GIT_TRACE is set","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-12T17:19:05Z","receivedAt":"2018-01-12T17:19:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jan 12, 2018 at 1:56 AM, Nguyễn Thái Ngọc Duy wrote:\n> v3 turns a single patch into a series. Changes from v2\n>\n> - env var quoting is now done correctly (from shell syntax perspective)\n> - the program name is prepended in git_cmd mode\n> - cwd is now printed too (because I have too, see 4/4)\n> - we don't blindly print the env delta anymore but print the actual\n> differences compared to parent env\n> - which means we also print \"unset XXX\" statements in $GIT_TRACE\n>\n> The new output on git.git looks like this\n>\n> trace: run_command: cd 'sha1collisiondetection'; unset GIT_PREFIX; GIT_DIR='.git' git 'status' '--porcelain=2'\n>\n> a bit longer than I would like, but that's because of\n> sha1collisiondetection and it's not long enough for me to invent\n> $GIT_TRACE_EXEC.\n>\n> I'm not adding \"sh -c\" back though because that looks like it should\n> be done in run_command.c itself, not just output tracing output.\n\nWow! This is really cool. Thanks for your effort.\n(Also I learned today that I can just copy the command, up to now I always\ndequoted the commands when rerunning it for debugging as I assumed I had to)\n\n>\n> Oh.. before anybody asks, I'm not adding git prefix to the \"cd\" part.\n> You're supposed to know that git moves back to worktree top dir when\n> you read $GIT_TRACE (or are welcome to improve the print out).\n\nHeh. Would appending \"; cd -\" solve the issue? (Though I would argue\nwe don't need or want to add that actually, as you'd want to copy and run\nthe command and surely want to run more tests in that directory afterwards.\nI mean you're debugging, right?)\n"},{"id":"336520","messageId":"CAGZ79kZRFCLEtyWCjKm-vM36Gf1EJfEAAFxEFz0wQHv4bqPc_A@mail.gmail.com","threadId":"47584","inReplyTo":"20180112133613.GF27499@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/4] run-command.c: print env vars when GIT_TRACE is set","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-12T18:06:02Z","receivedAt":"2018-01-12T18:06:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jan 12, 2018 at 5:36 AM, Jeff King wrote:\n> On Fri, Jan 12, 2018 at 04:56:03PM +0700, Nguyễn Thái Ngọc Duy wrote:\n>\n>> v3 turns a single patch into a series. Changes from v2\n>>\n>> - env var quoting is now done correctly (from shell syntax perspective)\n>> - the program name is prepended in git_cmd mode\n>> - cwd is now printed too (because I have too, see 4/4)\n>> - we don't blindly print the env delta anymore but print the actual\n>> differences compared to parent env\n>> - which means we also print \"unset XXX\" statements in $GIT_TRACE\n>\n> Overall I like it, though I raised on corner case in 4/4.\n>\n>> The new output on git.git looks like this\n>>\n>> trace: run_command: cd 'sha1collisiondetection'; unset GIT_PREFIX; GIT_DIR='.git' git 'status' '--porcelain=2'\n>>\n>> a bit longer than I would like, but that's because of\n>> sha1collisiondetection and it's not long enough for me to invent\n>> $GIT_TRACE_EXEC.\n>\n> IMHO the unconditional single-quotes make this longer and uglier than it\n> needs to be. I've often been tempted to have them kick in only when\n> necessary. Here are some patches to do that (on top of yours).\n>\n> [5/4]: sq_quote_argv: drop maxlen parameter\n> [6/4]: trace: avoid unnecessary quoting\n>\n\nI have reviewed the whole series 6/4 and think they can be applied as-is,\nthough I have not thought about the corner case you mentioned.\n\nThanks for this series!\nStefan\n"},{"id":"336521","messageId":"xmqqpo6fvstf.fsf@gitster.mtv.corp.google.com","threadId":"47584","inReplyTo":"20180112133355.GE27499@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-12T18:24:28Z","receivedAt":"2018-01-12T18:24:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +\t\t/*\n>> +\t\t * Do we have a sequence of \"unset GIT_DIR; GIT_DIR=foo\"?\n>> +\t\t * Then don't bother with the unset thing.\n>> +\t\t */\n>> +\t\tif (i + 1 < envs.nr &&\n>> +\t\t !strcmp(env, envs.items[i + 1].string))\n>> \t\t\tcontinue;\n>\n> Are we guaranteed that \"FOO\" and \"FOO=bar\" appear next to each other in the\n> sorted list? I think \"FOO123=bar\" could come between.\n\nAt this point, envs is a string list whose key is FOO for both \"FOO\"\n(unset) and \"FOO=bar\" (set); \"FOO123=bar\" would sort after these two.\n\nBut I did not see anything that attempts to guarantee that \"FOO\"\nsorts before \"FOO=bar\" with string_list_sort(). If the sort used in\nthe function is stable, and if the case we care about is unset\nfollowed by set, then the above would catch the case, but even if\nthat were the case, it is unclear what we want to do when a set of\nFOO is followed by an unset of FOO.\n\nAnd if the sort is not stable, then the above code is just simply\nbroken.\n\n> I also think this is a special case of a more general problem. FOO could\n> appear any number of times in the \"env\" array, as a deletion or with\n> multiple values. Our prep_childenv() would treat that as \"last one\n> wins\", I think. Could we just do the same here?\n\nYeah, if the last one is to set FOO=bar, then it feels sufficient to\njust check if FOO is set to bar in the original and deciding to show\nor hide; similarly if the last one is to unset FOO, it does not matter\nif the caller sets it to some other value before unsetting, so it\nfeels sufficient to just check if FOO is set to anything in the\noriginal environment.\n\n\n\n"},{"id":"336522","messageId":"20180112184546.GA15514@sigill.intra.peff.net","threadId":"47584","inReplyTo":"xmqqpo6fvstf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-12T18:45:46Z","receivedAt":"2018-01-12T18:45:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 12, 2018 at 10:24:28AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> +\t\t/*\n> >> +\t\t * Do we have a sequence of \"unset GIT_DIR; GIT_DIR=foo\"?\n> >> +\t\t * Then don't bother with the unset thing.\n> >> +\t\t */\n> >> +\t\tif (i + 1 < envs.nr &&\n> >> +\t\t !strcmp(env, envs.items[i + 1].string))\n> >> \t\t\tcontinue;\n> >\n> > Are we guaranteed that \"FOO\" and \"FOO=bar\" appear next to each other in the\n> > sorted list? I think \"FOO123=bar\" could come between.\n> \n> At this point, envs is a string list whose key is FOO for both \"FOO\"\n> (unset) and \"FOO=bar\" (set); \"FOO123=bar\" would sort after these two.\n\nAh, right, I didn't notice that we took just the key name.\n\n> But I did not see anything that attempts to guarantee that \"FOO\"\n> sorts before \"FOO=bar\" with string_list_sort(). If the sort used in\n> the function is stable, and if the case we care about is unset\n> followed by set, then the above would catch the case, but even if\n> that were the case, it is unclear what we want to do when a set of\n> FOO is followed by an unset of FOO.\n\nThe actual prep_childenv() code looks like it would always do\nlast-one-wins, so we should treat FOO as unset in that final case. But\nthat only kicks in on non-Windows.\n\nOn Windows we feed cmd->env straight to mingw_spawnvpe(). That ends up\nin make_environment_block(), which looks like it does something similar.\n\nIt's too bad the prep code is not shared, since then we could probably\njust ask _it_ which deltas it applied. I suspect it would be possible to\nshare it, since mingw_spawnvpe is our own compat thing (so we could\nchange its interface to take the whole prepared environment block). But\nI won't blame Duy if he doesn't want to refactor all of the\ncross-platform exec code. ;)\n\n> And if the sort is not stable, then the above code is just simply\n> broken.\n\nAgreed.\n\n> \n> > I also think this is a special case of a more general problem. FOO could\n> > appear any number of times in the \"env\" array, as a deletion or with\n> > multiple values. Our prep_childenv() would treat that as \"last one\n> > wins\", I think. Could we just do the same here?\n> \n> Yeah, if the last one is to set FOO=bar, then it feels sufficient to\n> just check if FOO is set to bar in the original and deciding to show\n> or hide; similarly if the last one is to unset FOO, it does not matter\n> if the caller sets it to some other value before unsetting, so it\n> feels sufficient to just check if FOO is set to anything in the\n> original environment.\n\nYep, agreed again.\n\n-Peff\n"},{"id":"336523","messageId":"xmqqlgh2x4tr.fsf@gitster.mtv.corp.google.com","threadId":"47584","inReplyTo":"20180112184546.GA15514@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-12T19:19:44Z","receivedAt":"2018-01-12T19:19:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The actual prep_childenv() code looks like it would always do\n> last-one-wins, so we should treat FOO as unset in that final case. But\n> that only kicks in on non-Windows.\n>\n> On Windows we feed cmd->env straight to mingw_spawnvpe(). That ends up\n> in make_environment_block(), which looks like it does something similar.\n>\n> It's too bad the prep code is not shared, since then we could probably\n> just ask _it_ which deltas it applied.\n\nYeah, but that function does a lot more than computing delta. \n\nIt's primary point, which comes from ae25394b (\"run-command: prepare\nchild environment before forking\", 2017-04-19), is to create a full\ncopy of the environment, not just a series of putenv/unsetenv that\ndescribes what gets changed, and that is done to avoid any\nallocation after fork before exec in the child process.\n\nI guess prep_childenv() could take an extra and optional string-list\nto report what \"deltas\" it applied to the tracing machinery. I am\nnot sure if that is worth it, though.\n\n\n\n"},{"id":"336524","messageId":"20180112192314.GA19004@sigill.intra.peff.net","threadId":"47584","inReplyTo":"xmqqlgh2x4tr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-12T19:23:15Z","receivedAt":"2018-01-12T19:23:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 12, 2018 at 11:19:44AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The actual prep_childenv() code looks like it would always do\n> > last-one-wins, so we should treat FOO as unset in that final case. But\n> > that only kicks in on non-Windows.\n> >\n> > On Windows we feed cmd->env straight to mingw_spawnvpe(). That ends up\n> > in make_environment_block(), which looks like it does something similar.\n> >\n> > It's too bad the prep code is not shared, since then we could probably\n> > just ask _it_ which deltas it applied.\n> \n> Yeah, but that function does a lot more than computing delta. \n> \n> It's primary point, which comes from ae25394b (\"run-command: prepare\n> child environment before forking\", 2017-04-19), is to create a full\n> copy of the environment, not just a series of putenv/unsetenv that\n> describes what gets changed, and that is done to avoid any\n> allocation after fork before exec in the child process.\n> \n> I guess prep_childenv() could take an extra and optional string-list\n> to report what \"deltas\" it applied to the tracing machinery. I am\n> not sure if that is worth it, though.\n\nYes, that's exactly what I meant. I think if it covered all platforms it\nmight be worth it (not so much for code de-duping, but because then we\nwould know our display logic always matched what we exec'd). But unless\nsomebody wants to refactor all of the Windows spawn code, it's\ndefinitely not a good idea.\n\n-Peff\n"},{"id":"336529","messageId":"20180112202835.GA108169@google.com","threadId":"47584","inReplyTo":"20180112192314.GA19004@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-01-12T20:28:35Z","receivedAt":"2018-01-12T20:28:43Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 01/12, Jeff King wrote:\n> On Fri, Jan 12, 2018 at 11:19:44AM -0800, Junio C Hamano wrote:\n> \n> > Jeff King writes:\n> > \n> > > The actual prep_childenv() code looks like it would always do\n> > > last-one-wins, so we should treat FOO as unset in that final case. But\n> > > that only kicks in on non-Windows.\n> > >\n> > > On Windows we feed cmd->env straight to mingw_spawnvpe(). That ends up\n> > > in make_environment_block(), which looks like it does something similar.\n> > >\n> > > It's too bad the prep code is not shared, since then we could probably\n> > > just ask _it_ which deltas it applied.\n> > \n> > Yeah, but that function does a lot more than computing delta. \n> > \n> > It's primary point, which comes from ae25394b (\"run-command: prepare\n> > child environment before forking\", 2017-04-19), is to create a full\n> > copy of the environment, not just a series of putenv/unsetenv that\n> > describes what gets changed, and that is done to avoid any\n> > allocation after fork before exec in the child process.\n> > \n> > I guess prep_childenv() could take an extra and optional string-list\n> > to report what \"deltas\" it applied to the tracing machinery. I am\n> > not sure if that is worth it, though.\n> \n> Yes, that's exactly what I meant. I think if it covered all platforms it\n> might be worth it (not so much for code de-duping, but because then we\n> would know our display logic always matched what we exec'd). But unless\n> somebody wants to refactor all of the Windows spawn code, it's\n> definitely not a good idea.\n\nAnd not having access to a windows box I didn't want to try and refactor\nthe windows-only code when I introduced prep_childenv() :)\n\n-- \nBrandon Williams\n"},{"id":"336531","messageId":"xmqq7esmwuwn.fsf@gitster.mtv.corp.google.com","threadId":"47584","inReplyTo":"20180112133355.GE27499@sigill.intra.peff.net","subject":"Re: [PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-12T22:54:00Z","receivedAt":"2018-01-12T22:54:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I also think this is a special case of a more general problem. FOO could\n> appear any number of times in the \"env\" array, as a deletion or with\n> multiple values. Our prep_childenv() would treat that as \"last one\n> wins\", I think. Could we just do the same here?\n\nPerhaps this should be squashed into the original 4/4 instead of\nbeing a separate patch. We'd probably want some sort of test, I\nwonder? Not tested at all beyond compiling...\n\n-- >8 --\nSubject: [PATCH 7/4] run-command.c: don't be too cute in concatenate_env()\n\nInstead of relying on \"sort\" being stable to sort \"unset VAR\"\nimmediately before \"VAR=VAL\" to remove the former, just pick the\nlast manipulation for each VAR from the list of environment tweaks\nand show them in the output.\n\nSigned-off-by: Junio C Hamano \n---\n trace.c | 68 ++++++++++++++++++++---------------------------------------------\n 1 file changed, 21 insertions(+), 47 deletions(-)\n\ndiff --git a/trace.c b/trace.c\nindex aba2825044..9f49bcdd03 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -272,76 +272,50 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,\n \n #endif /* HAVE_VARIADIC_MACROS */\n \n-static void sort_deltaenv(struct string_list *envs,\n-\t\t\t const char *const *deltaenv)\n+static void concatenate_env(struct strbuf *dst, const char *const *deltaenv)\n {\n-\tstruct strbuf key = STRBUF_INIT;\n+\tstruct string_list envs = STRING_LIST_INIT_DUP;\n \tconst char *const *e;\n+\tint i;\n \n+\t/* Last one wins... */\n \tfor (e = deltaenv; e && *e; e++) {\n+\t\tstruct strbuf key = STRBUF_INIT;\n \t\tchar *equals = strchr(*e, '=');\n \n \t\tif (equals) {\n \t\t\tstrbuf_reset(&key);\n \t\t\tstrbuf_add(&key, *e, equals - *e);\n-\t\t\tstring_list_append(envs, key.buf)->util = equals + 1;\n+\t\t\tstring_list_insert(&envs, key.buf)->util = equals + 1;\n \t\t} else {\n-\t\t\tstring_list_append(envs, *e)->util = NULL;\n+\t\t\tstring_list_insert(&envs, *e)->util = NULL;\n \t\t}\n \t}\n-\tstring_list_sort(envs);\n-\tstrbuf_release(&key);\n-}\n-\n-\n-static void concatenate_env(struct strbuf *dst, const char *const *deltaenv)\n-{\n-\tstruct string_list envs = STRING_LIST_INIT_DUP;\n-\tint i;\n-\n-\t/*\n-\t * Construct a sorted string list consisting of the delta\n-\t * env. We need this to detect the case when the same var is\n-\t * deleted first, then added again.\n-\t */\n-\tsort_deltaenv(&envs, deltaenv);\n \n-\t/*\n-\t * variable deletion first because it's printed like separate\n-\t * shell commands\n-\t */\n+\t/* series of \"unset X; unset Y;...\" */\n \tfor (i = 0; i < envs.nr; i++) {\n-\t\tconst char *env = envs.items[i].string;\n-\t\tconst char *p = envs.items[i].util;\n+\t\tconst char *var = envs.items[i].string;\n+\t\tconst char *val = envs.items[i].util;\n \n-\t\tif (p || !getenv(env))\n+\t\tif (val)\n \t\t\tcontinue;\n-\n-\t\t/*\n-\t\t * Do we have a sequence of \"unset GIT_DIR; GIT_DIR=foo\"?\n-\t\t * Then don't bother with the unset thing.\n-\t\t */\n-\t\tif (i + 1 < envs.nr &&\n-\t\t !strcmp(env, envs.items[i + 1].string))\n-\t\t\tcontinue;\n-\n-\t\tstrbuf_addf(dst, \" unset %s;\", env);\n+\t\tif (getenv(var))\n+\t\t\tstrbuf_addf(dst, \" unset %s;\", var);\n \t}\n \n+\t/* ... followed by \"A=B C=D ...\" */\n \tfor (i = 0; i < envs.nr; i++) {\n-\t\tconst char *env = envs.items[i].string;\n-\t\tconst char *p = envs.items[i].util;\n+\t\tconst char *var = envs.items[i].string;\n+\t\tconst char *val = envs.items[i].util;\n \t\tconst char *old_value;\n \n-\t\tif (!p)\n+\t\tif (!val)\n \t\t\tcontinue;\n-\n-\t\told_value = getenv(env);\n-\t\tif (old_value && !strcmp(old_value, p))\n+\t\told_value = getenv(var);\n+\t\tif (old_value && !strcmp(old_value, val))\n \t\t\tcontinue;\n-\n-\t\tstrbuf_addf(dst, \" %s=\", env);\n-\t\tsq_quote_buf_pretty(dst, p);\n+\t\tstrbuf_addf(dst, \" %s=\", var);\n+\t\tsq_quote_buf_pretty(dst, val);\n \t}\n \tstring_list_clear(&envs, 0);\n }\n"},{"id":"336533","messageId":"xmqq373awtns.fsf@gitster.mtv.corp.google.com","threadId":"47584","inReplyTo":"20180112133810.GA8290@sigill.intra.peff.net","subject":"Re: [PATCH 5/4] sq_quote_argv: drop maxlen parameter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-12T23:20:55Z","receivedAt":"2018-01-12T23:21:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> No caller passes anything but \"0\" for this parameter, which\n> requests that the function ignore it completely. In fact, in\n> all of history there was only one such caller, and it went\n> away in 7f51f8bc2b (alias: use run_command api to execute\n> aliases, 2011-01-07).\n>\n> Signed-off-by: Jeff King \n> ---\n> Just a cleanup, though it touches the caller you just added, too.\n\nYup. This is a good change in the longer term. I needed an evil\nmerge for a stalled topic in 'pu' to adjust a couple of new callers,\nthough ;-)..\n"},{"id":"336538","messageId":"CACsJy8CeYY34G=JjwHWqM+QbBG41pkW7VP+Aowak26rdM1qpPg@mail.gmail.com","threadId":"47584","inReplyTo":"xmqq7esmwuwn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-13T04:54:26Z","receivedAt":"2018-01-13T04:55:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 13, 2018 at 5:54 AM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> I also think this is a special case of a more general problem. FOO could\n>> appear any number of times in the \"env\" array, as a deletion or with\n>> multiple values. Our prep_childenv() would treat that as \"last one\n>> wins\", I think. Could we just do the same here?\n>\n> Perhaps this should be squashed into the original 4/4 instead of\n> being a separate patch. We'd probably want some sort of test, I\n> wonder? Not tested at all beyond compiling...\n>\n> -- >8 --\n> Subject: [PATCH 7/4] run-command.c: don't be too cute in concatenate_env()\n>\n> Instead of relying on \"sort\" being stable to sort \"unset VAR\"\n> immediately before \"VAR=VAL\" to remove the former, just pick the\n> last manipulation for each VAR from the list of environment tweaks\n> and show them in the output.\n\nThis is not enough. Imagine we have GIT_DIR=foo in parent env, then a\nsequence of \"GIT_DIR\", \"GIT_DIR=foo\" in deltaenv. Because we process\nset/unset in two separate loops, the \"last one wins\" rule does not see\nthat \"GIT_DIR=foo\" wins over \"unset GIT_DIR;\". So we might print\n\"unset GIT_DIR; GIT_DIR=foo\", which is fine even if it's redundant.\nExcept that we don't print that.\n\nThe problem comes from comparing with parent env. The new var has the\nsame value as parent env so we won't print \"GIT_DIR=foo\", just \"unset\nGIT_DIR;\". This is wrong.\n\nI'm tempted to just get the final child env from prep_childenv() then\ncompare with parent env and print the differences. It will not work\nwith Windows though, so Windows gets the old trace line without env\ndelta. I hope some Windows contributor will jump in at some point if\nthey want env tracing works for them too.\n-- \nDuy\n"},{"id":"336539","messageId":"CACsJy8Dc5gcaoKG6i7JoiuCaD2Xn-saeAgN=u2nK0zryiMJY3g@mail.gmail.com","threadId":"47584","inReplyTo":"CACsJy8CeYY34G=JjwHWqM+QbBG41pkW7VP+Aowak26rdM1qpPg@mail.gmail.com","subject":"Re: [PATCH v3 4/4] trace.c: be smart about what env to print in trace_run_command()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-13T06:47:32Z","receivedAt":"2018-01-13T06:48:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 13, 2018 at 11:54 AM, Duy Nguyen wrote:\n> On Sat, Jan 13, 2018 at 5:54 AM, Junio C Hamano wrote:\n>> Jeff King writes:\n>>\n>>> I also think this is a special case of a more general problem. FOO could\n>>> appear any number of times in the \"env\" array, as a deletion or with\n>>> multiple values. Our prep_childenv() would treat that as \"last one\n>>> wins\", I think. Could we just do the same here?\n>>\n>> Perhaps this should be squashed into the original 4/4 instead of\n>> being a separate patch. We'd probably want some sort of test, I\n>> wonder? Not tested at all beyond compiling...\n>>\n>> -- >8 --\n>> Subject: [PATCH 7/4] run-command.c: don't be too cute in concatenate_env()\n>>\n>> Instead of relying on \"sort\" being stable to sort \"unset VAR\"\n>> immediately before \"VAR=VAL\" to remove the former, just pick the\n>> last manipulation for each VAR from the list of environment tweaks\n>> and show them in the output.\n>\n> This is not enough.\n\nNo it is. I misunderstood string_list_insert() and my tests proved me\nwrong. Sorry for the noise.\n-- \nDuy\n"},{"id":"336540","messageId":"20180113064949.6043-1-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180112095607.18293-1-pclouds@gmail.com","subject":"[PATCH v4 0/7] Trace env variables in run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-13T06:49:42Z","receivedAt":"2018-01-13T06:50:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v4:\n\n- incorporates Jeff patches and moves them on top\n- removes strbuf release from print_trace_line\n- prints 'unset a b c' instead of 'unset a; unset b; unset c'\n- squashes v3 3/4 and 4/4 and Junio's patch into 6/7\n- adds tests\n- cwd is separated in 7/7\n\ninterdiff with what's on 'pu':\n\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex d24d157379..0ab71f14fb 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -56,6 +56,10 @@ int cmd_main(int argc, const char **argv)\n \n \tif (argc < 3)\n \t\treturn 1;\n+\twhile (!strcmp(argv[1], \"env\")) {\n+\t\targv_array_push(&proc.env_array, argv[2]);\n+\t\targv += 2;\n+\t}\n \tproc.argv = (const char **)argv + 2;\n \n \tif (!strcmp(argv[1], \"start-command-ENOENT\")) {\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex e4739170aa..e6208316c3 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -141,4 +141,26 @@ test_expect_success 'run_command outputs ' '\n \ttest_cmp expect actual\n '\n \n+test_trace() {\n+\tlocal expected=\"$1\"\n+\tshift\n+\tGIT_TRACE=1 test-run-command \"$@\" run-command true 2>&1 >/dev/null | \\\n+\t\tsed 's/.* run_command: //' >actual &&\n+\techo \"$expected true\" >expected &&\n+\ttest_cmp expected actual\n+}\n+\n+test_expect_success 'GIT_TRACE with environment variables' '\n+\ttest_trace \"abc=1 def=2\" env abc=1 env def=2 &&\n+\ttest_trace \"abc=2\" env abc env abc=1 env abc=2 &&\n+\ttest_trace \"abc=2\" env abc env abc=2 &&\n+\tabc=1 test_trace \"def=1\" env abc=1 env def=1 &&\n+\tabc=1 test_trace \"def=1\" env abc env abc=1 env def=1 &&\n+\ttest_trace \"def=1\" env non-exist env def=1 &&\n+\ttest_trace \"abc=2\" env abc=1 env abc env abc=2 &&\n+\tabc=1 def=2 test_trace \"unset abc def;\" env abc env def &&\n+\tabc=1 def=2 test_trace \"unset def; abc=3\" env abc env def env abc=3 &&\n+\tabc=1 test_trace \"unset abc;\" env abc=2 env abc\n+'\n+\n test_done\ndiff --git a/trace.c b/trace.c\nindex 9f49bcdd03..4bfd3fce10 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -132,7 +132,6 @@ static void print_trace_line(struct trace_key *key, struct strbuf *buf)\n {\n \tstrbuf_complete_line(buf);\n \ttrace_write(key, buf->buf, buf->len);\n-\tstrbuf_release(buf);\n }\n \n static void trace_vprintf_fl(const char *file, int line, struct trace_key *key,\n@@ -145,6 +144,7 @@ static void trace_vprintf_fl(const char *file, int line, struct trace_key *key,\n \n \tstrbuf_vaddf(&buf, format, ap);\n \tprint_trace_line(key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n static void trace_argv_vprintf_fl(const char *file, int line,\n@@ -160,6 +160,7 @@ static void trace_argv_vprintf_fl(const char *file, int line,\n \n \tsq_quote_argv_pretty(&buf, argv);\n \tprint_trace_line(&trace_default_key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n void trace_strbuf_fl(const char *file, int line, struct trace_key *key,\n@@ -172,6 +173,7 @@ void trace_strbuf_fl(const char *file, int line, struct trace_key *key,\n \n \tstrbuf_addbuf(&buf, data);\n \tprint_trace_line(key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n static void trace_performance_vprintf_fl(const char *file, int line,\n@@ -191,6 +193,7 @@ static void trace_performance_vprintf_fl(const char *file, int line,\n \t}\n \n \tprint_trace_line(&trace_perf_key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n #ifndef HAVE_VARIADIC_MACROS\n@@ -272,17 +275,17 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,\n \n #endif /* HAVE_VARIADIC_MACROS */\n \n-static void concatenate_env(struct strbuf *dst, const char *const *deltaenv)\n+static void add_env(struct strbuf *dst, const char *const *deltaenv)\n {\n \tstruct string_list envs = STRING_LIST_INIT_DUP;\n \tconst char *const *e;\n \tint i;\n+\tint printed_unset = 0;\n \n-\t/* Last one wins... */\n+\t/* Last one wins, see run-command.c:prep_childenv() for context */\n \tfor (e = deltaenv; e && *e; e++) {\n \t\tstruct strbuf key = STRBUF_INIT;\n \t\tchar *equals = strchr(*e, '=');\n-\n \t\tif (equals) {\n \t\t\tstrbuf_reset(&key);\n \t\t\tstrbuf_add(&key, *e, equals - *e);\n@@ -290,30 +293,39 @@ static void concatenate_env(struct strbuf *dst, const char *const *deltaenv)\n \t\t} else {\n \t\t\tstring_list_insert(&envs, *e)->util = NULL;\n \t\t}\n+\t\tstrbuf_release(&key);\n \t}\n \n-\t/* series of \"unset X; unset Y;...\" */\n+\t/* \"unset X Y...;\" */\n \tfor (i = 0; i < envs.nr; i++) {\n \t\tconst char *var = envs.items[i].string;\n \t\tconst char *val = envs.items[i].util;\n \n-\t\tif (val)\n+\t\tif (val || !getenv(var))\n \t\t\tcontinue;\n-\t\tif (getenv(var))\n-\t\t\tstrbuf_addf(dst, \" unset %s;\", var);\n+\n+\t\tif (!printed_unset) {\n+\t\t\tstrbuf_addstr(dst, \" unset\");\n+\t\t\tprinted_unset = 1;\n+\t\t}\n+\t\tstrbuf_addf(dst, \" %s\", var);\n \t}\n+\tif (printed_unset)\n+\t\tstrbuf_addch(dst, ';');\n \n \t/* ... followed by \"A=B C=D ...\" */\n \tfor (i = 0; i < envs.nr; i++) {\n \t\tconst char *var = envs.items[i].string;\n \t\tconst char *val = envs.items[i].util;\n-\t\tconst char *old_value;\n+\t\tconst char *oldval;\n \n \t\tif (!val)\n \t\t\tcontinue;\n-\t\told_value = getenv(var);\n-\t\tif (old_value && !strcmp(old_value, val))\n+\n+\t\toldval = getenv(var);\n+\t\tif (oldval && !strcmp(val, oldval))\n \t\t\tcontinue;\n+\n \t\tstrbuf_addf(dst, \" %s=\", var);\n \t\tsq_quote_buf_pretty(dst, val);\n \t}\n@@ -328,8 +340,6 @@ void trace_run_command(const struct child_process *cp)\n \t\t\t\t&trace_default_key, &buf))\n \t\treturn;\n \n-\tstrbuf_grow(&buf, 255);\n-\n \tstrbuf_addf(&buf, \"trace: run_command:\");\n \n \tif (cp->dir) {\n@@ -343,7 +353,7 @@ void trace_run_command(const struct child_process *cp)\n \t * cp->env_array if needed. We only check one place.\n \t */\n \tif (cp->env)\n-\t\tconcatenate_env(&buf, cp->env);\n+\t\tadd_env(&buf, cp->env);\n \n \tif (cp->git_cmd)\n \t\tstrbuf_addstr(&buf, \" git\");\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336541","messageId":"20180113064949.6043-2-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180113064949.6043-1-pclouds@gmail.com","subject":"[PATCH v4 1/7] sq_quote_argv: drop maxlen parameter","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-13T06:49:43Z","receivedAt":"2018-01-13T06:50:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Jeff King \n\nNo caller passes anything but \"0\" for this parameter, which\nrequests that the function ignore it completely. In fact, in\nall of history there was only one such caller, and it went\naway in 7f51f8bc2b (alias: use run_command api to execute\naliases, 2011-01-07).\n\nSigned-off-by: Jeff King \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/am.c | 2 +-\n builtin/rev-parse.c | 4 ++--\n quote.c | 4 +---\n quote.h | 2 +-\n trace.c | 4 ++--\n 5 files changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex acfe9d3c8c..5bdd2d7578 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1061,7 +1061,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t}\n \twrite_state_text(state, \"scissors\", str);\n \n-\tsq_quote_argv(&sb, state->git_apply_opts.argv, 0);\n+\tsq_quote_argv(&sb, state->git_apply_opts.argv);\n \twrite_state_text(state, \"apply-opt\", sb.buf);\n \n \tif (state->rebasing)\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 74aa644cbb..96d06a5d01 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -516,7 +516,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_SHELL_EVAL);\n \n \tstrbuf_addstr(&parsed, \" --\");\n-\tsq_quote_argv(&parsed, argv, 0);\n+\tsq_quote_argv(&parsed, argv);\n \tputs(parsed.buf);\n \treturn 0;\n }\n@@ -526,7 +526,7 @@ static int cmd_sq_quote(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \n \tif (argc)\n-\t\tsq_quote_argv(&buf, argv, 0);\n+\t\tsq_quote_argv(&buf, argv);\n \tprintf(\"%s\\n\", buf.buf);\n \tstrbuf_release(&buf);\n \ndiff --git a/quote.c b/quote.c\nindex de2922ddd6..b2970da627 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -56,7 +56,7 @@ void sq_quotef(struct strbuf *dst, const char *fmt, ...)\n \tstrbuf_release(&src);\n }\n \n-void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)\n+void sq_quote_argv(struct strbuf *dst, const char **argv)\n {\n \tint i;\n \n@@ -65,8 +65,6 @@ void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)\n \tfor (i = 0; argv[i]; ++i) {\n \t\tstrbuf_addch(dst, ' ');\n \t\tsq_quote_buf(dst, argv[i]);\n-\t\tif (maxlen && dst->len > maxlen)\n-\t\t\tdie(\"Too many or long arguments\");\n \t}\n }\n \ndiff --git a/quote.h b/quote.h\nindex 66f5644aa2..48a75a416b 100644\n--- a/quote.h\n+++ b/quote.h\n@@ -30,7 +30,7 @@ struct strbuf;\n */\n \n extern void sq_quote_buf(struct strbuf *, const char *src);\n-extern void sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);\n+extern void sq_quote_argv(struct strbuf *, const char **argv);\n extern void sq_quotef(struct strbuf *, const char *fmt, ...);\n \n /* This unwraps what sq_quote() produces in place, but returns\ndiff --git a/trace.c b/trace.c\nindex b7530b51a9..fa9174fc4b 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -157,7 +157,7 @@ static void trace_argv_vprintf_fl(const char *file, int line,\n \n \tstrbuf_vaddf(&buf, format, ap);\n \n-\tsq_quote_argv(&buf, argv, 0);\n+\tsq_quote_argv(&buf, argv);\n \tprint_trace_line(&trace_default_key, &buf);\n }\n \n@@ -426,6 +426,6 @@ void trace_command_performance(const char **argv)\n \t\tatexit(print_command_performance_atexit);\n \n \tstrbuf_reset(&command_line);\n-\tsq_quote_argv(&command_line, argv, 0);\n+\tsq_quote_argv(&command_line, argv);\n \tcommand_start_time = getnanotime();\n }\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336542","messageId":"20180113064949.6043-3-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180113064949.6043-1-pclouds@gmail.com","subject":"[PATCH v4 2/7] trace: avoid unnecessary quoting","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-13T06:49:44Z","receivedAt":"2018-01-13T06:50:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Jeff King \n\nTrace output which contains arbitrary strings (e.g., the\narguments to commands which we are running) is always passed\nthrough sq_quote_buf(). That function always adds\nsingle-quotes, even if the output consists of vanilla\ncharacters. This can make the output a bit hard to read.\n\nLet's avoid the quoting if there are no characters which a\nshell would interpret. Trace output doesn't necessarily need\nto be shell-compatible, but:\n\n - the shell language is a good ballpark for what humans\n consider readable (well, humans versed in command line\n tools)\n\n - the run_command bits can be cut-and-pasted to a shell,\n and we'll keep that property\n\n - it covers any cases which would make the output\n visually ambiguous (e.g., embedded whitespace or quotes)\n\nSigned-off-by: Jeff King \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n quote.c | 26 ++++++++++++++++++++++++++\n quote.h | 8 ++++++++\n trace.c | 4 ++--\n 3 files changed, 36 insertions(+), 2 deletions(-)\n\ndiff --git a/quote.c b/quote.c\nindex b2970da627..37d2686865 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -43,6 +43,22 @@ void sq_quote_buf(struct strbuf *dst, const char *src)\n \tfree(to_free);\n }\n \n+void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n+{\n+\tstatic const char ok_punct[] = \"+,-./:=@_^\";\n+\tconst char *p;\n+\n+\tfor (p = src; *p; p++) {\n+\t\tif (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {\n+\t\t\tsq_quote_buf(dst, src);\n+\t\t\treturn;\n+\t\t}\n+\t}\n+\n+\t/* if we get here, we did not need quoting */\n+\tstrbuf_addstr(dst, src);\n+}\n+\n void sq_quotef(struct strbuf *dst, const char *fmt, ...)\n {\n \tstruct strbuf src = STRBUF_INIT;\n@@ -68,6 +84,16 @@ void sq_quote_argv(struct strbuf *dst, const char **argv)\n \t}\n }\n \n+void sq_quote_argv_pretty(struct strbuf *dst, const char **argv)\n+{\n+\tint i;\n+\n+\tfor (i = 0; argv[i]; i++) {\n+\t\tstrbuf_addch(dst, ' ');\n+\t\tsq_quote_buf_pretty(dst, argv[i]);\n+\t}\n+}\n+\n static char *sq_dequote_step(char *arg, char **next)\n {\n \tchar *dst = arg;\ndiff --git a/quote.h b/quote.h\nindex 48a75a416b..ea992dcc91 100644\n--- a/quote.h\n+++ b/quote.h\n@@ -33,6 +33,14 @@ extern void sq_quote_buf(struct strbuf *, const char *src);\n extern void sq_quote_argv(struct strbuf *, const char **argv);\n extern void sq_quotef(struct strbuf *, const char *fmt, ...);\n \n+/*\n+ * These match their non-pretty variants, except that they avoid\n+ * quoting when there are no exotic characters. These should only be used for\n+ * human-readable output, as sq_dequote() is not smart enough to dequote it.\n+ */\n+void sq_quote_buf_pretty(struct strbuf *, const char *src);\n+void sq_quote_argv_pretty(struct strbuf *, const char **argv);\n+\n /* This unwraps what sq_quote() produces in place, but returns\n * NULL if the input does not look like what sq_quote would have\n * produced.\ndiff --git a/trace.c b/trace.c\nindex fa9174fc4b..9784915be1 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -157,7 +157,7 @@ static void trace_argv_vprintf_fl(const char *file, int line,\n \n \tstrbuf_vaddf(&buf, format, ap);\n \n-\tsq_quote_argv(&buf, argv);\n+\tsq_quote_argv_pretty(&buf, argv);\n \tprint_trace_line(&trace_default_key, &buf);\n }\n \n@@ -426,6 +426,6 @@ void trace_command_performance(const char **argv)\n \t\tatexit(print_command_performance_atexit);\n \n \tstrbuf_reset(&command_line);\n-\tsq_quote_argv(&command_line, argv);\n+\tsq_quote_argv_pretty(&command_line, argv);\n \tcommand_start_time = getnanotime();\n }\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336543","messageId":"20180113064949.6043-4-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180113064949.6043-1-pclouds@gmail.com","subject":"[PATCH v4 3/7] trace.c: move strbuf_release() out of print_trace_line()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-13T06:49:45Z","receivedAt":"2018-01-13T06:50:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The function is about printing a trace line, not releasing the buffer it\nreceives too. Move strbuf_release() back outside. This makes it easier\nto see how strbuf is managed.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n trace.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/trace.c b/trace.c\nindex 9784915be1..7f3b08e148 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -131,7 +131,6 @@ static void print_trace_line(struct trace_key *key, struct strbuf *buf)\n {\n \tstrbuf_complete_line(buf);\n \ttrace_write(key, buf->buf, buf->len);\n-\tstrbuf_release(buf);\n }\n \n static void trace_vprintf_fl(const char *file, int line, struct trace_key *key,\n@@ -144,6 +143,7 @@ static void trace_vprintf_fl(const char *file, int line, struct trace_key *key,\n \n \tstrbuf_vaddf(&buf, format, ap);\n \tprint_trace_line(key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n static void trace_argv_vprintf_fl(const char *file, int line,\n@@ -159,6 +159,7 @@ static void trace_argv_vprintf_fl(const char *file, int line,\n \n \tsq_quote_argv_pretty(&buf, argv);\n \tprint_trace_line(&trace_default_key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n void trace_strbuf_fl(const char *file, int line, struct trace_key *key,\n@@ -171,6 +172,7 @@ void trace_strbuf_fl(const char *file, int line, struct trace_key *key,\n \n \tstrbuf_addbuf(&buf, data);\n \tprint_trace_line(key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n static void trace_performance_vprintf_fl(const char *file, int line,\n@@ -190,6 +192,7 @@ static void trace_performance_vprintf_fl(const char *file, int line,\n \t}\n \n \tprint_trace_line(&trace_perf_key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n #ifndef HAVE_VARIADIC_MACROS\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336544","messageId":"20180113064949.6043-5-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180113064949.6043-1-pclouds@gmail.com","subject":"[PATCH v4 4/7] trace.c: introduce trace_run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-13T06:49:46Z","receivedAt":"2018-01-13T06:50:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This is the same as the old code that uses trace_argv_printf() in\nrun-command.c. This function will be improved in later patches to\nprint more information from struct child_process.\n\nA slight regression: the old code would print run-command.c:xxx as the\ntrace location site while the new code prints trace.c:xxx. This should\nbe fine until the second call site is added, then we might need a macro\nwrapper named trace_run_command() to capture the right source location.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n run-command.c | 3 ++-\n trace.c | 15 +++++++++++++++\n trace.h | 3 +++\n 3 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 31fc5ea86e..002074b128 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -624,7 +624,8 @@ int start_command(struct child_process *cmd)\n \t\tcmd->err = fderr[0];\n \t}\n \n-\ttrace_argv_printf(cmd->argv, \"trace: run_command:\");\n+\ttrace_run_command(cmd);\n+\n \tfflush(NULL);\n \n #ifndef GIT_WINDOWS_NATIVE\ndiff --git a/trace.c b/trace.c\nindex 7f3b08e148..25c333709b 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -23,6 +23,7 @@\n \n #include \"cache.h\"\n #include \"quote.h\"\n+#include \"run-command.h\"\n \n struct trace_key trace_default_key = { \"GIT_TRACE\", 0, 0, 0 };\n struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);\n@@ -275,6 +276,20 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,\n #endif /* HAVE_VARIADIC_MACROS */\n \n \n+void trace_run_command(const struct child_process *cp)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tif (!prepare_trace_line(__FILE__, __LINE__,\n+\t\t\t\t&trace_default_key, &buf))\n+\t\treturn;\n+\n+\tstrbuf_addf(&buf, \"trace: run_command:\");\n+\n+\tsq_quote_argv_pretty(&buf, cp->argv);\n+\tprint_trace_line(&trace_default_key, &buf);\n+}\n+\n static const char *quote_crnl(const char *path)\n {\n \tstatic struct strbuf new_path = STRBUF_INIT;\ndiff --git a/trace.h b/trace.h\nindex 88055abef7..e54c687f26 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -4,6 +4,8 @@\n #include \"git-compat-util.h\"\n #include \"strbuf.h\"\n \n+struct child_process;\n+\n struct trace_key {\n \tconst char * const key;\n \tint fd;\n@@ -17,6 +19,7 @@ extern struct trace_key trace_default_key;\n extern struct trace_key trace_perf_key;\n \n extern void trace_repo_setup(const char *prefix);\n+extern void trace_run_command(const struct child_process *cp);\n extern int trace_want(struct trace_key *key);\n extern void trace_disable(struct trace_key *key);\n extern uint64_t getnanotime(void);\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336545","messageId":"20180113064949.6043-6-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180113064949.6043-1-pclouds@gmail.com","subject":"[PATCH v4 5/7] trace.c: print program 'git' when child_process.git_cmd is set","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-13T06:49:47Z","receivedAt":"2018-01-13T06:50:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"We normally print full command line, including the program and its\nargument. When git_cmd is set, we have a special code path to run the\nright \"git\" program and child_process.argv[0] will not contain the\nprogram name anymore. As a result, we print just the command\narguments.\n\nI thought it was a regression when the code was refactored and git_cmd\nadded, but apparently it's not. git_cmd mode was introduced before\ntracing was added in 8852f5d704 (run_command(): respect GIT_TRACE -\n2008-07-07) so it's more like an oversight in 8852f5d704.\n\nFix it, print the program name \"git\" in git_cmd mode. It's nice to have\nnow. But it will be more important later when we start to print env\nvariables too, in shell syntax. The lack of a program name would look\nconfusing then.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n trace.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/trace.c b/trace.c\nindex 25c333709b..7f43da211a 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -286,6 +286,9 @@ void trace_run_command(const struct child_process *cp)\n \n \tstrbuf_addf(&buf, \"trace: run_command:\");\n \n+\tif (cp->git_cmd)\n+\t\tstrbuf_addstr(&buf, \" git\");\n+\n \tsq_quote_argv_pretty(&buf, cp->argv);\n \tprint_trace_line(&trace_default_key, &buf);\n }\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336546","messageId":"20180113064949.6043-7-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180113064949.6043-1-pclouds@gmail.com","subject":"[PATCH v4 6/7] trace.c: print env vars in trace_run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-13T06:49:48Z","receivedAt":"2018-01-13T06:50:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Occasionally submodule code could execute new commands with GIT_DIR set\nto some submodule. GIT_TRACE prints just the command line which makes it\nhard to tell that it's not really executed on this repository.\n\nPrint the env delta (compared to parent environment) in this case.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n t/helper/test-run-command.c | 4 +++\n t/t0061-run-command.sh | 22 ++++++++++++++++\n trace.c | 63 +++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 89 insertions(+)\n\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex d24d157379..0ab71f14fb 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -56,6 +56,10 @@ int cmd_main(int argc, const char **argv)\n \n \tif (argc < 3)\n \t\treturn 1;\n+\twhile (!strcmp(argv[1], \"env\")) {\n+\t\targv_array_push(&proc.env_array, argv[2]);\n+\t\targv += 2;\n+\t}\n \tproc.argv = (const char **)argv + 2;\n \n \tif (!strcmp(argv[1], \"start-command-ENOENT\")) {\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex e4739170aa..e6208316c3 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -141,4 +141,26 @@ test_expect_success 'run_command outputs ' '\n \ttest_cmp expect actual\n '\n \n+test_trace() {\n+\tlocal expected=\"$1\"\n+\tshift\n+\tGIT_TRACE=1 test-run-command \"$@\" run-command true 2>&1 >/dev/null | \\\n+\t\tsed 's/.* run_command: //' >actual &&\n+\techo \"$expected true\" >expected &&\n+\ttest_cmp expected actual\n+}\n+\n+test_expect_success 'GIT_TRACE with environment variables' '\n+\ttest_trace \"abc=1 def=2\" env abc=1 env def=2 &&\n+\ttest_trace \"abc=2\" env abc env abc=1 env abc=2 &&\n+\ttest_trace \"abc=2\" env abc env abc=2 &&\n+\tabc=1 test_trace \"def=1\" env abc=1 env def=1 &&\n+\tabc=1 test_trace \"def=1\" env abc env abc=1 env def=1 &&\n+\ttest_trace \"def=1\" env non-exist env def=1 &&\n+\ttest_trace \"abc=2\" env abc=1 env abc env abc=2 &&\n+\tabc=1 def=2 test_trace \"unset abc def;\" env abc env def &&\n+\tabc=1 def=2 test_trace \"unset def; abc=3\" env abc env def env abc=3 &&\n+\tabc=1 test_trace \"unset abc;\" env abc=2 env abc\n+'\n+\n test_done\ndiff --git a/trace.c b/trace.c\nindex 7f43da211a..ffa1cf9b91 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -275,6 +275,62 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,\n \n #endif /* HAVE_VARIADIC_MACROS */\n \n+static void add_env(struct strbuf *dst, const char *const *deltaenv)\n+{\n+\tstruct string_list envs = STRING_LIST_INIT_DUP;\n+\tconst char *const *e;\n+\tint i;\n+\tint printed_unset = 0;\n+\n+\t/* Last one wins, see run-command.c:prep_childenv() for context */\n+\tfor (e = deltaenv; e && *e; e++) {\n+\t\tstruct strbuf key = STRBUF_INIT;\n+\t\tchar *equals = strchr(*e, '=');\n+\t\tif (equals) {\n+\t\t\tstrbuf_reset(&key);\n+\t\t\tstrbuf_add(&key, *e, equals - *e);\n+\t\t\tstring_list_insert(&envs, key.buf)->util = equals + 1;\n+\t\t} else {\n+\t\t\tstring_list_insert(&envs, *e)->util = NULL;\n+\t\t}\n+\t\tstrbuf_release(&key);\n+\t}\n+\n+\t/* \"unset X Y...;\" */\n+\tfor (i = 0; i < envs.nr; i++) {\n+\t\tconst char *var = envs.items[i].string;\n+\t\tconst char *val = envs.items[i].util;\n+\n+\t\tif (val || !getenv(var))\n+\t\t\tcontinue;\n+\n+\t\tif (!printed_unset) {\n+\t\t\tstrbuf_addstr(dst, \" unset\");\n+\t\t\tprinted_unset = 1;\n+\t\t}\n+\t\tstrbuf_addf(dst, \" %s\", var);\n+\t}\n+\tif (printed_unset)\n+\t\tstrbuf_addch(dst, ';');\n+\n+\t/* ... followed by \"A=B C=D ...\" */\n+\tfor (i = 0; i < envs.nr; i++) {\n+\t\tconst char *var = envs.items[i].string;\n+\t\tconst char *val = envs.items[i].util;\n+\t\tconst char *oldval;\n+\n+\t\tif (!val)\n+\t\t\tcontinue;\n+\n+\t\toldval = getenv(var);\n+\t\tif (oldval && !strcmp(val, oldval))\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_addf(dst, \" %s=\", var);\n+\t\tsq_quote_buf_pretty(dst, val);\n+\t}\n+\tstring_list_clear(&envs, 0);\n+}\n \n void trace_run_command(const struct child_process *cp)\n {\n@@ -286,6 +342,13 @@ void trace_run_command(const struct child_process *cp)\n \n \tstrbuf_addf(&buf, \"trace: run_command:\");\n \n+\t/*\n+\t * The caller is responsible for initializing cp->env from\n+\t * cp->env_array if needed. We only check one place.\n+\t */\n+\tif (cp->env)\n+\t\tadd_env(&buf, cp->env);\n+\n \tif (cp->git_cmd)\n \t\tstrbuf_addstr(&buf, \" git\");\n \n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336547","messageId":"20180113064949.6043-8-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180113064949.6043-1-pclouds@gmail.com","subject":"[PATCH v4 7/7] trace.c: print new cwd in trace_run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-13T06:49:49Z","receivedAt":"2018-01-13T06:50:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If a command sets a new env variable GIT_DIR=.git, we need more context\nto know where that '.git' is related to.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n trace.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/trace.c b/trace.c\nindex ffa1cf9b91..4bfd3fce10 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -342,6 +342,12 @@ void trace_run_command(const struct child_process *cp)\n \n \tstrbuf_addf(&buf, \"trace: run_command:\");\n \n+\tif (cp->dir) {\n+\t\tstrbuf_addstr(&buf, \" cd \");\n+\t\tsq_quote_buf_pretty(&buf, cp->dir);\n+\t\tstrbuf_addch(&buf, ';');\n+\t}\n+\n \t/*\n \t * The caller is responsible for initializing cp->env from\n \t * cp->env_array if needed. We only check one place.\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336548","messageId":"CACsJy8BagXM=A6+yd9GbBxp-_Ht5Og1_LqkUQGhd+dqkMvKP9Q@mail.gmail.com","threadId":"47584","inReplyTo":"CAGZ79kYUyugA_BhMgr_k1aWfxg9+cNbOcVO+LOZkZ1YAAu7_mQ@mail.gmail.com","subject":"Re: [PATCH v3 0/4] run-command.c: print env vars when GIT_TRACE is set","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-13T06:54:42Z","receivedAt":"2018-01-13T06:55:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 13, 2018 at 12:19 AM, Stefan Beller wrote:\n>> Oh.. before anybody asks, I'm not adding git prefix to the \"cd\" part.\n>> You're supposed to know that git moves back to worktree top dir when\n>> you read $GIT_TRACE (or are welcome to improve the print out).\n>\n> Heh. Would appending \"; cd -\" solve the issue? (Though I would argue\n> we don't need or want to add that actually, as you'd want to copy and run\n> the command and surely want to run more tests in that directory afterwards.\n> I mean you're debugging, right?)\n\nThe problem is if you are tracing in a subdir foo/bar and the trace\noutput says \"cd xyz\". You can' just copy and execute that command and\nhave to do \"cd ../../xyz\". I don't see how \"cd -\" would solve that.\nPrinting \"cd $(git rev-parse --show-cdup)/xyz\" could solve it without\npassing the prefix down to this trace function. I guess. But then that\ncommand depends on $GIT_DIR. If your $GIT_DIR is being changed (i.e.\nsubmodules), it gets ugly again.\n-- \nDuy\n"},{"id":"336549","messageId":"20180113071450.GA27251@sigill.intra.peff.net","threadId":"47584","inReplyTo":"20180113064949.6043-4-pclouds@gmail.com","subject":"Re: [PATCH v4 3/7] trace.c: move strbuf_release() out of print_trace_line()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-13T07:14:50Z","receivedAt":"2018-01-13T07:14:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 13, 2018 at 01:49:45PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> The function is about printing a trace line, not releasing the buffer it\n> receives too. Move strbuf_release() back outside. This makes it easier\n> to see how strbuf is managed.\n\nThis is kind of a funny refactor, in that it makes the code longer. :)\nI think it does make things a bit more obvious, though.\n\n> diff --git a/trace.c b/trace.c\n> index 9784915be1..7f3b08e148 100644\n> --- a/trace.c\n> +++ b/trace.c\n> @@ -131,7 +131,6 @@ static void print_trace_line(struct trace_key *key, struct strbuf *buf)\n> {\n> \tstrbuf_complete_line(buf);\n> \ttrace_write(key, buf->buf, buf->len);\n> -\tstrbuf_release(buf);\n> }\n\nOne mental note: any print_trace_line() calls for topics in flight need\nto be adjusted, or they will leak. Probably OK for a static function\n(and I don't see any in pu, nor git-for-windows, nor do I have any in my\nprivate stash of patches).\n\n-Peff\n"},{"id":"336550","messageId":"20180113072524.GB27251@sigill.intra.peff.net","threadId":"47584","inReplyTo":"20180113064949.6043-7-pclouds@gmail.com","subject":"Re: [PATCH v4 6/7] trace.c: print env vars in trace_run_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-13T07:25:24Z","receivedAt":"2018-01-13T07:25:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 13, 2018 at 01:49:48PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\n> index d24d157379..0ab71f14fb 100644\n> --- a/t/helper/test-run-command.c\n> +++ b/t/helper/test-run-command.c\n> @@ -56,6 +56,10 @@ int cmd_main(int argc, const char **argv)\n> \n> \tif (argc < 3)\n> \t\treturn 1;\n> +\twhile (!strcmp(argv[1], \"env\")) {\n> +\t\targv_array_push(&proc.env_array, argv[2]);\n> +\t\targv += 2;\n> +\t}\n> \tproc.argv = (const char **)argv + 2;\n\nProbably not super important for a test command, but do we want to make\nsure we don't walk past the end of the array in our loop? E.g.:\n\n while (argv[1] && !strcmp(argv[1], \"env\")) {\n\tif (!argv[2]))\n\t\tdie(\"env specifier without a value\");\n\targv_array_push(&proc.env_array, argv[2]);\n\targv += 2;\n\targc -= 2;\n }\n /* make sure we still have 2 args left */\n if (argc < 3)\n\treturn 1;\n proc.argv = ...;\n\n> diff --git a/trace.c b/trace.c\n> index 7f43da211a..ffa1cf9b91 100644\n> --- a/trace.c\n> +++ b/trace.c\n> @@ -275,6 +275,62 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,\n> \n> #endif /* HAVE_VARIADIC_MACROS */\n> \n> +static void add_env(struct strbuf *dst, const char *const *deltaenv)\n> +{\n> +\tstruct string_list envs = STRING_LIST_INIT_DUP;\n> +\tconst char *const *e;\n> +\tint i;\n> +\tint printed_unset = 0;\n> +\n> +\t/* Last one wins, see run-command.c:prep_childenv() for context */\n> +\tfor (e = deltaenv; e && *e; e++) {\n> +\t\tstruct strbuf key = STRBUF_INIT;\n> +\t\tchar *equals = strchr(*e, '=');\n> +\t\tif (equals) {\n> +\t\t\tstrbuf_reset(&key);\n> +\t\t\tstrbuf_add(&key, *e, equals - *e);\n\nThis strbuf_reset() is unnecessary now, since the key strbuf is fresh in\neach loop (alternatively, it could be moved out of the loop to avoid\nrepeated allocations, and move the corresponding release out of the\nloop.\n\n> +\t\t\tstring_list_insert(&envs, key.buf)->util = equals + 1;\n> +\t\t} else {\n> +\t\t\tstring_list_insert(&envs, *e)->util = NULL;\n> +\t\t}\n\nNote that this is quadratic in the size of deltaenv (inserting into a\nsorted list). But it's the same technique that the actual\nprep_childenv() uses, so it's probably OK (we simply don't add that\nlarge a deltaenv in practice).\n\n> +\t/* \"unset X Y...;\" */\n> +\tfor (i = 0; i < envs.nr; i++) {\n> +\t\tconst char *var = envs.items[i].string;\n> +\t\tconst char *val = envs.items[i].util;\n> +\n> +\t\tif (val || !getenv(var))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (!printed_unset) {\n> +\t\t\tstrbuf_addstr(dst, \" unset\");\n> +\t\t\tprinted_unset = 1;\n> +\t\t}\n> +\t\tstrbuf_addf(dst, \" %s\", var);\n> +\t}\n> +\tif (printed_unset)\n> +\t\tstrbuf_addch(dst, ';');\n\nLooks good.\n\n> +\t/* ... followed by \"A=B C=D ...\" */\n> +\tfor (i = 0; i < envs.nr; i++) {\n> +\t\tconst char *var = envs.items[i].string;\n> +\t\tconst char *val = envs.items[i].util;\n> +\t\tconst char *oldval;\n> +\n> +\t\tif (!val)\n> +\t\t\tcontinue;\n> +\n> +\t\toldval = getenv(var);\n> +\t\tif (oldval && !strcmp(val, oldval))\n> +\t\t\tcontinue;\n> +\n> +\t\tstrbuf_addf(dst, \" %s=\", var);\n> +\t\tsq_quote_buf_pretty(dst, val);\n> +\t}\n> +\tstring_list_clear(&envs, 0);\n> +}\n\nLooks good.\n\nThe loops in this one are much easier to follow, IMHO.\n\n-Peff\n"},{"id":"336551","messageId":"20180113072629.GC27251@sigill.intra.peff.net","threadId":"47584","inReplyTo":"20180113064949.6043-1-pclouds@gmail.com","subject":"Re: [PATCH v4 0/7] Trace env variables in run_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-13T07:26:29Z","receivedAt":"2018-01-13T07:26:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 13, 2018 at 01:49:42PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> v4:\n> \n> - incorporates Jeff patches and moves them on top\n> - removes strbuf release from print_trace_line\n> - prints 'unset a b c' instead of 'unset a; unset b; unset c'\n> - squashes v3 3/4 and 4/4 and Junio's patch into 6/7\n> - adds tests\n> - cwd is separated in 7/7\n\nThanks. Aside from the minor nit about argument parsing in\ntest-run-command, this looks good to me.\n\n-Peff\n"},{"id":"336609","messageId":"20180115105949.18328-1-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180113064949.6043-1-pclouds@gmail.com","subject":"[PATCH v5 0/7] Trace env variables in run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-15T10:59:42Z","receivedAt":"2018-01-15T11:00:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v5 \n\n - makes test-run-command safer\n - deletes a dead line\n - fixes an embarassing memory leak\n\nInterdiff\n\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex 0ab71f14fb..153342e44d 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -57,9 +57,14 @@ int cmd_main(int argc, const char **argv)\n \tif (argc < 3)\n \t\treturn 1;\n \twhile (!strcmp(argv[1], \"env\")) {\n+\t\tif (!argv[2])\n+\t\t\tdie(\"env specifier without a value\");\n \t\targv_array_push(&proc.env_array, argv[2]);\n \t\targv += 2;\n+\t\targc -= 2;\n \t}\n+\tif (argc < 3)\n+\t\treturn 1;\n \tproc.argv = (const char **)argv + 2;\n \n \tif (!strcmp(argv[1], \"start-command-ENOENT\")) {\ndiff --git a/trace.c b/trace.c\nindex 4bfd3fce10..ad75d683bf 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -286,8 +286,8 @@ static void add_env(struct strbuf *dst, const char *const *deltaenv)\n \tfor (e = deltaenv; e && *e; e++) {\n \t\tstruct strbuf key = STRBUF_INIT;\n \t\tchar *equals = strchr(*e, '=');\n+\n \t\tif (equals) {\n-\t\t\tstrbuf_reset(&key);\n \t\t\tstrbuf_add(&key, *e, equals - *e);\n \t\t\tstring_list_insert(&envs, key.buf)->util = equals + 1;\n \t\t} else {\n@@ -360,6 +360,7 @@ void trace_run_command(const struct child_process *cp)\n \n \tsq_quote_argv_pretty(&buf, cp->argv);\n \tprint_trace_line(&trace_default_key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n static const char *quote_crnl(const char *path)\n\nJeff King (2):\n sq_quote_argv: drop maxlen parameter\n trace: avoid unnecessary quoting\n\nNguyễn Thái Ngọc Duy (5):\n trace.c: move strbuf_release() out of print_trace_line()\n trace.c: introduce trace_run_command()\n trace.c: print program 'git' when child_process.git_cmd is set\n trace.c: print env vars in trace_run_command()\n trace.c: print new cwd in trace_run_command()\n\n builtin/am.c | 2 +-\n builtin/rev-parse.c | 4 +-\n quote.c | 30 ++++++++++++--\n quote.h | 10 ++++-\n run-command.c | 3 +-\n t/helper/test-run-command.c | 9 +++++\n t/t0061-run-command.sh | 22 ++++++++++\n trace.c | 97 +++++++++++++++++++++++++++++++++++++++++++--\n trace.h | 3 ++\n 9 files changed, 169 insertions(+), 11 deletions(-)\n\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336610","messageId":"20180115105949.18328-2-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180115105949.18328-1-pclouds@gmail.com","subject":"[PATCH v5 1/7] sq_quote_argv: drop maxlen parameter","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-15T10:59:43Z","receivedAt":"2018-01-15T11:00:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Jeff King \n\nNo caller passes anything but \"0\" for this parameter, which\nrequests that the function ignore it completely. In fact, in\nall of history there was only one such caller, and it went\naway in 7f51f8bc2b (alias: use run_command api to execute\naliases, 2011-01-07).\n\nSigned-off-by: Jeff King \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/am.c | 2 +-\n builtin/rev-parse.c | 4 ++--\n quote.c | 4 +---\n quote.h | 2 +-\n trace.c | 4 ++--\n 5 files changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex acfe9d3c8c..5bdd2d7578 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1061,7 +1061,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t}\n \twrite_state_text(state, \"scissors\", str);\n \n-\tsq_quote_argv(&sb, state->git_apply_opts.argv, 0);\n+\tsq_quote_argv(&sb, state->git_apply_opts.argv);\n \twrite_state_text(state, \"apply-opt\", sb.buf);\n \n \tif (state->rebasing)\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 74aa644cbb..96d06a5d01 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -516,7 +516,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_SHELL_EVAL);\n \n \tstrbuf_addstr(&parsed, \" --\");\n-\tsq_quote_argv(&parsed, argv, 0);\n+\tsq_quote_argv(&parsed, argv);\n \tputs(parsed.buf);\n \treturn 0;\n }\n@@ -526,7 +526,7 @@ static int cmd_sq_quote(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \n \tif (argc)\n-\t\tsq_quote_argv(&buf, argv, 0);\n+\t\tsq_quote_argv(&buf, argv);\n \tprintf(\"%s\\n\", buf.buf);\n \tstrbuf_release(&buf);\n \ndiff --git a/quote.c b/quote.c\nindex de2922ddd6..b2970da627 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -56,7 +56,7 @@ void sq_quotef(struct strbuf *dst, const char *fmt, ...)\n \tstrbuf_release(&src);\n }\n \n-void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)\n+void sq_quote_argv(struct strbuf *dst, const char **argv)\n {\n \tint i;\n \n@@ -65,8 +65,6 @@ void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)\n \tfor (i = 0; argv[i]; ++i) {\n \t\tstrbuf_addch(dst, ' ');\n \t\tsq_quote_buf(dst, argv[i]);\n-\t\tif (maxlen && dst->len > maxlen)\n-\t\t\tdie(\"Too many or long arguments\");\n \t}\n }\n \ndiff --git a/quote.h b/quote.h\nindex 66f5644aa2..48a75a416b 100644\n--- a/quote.h\n+++ b/quote.h\n@@ -30,7 +30,7 @@ struct strbuf;\n */\n \n extern void sq_quote_buf(struct strbuf *, const char *src);\n-extern void sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);\n+extern void sq_quote_argv(struct strbuf *, const char **argv);\n extern void sq_quotef(struct strbuf *, const char *fmt, ...);\n \n /* This unwraps what sq_quote() produces in place, but returns\ndiff --git a/trace.c b/trace.c\nindex b7530b51a9..fa9174fc4b 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -157,7 +157,7 @@ static void trace_argv_vprintf_fl(const char *file, int line,\n \n \tstrbuf_vaddf(&buf, format, ap);\n \n-\tsq_quote_argv(&buf, argv, 0);\n+\tsq_quote_argv(&buf, argv);\n \tprint_trace_line(&trace_default_key, &buf);\n }\n \n@@ -426,6 +426,6 @@ void trace_command_performance(const char **argv)\n \t\tatexit(print_command_performance_atexit);\n \n \tstrbuf_reset(&command_line);\n-\tsq_quote_argv(&command_line, argv, 0);\n+\tsq_quote_argv(&command_line, argv);\n \tcommand_start_time = getnanotime();\n }\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336611","messageId":"20180115105949.18328-3-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180115105949.18328-1-pclouds@gmail.com","subject":"[PATCH v5 2/7] trace: avoid unnecessary quoting","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-15T10:59:44Z","receivedAt":"2018-01-15T11:00:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Jeff King \n\nTrace output which contains arbitrary strings (e.g., the\narguments to commands which we are running) is always passed\nthrough sq_quote_buf(). That function always adds\nsingle-quotes, even if the output consists of vanilla\ncharacters. This can make the output a bit hard to read.\n\nLet's avoid the quoting if there are no characters which a\nshell would interpret. Trace output doesn't necessarily need\nto be shell-compatible, but:\n\n - the shell language is a good ballpark for what humans\n consider readable (well, humans versed in command line\n tools)\n\n - the run_command bits can be cut-and-pasted to a shell,\n and we'll keep that property\n\n - it covers any cases which would make the output\n visually ambiguous (e.g., embedded whitespace or quotes)\n\nSigned-off-by: Jeff King \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n quote.c | 26 ++++++++++++++++++++++++++\n quote.h | 8 ++++++++\n trace.c | 4 ++--\n 3 files changed, 36 insertions(+), 2 deletions(-)\n\ndiff --git a/quote.c b/quote.c\nindex b2970da627..37d2686865 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -43,6 +43,22 @@ void sq_quote_buf(struct strbuf *dst, const char *src)\n \tfree(to_free);\n }\n \n+void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n+{\n+\tstatic const char ok_punct[] = \"+,-./:=@_^\";\n+\tconst char *p;\n+\n+\tfor (p = src; *p; p++) {\n+\t\tif (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {\n+\t\t\tsq_quote_buf(dst, src);\n+\t\t\treturn;\n+\t\t}\n+\t}\n+\n+\t/* if we get here, we did not need quoting */\n+\tstrbuf_addstr(dst, src);\n+}\n+\n void sq_quotef(struct strbuf *dst, const char *fmt, ...)\n {\n \tstruct strbuf src = STRBUF_INIT;\n@@ -68,6 +84,16 @@ void sq_quote_argv(struct strbuf *dst, const char **argv)\n \t}\n }\n \n+void sq_quote_argv_pretty(struct strbuf *dst, const char **argv)\n+{\n+\tint i;\n+\n+\tfor (i = 0; argv[i]; i++) {\n+\t\tstrbuf_addch(dst, ' ');\n+\t\tsq_quote_buf_pretty(dst, argv[i]);\n+\t}\n+}\n+\n static char *sq_dequote_step(char *arg, char **next)\n {\n \tchar *dst = arg;\ndiff --git a/quote.h b/quote.h\nindex 48a75a416b..ea992dcc91 100644\n--- a/quote.h\n+++ b/quote.h\n@@ -33,6 +33,14 @@ extern void sq_quote_buf(struct strbuf *, const char *src);\n extern void sq_quote_argv(struct strbuf *, const char **argv);\n extern void sq_quotef(struct strbuf *, const char *fmt, ...);\n \n+/*\n+ * These match their non-pretty variants, except that they avoid\n+ * quoting when there are no exotic characters. These should only be used for\n+ * human-readable output, as sq_dequote() is not smart enough to dequote it.\n+ */\n+void sq_quote_buf_pretty(struct strbuf *, const char *src);\n+void sq_quote_argv_pretty(struct strbuf *, const char **argv);\n+\n /* This unwraps what sq_quote() produces in place, but returns\n * NULL if the input does not look like what sq_quote would have\n * produced.\ndiff --git a/trace.c b/trace.c\nindex fa9174fc4b..9784915be1 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -157,7 +157,7 @@ static void trace_argv_vprintf_fl(const char *file, int line,\n \n \tstrbuf_vaddf(&buf, format, ap);\n \n-\tsq_quote_argv(&buf, argv);\n+\tsq_quote_argv_pretty(&buf, argv);\n \tprint_trace_line(&trace_default_key, &buf);\n }\n \n@@ -426,6 +426,6 @@ void trace_command_performance(const char **argv)\n \t\tatexit(print_command_performance_atexit);\n \n \tstrbuf_reset(&command_line);\n-\tsq_quote_argv(&command_line, argv);\n+\tsq_quote_argv_pretty(&command_line, argv);\n \tcommand_start_time = getnanotime();\n }\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336612","messageId":"20180115105949.18328-4-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180115105949.18328-1-pclouds@gmail.com","subject":"[PATCH v5 3/7] trace.c: move strbuf_release() out of print_trace_line()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-15T10:59:45Z","receivedAt":"2018-01-15T11:00:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The function is about printing a trace line, not releasing the buffer it\nreceives too. Move strbuf_release() back outside. This makes it easier\nto see how strbuf is managed.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n trace.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/trace.c b/trace.c\nindex 9784915be1..7f3b08e148 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -131,7 +131,6 @@ static void print_trace_line(struct trace_key *key, struct strbuf *buf)\n {\n \tstrbuf_complete_line(buf);\n \ttrace_write(key, buf->buf, buf->len);\n-\tstrbuf_release(buf);\n }\n \n static void trace_vprintf_fl(const char *file, int line, struct trace_key *key,\n@@ -144,6 +143,7 @@ static void trace_vprintf_fl(const char *file, int line, struct trace_key *key,\n \n \tstrbuf_vaddf(&buf, format, ap);\n \tprint_trace_line(key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n static void trace_argv_vprintf_fl(const char *file, int line,\n@@ -159,6 +159,7 @@ static void trace_argv_vprintf_fl(const char *file, int line,\n \n \tsq_quote_argv_pretty(&buf, argv);\n \tprint_trace_line(&trace_default_key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n void trace_strbuf_fl(const char *file, int line, struct trace_key *key,\n@@ -171,6 +172,7 @@ void trace_strbuf_fl(const char *file, int line, struct trace_key *key,\n \n \tstrbuf_addbuf(&buf, data);\n \tprint_trace_line(key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n static void trace_performance_vprintf_fl(const char *file, int line,\n@@ -190,6 +192,7 @@ static void trace_performance_vprintf_fl(const char *file, int line,\n \t}\n \n \tprint_trace_line(&trace_perf_key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n #ifndef HAVE_VARIADIC_MACROS\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336613","messageId":"20180115105949.18328-5-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180115105949.18328-1-pclouds@gmail.com","subject":"[PATCH v5 4/7] trace.c: introduce trace_run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-15T10:59:46Z","receivedAt":"2018-01-15T11:00:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This is the same as the old code that uses trace_argv_printf() in\nrun-command.c. This function will be improved in later patches to\nprint more information from struct child_process.\n\nA slight regression: the old code would print run-command.c:xxx as the\ntrace location site while the new code prints trace.c:xxx. This should\nbe fine until the second call site is added, then we might need a macro\nwrapper named trace_run_command() to capture the right source location.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n run-command.c | 3 ++-\n trace.c | 16 ++++++++++++++++\n trace.h | 3 +++\n 3 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 31fc5ea86e..002074b128 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -624,7 +624,8 @@ int start_command(struct child_process *cmd)\n \t\tcmd->err = fderr[0];\n \t}\n \n-\ttrace_argv_printf(cmd->argv, \"trace: run_command:\");\n+\ttrace_run_command(cmd);\n+\n \tfflush(NULL);\n \n #ifndef GIT_WINDOWS_NATIVE\ndiff --git a/trace.c b/trace.c\nindex 7f3b08e148..da3db301e7 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -23,6 +23,7 @@\n \n #include \"cache.h\"\n #include \"quote.h\"\n+#include \"run-command.h\"\n \n struct trace_key trace_default_key = { \"GIT_TRACE\", 0, 0, 0 };\n struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);\n@@ -275,6 +276,21 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,\n #endif /* HAVE_VARIADIC_MACROS */\n \n \n+void trace_run_command(const struct child_process *cp)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tif (!prepare_trace_line(__FILE__, __LINE__,\n+\t\t\t\t&trace_default_key, &buf))\n+\t\treturn;\n+\n+\tstrbuf_addf(&buf, \"trace: run_command:\");\n+\n+\tsq_quote_argv_pretty(&buf, cp->argv);\n+\tprint_trace_line(&trace_default_key, &buf);\n+\tstrbuf_release(&buf);\n+}\n+\n static const char *quote_crnl(const char *path)\n {\n \tstatic struct strbuf new_path = STRBUF_INIT;\ndiff --git a/trace.h b/trace.h\nindex 88055abef7..e54c687f26 100644\n--- a/trace.h\n+++ b/trace.h\n@@ -4,6 +4,8 @@\n #include \"git-compat-util.h\"\n #include \"strbuf.h\"\n \n+struct child_process;\n+\n struct trace_key {\n \tconst char * const key;\n \tint fd;\n@@ -17,6 +19,7 @@ extern struct trace_key trace_default_key;\n extern struct trace_key trace_perf_key;\n \n extern void trace_repo_setup(const char *prefix);\n+extern void trace_run_command(const struct child_process *cp);\n extern int trace_want(struct trace_key *key);\n extern void trace_disable(struct trace_key *key);\n extern uint64_t getnanotime(void);\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336614","messageId":"20180115105949.18328-6-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180115105949.18328-1-pclouds@gmail.com","subject":"[PATCH v5 5/7] trace.c: print program 'git' when child_process.git_cmd is set","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-15T10:59:47Z","receivedAt":"2018-01-15T11:00:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"We normally print full command line, including the program and its\nargument. When git_cmd is set, we have a special code path to run the\nright \"git\" program and child_process.argv[0] will not contain the\nprogram name anymore. As a result, we print just the command\narguments.\n\nI thought it was a regression when the code was refactored and git_cmd\nadded, but apparently it's not. git_cmd mode was introduced before\ntracing was added in 8852f5d704 (run_command(): respect GIT_TRACE -\n2008-07-07) so it's more like an oversight in 8852f5d704.\n\nFix it, print the program name \"git\" in git_cmd mode. It's nice to have\nnow. But it will be more important later when we start to print env\nvariables too, in shell syntax. The lack of a program name would look\nconfusing then.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n trace.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/trace.c b/trace.c\nindex da3db301e7..98bee2c259 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -286,6 +286,9 @@ void trace_run_command(const struct child_process *cp)\n \n \tstrbuf_addf(&buf, \"trace: run_command:\");\n \n+\tif (cp->git_cmd)\n+\t\tstrbuf_addstr(&buf, \" git\");\n+\n \tsq_quote_argv_pretty(&buf, cp->argv);\n \tprint_trace_line(&trace_default_key, &buf);\n \tstrbuf_release(&buf);\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336615","messageId":"20180115105949.18328-7-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180115105949.18328-1-pclouds@gmail.com","subject":"[PATCH v5 6/7] trace.c: print env vars in trace_run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-15T10:59:48Z","receivedAt":"2018-01-15T11:00:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Occasionally submodule code could execute new commands with GIT_DIR set\nto some submodule. GIT_TRACE prints just the command line which makes it\nhard to tell that it's not really executed on this repository.\n\nPrint the env delta (compared to parent environment) in this case.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n t/helper/test-run-command.c | 9 +++++++\n t/t0061-run-command.sh | 22 ++++++++++++++++\n trace.c | 63 +++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 94 insertions(+)\n\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex d24d157379..153342e44d 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -54,6 +54,15 @@ int cmd_main(int argc, const char **argv)\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n \tint jobs;\n \n+\tif (argc < 3)\n+\t\treturn 1;\n+\twhile (!strcmp(argv[1], \"env\")) {\n+\t\tif (!argv[2])\n+\t\t\tdie(\"env specifier without a value\");\n+\t\targv_array_push(&proc.env_array, argv[2]);\n+\t\targv += 2;\n+\t\targc -= 2;\n+\t}\n \tif (argc < 3)\n \t\treturn 1;\n \tproc.argv = (const char **)argv + 2;\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex e4739170aa..e6208316c3 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -141,4 +141,26 @@ test_expect_success 'run_command outputs ' '\n \ttest_cmp expect actual\n '\n \n+test_trace() {\n+\tlocal expected=\"$1\"\n+\tshift\n+\tGIT_TRACE=1 test-run-command \"$@\" run-command true 2>&1 >/dev/null | \\\n+\t\tsed 's/.* run_command: //' >actual &&\n+\techo \"$expected true\" >expected &&\n+\ttest_cmp expected actual\n+}\n+\n+test_expect_success 'GIT_TRACE with environment variables' '\n+\ttest_trace \"abc=1 def=2\" env abc=1 env def=2 &&\n+\ttest_trace \"abc=2\" env abc env abc=1 env abc=2 &&\n+\ttest_trace \"abc=2\" env abc env abc=2 &&\n+\tabc=1 test_trace \"def=1\" env abc=1 env def=1 &&\n+\tabc=1 test_trace \"def=1\" env abc env abc=1 env def=1 &&\n+\ttest_trace \"def=1\" env non-exist env def=1 &&\n+\ttest_trace \"abc=2\" env abc=1 env abc env abc=2 &&\n+\tabc=1 def=2 test_trace \"unset abc def;\" env abc env def &&\n+\tabc=1 def=2 test_trace \"unset def; abc=3\" env abc env def env abc=3 &&\n+\tabc=1 test_trace \"unset abc;\" env abc=2 env abc\n+'\n+\n test_done\ndiff --git a/trace.c b/trace.c\nindex 98bee2c259..8db5bbf3c9 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -275,6 +275,62 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,\n \n #endif /* HAVE_VARIADIC_MACROS */\n \n+static void add_env(struct strbuf *dst, const char *const *deltaenv)\n+{\n+\tstruct string_list envs = STRING_LIST_INIT_DUP;\n+\tconst char *const *e;\n+\tint i;\n+\tint printed_unset = 0;\n+\n+\t/* Last one wins, see run-command.c:prep_childenv() for context */\n+\tfor (e = deltaenv; e && *e; e++) {\n+\t\tstruct strbuf key = STRBUF_INIT;\n+\t\tchar *equals = strchr(*e, '=');\n+\n+\t\tif (equals) {\n+\t\t\tstrbuf_add(&key, *e, equals - *e);\n+\t\t\tstring_list_insert(&envs, key.buf)->util = equals + 1;\n+\t\t} else {\n+\t\t\tstring_list_insert(&envs, *e)->util = NULL;\n+\t\t}\n+\t\tstrbuf_release(&key);\n+\t}\n+\n+\t/* \"unset X Y...;\" */\n+\tfor (i = 0; i < envs.nr; i++) {\n+\t\tconst char *var = envs.items[i].string;\n+\t\tconst char *val = envs.items[i].util;\n+\n+\t\tif (val || !getenv(var))\n+\t\t\tcontinue;\n+\n+\t\tif (!printed_unset) {\n+\t\t\tstrbuf_addstr(dst, \" unset\");\n+\t\t\tprinted_unset = 1;\n+\t\t}\n+\t\tstrbuf_addf(dst, \" %s\", var);\n+\t}\n+\tif (printed_unset)\n+\t\tstrbuf_addch(dst, ';');\n+\n+\t/* ... followed by \"A=B C=D ...\" */\n+\tfor (i = 0; i < envs.nr; i++) {\n+\t\tconst char *var = envs.items[i].string;\n+\t\tconst char *val = envs.items[i].util;\n+\t\tconst char *oldval;\n+\n+\t\tif (!val)\n+\t\t\tcontinue;\n+\n+\t\toldval = getenv(var);\n+\t\tif (oldval && !strcmp(val, oldval))\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_addf(dst, \" %s=\", var);\n+\t\tsq_quote_buf_pretty(dst, val);\n+\t}\n+\tstring_list_clear(&envs, 0);\n+}\n \n void trace_run_command(const struct child_process *cp)\n {\n@@ -286,6 +342,13 @@ void trace_run_command(const struct child_process *cp)\n \n \tstrbuf_addf(&buf, \"trace: run_command:\");\n \n+\t/*\n+\t * The caller is responsible for initializing cp->env from\n+\t * cp->env_array if needed. We only check one place.\n+\t */\n+\tif (cp->env)\n+\t\tadd_env(&buf, cp->env);\n+\n \tif (cp->git_cmd)\n \t\tstrbuf_addstr(&buf, \" git\");\n \n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336616","messageId":"20180115105949.18328-8-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180115105949.18328-1-pclouds@gmail.com","subject":"[PATCH v5 7/7] trace.c: print new cwd in trace_run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-15T10:59:49Z","receivedAt":"2018-01-15T11:00:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If a command sets a new env variable GIT_DIR=.git, we need more context\nto know where that '.git' is related to.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n trace.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/trace.c b/trace.c\nindex 8db5bbf3c9..ad75d683bf 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -342,6 +342,12 @@ void trace_run_command(const struct child_process *cp)\n \n \tstrbuf_addf(&buf, \"trace: run_command:\");\n \n+\tif (cp->dir) {\n+\t\tstrbuf_addstr(&buf, \" cd \");\n+\t\tsq_quote_buf_pretty(&buf, cp->dir);\n+\t\tstrbuf_addch(&buf, ';');\n+\t}\n+\n \t/*\n \t * The caller is responsible for initializing cp->env from\n \t * cp->env_array if needed. We only check one place.\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336664","messageId":"20180116182432.GA135179@google.com","threadId":"47584","inReplyTo":"20180115105949.18328-4-pclouds@gmail.com","subject":"Re: [PATCH v5 3/7] trace.c: move strbuf_release() out of print_trace_line()","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-01-16T18:24:32Z","receivedAt":"2018-01-16T18:24:39Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 01/15, Nguyễn Thái Ngọc Duy wrote:\n> The function is about printing a trace line, not releasing the buffer it\n> receives too. Move strbuf_release() back outside. This makes it easier\n> to see how strbuf is managed.\n\nThis makes the code much more readable.\n\n-- \nBrandon Williams\n"},{"id":"336666","messageId":"20180116183215.GB135179@google.com","threadId":"47584","inReplyTo":"20180115105949.18328-7-pclouds@gmail.com","subject":"Re: [PATCH v5 6/7] trace.c: print env vars in trace_run_command()","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-01-16T18:32:15Z","receivedAt":"2018-01-16T18:32:23Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 01/15, Nguyễn Thái Ngọc Duy wrote:\n> Occasionally submodule code could execute new commands with GIT_DIR set\n> to some submodule. GIT_TRACE prints just the command line which makes it\n> hard to tell that it's not really executed on this repository.\n> \n> Print the env delta (compared to parent environment) in this case.\n> \n\nCode looks clean and very readable. Also matches the last one wins\nsemantics in prep_childenv.\n\n-- \nBrandon Williams\n"},{"id":"336677","messageId":"xmqqmv1dtptj.fsf@gitster.mtv.corp.google.com","threadId":"47584","inReplyTo":"20180113064949.6043-7-pclouds@gmail.com","subject":"Re: [PATCH v4 6/7] trace.c: print env vars in trace_run_command()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-16T22:13:28Z","receivedAt":"2018-01-16T22:13:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> +test_trace() {\n> +\tlocal expected=\"$1\"\n\nStyle: \"test_trace () {\" is how we start a shell function.\nPortability: we do not use \"local\".\n\n> +\tshift\n> +\tGIT_TRACE=1 test-run-command \"$@\" run-command true 2>&1 >/dev/null | \\\n> +\t\tsed 's/.* run_command: //' >actual &&\n> +\techo \"$expected true\" >expected &&\n> +\ttest_cmp expected actual\n\nConsistency: everybody else in the test script contrasts \"actual\" vs\n\"expect\" (they happen to be of the same length ;-); don't say\nexpectED just to be different.\n\n> +}\n> +\n> +test_expect_success 'GIT_TRACE with environment variables' '\n> +\ttest_trace \"abc=1 def=2\" env abc=1 env def=2 &&\n> +\ttest_trace \"abc=2\" env abc env abc=1 env abc=2 &&\n> +\ttest_trace \"abc=2\" env abc env abc=2 &&\n> +\tabc=1 test_trace \"def=1\" env abc=1 env def=1 &&\n\nPortability: \"VAR=VAL shell_function args...\" does not work\neverywhere, unlike \"VAR=VAL command args...\" that exports the set of\nenvironment variables for just a single shot invocation of the\ncommand.\n\nRunning this script with dash fails, even though bash does behave as\nyou expect.\n\n> +\tabc=1 test_trace \"def=1\" env abc env abc=1 env def=1 &&\n> +\ttest_trace \"def=1\" env non-exist env def=1 &&\n> +\ttest_trace \"abc=2\" env abc=1 env abc env abc=2 &&\n> +\tabc=1 def=2 test_trace \"unset abc def;\" env abc env def &&\n> +\tabc=1 def=2 test_trace \"unset def; abc=3\" env abc env def env abc=3 &&\n> +\tabc=1 test_trace \"unset abc;\" env abc=2 env abc\n> +'\n\nTaking them together, here is what I'll be queuing on top to make\ntoday's push-out to pass the tests.\n\nThanks.\n\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex e6208316c3..24c92b6cd7 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -141,26 +141,41 @@ test_expect_success 'run_command outputs ' '\n \ttest_cmp expect actual\n '\n \n-test_trace() {\n-\tlocal expected=\"$1\"\n+test_trace () {\n+\texpect=\"$1\"\n \tshift\n \tGIT_TRACE=1 test-run-command \"$@\" run-command true 2>&1 >/dev/null | \\\n \t\tsed 's/.* run_command: //' >actual &&\n-\techo \"$expected true\" >expected &&\n-\ttest_cmp expected actual\n+\techo \"$expect true\" >expect &&\n+\ttest_cmp expect actual\n }\n \n test_expect_success 'GIT_TRACE with environment variables' '\n \ttest_trace \"abc=1 def=2\" env abc=1 env def=2 &&\n \ttest_trace \"abc=2\" env abc env abc=1 env abc=2 &&\n \ttest_trace \"abc=2\" env abc env abc=2 &&\n-\tabc=1 test_trace \"def=1\" env abc=1 env def=1 &&\n-\tabc=1 test_trace \"def=1\" env abc env abc=1 env def=1 &&\n+\t(\n+\t\tabc=1 && export abc &&\n+\t\ttest_trace \"def=1\" env abc=1 env def=1\n+\t) &&\n+\t(\n+\t\tabc=1 && export abc &&\n+\t\ttest_trace \"def=1\" env abc env abc=1 env def=1\n+\t) &&\n \ttest_trace \"def=1\" env non-exist env def=1 &&\n \ttest_trace \"abc=2\" env abc=1 env abc env abc=2 &&\n-\tabc=1 def=2 test_trace \"unset abc def;\" env abc env def &&\n-\tabc=1 def=2 test_trace \"unset def; abc=3\" env abc env def env abc=3 &&\n-\tabc=1 test_trace \"unset abc;\" env abc=2 env abc\n+\t(\n+\t\tabc=1 def=2 && export abc def &&\n+\t\ttest_trace \"unset abc def;\" env abc env def\n+\t) &&\n+\t(\n+\t\tabc=1 def=2 && export abc def &&\n+\t\ttest_trace \"unset def; abc=3\" env abc env def env abc=3\n+\t) &&\n+\t(\n+\t\tabc=1 && export abc &&\n+\t\ttest_trace \"unset abc;\" env abc=2 env abc\n+\t)\n '\n \n test_done\n-- \n2.16.0-rc2-179-g03f5267cd7\n\n"},{"id":"336678","messageId":"CAGZ79kbWvxoBtcU9iKPHSy_Agz-ko_KSE_M9budrtCOq7-Swdw@mail.gmail.com","threadId":"47584","inReplyTo":"xmqqmv1dtptj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 6/7] trace.c: print env vars in trace_run_command()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-01-16T22:20:53Z","receivedAt":"2018-01-16T22:21:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 16, 2018 at 2:13 PM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> +test_trace() {\n>> + local expected=\"$1\"\n>\n> Style: \"test_trace () {\" is how we start a shell function.\n> Portability: we do not use \"local\".\n>\n>> + shift\n>> + GIT_TRACE=1 test-run-command \"$@\" run-command true 2>&1 >/dev/null | \\\n>> + sed 's/.* run_command: //' >actual &&\n>> + echo \"$expected true\" >expected &&\n>> + test_cmp expected actual\n>\n> Consistency: everybody else in the test script contrasts \"actual\" vs\n> \"expect\" (they happen to be of the same length ;-); don't say\n> expectED just to be different.\n\n3491 entries in the t/ directory disagree with your imagination of\nconsistency. ;)\nBut I agree that we want to go for consistency, and most likely it's\nbest to go for 'expect'.\n"},{"id":"336688","messageId":"xmqqwp0hs20q.fsf@gitster.mtv.corp.google.com","threadId":"47584","inReplyTo":"CAGZ79kbWvxoBtcU9iKPHSy_Agz-ko_KSE_M9budrtCOq7-Swdw@mail.gmail.com","subject":"Re: [PATCH v4 6/7] trace.c: print env vars in trace_run_command()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-17T01:32:53Z","receivedAt":"2018-01-17T01:33:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Tue, Jan 16, 2018 at 2:13 PM, Junio C Hamano wrote:\n>> Nguyễn Thái Ngọc Duy writes:\n>>\n>>> +test_trace() {\n>>> + local expected=\"$1\"\n>>\n>> Style: \"test_trace () {\" is how we start a shell function.\n>> Portability: we do not use \"local\".\n>>\n>>> + shift\n>>> + GIT_TRACE=1 test-run-command \"$@\" run-command true 2>&1 >/dev/null | \\\n>>> + sed 's/.* run_command: //' >actual &&\n>>> + echo \"$expected true\" >expected &&\n>>> + test_cmp expected actual\n>>\n>> Consistency: everybody else in the test script contrasts \"actual\" vs\n>> \"expect\" (they happen to be of the same length ;-); don't say\n>> expectED just to be different.\n>\n> 3491 entries in the t/ directory disagree with your imagination of\n> consistency. ;)\n\nHeh. I thought it was clear enough from the context that I was\ntalking about matching the remainder of the same test script; I do\nnot have time time to count 'test_cmp expected' in all others in t/\n;-)\n\n> But I agree that we want to go for consistency, and most likely it's\n> best to go for 'expect'.\n\nIt is unclear if you are volunteering, but do not change things for\nthe sake of making them consistent. Not adding new ones and fixing\nexisting ones when they need to change anyway for some other reason\nis the way to do this without unnecessary code churn.\n\n"},{"id":"336733","messageId":"20180117222143.GA15317@sigill.intra.peff.net","threadId":"47584","inReplyTo":"20180115105949.18328-5-pclouds@gmail.com","subject":"Re: [PATCH v5 4/7] trace.c: introduce trace_run_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-17T22:21:43Z","receivedAt":"2018-01-17T22:21:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 15, 2018 at 05:59:46PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> diff --git a/trace.c b/trace.c\n> index 7f3b08e148..da3db301e7 100644\n> --- a/trace.c\n> +++ b/trace.c\n> @@ -23,6 +23,7 @@\n> \n> #include \"cache.h\"\n> #include \"quote.h\"\n> +#include \"run-command.h\"\n> \n> struct trace_key trace_default_key = { \"GIT_TRACE\", 0, 0, 0 };\n> struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);\n> @@ -275,6 +276,21 @@ void trace_performance_fl(const char *file, int line, uint64_t nanos,\n> #endif /* HAVE_VARIADIC_MACROS */\n> \n> \n> +void trace_run_command(const struct child_process *cp)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\tif (!prepare_trace_line(__FILE__, __LINE__,\n> +\t\t\t\t&trace_default_key, &buf))\n> +\t\treturn;\n\nI think technically this should be TRACE_CONTEXT instead of __FILE__,\nthough I wonder if anybody has ever set that (there is not even a\nMakefile knob for it, so you'd have to -DTRACE_CONTEXT manually).\n\nI actually wonder if it would be nicer to keep this whole thing in\nrun-command.c, and just prepare it all in a buffer. That's basically\nwhat we're doing here anyway, and then we could get rid of the funny\n__FILE__ stuff. I.e., something like:\n\n struct strbuf buf = STRBUF_INIT;\n\n if (!trace_want(&trace_default_key))\n\treturn;\n\n strbuf_addf(&buf, \"trace: run_command: \");\n sq_quote_argv_pretty(&buf, cp->argv);\n\n trace_printf(\"%s\", buf.buf);\n strbuf_release(&buf);\n\nAFAICT we aren't really using any internals of trace.c in our new\nfunction. That would get rid of this __FILE__ bit, it would eliminate\nthe need for trace.h to know about \"struct child_process\", and it would\nmean the output still says \"run-command.c\" in it.\n\n-Peff\n"},{"id":"336738","messageId":"xmqqr2qoqfaf.fsf@gitster.mtv.corp.google.com","threadId":"47584","inReplyTo":"20180117222143.GA15317@sigill.intra.peff.net","subject":"Re: [PATCH v5 4/7] trace.c: introduce trace_run_command()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-17T22:41:28Z","receivedAt":"2018-01-17T22:41:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +void trace_run_command(const struct child_process *cp)\n>> +{\n>> +\tstruct strbuf buf = STRBUF_INIT;\n>> +\n>> +\tif (!prepare_trace_line(__FILE__, __LINE__,\n>> +\t\t\t\t&trace_default_key, &buf))\n>> +\t\treturn;\n>\n> I think technically this should be TRACE_CONTEXT instead of __FILE__,\n> though I wonder if anybody has ever set that (there is not even a\n> Makefile knob for it, so you'd have to -DTRACE_CONTEXT manually).\n\nOooh. Today I learned a new thing ;-)\n\n> I actually wonder if it would be nicer to keep this whole thing in\n> run-command.c, and just prepare it all in a buffer. That's basically\n> what we're doing here anyway, and then we could get rid of the funny\n> __FILE__ stuff. I.e., something like:\n>\n> struct strbuf buf = STRBUF_INIT;\n>\n> if (!trace_want(&trace_default_key))\n> \treturn;\n>\n> strbuf_addf(&buf, \"trace: run_command: \");\n> sq_quote_argv_pretty(&buf, cp->argv);\n>\n> trace_printf(\"%s\", buf.buf);\n> strbuf_release(&buf);\n>\n> AFAICT we aren't really using any internals of trace.c in our new\n> function. That would get rid of this __FILE__ bit, it would eliminate\n> the need for trace.h to know about \"struct child_process\", and it would\n> mean the output still says \"run-command.c\" in it.\n\nSounds nice.\n"},{"id":"336754","messageId":"20180118094512.27849-1-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180115105949.18328-1-pclouds@gmail.com","subject":"[PATCH v6 0/7] Trace env variables in run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-18T09:45:05Z","receivedAt":"2018-01-18T09:45:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v6 squashes Junio's changes in 6/7 and moves trace_run_command() from\ntrace.c to run-command.c.\n\nJeff King (2):\n sq_quote_argv: drop maxlen parameter\n trace: avoid unnecessary quoting\n\nNguyễn Thái Ngọc Duy (5):\n trace.c: move strbuf_release() out of print_trace_line()\n run-command.c: introduce trace_run_command()\n run-command.c: print program 'git' when tracing git_cmd mode\n run-command.c: print env vars in trace_run_command()\n run-command.c: print new cwd in trace_run_command()\n\n builtin/am.c | 2 +-\n builtin/rev-parse.c | 4 +--\n quote.c | 30 ++++++++++++++--\n quote.h | 10 +++++-\n run-command.c | 88 ++++++++++++++++++++++++++++++++++++++++++++-\n t/helper/test-run-command.c | 9 +++++\n t/t0061-run-command.sh | 37 +++++++++++++++++++\n trace.c | 9 +++--\n 8 files changed, 178 insertions(+), 11 deletions(-)\n\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336755","messageId":"20180118094512.27849-2-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180118094512.27849-1-pclouds@gmail.com","subject":"[PATCH v6 1/7] sq_quote_argv: drop maxlen parameter","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-18T09:45:06Z","receivedAt":"2018-01-18T09:45:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Jeff King \n\nNo caller passes anything but \"0\" for this parameter, which\nrequests that the function ignore it completely. In fact, in\nall of history there was only one such caller, and it went\naway in 7f51f8bc2b (alias: use run_command api to execute\naliases, 2011-01-07).\n\nSigned-off-by: Jeff King \nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n builtin/am.c | 2 +-\n builtin/rev-parse.c | 4 ++--\n quote.c | 4 +---\n quote.h | 2 +-\n trace.c | 4 ++--\n 5 files changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex acfe9d3c8c..5bdd2d7578 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1061,7 +1061,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t}\n \twrite_state_text(state, \"scissors\", str);\n \n-\tsq_quote_argv(&sb, state->git_apply_opts.argv, 0);\n+\tsq_quote_argv(&sb, state->git_apply_opts.argv);\n \twrite_state_text(state, \"apply-opt\", sb.buf);\n \n \tif (state->rebasing)\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 74aa644cbb..96d06a5d01 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -516,7 +516,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_SHELL_EVAL);\n \n \tstrbuf_addstr(&parsed, \" --\");\n-\tsq_quote_argv(&parsed, argv, 0);\n+\tsq_quote_argv(&parsed, argv);\n \tputs(parsed.buf);\n \treturn 0;\n }\n@@ -526,7 +526,7 @@ static int cmd_sq_quote(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \n \tif (argc)\n-\t\tsq_quote_argv(&buf, argv, 0);\n+\t\tsq_quote_argv(&buf, argv);\n \tprintf(\"%s\\n\", buf.buf);\n \tstrbuf_release(&buf);\n \ndiff --git a/quote.c b/quote.c\nindex de2922ddd6..b2970da627 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -56,7 +56,7 @@ void sq_quotef(struct strbuf *dst, const char *fmt, ...)\n \tstrbuf_release(&src);\n }\n \n-void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)\n+void sq_quote_argv(struct strbuf *dst, const char **argv)\n {\n \tint i;\n \n@@ -65,8 +65,6 @@ void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)\n \tfor (i = 0; argv[i]; ++i) {\n \t\tstrbuf_addch(dst, ' ');\n \t\tsq_quote_buf(dst, argv[i]);\n-\t\tif (maxlen && dst->len > maxlen)\n-\t\t\tdie(\"Too many or long arguments\");\n \t}\n }\n \ndiff --git a/quote.h b/quote.h\nindex 66f5644aa2..48a75a416b 100644\n--- a/quote.h\n+++ b/quote.h\n@@ -30,7 +30,7 @@ struct strbuf;\n */\n \n extern void sq_quote_buf(struct strbuf *, const char *src);\n-extern void sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);\n+extern void sq_quote_argv(struct strbuf *, const char **argv);\n extern void sq_quotef(struct strbuf *, const char *fmt, ...);\n \n /* This unwraps what sq_quote() produces in place, but returns\ndiff --git a/trace.c b/trace.c\nindex b7530b51a9..fa9174fc4b 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -157,7 +157,7 @@ static void trace_argv_vprintf_fl(const char *file, int line,\n \n \tstrbuf_vaddf(&buf, format, ap);\n \n-\tsq_quote_argv(&buf, argv, 0);\n+\tsq_quote_argv(&buf, argv);\n \tprint_trace_line(&trace_default_key, &buf);\n }\n \n@@ -426,6 +426,6 @@ void trace_command_performance(const char **argv)\n \t\tatexit(print_command_performance_atexit);\n \n \tstrbuf_reset(&command_line);\n-\tsq_quote_argv(&command_line, argv, 0);\n+\tsq_quote_argv(&command_line, argv);\n \tcommand_start_time = getnanotime();\n }\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336756","messageId":"20180118094512.27849-3-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180118094512.27849-1-pclouds@gmail.com","subject":"[PATCH v6 2/7] trace: avoid unnecessary quoting","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-18T09:45:07Z","receivedAt":"2018-01-18T09:45:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Jeff King \n\nTrace output which contains arbitrary strings (e.g., the\narguments to commands which we are running) is always passed\nthrough sq_quote_buf(). That function always adds\nsingle-quotes, even if the output consists of vanilla\ncharacters. This can make the output a bit hard to read.\n\nLet's avoid the quoting if there are no characters which a\nshell would interpret. Trace output doesn't necessarily need\nto be shell-compatible, but:\n\n - the shell language is a good ballpark for what humans\n consider readable (well, humans versed in command line\n tools)\n\n - the run_command bits can be cut-and-pasted to a shell,\n and we'll keep that property\n\n - it covers any cases which would make the output\n visually ambiguous (e.g., embedded whitespace or quotes)\n\nSigned-off-by: Jeff King \nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n quote.c | 26 ++++++++++++++++++++++++++\n quote.h | 8 ++++++++\n trace.c | 4 ++--\n 3 files changed, 36 insertions(+), 2 deletions(-)\n\ndiff --git a/quote.c b/quote.c\nindex b2970da627..37d2686865 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -43,6 +43,22 @@ void sq_quote_buf(struct strbuf *dst, const char *src)\n \tfree(to_free);\n }\n \n+void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n+{\n+\tstatic const char ok_punct[] = \"+,-./:=@_^\";\n+\tconst char *p;\n+\n+\tfor (p = src; *p; p++) {\n+\t\tif (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {\n+\t\t\tsq_quote_buf(dst, src);\n+\t\t\treturn;\n+\t\t}\n+\t}\n+\n+\t/* if we get here, we did not need quoting */\n+\tstrbuf_addstr(dst, src);\n+}\n+\n void sq_quotef(struct strbuf *dst, const char *fmt, ...)\n {\n \tstruct strbuf src = STRBUF_INIT;\n@@ -68,6 +84,16 @@ void sq_quote_argv(struct strbuf *dst, const char **argv)\n \t}\n }\n \n+void sq_quote_argv_pretty(struct strbuf *dst, const char **argv)\n+{\n+\tint i;\n+\n+\tfor (i = 0; argv[i]; i++) {\n+\t\tstrbuf_addch(dst, ' ');\n+\t\tsq_quote_buf_pretty(dst, argv[i]);\n+\t}\n+}\n+\n static char *sq_dequote_step(char *arg, char **next)\n {\n \tchar *dst = arg;\ndiff --git a/quote.h b/quote.h\nindex 48a75a416b..ea992dcc91 100644\n--- a/quote.h\n+++ b/quote.h\n@@ -33,6 +33,14 @@ extern void sq_quote_buf(struct strbuf *, const char *src);\n extern void sq_quote_argv(struct strbuf *, const char **argv);\n extern void sq_quotef(struct strbuf *, const char *fmt, ...);\n \n+/*\n+ * These match their non-pretty variants, except that they avoid\n+ * quoting when there are no exotic characters. These should only be used for\n+ * human-readable output, as sq_dequote() is not smart enough to dequote it.\n+ */\n+void sq_quote_buf_pretty(struct strbuf *, const char *src);\n+void sq_quote_argv_pretty(struct strbuf *, const char **argv);\n+\n /* This unwraps what sq_quote() produces in place, but returns\n * NULL if the input does not look like what sq_quote would have\n * produced.\ndiff --git a/trace.c b/trace.c\nindex fa9174fc4b..9784915be1 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -157,7 +157,7 @@ static void trace_argv_vprintf_fl(const char *file, int line,\n \n \tstrbuf_vaddf(&buf, format, ap);\n \n-\tsq_quote_argv(&buf, argv);\n+\tsq_quote_argv_pretty(&buf, argv);\n \tprint_trace_line(&trace_default_key, &buf);\n }\n \n@@ -426,6 +426,6 @@ void trace_command_performance(const char **argv)\n \t\tatexit(print_command_performance_atexit);\n \n \tstrbuf_reset(&command_line);\n-\tsq_quote_argv(&command_line, argv);\n+\tsq_quote_argv_pretty(&command_line, argv);\n \tcommand_start_time = getnanotime();\n }\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336757","messageId":"20180118094512.27849-4-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180118094512.27849-1-pclouds@gmail.com","subject":"[PATCH v6 3/7] trace.c: move strbuf_release() out of print_trace_line()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-18T09:45:08Z","receivedAt":"2018-01-18T09:45:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The function is about printing a trace line, not releasing the buffer it\nreceives too. Move strbuf_release() back outside. This makes it easier\nto see how strbuf is managed.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n trace.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/trace.c b/trace.c\nindex 9784915be1..7f3b08e148 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -131,7 +131,6 @@ static void print_trace_line(struct trace_key *key, struct strbuf *buf)\n {\n \tstrbuf_complete_line(buf);\n \ttrace_write(key, buf->buf, buf->len);\n-\tstrbuf_release(buf);\n }\n \n static void trace_vprintf_fl(const char *file, int line, struct trace_key *key,\n@@ -144,6 +143,7 @@ static void trace_vprintf_fl(const char *file, int line, struct trace_key *key,\n \n \tstrbuf_vaddf(&buf, format, ap);\n \tprint_trace_line(key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n static void trace_argv_vprintf_fl(const char *file, int line,\n@@ -159,6 +159,7 @@ static void trace_argv_vprintf_fl(const char *file, int line,\n \n \tsq_quote_argv_pretty(&buf, argv);\n \tprint_trace_line(&trace_default_key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n void trace_strbuf_fl(const char *file, int line, struct trace_key *key,\n@@ -171,6 +172,7 @@ void trace_strbuf_fl(const char *file, int line, struct trace_key *key,\n \n \tstrbuf_addbuf(&buf, data);\n \tprint_trace_line(key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n static void trace_performance_vprintf_fl(const char *file, int line,\n@@ -190,6 +192,7 @@ static void trace_performance_vprintf_fl(const char *file, int line,\n \t}\n \n \tprint_trace_line(&trace_perf_key, &buf);\n+\tstrbuf_release(&buf);\n }\n \n #ifndef HAVE_VARIADIC_MACROS\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336758","messageId":"20180118094512.27849-5-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180118094512.27849-1-pclouds@gmail.com","subject":"[PATCH v6 4/7] run-command.c: introduce trace_run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-18T09:45:09Z","receivedAt":"2018-01-18T09:45:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This is the same as the old code that uses trace_argv_printf() in\nrun-command.c. This function will be improved in later patches to\nprint more information from struct child_process.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n run-command.c | 18 +++++++++++++++++-\n 1 file changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 31fc5ea86e..eadba8cb0d 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -6,6 +6,7 @@\n #include \"thread-utils.h\"\n #include \"strbuf.h\"\n #include \"string-list.h\"\n+#include \"quote.h\"\n \n void child_process_init(struct child_process *child)\n {\n@@ -556,6 +557,20 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n \treturn code;\n }\n \n+static void trace_run_command(const struct child_process *cp)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tif (!trace_want(&trace_default_key))\n+\t\treturn;\n+\n+\tstrbuf_addf(&buf, \"trace: run_command:\");\n+\tsq_quote_argv_pretty(&buf, cp->argv);\n+\n+\ttrace_printf(\"%s\", buf.buf);\n+\tstrbuf_release(&buf);\n+}\n+\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n@@ -624,7 +639,8 @@ int start_command(struct child_process *cmd)\n \t\tcmd->err = fderr[0];\n \t}\n \n-\ttrace_argv_printf(cmd->argv, \"trace: run_command:\");\n+\ttrace_run_command(cmd);\n+\n \tfflush(NULL);\n \n #ifndef GIT_WINDOWS_NATIVE\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336759","messageId":"20180118094512.27849-6-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180118094512.27849-1-pclouds@gmail.com","subject":"[PATCH v6 5/7] run-command.c: print program 'git' when tracing git_cmd mode","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-18T09:45:10Z","receivedAt":"2018-01-18T09:46:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"We normally print full command line, including the program and its\nargument. When git_cmd is set, we have a special code path to run the\nright \"git\" program and child_process.argv[0] will not contain the\nprogram name anymore. As a result, we print just the command\narguments.\n\nI thought it was a regression when the code was refactored and git_cmd\nadded, but apparently it's not. git_cmd mode was introduced before\ntracing was added in 8852f5d704 (run_command(): respect GIT_TRACE -\n2008-07-07) so it's more like an oversight in 8852f5d704.\n\nFix it, print the program name \"git\" in git_cmd mode. It's nice to have\nnow. But it will be more important later when we start to print env\nvariables too, in shell syntax. The lack of a program name would look\nconfusing then.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n run-command.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex eadba8cb0d..326c33e3cd 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -565,6 +565,8 @@ static void trace_run_command(const struct child_process *cp)\n \t\treturn;\n \n \tstrbuf_addf(&buf, \"trace: run_command:\");\n+\tif (cp->git_cmd)\n+\t\tstrbuf_addstr(&buf, \" git\");\n \tsq_quote_argv_pretty(&buf, cp->argv);\n \n \ttrace_printf(\"%s\", buf.buf);\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336760","messageId":"20180118094512.27849-7-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180118094512.27849-1-pclouds@gmail.com","subject":"[PATCH v6 6/7] run-command.c: print env vars in trace_run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-18T09:45:11Z","receivedAt":"2018-01-18T09:46:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Occasionally submodule code could execute new commands with GIT_DIR set\nto some submodule. GIT_TRACE prints just the command line which makes it\nhard to tell that it's not really executed on this repository.\n\nPrint the env delta (compared to parent environment) in this case.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n run-command.c | 63 +++++++++++++++++++++++++++++++++++++++++++++\n t/helper/test-run-command.c | 9 +++++++\n t/t0061-run-command.sh | 37 ++++++++++++++++++++++++++\n 3 files changed, 109 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 326c33e3cd..1301b878c7 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -557,6 +557,63 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n \treturn code;\n }\n \n+static void trace_add_env(struct strbuf *dst, const char *const *deltaenv)\n+{\n+\tstruct string_list envs = STRING_LIST_INIT_DUP;\n+\tconst char *const *e;\n+\tint i;\n+\tint printed_unset = 0;\n+\n+\t/* Last one wins, see run-command.c:prep_childenv() for context */\n+\tfor (e = deltaenv; e && *e; e++) {\n+\t\tstruct strbuf key = STRBUF_INIT;\n+\t\tchar *equals = strchr(*e, '=');\n+\n+\t\tif (equals) {\n+\t\t\tstrbuf_add(&key, *e, equals - *e);\n+\t\t\tstring_list_insert(&envs, key.buf)->util = equals + 1;\n+\t\t} else {\n+\t\t\tstring_list_insert(&envs, *e)->util = NULL;\n+\t\t}\n+\t\tstrbuf_release(&key);\n+\t}\n+\n+\t/* \"unset X Y...;\" */\n+\tfor (i = 0; i < envs.nr; i++) {\n+\t\tconst char *var = envs.items[i].string;\n+\t\tconst char *val = envs.items[i].util;\n+\n+\t\tif (val || !getenv(var))\n+\t\t\tcontinue;\n+\n+\t\tif (!printed_unset) {\n+\t\t\tstrbuf_addstr(dst, \" unset\");\n+\t\t\tprinted_unset = 1;\n+\t\t}\n+\t\tstrbuf_addf(dst, \" %s\", var);\n+\t}\n+\tif (printed_unset)\n+\t\tstrbuf_addch(dst, ';');\n+\n+\t/* ... followed by \"A=B C=D ...\" */\n+\tfor (i = 0; i < envs.nr; i++) {\n+\t\tconst char *var = envs.items[i].string;\n+\t\tconst char *val = envs.items[i].util;\n+\t\tconst char *oldval;\n+\n+\t\tif (!val)\n+\t\t\tcontinue;\n+\n+\t\toldval = getenv(var);\n+\t\tif (oldval && !strcmp(val, oldval))\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_addf(dst, \" %s=\", var);\n+\t\tsq_quote_buf_pretty(dst, val);\n+\t}\n+\tstring_list_clear(&envs, 0);\n+}\n+\n static void trace_run_command(const struct child_process *cp)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -565,6 +622,12 @@ static void trace_run_command(const struct child_process *cp)\n \t\treturn;\n \n \tstrbuf_addf(&buf, \"trace: run_command:\");\n+\t/*\n+\t * The caller is responsible for initializing cp->env from\n+\t * cp->env_array if needed. We only check one place.\n+\t */\n+\tif (cp->env)\n+\t\ttrace_add_env(&buf, cp->env);\n \tif (cp->git_cmd)\n \t\tstrbuf_addstr(&buf, \" git\");\n \tsq_quote_argv_pretty(&buf, cp->argv);\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex d24d157379..153342e44d 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -54,6 +54,15 @@ int cmd_main(int argc, const char **argv)\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n \tint jobs;\n \n+\tif (argc < 3)\n+\t\treturn 1;\n+\twhile (!strcmp(argv[1], \"env\")) {\n+\t\tif (!argv[2])\n+\t\t\tdie(\"env specifier without a value\");\n+\t\targv_array_push(&proc.env_array, argv[2]);\n+\t\targv += 2;\n+\t\targc -= 2;\n+\t}\n \tif (argc < 3)\n \t\treturn 1;\n \tproc.argv = (const char **)argv + 2;\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nindex e4739170aa..24c92b6cd7 100755\n--- a/t/t0061-run-command.sh\n+++ b/t/t0061-run-command.sh\n@@ -141,4 +141,41 @@ test_expect_success 'run_command outputs ' '\n \ttest_cmp expect actual\n '\n \n+test_trace () {\n+\texpect=\"$1\"\n+\tshift\n+\tGIT_TRACE=1 test-run-command \"$@\" run-command true 2>&1 >/dev/null | \\\n+\t\tsed 's/.* run_command: //' >actual &&\n+\techo \"$expect true\" >expect &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'GIT_TRACE with environment variables' '\n+\ttest_trace \"abc=1 def=2\" env abc=1 env def=2 &&\n+\ttest_trace \"abc=2\" env abc env abc=1 env abc=2 &&\n+\ttest_trace \"abc=2\" env abc env abc=2 &&\n+\t(\n+\t\tabc=1 && export abc &&\n+\t\ttest_trace \"def=1\" env abc=1 env def=1\n+\t) &&\n+\t(\n+\t\tabc=1 && export abc &&\n+\t\ttest_trace \"def=1\" env abc env abc=1 env def=1\n+\t) &&\n+\ttest_trace \"def=1\" env non-exist env def=1 &&\n+\ttest_trace \"abc=2\" env abc=1 env abc env abc=2 &&\n+\t(\n+\t\tabc=1 def=2 && export abc def &&\n+\t\ttest_trace \"unset abc def;\" env abc env def\n+\t) &&\n+\t(\n+\t\tabc=1 def=2 && export abc def &&\n+\t\ttest_trace \"unset def; abc=3\" env abc env def env abc=3\n+\t) &&\n+\t(\n+\t\tabc=1 && export abc &&\n+\t\ttest_trace \"unset abc;\" env abc=2 env abc\n+\t)\n+'\n+\n test_done\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336761","messageId":"20180118094512.27849-8-pclouds@gmail.com","threadId":"47584","inReplyTo":"20180118094512.27849-1-pclouds@gmail.com","subject":"[PATCH v6 7/7] run-command.c: print new cwd in trace_run_command()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-18T09:45:12Z","receivedAt":"2018-01-18T09:46:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If a command sets a new env variable GIT_DIR=.git, we need more context\nto know where that '.git' is related to.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n run-command.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 1301b878c7..a483d5904a 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -622,6 +622,11 @@ static void trace_run_command(const struct child_process *cp)\n \t\treturn;\n \n \tstrbuf_addf(&buf, \"trace: run_command:\");\n+\tif (cp->dir) {\n+\t\tstrbuf_addstr(&buf, \" cd \");\n+\t\tsq_quote_buf_pretty(&buf, cp->dir);\n+\t\tstrbuf_addch(&buf, ';');\n+\t}\n \t/*\n \t * The caller is responsible for initializing cp->env from\n \t * cp->env_array if needed. We only check one place.\n-- \n2.15.1.600.g899a5f85c6\n\n"},{"id":"336940","messageId":"20180119213943.GA12074@sigill.intra.peff.net","threadId":"47584","inReplyTo":"20180118094512.27849-1-pclouds@gmail.com","subject":"Re: [PATCH v6 0/7] Trace env variables in run_command()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-19T21:39:44Z","receivedAt":"2018-01-19T21:39:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 18, 2018 at 04:45:05PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> v6 squashes Junio's changes in 6/7 and moves trace_run_command() from\n> trace.c to run-command.c.\n\nThanks, this version looks good to me.\n\n-Peff\n"}]}