[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings
From:       Ævar Arnfjörð Bjarmason  <avarab () gmail ! com>
Date:       2022-10-31 20:47:08
Message-ID: RFC-patch-1.1-0266485bc6c-20221031T204149Z-avarab () gmail ! com
[Download RAW message or body]

Improve on 6dcbdc0d661 (remote: create fetch.credentialsInUrl config,
2022-06-06) by fixing the cases where we emit duplicate warnings,
which were:

 * In the same process, as we'd get the same "struct remote *"
   again. We could probably save ourselves more work in those scenarios,
   but adding a flag to the "struct remote" indicating that we've validated
   the URLs will fix those issues.

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

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

[Grabbing a quote from
https://lore.kernel.org/git/98fa5270cb720f2f038c4bb9571c7d306ff5d759.1667245639.git.gitgitgadget@gmail.com/
 for a reply]

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> It is unclear as to _why_, but under certain circumstances the warning
> about credentials being passed as part of the URL seems to be swallowed
> by the `git remote-https` helper in the Windows jobs of Git's CI builds.

I think this should fix the root cause of the issue you're seeing. I
have a larger local branch to fix various issues with this
credentialsInUrl facility that I hadn't gotten around to submitting:

	https://github.com/git/git/compare/master...avar:avar/fix-cred-in-url-warnings-2

This is a cherry-pick of 7/* of that (the first were doc changes,
missing test coverage etc).

> Since it is not actually important how many times Git prints the
> warning/error message, as long as it prints it at least once, let's just
> make the test a bit more lenient and test for the latter instead of the
> former, which works around these CI issues.

That being said your change is obviously smaller, so if we'd prefer
that in first as a band-aid I'm fine with that.

But I'd really like to object to the "it is not actually important how
many...", it's crappy UX to spew duplicate output at the user, and we
should avoid it.

So it does matter, and we get it wrong not just in this but also some
other cases.

 builtin/clone.c       |  5 +++-
 builtin/fetch.c       |  4 ++++
 builtin/push.c        |  6 ++++-
 remote.c              | 53 ++++++++++++++++++++++++++++++++-----------
 remote.h              | 14 ++++++++++++
 t/t5516-fetch-push.sh |  2 +-
 t/t5601-clone.sh      |  2 +-
 7 files changed, 69 insertions(+), 17 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 547d6464b3c..da219b74e43 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -903,12 +903,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	int err = 0, complete_refs_before_fetch = 1;
 	int submodule_progress;
 	int filter_submodules = 0;
-
+	enum credentials_in_url cred_in_url_cfg = get_credentials_in_url();
 	struct transport_ls_refs_options transport_ls_refs_options =
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
 
 	packet_trace_identity("clone");
 
+	if (cred_in_url_cfg == CRED_IN_URL_WARN)
+		git_config_push_parameter("transfer.credentialsInUrl=allow");
+
 	git_config(git_clone_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, builtin_clone_options,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b06e454cbdd..34a10e1927f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -2110,9 +2110,13 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	struct remote *remote = NULL;
 	int result = 0;
 	int prune_tags_ok = 1;
+	enum credentials_in_url cred_in_url_cfg = get_credentials_in_url();
 
 	packet_trace_identity("fetch");
 
+	if (cred_in_url_cfg == CRED_IN_URL_WARN)
+		git_config_push_parameter("transfer.credentialsInUrl=allow");
+
 	/* Record the command line for the reflog */
 	strbuf_addstr(&default_rla, "fetch");
 	for (i = 1; i < argc; i++) {
diff --git a/builtin/push.c b/builtin/push.c
index f0329c62a2d..a4890b1586d 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -576,7 +576,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	struct string_list *push_options;
 	const struct string_list_item *item;
 	struct remote *remote;
-
+	enum credentials_in_url cred_in_url_cfg = get_credentials_in_url();
 	struct option options[] = {
 		OPT__VERBOSITY(&verbosity),
 		OPT_STRING( 0 , "repo", &repo, N_("repository"), N_("repository")),
@@ -619,6 +619,10 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	};
 
 	packet_trace_identity("push");
+
+	if (cred_in_url_cfg == CRED_IN_URL_WARN)
+		git_config_push_parameter("transfer.credentialsInUrl=allow");
+
 	git_config(git_push_config, &flags);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 	push_options = (push_options_cmdline.nr
diff --git a/remote.c b/remote.c
index 60869beebe7..a35da349629 100644
--- a/remote.c
+++ b/remote.c
@@ -615,24 +615,42 @@ const char *remote_ref_for_branch(struct branch *branch, int \
for_push)  return NULL;
 }
 
-static void validate_remote_url(struct remote *remote)
+static enum credentials_in_url cred_in_url = CRED_IN_URL_UNKNOWN;
+enum credentials_in_url get_credentials_in_url(void)
 {
-	int i;
+	enum credentials_in_url new = CRED_IN_URL_ALLOW;
 	const char *value;
-	struct strbuf redacted = STRBUF_INIT;
-	int warn_not_die;
+
+	if (cred_in_url != CRED_IN_URL_UNKNOWN)
+		return cred_in_url;
 
 	if (git_config_get_string_tmp("transfer.credentialsinurl", &value))
-		return;
+		goto done;
 
 	if (!strcmp("warn", value))
-		warn_not_die = 1;
+		new = CRED_IN_URL_WARN;
 	else if (!strcmp("die", value))
-		warn_not_die = 0;
+		new = CRED_IN_URL_DIE;
 	else if (!strcmp("allow", value))
-		return;
+		goto done;
 	else
-		die(_("unrecognized value transfer.credentialsInUrl: '%s'"), value);
+		die(_("unrecognized value transfer.credentialsInURL: '%s'"), value);
+
+done:
+	cred_in_url = new;
+	return cred_in_url;
+}
+
+static void validate_remote_url(struct remote *remote)
+{
+	int i;
+	struct strbuf redacted = STRBUF_INIT;
+	enum credentials_in_url cfg = get_credentials_in_url();
+
+	if (remote->validated_urls)
+		goto done;
+	if (cfg == CRED_IN_URL_ALLOW)
+		goto done;
 
 	for (i = 0; i < remote->url_nr; i++) {
 		struct url_info url_info = { 0 };
@@ -647,16 +665,25 @@ static void validate_remote_url(struct remote *remote)
 		strbuf_addstr(&redacted,
 			      url_info.url + url_info.passwd_off + url_info.passwd_len);
 
-		if (warn_not_die)
+		switch (cfg) {
+		case CRED_IN_URL_WARN:
 			warning(_("URL '%s' uses plaintext credentials"), redacted.buf);
-		else
+			break;
+		case CRED_IN_URL_DIE:
 			die(_("URL '%s' uses plaintext credentials"), redacted.buf);
-
-loop_cleanup:
+			break;
+		case CRED_IN_URL_ALLOW:
+		case CRED_IN_URL_UNKNOWN:
+			BUG("unreachable");
+			break;
+		}
+	loop_cleanup:
 		free(url_info.url);
 	}
 
 	strbuf_release(&redacted);
+done:
+	remote->validated_urls = 1;
 }
 
 static struct remote *
diff --git a/remote.h b/remote.h
index 1c4621b414b..5a2da13b4cb 100644
--- a/remote.h
+++ b/remote.h
@@ -98,6 +98,8 @@ struct remote {
 	int prune;
 	int prune_tags;
 
+	int validated_urls;
+
 	/**
 	 * The configured helper programs to run on the remote side, for
 	 * Git-native protocols.
@@ -445,4 +447,16 @@ void apply_push_cas(struct push_cas_option *, struct remote *, \
struct ref *);  char *relative_url(const char *remote_url, const char *url,
 		   const char *up_path);
 
+enum credentials_in_url {
+	CRED_IN_URL_UNKNOWN,
+	CRED_IN_URL_ALLOW,
+	CRED_IN_URL_WARN,
+	CRED_IN_URL_DIE,
+};
+
+/**
+ * Get the transfer.credentialsInUrl config setting as an "enum
+ * credentials_in_url".
+ */
+enum credentials_in_url get_credentials_in_url(void);
 #endif
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 79dc470c014..d01f5cd349f 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1860,7 +1860,7 @@ test_expect_success LIBCURL 'fetch warns or fails when using \
username:password'  
 	test_must_fail git -c transfer.credentialsInUrl=warn fetch \
https://username:password@localhost 2>err &&  grep "warning: $message" err >warnings \
                &&
-	test_line_count = 3 warnings &&
+	test_line_count = 1 warnings &&
 
 	test_must_fail git -c transfer.credentialsInUrl=die fetch \
https://username:password@localhost 2>err &&  grep "fatal: $message" err >warnings &&
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 45f0803ed4d..37adfd9f83b 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -78,7 +78,7 @@ test_expect_success LIBCURL 'clone warns or fails when using \
username:password'  
 	test_must_fail git -c transfer.credentialsInUrl=warn clone \
https://username:password@localhost attempt2 2>err &&  grep "warning: $message" err \
                >warnings &&
-	test_line_count = 2 warnings &&
+	test_line_count = 1 warnings &&
 
 	test_must_fail git -c transfer.credentialsInUrl=die clone \
https://username:password@localhost attempt3 2>err &&  grep "fatal: $message" err \
                >warnings &&
-- 
2.38.0.1280.g8136eb6fab2


[prev in list] [next in list] [prev in thread] [next in thread]