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.
next prev parent reply other threads:[~2022-11-01 14:02 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-31 19:47 [PATCH 0/2] t5516/t5601: avoid using localhost for failing HTTPS requests Johannes Schindelin via GitGitGadget
2022-10-31 19:47 ` [PATCH 1/2] t5516/t5601: avoid using `localhost` " Johannes Schindelin via GitGitGadget
2022-10-31 20:49 ` Ævar Arnfjörð Bjarmason
2022-10-31 23:20 ` Jeff King
2022-11-01 0:59 ` Taylor Blau
2022-11-01 2:28 ` Jeff King
2022-11-01 2:03 ` Jeff King
2022-11-01 2:25 ` Jeff King
2022-11-01 2:26 ` [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516 Jeff King
2022-11-01 3:18 ` Ævar Arnfjörð Bjarmason
2022-11-01 7:32 ` Jeff King
2022-11-01 20:37 ` Taylor Blau
2022-11-01 2:26 ` [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings Jeff King
2022-11-01 3:29 ` Ævar Arnfjörð Bjarmason
2022-11-01 7:39 ` Jeff King
2022-11-01 8:15 ` Ævar Arnfjörð Bjarmason
2022-11-01 9:12 ` Jeff King
2022-11-01 14:05 ` Ævar Arnfjörð Bjarmason
2022-11-01 4:54 ` Junio C Hamano
2022-11-01 7:42 ` Jeff King
2022-11-01 20:50 ` Taylor Blau
2022-10-31 19:47 ` Johannes Schindelin via GitGitGadget
2022-10-31 23:22 ` Jeff King
2022-11-01 0:57 ` Taylor Blau
2022-11-01 2:27 ` Jeff King
2022-10-31 20:47 ` [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings Ævar Arnfjörð Bjarmason
2022-11-01 1:06 ` Taylor Blau
2022-11-01 2:32 ` Jeff King
2022-11-01 3:01 ` Ævar Arnfjörð Bjarmason
2022-11-01 20:54 ` Taylor Blau
2022-11-01 22:17 ` Ævar Arnfjörð Bjarmason
2022-11-02 0:53 ` Taylor Blau
2022-11-02 8:42 ` [PATCH v3 2/2] t5551: be less strict about the number of credential warnings Jeff King
2022-11-02 8:49 ` Eric Sunshine
2022-11-02 9:15 ` Jeff King
2022-11-02 9:31 ` Eric Sunshine
2022-11-02 9:18 ` Jeff King
2022-11-03 1:31 ` Taylor Blau
2022-11-01 9:35 ` [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings Jeff King
2022-11-01 13:07 ` Ævar Arnfjörð Bjarmason [this message]
2022-11-01 21:00 ` Taylor Blau
2022-11-01 21:57 ` Ævar Arnfjörð Bjarmason
2022-11-02 8:19 ` Jeff King
2022-11-04 9:01 ` Ævar Arnfjörð Bjarmason
2022-11-04 13:16 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=221101.86o7tq4vsn.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=johannes.schindelin@gmx.de \
--cc=me@ttaylorr.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).