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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Taylor Blau <me@ttaylorr.com>,
	Derrick Stolee <derrickstolee@github.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings
Date: Tue, 01 Nov 2022 14:07:39 +0100	[thread overview]
Message-ID: <221101.86o7tq4vsn.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <Y2Doe0ZGb3Zmmmen@coredump.intra.peff.net>

On Tue, Nov 01 2022, Jeff King wrote:

> 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.

Sure, we can do it with an environment variable, in the end that's all
git_config_push_parameter() is doing too. It's just setting things in
"GIT_CONFIG_PARAMETERS".

And yes, we can set this in the low-level function instead of with
git_config_push_parameter() in builtin/*.c as I did. I was aiming for
something demonstrably narrow, at the cost of some verbosity.

But I don't get how other things being equal you think sticking this in
"GIT_CHECKED_CREDENTIALS_IN_URL" instead of "GIT_CONFIG_PARAMETERS"
helps.

We already pass config to ourselves like that (and via "-c") in other
places. Can you think of a case where these would be different?

The only ones I can think of are e.g. because we know about
"GIT_CONFIG_PARAMETERS", and not this new custom variable, e.g. in
"prepare_other_repo_env()", but those seem like exactly the reason to
use the existing variable.

I can think of potential pitfalls here, e.g. how does it interact with
submodules? That's one reason I submitted it as an RFC, the tests need
to be better (with or without this change). E.g. "git ls-remote" is also
not covered by the upthread patch.

But that's all separate from what the environment variable is named, or
if it lives in the config space.