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.
