{"thread":{"id":"59442","subject":"[RFC PATCH] bugreport: also print value of no_proxy envvar","startedAt":"2023-03-22T17:34:29Z","lastAt":"2023-03-27T17:49:24Z","messageCount":3,"participants":["Jonathan Tan","Junio C Hamano","Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"473940","messageId":"20230322173421.2243947-1-jonathantanmy@google.com","threadId":"59442","inReplyTo":null,"subject":"[RFC PATCH] bugreport: also print value of no_proxy envvar","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-03-22T17:34:21Z","receivedAt":"2023-03-22T17:34:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"At $DAYJOB, there was an issue that could have been diagnosed much more\nquickly had we known what the $no_proxy environment variable was set\nto. Print this value when the user runs \"git bugreport\". This is useful\nnot only when a command that explicitly uses the network (e.g. fetch\nor clone) is run, but also in a partial clone (in which lazy fetches\nmay occur).\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nTaking off the author's hat and putting on the reviewer's hat, I'm\nnot so sure if this is the right approach, since $no_proxy might have\ninformation that the user doesn't want to share (especially since it\ncould be used beyond the current repository, and even beyond Git), the\nuser being informed that they can delete any lines notwithstanding.\n\nTherefore I'm sending this patch as RFC.\n---\n builtin/bugreport.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex 5bc254be80..c9dd817e70 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -12,6 +12,7 @@ static void get_system_info(struct strbuf *sys_info)\n {\n \tstruct utsname uname_info;\n \tchar *shell = NULL;\n+\tchar *no_proxy = NULL;\n \n \t/* get git version from native cmd */\n \tstrbuf_addstr(sys_info, _(\"git version:\\n\"));\n@@ -39,6 +40,9 @@ static void get_system_info(struct strbuf *sys_info)\n \tshell = getenv(\"SHELL\");\n \tstrbuf_addf(sys_info, \"$SHELL (typically, interactive shell): %s\\n\",\n \t\t    shell ? shell : \"<unset>\");\n+\n+\tno_proxy = getenv(\"no_proxy\");\n+\tstrbuf_addf(sys_info, \"$no_proxy: %s\\n\", no_proxy ? no_proxy : \"<unset>\");\n }\n \n static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n-- \n2.40.0.348.gf938b09366-goog\n\n"},{"id":"473967","messageId":"xmqqsfdwbeh0.fsf@gitster.g","threadId":"59442","inReplyTo":"20230322173421.2243947-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] bugreport: also print value of no_proxy envvar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-22T22:36:43Z","receivedAt":"2023-03-22T22:36:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> At $DAYJOB, there was an issue that could have been diagnosed much more\n> quickly had we known what the $no_proxy environment variable was set\n> to. Print this value when the user runs \"git bugreport\". This is useful\n> not only when a command that explicitly uses the network (e.g. fetch\n> or clone) is run, but also in a partial clone (in which lazy fetches\n> may occur).\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Taking off the author's hat and putting on the reviewer's hat, I'm\n> not so sure if this is the right approach, since $no_proxy might have\n> information that the user doesn't want to share (especially since it\n> could be used beyond the current repository, and even beyond Git), the\n> user being informed that they can delete any lines notwithstanding.\n\nHow certain are you that the \"git bugreport\" process will see more\nor less the same set of environment variables as the process that\nruns \"git\" for real work for the same user?  The latter may have its\nenvironment affected due to $CORP wrapper scripts, etc.\n\nOr other environment variables like http_proxy may be causing harm\nto the user's use of Git, and inspecting no_proxy alone may not give\nyou anything useful.  With devil's advocate hat on, I am tempted to\nsuggest another approach at the the total opposite of the spectrum:\ndump everything in **environ and let the user edit out what ought to\nbe private.  Intelligent ones may even notice a fishy setting they\nforgot about while trying to cull the report of these environment\nvariables ;-)\n\n> Therefore I'm sending this patch as RFC.\n> ---\n>  builtin/bugreport.c | 4 ++++\n>  1 file changed, 4 insertions(+)\n>\n> diff --git a/builtin/bugreport.c b/builtin/bugreport.c\n> index 5bc254be80..c9dd817e70 100644\n> --- a/builtin/bugreport.c\n> +++ b/builtin/bugreport.c\n> @@ -12,6 +12,7 @@ static void get_system_info(struct strbuf *sys_info)\n>  {\n>  \tstruct utsname uname_info;\n>  \tchar *shell = NULL;\n> +\tchar *no_proxy = NULL;\n>  \n>  \t/* get git version from native cmd */\n>  \tstrbuf_addstr(sys_info, _(\"git version:\\n\"));\n> @@ -39,6 +40,9 @@ static void get_system_info(struct strbuf *sys_info)\n>  \tshell = getenv(\"SHELL\");\n>  \tstrbuf_addf(sys_info, \"$SHELL (typically, interactive shell): %s\\n\",\n>  \t\t    shell ? shell : \"<unset>\");\n> +\n> +\tno_proxy = getenv(\"no_proxy\");\n> +\tstrbuf_addf(sys_info, \"$no_proxy: %s\\n\", no_proxy ? no_proxy : \"<unset>\");\n>  }\n>  \n>  static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n"},{"id":"474249","messageId":"kl6lzg7yccg0.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59442","inReplyTo":"xmqqsfdwbeh0.fsf@gitster.g","subject":"Re: [RFC PATCH] bugreport: also print value of no_proxy envvar","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-03-27T17:48:47Z","receivedAt":"2023-03-27T17:49:24Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> Taking off the author's hat and putting on the reviewer's hat, I'm\n>> not so sure if this is the right approach, since $no_proxy might have\n>> information that the user doesn't want to share (especially since it\n>> could be used beyond the current repository, and even beyond Git), the\n>> user being informed that they can delete any lines notwithstanding.\n>\n> [...]\n>\n> Or other environment variables like http_proxy may be causing harm\n> to the user's use of Git, and inspecting no_proxy alone may not give\n> you anything useful.  With devil's advocate hat on, I am tempted to\n> suggest another approach at the the total opposite of the spectrum:\n> dump everything in **environ and let the user edit out what ought to\n> be private.  Intelligent ones may even notice a fishy setting they\n> forgot about while trying to cull the report of these environment\n> variables ;-)\n\nFor this series, I think the natural extension would be to grow the list\nof environment variables to include everything that might affect Git?\nI don't know how possible or sustainable that is, so Junio's idea (or\nJunio's devil's advocate idea?) makes some sense from that perspective.\nHowever, oversharing by default sounds far too unsafe to me.\n\nAlso, I suspect that dumping all of the environment variables might\nsometimes be a hindrance. e.g. I can imagine a conservative user\nremoving $no_proxy. Then, on the other side, the report reader sees the\nlist of environment variables, assumes that since $no_proxy is unset\nbecause it's absent from the report, and goes on a wild goose chase.\n\nI wonder if it would be helpful to know that $no_proxy is set, even if\nthe report doesn't say what the value is. If so, an okay-ish middle\nground might be to dump all of the environment variables but redact them\nby default (maybe something like git bugreport\n--dump-envvars=[redact|no-redact]). I don't know how well this will work\nin practice though. Personally, I'd still lean towards trimming down the\nlist of environment variables, even if the values are redacted.\n"}]}