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

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