Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings

[Jeff King <peff@peff.net>]

On Mon, Oct 31, 2022 at 10:32:36PM -0400, Jeff King wrote:

> On Mon, Oct 31, 2022 at 09:47:08PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> >  * When we invoke e.g. "git-remote-https" (aka. "git-remote-curl")
> >    from "git fetch". For those cases let's pass down the equivalent of a
> >    "-c transfer.credentialsInUrl=allow", since we know that we've already
> >    inspected our remotes in the parent process.
> > 
> >    See 7390f05a3c6 (fetch: after refetch, encourage auto gc repacking,
> >    2022-03-28) for prior use of git_config_push_parameter() for this
> >    purpose, i.e. to push config parameters before invoking a
> >    sub-process.
> 
> So I guess I don't have any _specific_ thing that goes wrong with this
> approach, but it really feels sketchy to me. We are lying to
> sub-processes about the config the user asked for. And again, I see how
> it works, but I wonder if this kind of approach would ever backfire on
> us. For example, if transfer.credentialsInUrl=warn ever ended up having
> any side effects besides the warning, and the sub-processes ended up
> skipping those effects.
> 
> I know that's a hypothetical, and probably not even a likely one, but it
> just gets my spider sense tingling.

So inherently this is going to be ugly because it's crossing process
boundaries. But the more minimal fix I was thinking of would be
something like this:

diff --git a/remote.c b/remote.c
index 60869beebe..af5f95c719 100644
--- a/remote.c
+++ b/remote.c
@@ -615,6 +615,14 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)
 	return NULL;
 }
 
+static int test_and_set_env(const char *var)
+{
+	int ret = git_env_bool(var, 0);
+	if (!ret)
+		setenv(var, "1", 0);
+	return ret;
+}
+
 static void validate_remote_url(struct remote *remote)
 {
 	int i;
@@ -634,6 +642,9 @@ static void validate_remote_url(struct remote *remote)
 	else
 		die(_("unrecognized value transfer.credentialsInUrl: '%s'"), value);
 
+	if (test_and_set_env("GIT_CHECKED_CREDENTIALS_IN_URL"))
+		return;
+
 	for (i = 0; i < remote->url_nr; i++) {
 		struct url_info url_info = { 0 };
 

You can also put it lower in the function, when we actually warn, which
saves adding to the environment when there is nothing to warn about
(though this way, we avoid doing the redundant work).

Compared to munging the config, this seems shorter and simpler, and
there's no chance of us ever getting confused between the fake
"suppress" value and something the user actually asked for.

-Peff