threads / rfc / 59442

RFC patchbugreport: also print value of no_proxy envvar

Subject: [RFC PATCH] bugreport: also print value of no_proxy envvar

## tl;dr

3 messages between Mar 22, 2023 and Mar 27, 2023. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Jonathan Tan· Mar 22, 2023, 17:34 UTC · lore

At $DAYJOB, there was an issue that could have been diagnosed much more quickly had we known what the $no_proxy environment variable was set to. Print this value when the user runs "git bugreport". This is useful not only when a command that explicitly uses the network (e.g. fetch or clone) is run, but also in a partial clone (in which lazy fetches may occur).

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Taking off the author's hat and putting on the reviewer's hat, I'm
not so sure if this is the right approach, since $no_proxy might have
information that the user doesn't want to share (especially since it
could be used beyond the current repository, and even beyond Git), the
user being informed that they can delete any lines notwithstanding.
Therefore I'm sending this patch as RFC.
---
 builtin/bugreport.c | 4 ++++
 1 file changed, 4 insertions(+)
Show changes to builtin/bugreport.c +4 −0
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 5bc254be80..c9dd817e70 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -12,6 +12,7 @@ static void get_system_info(struct strbuf *sys_info)
 {
 	struct utsname uname_info;
 	char *shell = NULL;
+	char *no_proxy = NULL;
 
 	/* get git version from native cmd */
 	strbuf_addstr(sys_info, _("git version:\n"));
@@ -39,6 +40,9 @@ static void get_system_info(struct strbuf *sys_info)
 	shell = getenv("SHELL");
 	strbuf_addf(sys_info, "$SHELL (typically, interactive shell): %s\n",
 		    shell ? shell : "<unset>");
+
+	no_proxy = getenv("no_proxy");
+	strbuf_addf(sys_info, "$no_proxy: %s\n", no_proxy ? no_proxy : "<unset>");
 }
 
 static void get_populated_hooks(struct strbuf *hook_info, int nongit)

-- 
2.40.0.348.gf938b09366-goog
Junio C Hamano· Mar 22, 2023, 22:36 UTC · re: Jonathan Tan · lore

Re: [RFC PATCH] bugreport: also print value of no_proxy envvar

Jonathan Tan <jonathantanmy@google.com> writes:
Show 14 quoted lines
> At $DAYJOB, there was an issue that could have been diagnosed much more
> quickly had we known what the $no_proxy environment variable was set
> to. Print this value when the user runs "git bugreport". This is useful
> not only when a command that explicitly uses the network (e.g. fetch
> or clone) is run, but also in a partial clone (in which lazy fetches
> may occur).
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> Taking off the author's hat and putting on the reviewer's hat, I'm
> not so sure if this is the right approach, since $no_proxy might have
> information that the user doesn't want to share (especially since it
> could be used beyond the current repository, and even beyond Git), the
> user being informed that they can delete any lines notwithstanding.

How certain are you that the "git bugreport" process will see more or less the same set of environment variables as the process that runs "git" for real work for the same user? The latter may have its environment affected due to $CORP wrapper scripts, etc.

Or other environment variables like http_proxy may be causing harm to the user's use of Git, and inspecting no_proxy alone may not give you anything useful. With devil's advocate hat on, I am tempted to suggest another approach at the the total opposite of the spectrum: dump everything in **environ and let the user edit out what ought to be private. Intelligent ones may even notice a fishy setting they forgot about while trying to cull the report of these environment variables ;-)

Show 27 quoted lines
> Therefore I'm sending this patch as RFC.
> ---
>  builtin/bugreport.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 5bc254be80..c9dd817e70 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -12,6 +12,7 @@ static void get_system_info(struct strbuf *sys_info)
>  {
>  	struct utsname uname_info;
>  	char *shell = NULL;
> +	char *no_proxy = NULL;
>  
>  	/* get git version from native cmd */
>  	strbuf_addstr(sys_info, _("git version:\n"));
> @@ -39,6 +40,9 @@ static void get_system_info(struct strbuf *sys_info)
>  	shell = getenv("SHELL");
>  	strbuf_addf(sys_info, "$SHELL (typically, interactive shell): %s\n",
>  		    shell ? shell : "<unset>");
> +
> +	no_proxy = getenv("no_proxy");
> +	strbuf_addf(sys_info, "$no_proxy: %s\n", no_proxy ? no_proxy : "<unset>");
>  }
>  
>  static void get_populated_hooks(struct strbuf *hook_info, int nongit)
Glen Choo· Mar 27, 2023, 17:48 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] bugreport: also print value of no_proxy envvar

Junio C Hamano <gitster@pobox.com> writes:
Show 16 quoted lines
>> Taking off the author's hat and putting on the reviewer's hat, I'm
>> not so sure if this is the right approach, since $no_proxy might have
>> information that the user doesn't want to share (especially since it
>> could be used beyond the current repository, and even beyond Git), the
>> user being informed that they can delete any lines notwithstanding.
>
> [...]
>
> Or other environment variables like http_proxy may be causing harm
> to the user's use of Git, and inspecting no_proxy alone may not give
> you anything useful.  With devil's advocate hat on, I am tempted to
> suggest another approach at the the total opposite of the spectrum:
> dump everything in **environ and let the user edit out what ought to
> be private.  Intelligent ones may even notice a fishy setting they
> forgot about while trying to cull the report of these environment
> variables ;-)

For this series, I think the natural extension would be to grow the list of environment variables to include everything that might affect Git? I don't know how possible or sustainable that is, so Junio's idea (or Junio's devil's advocate idea?) makes some sense from that perspective. However, oversharing by default sounds far too unsafe to me.

Also, I suspect that dumping all of the environment variables might sometimes be a hindrance. e.g. I can imagine a conservative user removing $no_proxy. Then, on the other side, the report reader sees the list of environment variables, assumes that since $no_proxy is unset because it's absent from the report, and goes on a wild goose chase.

I wonder if it would be helpful to know that $no_proxy is set, even if the report doesn't say what the value is. If so, an okay-ish middle ground might be to dump all of the environment variables but redact them by default (maybe something like git bugreport --dump-envvars=[redact|no-redact]). I don't know how well this will work in practice though. Personally, I'd still lean towards trimming down the list of environment variables, even if the values are redacted.

← back to recent threads