[PATCH 0/2] t5516/t5601: avoid using localhost for failing HTTPS requests

[PATCH 0/2] t5516/t5601: avoid using localhost for failing HTTPS requests

[Johannes Schindelin via GitGitGadget]

While trying to figure out why the current CI builds fail in the Windows
jobs [https://github.com/git/git/actions/runs/3358829078#annotations], I
noticed that I cannot really run the offending test cases on my box because
the supposedly failing connection to https://localhost never seems to time
out as intended.

This patch fixes that, and incidentally also seems to fix the CI failures.

Johannes Schindelin (2):
  t5516/t5601: avoid using `localhost` for failing HTTPS requests
  t5516/t5601: be less strict about the number of credential warnings

 t/t5516-fetch-push.sh | 26 +++++++++++++-------------
 t/t5601-clone.sh      | 18 +++++++++---------
 2 files changed, 22 insertions(+), 22 deletions(-)


base-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1399%2Fdscho%2Favoid-using-localhost-in-url-tests-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1399/dscho/avoid-using-localhost-in-url-tests-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1399
-- 
gitgitgadget

[PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 6dcbdc0d6616 (remote: create fetch.credentialsInUrl config,
2022-06-06), we added four test cases that validate various behavior
around passing credentials as part of the URL (which is considered
unsafe in general).

These tests do not _actually_ try to connect anywhere, but have to use
the https:// protocol in order to validate the intended code paths.

However, using `localhost` for such a connection causes several
problems:

- There might be a web server running on localhost, and we do not
  actually want to connect to that.

- The DNS resolver, or the local firewall, might take a substantial
  amount of time (or forever, whichever comes first) to fail to connect,
  slowing down the test cases unnecessarily.

Let's instead use an IPv4 address that is guaranteed never to offer a
web server: 224.0.0.1 (which is part of the IP multicast range).

Incidentally, this seems to fix an issue where the tests fail in the
Windows jobs of Git's CI builds.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5516-fetch-push.sh | 18 +++++++++---------
 t/t5601-clone.sh      | 10 +++++-----
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 79dc470c014..8dd4610a8c2 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1854,32 +1854,32 @@ test_expect_success 'refuse to push a hidden ref, and make sure do not pollute t
 '
 
 test_expect_success LIBCURL 'fetch warns or fails when using username:password' '
-	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
-	test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&
+	message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" &&
+	test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err &&
 	! grep "$message" err &&
 
-	test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@localhost 2>err &&
+	test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@224.0.0.1 2>err &&
 	grep "warning: $message" err >warnings &&
 	test_line_count = 3 warnings &&
 
-	test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@localhost 2>err &&
+	test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@224.0.0.1 2>err &&
 	grep "fatal: $message" err >warnings &&
 	test_line_count = 1 warnings &&
 
-	test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@localhost 2>err &&
+	test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@224.0.0.1 2>err &&
 	grep "fatal: $message" err >warnings &&
 	test_line_count = 1 warnings
 '
 
 
 test_expect_success LIBCURL 'push warns or fails when using username:password' '
-	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
-	test_must_fail git -c transfer.credentialsInUrl=allow push https://username:password@localhost 2>err &&
+	message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" &&
+	test_must_fail git -c transfer.credentialsInUrl=allow push https://username:password@224.0.0.1 2>err &&
 	! grep "$message" err &&
 
-	test_must_fail git -c transfer.credentialsInUrl=warn push https://username:password@localhost 2>err &&
+	test_must_fail git -c transfer.credentialsInUrl=warn push https://username:password@224.0.0.1 2>err &&
 	grep "warning: $message" err >warnings &&
-	test_must_fail git -c transfer.credentialsInUrl=die push https://username:password@localhost 2>err &&
+	test_must_fail git -c transfer.credentialsInUrl=die push https://username:password@224.0.0.1 2>err &&
 	grep "fatal: $message" err >warnings &&
 	test_line_count = 1 warnings
 '
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 45f0803ed4d..0b386c74818 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -72,19 +72,19 @@ test_expect_success 'clone respects GIT_WORK_TREE' '
 '
 
 test_expect_success LIBCURL 'clone warns or fails when using username:password' '
-	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
-	test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&
+	message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" &&
+	test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@224.0.0.1 attempt1 2>err &&
 	! grep "$message" err &&
 
-	test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&
+	test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@224.0.0.1 attempt2 2>err &&
 	grep "warning: $message" err >warnings &&
 	test_line_count = 2 warnings &&
 
-	test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&
+	test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@224.0.0.1 attempt3 2>err &&
 	grep "fatal: $message" err >warnings &&
 	test_line_count = 1 warnings &&
 
-	test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@localhost attempt3 2>err &&
+	test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@224.0.0.1 attempt3 2>err &&
 	grep "fatal: $message" err >warnings &&
 	test_line_count = 1 warnings
 '
-- 
gitgitgadget

[PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings

[Johannes Schindelin via GitGitGadget]

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.

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5516-fetch-push.sh | 8 ++++----
 t/t5601-clone.sh      | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8dd4610a8c2..980c594940b 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1860,15 +1860,15 @@ test_expect_success LIBCURL 'fetch warns or fails when using username:password'
 
 	test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@224.0.0.1 2>err &&
 	grep "warning: $message" err >warnings &&
-	test_line_count = 3 warnings &&
+	test_line_count -ge 1 warnings &&
 
 	test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@224.0.0.1 2>err &&
 	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings &&
+	test_line_count -ge 1 warnings &&
 
 	test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@224.0.0.1 2>err &&
 	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings
+	test_line_count -ge 1 warnings
 '
 
 
@@ -1881,7 +1881,7 @@ test_expect_success LIBCURL 'push warns or fails when using username:password' '
 	grep "warning: $message" err >warnings &&
 	test_must_fail git -c transfer.credentialsInUrl=die push https://username:password@224.0.0.1 2>err &&
 	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings
+	test_line_count -ge 1 warnings
 '
 
 test_expect_success 'push with config push.useBitmaps' '
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 0b386c74818..b72cdeb6243 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -78,19 +78,19 @@ test_expect_success LIBCURL 'clone warns or fails when using username:password'
 
 	test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@224.0.0.1 attempt2 2>err &&
 	grep "warning: $message" err >warnings &&
-	test_line_count = 2 warnings &&
+	test_line_count -ge 1 warnings &&
 
 	test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@224.0.0.1 attempt3 2>err &&
 	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings &&
+	test_line_count -ge 1 warnings &&
 
 	test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@224.0.0.1 attempt3 2>err &&
 	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings
+	test_line_count -ge 1 warnings
 '
 
 test_expect_success LIBCURL 'clone does not detect username:password when it is https://username@domain:port/' '
-	test_must_fail git -c transfer.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&
+	test_must_fail git -c transfer.credentialsInUrl=warn clone https://username@224.0.0.1:8080 attempt3 2>err &&
 	! grep "uses plaintext credentials" err
 '
 
-- 
gitgitgadget

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

[Ævar Arnfjörð Bjarmason]

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

Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 31 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 6dcbdc0d6616 (remote: create fetch.credentialsInUrl config,
> 2022-06-06), we added four test cases that validate various behavior
> around passing credentials as part of the URL (which is considered
> unsafe in general).
>
> These tests do not _actually_ try to connect anywhere, but have to use
> the https:// protocol in order to validate the intended code paths.
>
> However, using `localhost` for such a connection causes several
> problems:
>
> - There might be a web server running on localhost, and we do not
>   actually want to connect to that.
>
> - The DNS resolver, or the local firewall, might take a substantial
>   amount of time (or forever, whichever comes first) to fail to connect,
>   slowing down the test cases unnecessarily.
>
> Let's instead use an IPv4 address that is guaranteed never to offer a
> web server: 224.0.0.1 (which is part of the IP multicast range).
>
> Incidentally, this seems to fix an issue where the tests fail in the
> Windows jobs of Git's CI builds.
> [...]
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 45f0803ed4d..0b386c74818 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -72,19 +72,19 @@ test_expect_success 'clone respects GIT_WORK_TREE' '
>  '
>  
>  test_expect_success LIBCURL 'clone warns or fails when using username:password' '
> -	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
> -	test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&
> +	message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" &&
> +	test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@224.0.0.1 attempt1 2>err &&
>  	! grep "$message" err &&
>  
> -	test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&
> +	test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@224.0.0.1 attempt2 2>err &&
>  	grep "warning: $message" err >warnings &&
>  	test_line_count = 2 warnings &&
>  
> -	test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&
> +	test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@224.0.0.1 attempt3 2>err &&
>  	grep "fatal: $message" err >warnings &&
>  	test_line_count = 1 warnings &&
>  
> -	test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@localhost attempt3 2>err &&
> +	test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@224.0.0.1 attempt3 2>err &&
>  	grep "fatal: $message" err >warnings &&
>  	test_line_count = 1 warnings
>  '

For this one one at least, it eventually gets around to setting up an
actual httpd server with cloning etc. from $HTTPD_URL.

Can't we just do that for both of these tests rather than the the
224.0.0.0 hack? I.e. the root cause is that we're cleverly faking a
not-a-server here, and now we're going to add another somewhat clever
hack on top.

but since the test coverage is for https:// anyway, and we have other
https tests against an actual server...

Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests

[Jeff King]

On Mon, Oct 31, 2022 at 07:47:17PM +0000, Johannes Schindelin via GitGitGadget wrote:

> In 6dcbdc0d6616 (remote: create fetch.credentialsInUrl config,
> 2022-06-06), we added four test cases that validate various behavior
> around passing credentials as part of the URL (which is considered
> unsafe in general).
> 
> These tests do not _actually_ try to connect anywhere, but have to use
> the https:// protocol in order to validate the intended code paths.

By "actually" here, I assume you mean "they do not expect to succeed".
But I think the first one (with credentialsInUrl=allow), does try to
make a connection.

> However, using `localhost` for such a connection causes several
> problems:
> 
> - There might be a web server running on localhost, and we do not
>   actually want to connect to that.
> 
> - The DNS resolver, or the local firewall, might take a substantial
>   amount of time (or forever, whichever comes first) to fail to connect,
>   slowing down the test cases unnecessarily.

Right. I think we assume that DNS resolution of localhost is fast-ish,
as we use it in other https tests. But I could certainly imagine a local
firewall causing issues (especially as this is real port 443, whereas
our other tests are usually high ports).

> Let's instead use an IPv4 address that is guaranteed never to offer a
> web server: 224.0.0.1 (which is part of the IP multicast range).

This feels pretty magical. I think it would be pretty unlikely for it to
have a web server, but I wouldn't be surprised if there are systems
where we get similar IP-routing hangs.

Is there a reason not to move all of these tests into t5550 or t5551,
where we have a real http server? That would be less magical, and then
this first test:

>  test_expect_success LIBCURL 'fetch warns or fails when using username:password' '
> -	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
> -	test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&
> +	message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" &&
> +	test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err &&
>  	! grep "$message" err &&

could be more robust. It would actually check that we succeeded in using
the URL.

-Peff

Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings

[Jeff King]

On Mon, Oct 31, 2022 at 07:47:18PM +0000, Johannes Schindelin via GitGitGadget wrote:

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

This makes sense to me. Regardless of whether we actually fix the
multiple outputs (which are user-visible and kind of ugly), I don't
think there's any reason for our tests to assert the somewhat
undesirable output. So this can proceed independently of any fix.

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 8dd4610a8c2..980c594940b 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1860,15 +1860,15 @@ test_expect_success LIBCURL 'fetch warns or fails when using username:password'
>  
>  	test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@224.0.0.1 2>err &&
>  	grep "warning: $message" err >warnings &&
> -	test_line_count = 3 warnings &&
> +	test_line_count -ge 1 warnings &&

I think this test_line_count (and all the others) is now superfluous;
the exit code of grep will tell us if we found anything.

I don't mind it too much, though, if we're planning to fix the duplicate
messages, at which point it becomes s/-ge/=/.

-Peff

Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings

[Taylor Blau]

On Mon, Oct 31, 2022 at 07:22:48PM -0400, Jeff King wrote:
> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> > index 8dd4610a8c2..980c594940b 100755
> > --- a/t/t5516-fetch-push.sh
> > +++ b/t/t5516-fetch-push.sh
> > @@ -1860,15 +1860,15 @@ test_expect_success LIBCURL 'fetch warns or fails when using username:password'
> >
> >  	test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@224.0.0.1 2>err &&
> >  	grep "warning: $message" err >warnings &&
> > -	test_line_count = 3 warnings &&
> > +	test_line_count -ge 1 warnings &&
>
> I think this test_line_count (and all the others) is now superfluous;
> the exit code of grep will tell us if we found anything.
>
> I don't mind it too much, though, if we're planning to fix the duplicate
> messages, at which point it becomes s/-ge/=/.

Yeah, agreed. Let's leave it alone, unless somebody else feels strongly.

Thanks,
Taylor

Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests

[Taylor Blau]

On Mon, Oct 31, 2022 at 07:20:11PM -0400, Jeff King wrote:
> > Let's instead use an IPv4 address that is guaranteed never to offer a
> > web server: 224.0.0.1 (which is part of the IP multicast range).
>
> This feels pretty magical. I think it would be pretty unlikely for it to
> have a web server, but I wouldn't be surprised if there are systems
> where we get similar IP-routing hangs.
>
> Is there a reason not to move all of these tests into t5550 or t5551,
> where we have a real http server? That would be less magical, and then
> this first test:
>
> >  test_expect_success LIBCURL 'fetch warns or fails when using username:password' '
> > -	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
> > -	test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&
> > +	message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" &&
> > +	test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err &&
> >  	! grep "$message" err &&
>
> could be more robust. It would actually check that we succeeded in using
> the URL.

It is magical, indeed. If it's significantly more difficult to move
these into t5550 or t5551, then I'm OK with this in the meantime (since
GitHub Actions really is our primary CI target that we care about this
not hanging on).

But assuming that moving these around isn't that difficult, I would be
slightly happier to see these tests in one of the aforementioned spots.

Thanks,
Taylor

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

[Taylor Blau]

On Mon, Oct 31, 2022 at 09:47:08PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 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.

Both make sense. I was skimming the diff before I read the patch message
here and was scratching my head at the calls to
git_config_push_parameter().

But for crossing process boundaries, that makes sense.

Thanks,
Taylor

Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests

[Jeff King]

On Mon, Oct 31, 2022 at 07:20:11PM -0400, Jeff King wrote:

> > - The DNS resolver, or the local firewall, might take a substantial
> >   amount of time (or forever, whichever comes first) to fail to connect,
> >   slowing down the test cases unnecessarily.
> 
> Right. I think we assume that DNS resolution of localhost is fast-ish,
> as we use it in other https tests. But I could certainly imagine a local
> firewall causing issues (especially as this is real port 443, whereas
> our other tests are usually high ports).

Actually, I am wrong about DNS here. We use a bare 127.0.0.1 in
lib-httpd.sh, so DNS may indeed be the source of the issue here. That
_might_ mean that using the bare IP would be safe here, but I would not
want to bet on it. Using different port numbers, plus not expecting a
listener on the other end, might cause different outcomes than we see in
the other tests.

I do think moving these tests to use a real http server is a better
solution, though. I'll provide a patch in a moment.

-Peff

Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests

[Jeff King]

On Mon, Oct 31, 2022 at 10:03:58PM -0400, Jeff King wrote:

> I do think moving these tests to use a real http server is a better
> solution, though. I'll provide a patch in a moment.

Here that is. I hope this isn't co-opting your series; my goal was to do
the work so that you wouldn't have to, but I admit it is assuming you
agree with my approach. ;)

The first patch is the interesting one. The second one is just your 2/2
rebased onto the new location.

  [1/2]: t5516: move plaintext-password tests from t5601 and t5516
  [2/2]: t5516/t5601: be less strict about the number of credential warnings

 t/t5516-fetch-push.sh       | 31 ---------------
 t/t5551-http-fetch-smart.sh | 77 +++++++++++++++++++++++++++++++++++++
 t/t5601-clone.sh            | 23 -----------
 3 files changed, 77 insertions(+), 54 deletions(-)

-Peff

[PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516

[Jeff King]

Commit 6dcbdc0d66 (remote: create fetch.credentialsInUrl config,
2022-06-06) added tests for our handling of passwords in URLs. Since the
obvious URL to be affected is git-over-http, the tests use http. However
they don't set up a test server; they just try to access
https://localhost, assuming it will fail (because the nothing is
listening there).

This causes some possible problems:

  - There might be a web server running on localhost, and we do not
    actually want to connect to that.

  - The DNS resolver, or the local firewall, might take a substantial
    amount of time (or forever, whichever comes first) to fail to
    connect, slowing down the tests cases unnecessarily.

  - Since there's no server, our tests for "allow" and "warn" still
    expect the clone/fetch/push operations to fail, even though in the
    real world we'd expect these to succeed. We scrape stderr to see
    what happened, but it's not as robust as a more realistic test.

Let's instead move these to t5551, which is all about testing http and
where we have a real server. That eliminates any issues with contacting
a strange URL, and lets the "allow" and "warn" tests confirm that the
operation actually succeeds.

It's not quite a verbatim move for a few reasons:

  - we can drop the LIBCURL dependency; it's already part of
    lib-httpd.sh

  - we'll use HTTPD_URL_USER_PASS, etc, instead of our fake URL. To
    avoid repetition, we'll add a few extra variables.

  - the "https://username:@localhost" test uses a funny URL that
    lib-httpd.sh doesn't provide. We'll similarly construct it in a
    variable. Note that we're hard-coding the lib-httpd username here,
    but t5551 already does that everywhere.

  - for the "domain:port" test, the URL provided by lib-httpd is fine,
    since our test server will always be on an exotic port. But we'll
    confirm in the test that this is so.

  - since our message-matching is done via grep, I simplified it to use
    a regex, rather than trying to massage lib-httpd's variables.
    Arguably this makes it more readable, too, while retaining the bits
    we care about: the fatal/warning distinction, the "uses plaintext"
    message, and the fact that the password was redacted.

  - we'll use the /auth/ path for the repo, which shows that we are
    indeed making use of the auth information when needed.

  - we'll also use /smart/; most of these tests could be done via /dumb/
    in t5550, but setting up pushes there requires extra effort and
    dependencies. The smart protocol is what most everyone is using
    these days anyway.

This patch is my own, but I stole the analysis and a few bits of the
commit message from a patch by Johannes Schindelin.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5516-fetch-push.sh       | 31 ---------------
 t/t5551-http-fetch-smart.sh | 77 +++++++++++++++++++++++++++++++++++++
 t/t5601-clone.sh            | 23 -----------
 3 files changed, 77 insertions(+), 54 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 79dc470c01..4f2bfaf005 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1853,37 +1853,6 @@ test_expect_success 'refuse to push a hidden ref, and make sure do not pollute t
 	test_dir_is_empty testrepo/.git/objects/pack
 '
 
-test_expect_success LIBCURL 'fetch warns or fails when using username:password' '
-	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
-	test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&
-	! grep "$message" err &&
-
-	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_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@localhost 2>err &&
-	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings &&
-
-	test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@localhost 2>err &&
-	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings
-'
-
-
-test_expect_success LIBCURL 'push warns or fails when using username:password' '
-	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
-	test_must_fail git -c transfer.credentialsInUrl=allow push https://username:password@localhost 2>err &&
-	! grep "$message" err &&
-
-	test_must_fail git -c transfer.credentialsInUrl=warn push https://username:password@localhost 2>err &&
-	grep "warning: $message" err >warnings &&
-	test_must_fail git -c transfer.credentialsInUrl=die push https://username:password@localhost 2>err &&
-	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings
-'
-
 test_expect_success 'push with config push.useBitmaps' '
 	mk_test testrepo heads/main &&
 	git checkout main &&
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 6a38294a47..bbe3d5721f 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -580,4 +580,81 @@ test_expect_success 'passing hostname resolution information works' '
 	git -c "http.curloptResolve=$BOGUS_HOST:$LIB_HTTPD_PORT:127.0.0.1" ls-remote "$BOGUS_HTTPD_URL/smart/repo.git" >/dev/null
 '
 
+# here user%40host is the URL-encoded version of user@host,
+# which is our intentionally-odd username to catch parsing errors
+url_user=$HTTPD_URL_USER/auth/smart/repo.git
+url_userpass=$HTTPD_URL_USER_PASS/auth/smart/repo.git
+url_userblank=$HTTPD_PROTO://user%40host:@$HTTPD_DEST/auth/smart/repo.git
+message="URL .*:<redacted>@.* uses plaintext credentials"
+
+test_expect_success 'clone warns or fails when using username:password' '
+	test_when_finished "rm -rf attempt*" &&
+
+	git -c transfer.credentialsInUrl=allow \
+		clone $url_userpass attempt1 2>err &&
+	! grep "$message" err &&
+
+	git -c transfer.credentialsInUrl=warn \
+		clone $url_userpass attempt2 2>err &&
+	grep "warning: $message" err >warnings &&
+	test_line_count = 2 warnings &&
+
+	test_must_fail git -c transfer.credentialsInUrl=die \
+		clone $url_userpass attempt3 2>err &&
+	grep "fatal: $message" err >warnings &&
+	test_line_count = 1 warnings &&
+
+	test_must_fail git -c transfer.credentialsInUrl=die \
+		clone $url_userblank attempt4 2>err &&
+	grep "fatal: $message" err >warnings &&
+	test_line_count = 1 warnings
+'
+
+test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '
+	test_when_finished "rm -rf attempt1" &&
+
+	# we are relying on lib-httpd for url construction, so document our
+	# assumptions
+	case "$HTTPD_URL_USER" in
+	*:[0-9]*) : ok ;;
+	*) BUG "httpd url does not have port: $HTTPD_URL_USER"
+	esac &&
+
+	git -c transfer.credentialsInUrl=warn clone $url_user attempt1 2>err &&
+	! grep "uses plaintext credentials" err
+'
+
+test_expect_success 'fetch warns or fails when using username:password' '
+	git -c transfer.credentialsInUrl=allow fetch $url_userpass 2>err &&
+	! grep "$message" err &&
+
+	git -c transfer.credentialsInUrl=warn fetch $url_userpass 2>err &&
+	grep "warning: $message" err >warnings &&
+	test_line_count = 3 warnings &&
+
+	test_must_fail git -c transfer.credentialsInUrl=die \
+		fetch $url_userpass 2>err &&
+	grep "fatal: $message" err >warnings &&
+	test_line_count = 1 warnings &&
+
+	test_must_fail git -c transfer.credentialsInUrl=die \
+		fetch $url_userblank 2>err &&
+	grep "fatal: $message" err >warnings &&
+	test_line_count = 1 warnings
+'
+
+
+test_expect_success 'push warns or fails when using username:password' '
+	git -c transfer.credentialsInUrl=allow push $url_userpass 2>err &&
+	! grep "$message" err &&
+
+	git -c transfer.credentialsInUrl=warn push $url_userpass 2>err &&
+	grep "warning: $message" err >warnings &&
+
+	test_must_fail git -c transfer.credentialsInUrl=die \
+		push $url_userpass 2>err &&
+	grep "fatal: $message" err >warnings &&
+	test_line_count = 1 warnings
+'
+
 test_done
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 45f0803ed4..b2524a24c2 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -71,29 +71,6 @@ test_expect_success 'clone respects GIT_WORK_TREE' '
 
 '
 
-test_expect_success LIBCURL 'clone warns or fails when using username:password' '
-	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
-	test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&
-	! grep "$message" err &&
-
-	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_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&
-	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings &&
-
-	test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@localhost attempt3 2>err &&
-	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings
-'
-
-test_expect_success LIBCURL 'clone does not detect username:password when it is https://username@domain:port/' '
-	test_must_fail git -c transfer.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&
-	! grep "uses plaintext credentials" err
-'
-
 test_expect_success 'clone from hooks' '
 
 	test_create_repo r0 &&
-- 
2.38.1.669.g2ee9a5b0e3

[PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings

[Jeff King]

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.

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5551-http-fetch-smart.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index bbe3d5721f..64c6c9f59e 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -597,17 +597,17 @@ test_expect_success 'clone warns or fails when using username:password' '
 	git -c transfer.credentialsInUrl=warn \
 		clone $url_userpass attempt2 2>err &&
 	grep "warning: $message" err >warnings &&
-	test_line_count = 2 warnings &&
+	test_line_count -ge 1 warnings &&
 
 	test_must_fail git -c transfer.credentialsInUrl=die \
 		clone $url_userpass attempt3 2>err &&
 	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings &&
+	test_line_count -ge 1 warnings &&
 
 	test_must_fail git -c transfer.credentialsInUrl=die \
 		clone $url_userblank attempt4 2>err &&
 	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings
+	test_line_count -ge 1 warnings
 '
 
 test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '
@@ -630,17 +630,17 @@ test_expect_success 'fetch warns or fails when using username:password' '
 
 	git -c transfer.credentialsInUrl=warn fetch $url_userpass 2>err &&
 	grep "warning: $message" err >warnings &&
-	test_line_count = 3 warnings &&
+	test_line_count -ge 1 warnings &&
 
 	test_must_fail git -c transfer.credentialsInUrl=die \
 		fetch $url_userpass 2>err &&
 	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings &&
+	test_line_count -ge 1 warnings &&
 
 	test_must_fail git -c transfer.credentialsInUrl=die \
 		fetch $url_userblank 2>err &&
 	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings
+	test_line_count -ge 1 warnings
 '
 
 
@@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' '
 	test_must_fail git -c transfer.credentialsInUrl=die \
 		push $url_userpass 2>err &&
 	grep "fatal: $message" err >warnings &&
-	test_line_count = 1 warnings
+	test_line_count -ge 1 warnings
 '
 
 test_done
-- 
2.38.1.669.g2ee9a5b0e3

Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings

[Jeff King]

On Mon, Oct 31, 2022 at 07:47:18PM +0000, Johannes Schindelin via GitGitGadget wrote:

> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 0b386c74818..b72cdeb6243 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -78,19 +78,19 @@ test_expect_success LIBCURL 'clone warns or fails when using username:password'
>  
>  	test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@224.0.0.1 attempt2 2>err &&
>  	grep "warning: $message" err >warnings &&
> -	test_line_count = 2 warnings &&
> +	test_line_count -ge 1 warnings &&
>  
>  	test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@224.0.0.1 attempt3 2>err &&
>  	grep "fatal: $message" err >warnings &&
> -	test_line_count = 1 warnings &&
> +	test_line_count -ge 1 warnings &&
>  
>  	test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@224.0.0.1 attempt3 2>err &&
>  	grep "fatal: $message" err >warnings &&
> -	test_line_count = 1 warnings
> +	test_line_count -ge 1 warnings
>  '
>  
>  test_expect_success LIBCURL 'clone does not detect username:password when it is https://username@domain:port/' '
> -	test_must_fail git -c transfer.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&
> +	test_must_fail git -c transfer.credentialsInUrl=warn clone https://username@224.0.0.1:8080 attempt3 2>err &&
>  	! grep "uses plaintext credentials" err
>  '

By the way, this last part of the hunk obviously belongs in the first
patch. I adjusted as appropriate in the patch I just sent.

-Peff

Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests

[Jeff King]

On Mon, Oct 31, 2022 at 08:59:21PM -0400, Taylor Blau wrote:

> > >  test_expect_success LIBCURL 'fetch warns or fails when using username:password' '
> > > -	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
> > > -	test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&
> > > +	message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" &&
> > > +	test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err &&
> > >  	! grep "$message" err &&
> >
> > could be more robust. It would actually check that we succeeded in using
> > the URL.
> 
> It is magical, indeed. If it's significantly more difficult to move
> these into t5550 or t5551, then I'm OK with this in the meantime (since
> GitHub Actions really is our primary CI target that we care about this
> not hanging on).
> 
> But assuming that moving these around isn't that difficult, I would be
> slightly happier to see these tests in one of the aforementioned spots.

I don't think it was too difficult to move them. I just sent in patches
(which I just realized you were not cc'd on).

-Peff

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

[Jeff King]

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.

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

Yeah, I think it is crappy UX, too. It's just that I think the tests
should not _asserting_ the bad behavior. At most, they should tolerate
the bad behavior as a band-aid. So I think Dscho's patch is doing the
right thing (and I do agree that we should fix the immediate CI pain by
adjusting the tests, and letting the user-visible fix proceed
independently).

-Peff

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

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 31 2022, Jeff King wrote:

> On Mon, Oct 31, 2022 at 09:47:08PM +0100, Ævar Arnfjörð Bjarmason wrote:
> [...]
>> 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.
>
> Yeah, I think it is crappy UX, too. It's just that I think the tests
> should not _asserting_ the bad behavior. At most, they should tolerate
> the bad behavior as a band-aid. So I think Dscho's patch is doing the
> right thing (and I do agree that we should fix the immediate CI pain by
> adjusting the tests, and letting the user-visible fix proceed
> independently).

The tests aren't just asserting the bad behavior, they're also ensuring
that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but
tolerable, but if we start emitting 500 of these it would be nice to
know.

I've found a couple of regressions like that in other areas, i.e. where
our tests didn't spot some output change because we were selectively
grepping things, but where it would have been nice to spot it at the
time.

Re: [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 31 2022, Jeff King wrote:

> Commit 6dcbdc0d66 (remote: create fetch.credentialsInUrl config,
> 2022-06-06) added tests for our handling of passwords in URLs. Since the
> obvious URL to be affected is git-over-http, the tests use http. However
> they don't set up a test server; they just try to access
> https://localhost, assuming it will fail (because the nothing is
> listening there).
>
> This causes some possible problems:
>
>   - There might be a web server running on localhost, and we do not
>     actually want to connect to that.
>
>   - The DNS resolver, or the local firewall, might take a substantial
>     amount of time (or forever, whichever comes first) to fail to
>     connect, slowing down the tests cases unnecessarily.
>
>   - Since there's no server, our tests for "allow" and "warn" still
>     expect the clone/fetch/push operations to fail, even though in the
>     real world we'd expect these to succeed. We scrape stderr to see
>     what happened, but it's not as robust as a more realistic test.
>
> Let's instead move these to t5551, which is all about testing http and
> where we have a real server. That eliminates any issues with contacting
> a strange URL, and lets the "allow" and "warn" tests confirm that the
> operation actually succeeds.
>
> It's not quite a verbatim move for a few reasons:
>
>   - we can drop the LIBCURL dependency; it's already part of
>     lib-httpd.sh
>
>   - we'll use HTTPD_URL_USER_PASS, etc, instead of our fake URL. To
>     avoid repetition, we'll add a few extra variables.
>
>   - the "https://username:@localhost" test uses a funny URL that
>     lib-httpd.sh doesn't provide. We'll similarly construct it in a
>     variable. Note that we're hard-coding the lib-httpd username here,
>     but t5551 already does that everywhere.
>
>   - for the "domain:port" test, the URL provided by lib-httpd is fine,
>     since our test server will always be on an exotic port. But we'll
>     confirm in the test that this is so.
>
>   - since our message-matching is done via grep, I simplified it to use
>     a regex, rather than trying to massage lib-httpd's variables.
>     Arguably this makes it more readable, too, while retaining the bits
>     we care about: the fatal/warning distinction, the "uses plaintext"
>     message, and the fact that the password was redacted.
>
>   - we'll use the /auth/ path for the repo, which shows that we are
>     indeed making use of the auth information when needed.
>
>   - we'll also use /smart/; most of these tests could be done via /dumb/
>     in t5550, but setting up pushes there requires extra effort and
>     dependencies. The smart protocol is what most everyone is using
>     these days anyway.
>
> This patch is my own, but I stole the analysis and a few bits of the
> commit message from a patch by Johannes Schindelin.

Did you consider just using git://; on the WIP branch I linked to where
I fixed these tests quite a bit already I left them in their own file in
anticipation of having to test that (but didn't finish that yet). I.e.:

	+ git -C /home/avar/g/git/t/trash directory.t5700-protocol-v1/repo/parent/ tag two
	+ GIT_TRACE_PACKET=1 git -C daemon_child -c protocol.version=1 -c transfer.credentialsInUrl=die fetch git://foo:bar@127.0.0.1:5700/parent
	fatal: URL 'git://foo:<redacted>@127.0.0.1:5700/parent' uses plaintext credentials

I can't remember if anything can do anything sensible with
user:passwords over non-http(s), but at the point where we emit this
error in remote.c we don't care, so perhaps we could just test it that
way.

Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 31 2022, Jeff King wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>

Whatever our difference of opinion about the usefulness of not asserting
that a thing doesn't get worse, even if it isn't perfect (c.f.:
https://lore.kernel.org/git/221101.86a65b5q9q.gmgdl@evledraar.gmail.com/)
I think that...

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

..this description dosen't match what the patch is doing, okey, so
there's a case where the remote helper swallows the warnings, i.e. we'll
emit fewer than we expected...

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t5551-http-fetch-smart.sh | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index bbe3d5721f..64c6c9f59e 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -597,17 +597,17 @@ test_expect_success 'clone warns or fails when using username:password' '
>  	git -c transfer.credentialsInUrl=warn \
>  		clone $url_userpass attempt2 2>err &&
>  	grep "warning: $message" err >warnings &&
> -	test_line_count = 2 warnings &&
> +	test_line_count -ge 1 warnings &&
>  
>  	test_must_fail git -c transfer.credentialsInUrl=die \
>  		clone $url_userpass attempt3 2>err &&
>  	grep "fatal: $message" err >warnings &&
> -	test_line_count = 1 warnings &&
> +	test_line_count -ge 1 warnings &&
>  
>  	test_must_fail git -c transfer.credentialsInUrl=die \
>  		clone $url_userblank attempt4 2>err &&
>  	grep "fatal: $message" err >warnings &&
> -	test_line_count = 1 warnings
> +	test_line_count -ge 1 warnings
>  '
>  
>  test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '
> @@ -630,17 +630,17 @@ test_expect_success 'fetch warns or fails when using username:password' '
>  
>  	git -c transfer.credentialsInUrl=warn fetch $url_userpass 2>err &&
>  	grep "warning: $message" err >warnings &&
> -	test_line_count = 3 warnings &&
> +	test_line_count -ge 1 warnings &&
>  
>  	test_must_fail git -c transfer.credentialsInUrl=die \
>  		fetch $url_userpass 2>err &&
>  	grep "fatal: $message" err >warnings &&
> -	test_line_count = 1 warnings &&
> +	test_line_count -ge 1 warnings &&
>  
>  	test_must_fail git -c transfer.credentialsInUrl=die \
>  		fetch $url_userblank 2>err &&
>  	grep "fatal: $message" err >warnings &&
> -	test_line_count = 1 warnings
> +	test_line_count -ge 1 warnings
>  '
>  
>  
> @@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' '
>  	test_must_fail git -c transfer.credentialsInUrl=die \
>  		push $url_userpass 2>err &&
>  	grep "fatal: $message" err >warnings &&
> -	test_line_count = 1 warnings
> +	test_line_count -ge 1 warnings
>  '

...but then why are we modifying these codepaths that have nothing to do
with invoking the remote helper, i.e. where we die early before we get
to that?

And even if some of this was invoking that remote helper and we were
modifying it blindly, then presumably the helper swallowing it would
result in 0 some of the time, so "-ge 1" would be wrong.

(That's not the case, but it's why I think the patch doesn't make much
sense).

Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.
>
> Since it is not actually important how many times Git prints the
> warning/error message, as long as it prints it at least once, ...

Sorry, but I do not quite see the value of keeping the test to
expect success in a weakend form.  If we think we are emitting three
warnings, whether we do so by mistake or by design, and some of them
are lost and not shown for an unknown reason, is there a guarantee
that at least one would survive?  And when all three are lost, even
the test in the weakened form would fail and stop the CI builds, no?

If we do not know why we are losing some messages, and if we do not
have resources to track down why, that is perfectly fine.  Fixes can
be prioritised.  But wouldn't test_expect_failure be a better way to
express "we think we ought to get 3 but for some reason we may not
get all of them and we haven't figured it out"?

Re: [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516

[Jeff King]

On Tue, Nov 01, 2022 at 04:18:32AM +0100, Ævar Arnfjörð Bjarmason wrote:

> Did you consider just using git://; on the WIP branch I linked to where
> I fixed these tests quite a bit already I left them in their own file in
> anticipation of having to test that (but didn't finish that yet). I.e.:

No, I didn't really consider that. Mostly because I was trying to stick
with the original intent of 6dcbdc0d66 that created them.

If we toss that out, then in theory that widens the options. And in some
ways it's nice to use git://, because it has fewer dependencies and so
is run on more platforms. But it strikes me as a pretty unrealistic
test, just because credentials in git:// URLs make no sense and are not
something anybody would do.

As you note, since the error comes from remote.c, it would still
trigger. But it's a bit more "white box" testing than I think makes
sense here. I prefer the original tests' method of trying to create
plausible real-world scenarios and seeing how they behave (and I think
my patch even improves that, since having an actual server on the other
end is the usual case).

-Peff

Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings

[Jeff King]

On Tue, Nov 01, 2022 at 04:29:31AM +0100, Ævar Arnfjörð Bjarmason wrote:

> > 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.
> 
> ..this description dosen't match what the patch is doing, okey, so
> there's a case where the remote helper swallows the warnings, i.e. we'll
> emit fewer than we expected...

So I really didn't revisit this commit much at all, and was just trying
to save Dscho (or Taylor) the work of having to rebase it, if we go with
my patch 1.

IMHO it is OK enough as it is, but if I were writing it from scratch I
probably would have given the rationale that the tests are insiting on a
dumb, sub-optimal behavior. And flakes or inconsistencies aside, they
should be asserting only the presence or absence of the message. And
probably would have left each at "grep" and dropped the test_line_count
totally.

It is not even clear to me that the remote-https is the one being
swallowed (at least, I have not seen an argument or evidence that this
is so; it does seem plausible).

> > @@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' '
> >  	test_must_fail git -c transfer.credentialsInUrl=die \
> >  		push $url_userpass 2>err &&
> >  	grep "fatal: $message" err >warnings &&
> > -	test_line_count = 1 warnings
> > +	test_line_count -ge 1 warnings
> >  '
> 
> ...but then why are we modifying these codepaths that have nothing to do
> with invoking the remote helper, i.e. where we die early before we get
> to that?

If you're arguing that we should only do s/= 3/-ge 1/ for the test that
is flaking, I could buy that. Though like I said, I consider the value
here to be focusing the purpose of the tests as much as dealing with the
flake.

I really don't care that much either way, though.

I'd also be fine with a separate test (marked expect_failure) that
checks the number of messages.

> And even if some of this was invoking that remote helper and we were
> modifying it blindly, then presumably the helper swallowing it would
> result in 0 some of the time, so "-ge 1" would be wrong.
> 
> (That's not the case, but it's why I think the patch doesn't make much
> sense).

I thought the point is that the outer program calling the helper would
consistently produce the error, always yielding at least one instance.
The helper one is generally "extra" and undesired.

-Peff

Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings

[Jeff King]

On Mon, Oct 31, 2022 at 09:54:23PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> >
> > Since it is not actually important how many times Git prints the
> > warning/error message, as long as it prints it at least once, ...
> 
> Sorry, but I do not quite see the value of keeping the test to
> expect success in a weakend form.  If we think we are emitting three
> warnings, whether we do so by mistake or by design, and some of them
> are lost and not shown for an unknown reason, is there a guarantee
> that at least one would survive?  And when all three are lost, even
> the test in the weakened form would fail and stop the CI builds, no?

Without understanding the cause of the loss, I agree that things are a
little hand-wavy. But the assumption _does_ seem to hold that we
consistently produce at least one (presumably from the parent
clone/fetch/push process). And if we can rely on that, there's value in
the tests asserting that the message was shown to the user at least
once.

> If we do not know why we are losing some messages, and if we do not
> have resources to track down why, that is perfectly fine.  Fixes can
> be prioritised.  But wouldn't test_expect_failure be a better way to
> express "we think we ought to get 3 but for some reason we may not
> get all of them and we haven't figured it out"?

Marking it as expect_failure throws out the main point of the test,
though, which is that the user sees _some_ message (and that the "die"
form aborts the operation).

It might make sense to add a separate test in the meantime that
documents the "oops, we get the wrong number" sometimes state (and
eventually, if fixed, that could be folded back into the main test for
efficiency / simplicity).

-Peff

Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 01 2022, Jeff King wrote:

> On Tue, Nov 01, 2022 at 04:29:31AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > 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.
>> 
>> ..this description dosen't match what the patch is doing, okey, so
>> there's a case where the remote helper swallows the warnings, i.e. we'll
>> emit fewer than we expected...
>
> So I really didn't revisit this commit much at all, and was just trying
> to save Dscho (or Taylor) the work of having to rebase it, if we go with
> my patch 1.
>
> IMHO it is OK enough as it is, but if I were writing it from scratch I
> probably would have given the rationale that the tests are insiting on a
> dumb, sub-optimal behavior. And flakes or inconsistencies aside, they
> should be asserting only the presence or absence of the message. And
> probably would have left each at "grep" and dropped the test_line_count
> totally.

Do you mean that even if we fix the bug and consistently emit one and
only one such message you'd like to have the tests not assert that
that's the case?

I do think that UX is important enough to test for, particularly if
we've had a bug related to that that we've fixed. I.e. if something in
the direction of my [1] goes in.

> It is not even clear to me that the remote-https is the one being
> swallowed (at least, I have not seen an argument or evidence that this
> is so; it does seem plausible).

It is the case, the only ones that are going to be duplicated are the
"warn" ones, because for "die" we'll die right away in the parent
process.

Which is what I'm trying to get across here, and why I'm a bit
confused. I.e. I thought you'd agree that we should test that we emit
exactly one warning if & when we've fixed the underlying issue.

That issue is already fixed for "die", so even if you want to loosen up
the test your [2] should only keep the first line removal/addition in
the first two hunks, and drop the 3rd one.

>> > @@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' '
>> >  	test_must_fail git -c transfer.credentialsInUrl=die \
>> >  		push $url_userpass 2>err &&
>> >  	grep "fatal: $message" err >warnings &&
>> > -	test_line_count = 1 warnings
>> > +	test_line_count -ge 1 warnings
>> >  '
>> 
>> ...but then why are we modifying these codepaths that have nothing to do
>> with invoking the remote helper, i.e. where we die early before we get
>> to that?
>
> If you're arguing that we should only do s/= 3/-ge 1/ for the test that
> is flaking, I could buy that.

I'm saying that if we're doing a handwaivy-fix and saying "sometimes the
message gets swallowed" and fixing this blindly without checking how it
works, then changing "= 1" to "-ge 1" doesn't make sense.

It should be "-ge 0", i.e. surely that "one warning" can get swallowed
too?

Now, I know that never happens, because we'll always get at least
one.

I'm just saying that as soon as you stop to think about that you must
also come to the conclusion that the "die" ones are OK as-is.

That's because the reason we always get at least one is the same as
we're always guaranteed to emit just one in the "die" case: The parent
process emits it, then dies.

>> And even if some of this was invoking that remote helper and we were
>> modifying it blindly, then presumably the helper swallowing it would
>> result in 0 some of the time, so "-ge 1" would be wrong.
>> 
>> (That's not the case, but it's why I think the patch doesn't make much
>> sense).
>
> I thought the point is that the outer program calling the helper would
> consistently produce the error, always yielding at least one instance.
> The helper one is generally "extra" and undesired.

Yes, exactly. Which is what my fix[1] the root cause addresses.

Anyway, I'm just trying to help here. If you/Johannes/others want to go
with the "hotfix" as-is that's fine my me.

I just don't see what the hurry is, it's been this way for two releases,
if it's flaky that's been the case for months, I'd think we could just
fix the root cause.

1. http://lore.kernel.org/git/RFC-patch-1.1-0266485bc6c-20221031T204149Z-avarab@gmail.com
2. https://lore.kernel.org/git/Y2CD6GBl6ORqKsug@coredump.intra.peff.net/

Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings

[Jeff King]

On Tue, Nov 01, 2022 at 09:15:00AM +0100, Ævar Arnfjörð Bjarmason wrote:

> > So I really didn't revisit this commit much at all, and was just trying
> > to save Dscho (or Taylor) the work of having to rebase it, if we go with
> > my patch 1.
> >
> > IMHO it is OK enough as it is, but if I were writing it from scratch I
> > probably would have given the rationale that the tests are insiting on a
> > dumb, sub-optimal behavior. And flakes or inconsistencies aside, they
> > should be asserting only the presence or absence of the message. And
> > probably would have left each at "grep" and dropped the test_line_count
> > totally.
> 
> Do you mean that even if we fix the bug and consistently emit one and
> only one such message you'd like to have the tests not assert that
> that's the case?

No, I wouldn't mind it, if that is a bug we've fixed. I just mean that
the tests as written never wanted to say "3 is the absolute right number
of times to write this message". They only put "3" there because it made
things pass.

> I do think that UX is important enough to test for, particularly if
> we've had a bug related to that that we've fixed. I.e. if something in
> the direction of my [1] goes in.

Sure, I don't mind at all a test for it. In the short-term, if you want
a test that fails, I'd prefer it be separate so that we can test the
useful existing behavior that _does_ work. If the multiple-messages bug
is fixed, I don't mind folding them together into a single test that
passes.

> > It is not even clear to me that the remote-https is the one being
> > swallowed (at least, I have not seen an argument or evidence that this
> > is so; it does seem plausible).
> 
> It is the case, the only ones that are going to be duplicated are the
> "warn" ones, because for "die" we'll die right away in the parent
> process.

Right, I understand why "die" produces only one. My question was when we
produce 2 on Windows (sometimes?) but 3 elsewhere, are we sure it is the
one from remote-https that is eaten, or could it ever be one of the
others?

In a sense we do not need to worry about "why is it sometimes eaten" if
the bug is fixed to produce only the one message. But it may point to a
separate and interesting problem (e.g., is stderr from remote-https not
reliable?).

> >> > @@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' '
> >> >  	test_must_fail git -c transfer.credentialsInUrl=die \
> >> >  		push $url_userpass 2>err &&
> >> >  	grep "fatal: $message" err >warnings &&
> >> > -	test_line_count = 1 warnings
> >> > +	test_line_count -ge 1 warnings
> >> >  '
> >> 
> >> ...but then why are we modifying these codepaths that have nothing to do
> >> with invoking the remote helper, i.e. where we die early before we get
> >> to that?
> >
> > If you're arguing that we should only do s/= 3/-ge 1/ for the test that
> > is flaking, I could buy that.
> 
> I'm saying that if we're doing a handwaivy-fix and saying "sometimes the
> message gets swallowed" and fixing this blindly without checking how it
> works, then changing "= 1" to "-ge 1" doesn't make sense.

Right, I'm fine with that (I perhaps should have said something stronger
than "I could buy that"). As I said, I was mostly just rebasing Dscho's
patch and I think it was good enough in the sense that it was
hand-waving away the whole "there may be more than one" problem.

But I do agree that we'll never see more in the "die" cases, and there
is no need to change them.

> > I thought the point is that the outer program calling the helper would
> > consistently produce the error, always yielding at least one instance.
> > The helper one is generally "extra" and undesired.
> 
> Yes, exactly. Which is what my fix[1] the root cause addresses.
> 
> Anyway, I'm just trying to help here. If you/Johannes/others want to go
> with the "hotfix" as-is that's fine my me.
> 
> I just don't see what the hurry is, it's been this way for two releases,
> if it's flaky that's been the case for months, I'd think we could just
> fix the root cause.

It recently bit me twice, so maybe I am giving it more urgency than it
deserves (or maybe something changed in CI to make it more likely).

I do think it would be nice to fix it. I don't love your patch for the
reasons I replied there (not your fault; it's inherently a crappy and
complicated problem). In the meantime, I'd like to see CI fixed, as
it is wasting developer's time. And that's why I called Dscho's
loosening "good enough". It is hopefully a temporary state anyway.

But I would be just as happy to see a similar patch which just changed
the 2/3 lines to "-ge 1" (or just a straight grep).

-Peff

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

[Jeff King]

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.

-Peff

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

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 01 2022, Jeff King wrote:

> On Tue, Nov 01, 2022 at 09:15:00AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > So I really didn't revisit this commit much at all, and was just trying
>> > to save Dscho (or Taylor) the work of having to rebase it, if we go with
>> > my patch 1.
>> >
>> > IMHO it is OK enough as it is, but if I were writing it from scratch I
>> > probably would have given the rationale that the tests are insiting on a
>> > dumb, sub-optimal behavior. And flakes or inconsistencies aside, they
>> > should be asserting only the presence or absence of the message. And
>> > probably would have left each at "grep" and dropped the test_line_count
>> > totally.
>> 
>> Do you mean that even if we fix the bug and consistently emit one and
>> only one such message you'd like to have the tests not assert that
>> that's the case?
>
> No, I wouldn't mind it, if that is a bug we've fixed. I just mean that
> the tests as written never wanted to say "3 is the absolute right number
> of times to write this message". They only put "3" there because it made
> things pass.

That's one reason, another is to assert current behavior, and to be able
to answer questions like "does this patch change behavior" without
having to recursively diff the trash directory output of a test run,
because everything's so fuzzy.

If and when it's 1 instead of 3, great, adjusting the test isn't a big
deal.

Anyway, we're off into general testing philosophy again, which I think
is off topic here.

>> I do think that UX is important enough to test for, particularly if
>> we've had a bug related to that that we've fixed. I.e. if something in
>> the direction of my [1] goes in.
>
> Sure, I don't mind at all a test for it. In the short-term, if you want
> a test that fails, I'd prefer it be separate so that we can test the
> useful existing behavior that _does_ work. If the multiple-messages bug
> is fixed, I don't mind folding them together into a single test that
> passes.

Right, I'm not saying "keep the flaky test", I'm saying let's keep the
ones we know aren't flaky.

>> > It is not even clear to me that the remote-https is the one being
>> > swallowed (at least, I have not seen an argument or evidence that this
>> > is so; it does seem plausible).
>> 
>> It is the case, the only ones that are going to be duplicated are the
>> "warn" ones, because for "die" we'll die right away in the parent
>> process.
>
> Right, I understand why "die" produces only one. My question was when we
> produce 2 on Windows (sometimes?) but 3 elsewhere, are we sure it is the
> one from remote-https that is eaten, or could it ever be one of the
> others?

I don't have a test case in front of me, and Johannes didn't provide one
(or even a link to CI output).

But from his description and being familiar with the code I'm pretty
certain isn't not the "die" cases, those are all in-process, and it
happens before we spawn sub-processes, I don't see how that would be
different on Windows.

>> > I thought the point is that the outer program calling the helper would
>> > consistently produce the error, always yielding at least one instance.
>> > The helper one is generally "extra" and undesired.
>> 
>> Yes, exactly. Which is what my fix[1] the root cause addresses.
>> 
>> Anyway, I'm just trying to help here. If you/Johannes/others want to go
>> with the "hotfix" as-is that's fine my me.
>> 
>> I just don't see what the hurry is, it's been this way for two releases,
>> if it's flaky that's been the case for months, I'd think we could just
>> fix the root cause.
>
> It recently bit me twice, so maybe I am giving it more urgency than it
> deserves (or maybe something changed in CI to make it more likely).

Bit you in GitHub Windows CI?

> I do think it would be nice to fix it. I don't love your patch for the
> reasons I replied there (not your fault; it's inherently a crappy and
> complicated problem). In the meantime, I'd like to see CI fixed, as
> it is wasting developer's time. And that's why I called Dscho's
> loosening "good enough". It is hopefully a temporary state anyway.
>
> But I would be just as happy to see a similar patch which just changed
> the 2/3 lines to "-ge 1" (or just a straight grep).

Sure, if we're deciding not to care about tests that are unrelated to
the flakyness problem at hand being loosened.

Re: [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516

[Taylor Blau]

On Tue, Nov 01, 2022 at 03:32:06AM -0400, Jeff King wrote:
> If we toss that out, then in theory that widens the options. And in some
> ways it's nice to use git://, because it has fewer dependencies and so
> is run on more platforms. But it strikes me as a pretty unrealistic
> test, just because credentials in git:// URLs make no sense and are not
> something anybody would do.
>
> As you note, since the error comes from remote.c, it would still
> trigger. But it's a bit more "white box" testing than I think makes
> sense here. I prefer the original tests' method of trying to create
> plausible real-world scenarios and seeing how they behave (and I think
> my patch even improves that, since having an actual server on the other
> end is the usual case).

Agreed. Despite some of the niceties you mention, I concur that testing
http(s) is more realistic and worth doing.

Thanks,
Taylor

Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings

[Taylor Blau]

On Tue, Nov 01, 2022 at 03:42:59AM -0400, Jeff King wrote:
> On Mon, Oct 31, 2022 at 09:54:23PM -0700, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > 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.
> > >
> > > Since it is not actually important how many times Git prints the
> > > warning/error message, as long as it prints it at least once, ...
> >
> > Sorry, but I do not quite see the value of keeping the test to
> > expect success in a weakend form.  If we think we are emitting three
> > warnings, whether we do so by mistake or by design, and some of them
> > are lost and not shown for an unknown reason, is there a guarantee
> > that at least one would survive?  And when all three are lost, even
> > the test in the weakened form would fail and stop the CI builds, no?
>
> Without understanding the cause of the loss, I agree that things are a
> little hand-wavy. But the assumption _does_ seem to hold that we
> consistently produce at least one (presumably from the parent
> clone/fetch/push process). And if we can rely on that, there's value in
> the tests asserting that the message was shown to the user at least
> once.

Part of me wonders whether the local DNS-resolution issue you fixed in
the first patch would be sufficient to get us to produce the wrong
number of warnings consistently.

I.e., if I queue just the first patch and drop Johannes's, would that be
sufficient to get CI working consistently again?

I don't know. It's frustrating to rely so much on an external
environment that our feedback loop can only be as short as "push out
some combination of these patches and wait for CI". That's
disappointing, and TBH I would rather spend time focusing on other
patches than play games with CI.

The pair of patches look good to me. Perhaps we could avoid the weakened
assumption, but I do not mind too much in the meantime. Especially since
we already have a series[1] in the works that resolves the issue for
good.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/RFC-patch-1.1-0266485bc6c-20221031T204149Z-avarab@gmail.com/

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

[Taylor Blau]

On Tue, Nov 01, 2022 at 04:01:18AM +0100, Ævar Arnfjörð Bjarmason wrote:
> > Yeah, I think it is crappy UX, too. It's just that I think the tests
> > should not _asserting_ the bad behavior. At most, they should tolerate
> > the bad behavior as a band-aid. So I think Dscho's patch is doing the
> > right thing (and I do agree that we should fix the immediate CI pain by
> > adjusting the tests, and letting the user-visible fix proceed
> > independently).
>
> The tests aren't just asserting the bad behavior, they're also ensuring
> that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but
> tolerable, but if we start emitting 500 of these it would be nice to
> know.

I admit that this kind of argument does not sway me.

Is it likely that we would suddenly start spewing 500 such warnings? If
we did, are there no other tests that would catch it? And even if *that*
were the case, would nobody happen to notice it in the meantime either
during development or when we queue an affected topic onto 'next' for
wider testing?

I guess the answer is that it's possible that we'd miss such a
regression in all of those above places, but to me it seems extremely
unlikely that we'd let such a regression through without noticing.

Thanks,
Taylor

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

[Taylor Blau]

On Tue, Nov 01, 2022 at 02:07:39PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > 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.

I vaguely prefer calling this GIT_CHECKED_CREDENTIALS_IN_URL instead of
stuffing it in the configuration. All other things *aren't* equal here,
since we're not lying to sub-processes about configuration values set
by the user.

Instead, we're saying: "regardless of what value the user assigned
transfer.credentialsInUrl, we can avoid looking at it because we have
already checked for the presence of credentials in the URL".

Thanks,
Taylor

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

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 01 2022, Taylor Blau wrote:

> On Tue, Nov 01, 2022 at 02:07:39PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> > 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.
>
> I vaguely prefer calling this GIT_CHECKED_CREDENTIALS_IN_URL instead of
> stuffing it in the configuration.[...]

To be clear, I'm asking if there's cases where we think one method or
the other produces different results, which I understood Jeff hinting
at.

> Instead, we're saying: "regardless of what value the user assigned
> transfer.credentialsInUrl, we can avoid looking at it because we have
> already checked for the presence of credentials in the URL".[...] All
> other things *aren't* equal here, since we're not lying to
> sub-processes about configuration values set by the user.

The user is asking us to warn about storing certain things in config, we
know we already warned, so we're looking to flip that value to the
"quiet" setting.

If you consider that overriding I don't get how doing so via a different
environment variable changes anything. It would be doing the same thing,
just making it less obvious.

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

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 01 2022, Taylor Blau wrote:

> On Tue, Nov 01, 2022 at 04:01:18AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> > Yeah, I think it is crappy UX, too. It's just that I think the tests
>> > should not _asserting_ the bad behavior. At most, they should tolerate
>> > the bad behavior as a band-aid. So I think Dscho's patch is doing the
>> > right thing (and I do agree that we should fix the immediate CI pain by
>> > adjusting the tests, and letting the user-visible fix proceed
>> > independently).
>>
>> The tests aren't just asserting the bad behavior, they're also ensuring
>> that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but
>> tolerable, but if we start emitting 500 of these it would be nice to
>> know.
>
> I admit that this kind of argument does not sway me.
>
> Is it likely that we would suddenly start spewing 500 such warnings? If
> we did, are there no other tests that would catch it? And even if *that*
> were the case, would nobody happen to notice it in the meantime either
> during development or when we queue an affected topic onto 'next' for
> wider testing?
>
> I guess the answer is that it's possible that we'd miss such a
> regression in all of those above places, but to me it seems extremely
> unlikely that we'd let such a regression through without noticing.

Literally 500? Probably not, that was hyperbole to make a point, but
several, low tens? Yeah, I know of at least a couple in-tree off the top
of my head.

The point, which I assumed was clear is that we literally wouldn't
notice if it were 500, and that sort of thing is a common pattern in our
tests. I.e. in most cases we'd ideally test_cmp known output (at least
to the extent of assuring ourselves that we're getting it right).

Instead we often just grep it, or don't test it at all. Sometimes for a
good reason (e.g. the output containing absolute paths), but more often
than not for no good reason.

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

[Taylor Blau]

On Tue, Nov 01, 2022 at 11:17:42PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> The tests aren't just asserting the bad behavior, they're also ensuring
> >> that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but
> >> tolerable, but if we start emitting 500 of these it would be nice to
> >> know.
> >
> > I admit that this kind of argument does not sway me.
> >
> > Is it likely that we would suddenly start spewing 500 such warnings? If
> > we did, are there no other tests that would catch it? And even if *that*
> > were the case, would nobody happen to notice it in the meantime either
> > during development or when we queue an affected topic onto 'next' for
> > wider testing?
> >
> > I guess the answer is that it's possible that we'd miss such a
> > regression in all of those above places, but to me it seems extremely
> > unlikely that we'd let such a regression through without noticing.
>
> Literally 500? Probably not, that was hyperbole to make a point, but
> several, low tens? Yeah, I know of at least a couple in-tree off the top
> of my head.

Still, I find it suspect to assume that a developer working in this area
wouldn't notice even a minor regression (say, adding an additional
warning).

Thanks,
Taylor

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

[Jeff King]

On Tue, Nov 01, 2022 at 10:57:46PM +0100, Ævar Arnfjörð Bjarmason wrote:

> >> 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.
> >
> > I vaguely prefer calling this GIT_CHECKED_CREDENTIALS_IN_URL instead of
> > stuffing it in the configuration.[...]
> 
> To be clear, I'm asking if there's cases where we think one method or
> the other produces different results, which I understood Jeff hinting
> at.

What I was hinting before was not that I knew of a particular bug in
your patch, but that I think the technique of munging
GIT_CONFIG_PARAMETERS is fragile in error-prone in the general case,
because the sub-programs can't differentiate between the config the user
asked for, and what was set by the suppression mechanism.

For this variable, there's no need to differentiate between "the user
asked us to be quiet" and "the calling program asked us to be quiet",
but I could imagine cases where there are subtle distinctions. Imagine
if there was a setting for "warn and rewrite the URL". We'd need to
change that to "don't warn, but just rewrite the URL", which otherwise
is a mode that doesn't need to exist.

Keeping it in a separate variable keeps the concerns orthogonal. The
code still gets to see what the user actually wants (via the config),
but has extra information from the calling process about how noisy/quiet
to be.

But you mentioned submodules in your other mail. And you're right that
the patch I showed doesn't handle that (it would need to add the new
variable to local_repo_env). It seems like yours should work because
CONFIG_DATA_ENVIRONMENT as part of local_repo_env. But I don't think it
actually does; in prepare_other_repo_env(), we retain the variables for
config in the environment, so that:

  git -c foo.bar=whatever fetch

will override variables in both the superproject and in submodules.

I didn't try it, but I suspect with your patch that a superproject with
"warn" and a submodule with "die" (both in their on-disk config files)
would misbehave. The superproject process will warn and say "yes, I've
checked everything" by munging the in-environment config to "allow".
Then the submodule process will see that config, and will override the
on-disk setting (in the usual last-one-wins config way). I.e., the
problem is that it cannot tell the difference between "the user asked to
override this" and the suppression mechanism.

-Peff

[PATCH v3 2/2] t5551: be less strict about the number of credential warnings

[Jeff King]

On Tue, Nov 01, 2022 at 04:54:38PM -0400, Taylor Blau wrote:

> > The tests aren't just asserting the bad behavior, they're also ensuring
> > that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but
> > tolerable, but if we start emitting 500 of these it would be nice to
> > know.
> 
> I admit that this kind of argument does not sway me.
> 
> Is it likely that we would suddenly start spewing 500 such warnings? If
> we did, are there no other tests that would catch it? And even if *that*
> were the case, would nobody happen to notice it in the meantime either
> during development or when we queue an affected topic onto 'next' for
> wider testing?
> 
> I guess the answer is that it's possible that we'd miss such a
> regression in all of those above places, but to me it seems extremely
> unlikely that we'd let such a regression through without noticing.

Like you, I don't find much value in asserting "2 or 3, but not 500".
But it is easy enough to at least only loosen the few cases that need
it.

So here's a replacement for 2/2 that does the minimal thing. I rewrote
the commit message to explain my view (incidentally, it also fixes the
subject line, which mentioned the wrong test number after the rebase).

As I said, I had tried to mostly leave patch 2 alone to avoid derailing
Dscho's attempt to fix things. But somehow things got derailed anyway,
so maybe we can just all agree on this patch and move on with our lives?
I can't over-emphasize how little I care about this credentialsInUrl
feature in the first place, and somehow it has consumed hours of my life
now.

-- >8 --
Subject: t5551: be less strict about the number of credential warnings

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.

This causes the tests to fail, because they assert that in a few cases
we should print the warning 2 or even 3 times. The reason for that is
given in 6dcbdc0d66 (remote: create fetch.credentialsInUrl config,
2022-06-06), which is that multiple processes are involved, and each
warns.

In an ideal world, the user would just see the message once per logical
operation; they don't care how many underlying processes are involved.
And we may fix that eventually. But in the meantime, let's loosen the
tests to just assert that the user sees the message _at least_ once.

This won't catch a case where we accidentally start producing it 500
times, but that's not a likely regression. A more likely thing is that
we'd start producing it four times because the underlying code changes
to use a new process. But that's exactly the kind of thing we'd prefer
to be ignoring in the tests.

Note that the tests for the "die" mode don't need adjusted. They die
immediately in the first process that sees the problem, so they
consistently show the error once. It's only the "warn" mode which must
be loose. If we eventually fix it, then we can tighten its assertion. In
the meantime, this works around the CI issues.

Based on a patch by Johannes Schindelin.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5551-http-fetch-smart.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index bbe3d5721f..4f559722f4 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -597,7 +597,7 @@ test_expect_success 'clone warns or fails when using username:password' '
 	git -c transfer.credentialsInUrl=warn \
 		clone $url_userpass attempt2 2>err &&
 	grep "warning: $message" err >warnings &&
-	test_line_count = 2 warnings &&
+	test_line_count -ge 1 warnings &&
 
 	test_must_fail git -c transfer.credentialsInUrl=die \
 		clone $url_userpass attempt3 2>err &&
@@ -630,7 +630,7 @@ test_expect_success 'fetch warns or fails when using username:password' '
 
 	git -c transfer.credentialsInUrl=warn fetch $url_userpass 2>err &&
 	grep "warning: $message" err >warnings &&
-	test_line_count = 3 warnings &&
+	test_line_count -ge 1 warnings &&
 
 	test_must_fail git -c transfer.credentialsInUrl=die \
 		fetch $url_userpass 2>err &&
-- 
2.38.1.677.g9b1428ac2e

Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings

[Eric Sunshine]

On Wed, Nov 2, 2022 at 4:46 AM Jeff King <peff@peff.net> wrote:
> Subject: t5551: be less strict about the number of credential warnings
>
> 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.
>
> This causes the tests to fail, because they assert that in a few cases
> we should print the warning 2 or even 3 times. The reason for that is
> given in 6dcbdc0d66 (remote: create fetch.credentialsInUrl config,
> 2022-06-06), which is that multiple processes are involved, and each
> warns.
>
> In an ideal world, the user would just see the message once per logical
> operation; they don't care how many underlying processes are involved.
> And we may fix that eventually. But in the meantime, let's loosen the
> tests to just assert that the user sees the message _at least_ once.
>
> This won't catch a case where we accidentally start producing it 500
> times, but that's not a likely regression. A more likely thing is that
> we'd start producing it four times because the underlying code changes
> to use a new process. But that's exactly the kind of thing we'd prefer
> to be ignoring in the tests.
>
> Note that the tests for the "die" mode don't need adjusted. They die

s/adjusted/adjustment --or -- s/need/& to be/

> immediately in the first process that sees the problem, so they
> consistently show the error once. It's only the "warn" mode which must
> be loose. If we eventually fix it, then we can tighten its assertion. In
> the meantime, this works around the CI issues.
>
> Based on a patch by Johannes Schindelin.
>
> Signed-off-by: Jeff King <peff@peff.net>

Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings

[Jeff King]

On Wed, Nov 02, 2022 at 04:49:37AM -0400, Eric Sunshine wrote:

> > Note that the tests for the "die" mode don't need adjusted. They die
> 
> s/adjusted/adjustment --or -- s/need/& to be/

https://english.stackexchange.com/questions/5407/central-pennsylvanian-english-speakers-what-are-the-limitations-on-the-needs-w

Don't stomp on my linguistic heritage. :)

-Peff

Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings

[Jeff King]

On Wed, Nov 02, 2022 at 04:42:13AM -0400, Jeff King wrote:

> As I said, I had tried to mostly leave patch 2 alone to avoid derailing
> Dscho's attempt to fix things. But somehow things got derailed anyway,
> so maybe we can just all agree on this patch and move on with our lives?

By the way, I think you (or somebody?) mentioned elsewhere in the thread
that it's possible the first patch fixes things by itself. I would also
be content to just apply the first one and see if CI improves.

Of course, when I just pushed all this out to CI, it flaked
independently on both osx (looks like racy p4 stuff) and fedora (network
dropout failed to set up the container). Sigh.

-Peff

Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings

[Eric Sunshine]

On Wed, Nov 2, 2022 at 5:15 AM Jeff King <peff@peff.net> wrote:
> On Wed, Nov 02, 2022 at 04:49:37AM -0400, Eric Sunshine wrote:
> > > Note that the tests for the "die" mode don't need adjusted. They die
> >
> > s/adjusted/adjustment --or -- s/need/& to be/
>
> https://english.stackexchange.com/questions/5407/central-pennsylvanian-english-speakers-what-are-the-limitations-on-the-needs-w
>
> Don't stomp on my linguistic heritage. :)

Sorry. My head needs hanged in shame. I forgot that I can't grammar.
(I can't math either.)

Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings

[Taylor Blau]

On Wed, Nov 02, 2022 at 05:18:29AM -0400, Jeff King wrote:
> On Wed, Nov 02, 2022 at 04:42:13AM -0400, Jeff King wrote:
>
> > As I said, I had tried to mostly leave patch 2 alone to avoid derailing
> > Dscho's attempt to fix things. But somehow things got derailed anyway,
> > so maybe we can just all agree on this patch and move on with our lives?
>
> By the way, I think you (or somebody?) mentioned elsewhere in the thread
> that it's possible the first patch fixes things by itself. I would also
> be content to just apply the first one and see if CI improves.
>
> Of course, when I just pushed all this out to CI, it flaked
> independently on both osx (looks like racy p4 stuff) and fedora (network
> dropout failed to set up the container). Sigh.

Dreams are free, eh?

Thanks,
Taylor

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

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 02 2022, Jeff King wrote:

> On Tue, Nov 01, 2022 at 10:57:46PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> >> 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.
>> >
>> > I vaguely prefer calling this GIT_CHECKED_CREDENTIALS_IN_URL instead of
>> > stuffing it in the configuration.[...]
>> 
>> To be clear, I'm asking if there's cases where we think one method or
>> the other produces different results, which I understood Jeff hinting
>> at.
>
> What I was hinting before was not that I knew of a particular bug in
> your patch, but that I think the technique of munging
> GIT_CONFIG_PARAMETERS is fragile in error-prone in the general case,
> because the sub-programs can't differentiate between the config the user
> asked for, and what was set by the suppression mechanism.
>
> For this variable, there's no need to differentiate between "the user
> asked us to be quiet" and "the calling program asked us to be quiet",
> but I could imagine cases where there are subtle distinctions. Imagine
> if there was a setting for "warn and rewrite the URL". We'd need to
> change that to "don't warn, but just rewrite the URL", which otherwise
> is a mode that doesn't need to exist.
>
> Keeping it in a separate variable keeps the concerns orthogonal. The
> code still gets to see what the user actually wants (via the config),
> but has extra information from the calling process about how noisy/quiet
> to be.

... (replied below) ...

> But you mentioned submodules in your other mail. And you're right that
> the patch I showed doesn't handle that (it would need to add the new
> variable to local_repo_env). It seems like yours should work because
> CONFIG_DATA_ENVIRONMENT as part of local_repo_env. But I don't think it
> actually does; in prepare_other_repo_env(), we retain the variables for
> config in the environment, so that:
>
>   git -c foo.bar=whatever fetch
>
> will override variables in both the superproject and in submodules.

Replying to your main point below, just an aside on this:

To be clear I'm not saying it would handle it sensibly now, but just
that if we're using env vars to communicate to sub-processes then using
CONFIG_DATA_ENVIRONMENT seems better to me.

Because even if we're getting it wrong now, then surely that's something
we're probably getting wrong in more than one place.

So e.g. if we set an env var "for ourselves", i.e. "pull->fetch" then we
could detect that condition in run_command(), and if we then spot that
we're carrying an env variable we set earlier up the chain reset it
before we spawn a submodule.

Whereas if it's all custom env variables here & there we'll need to add
all that special-casing in.

> I didn't try it, but I suspect with your patch that a superproject with
> "warn" and a submodule with "die" (both in their on-disk config files)
> would misbehave. The superproject process will warn and say "yes, I've
> checked everything" by munging the in-environment config to "allow".
> Then the submodule process will see that config, and will override the
> on-disk setting (in the usual last-one-wins config way). I.e., the
> problem is that it cannot tell the difference between "the user asked to
> override this" and the suppression mechanism.

Yes, definitely, and now I see what you're saying. I.e. imagine a chain
like this (not actual process chains, but let's go with the example);

	user config = warn
	run: pull
	our config = allow
		# OK: doesn't "warn" now
		run: fetch
			# Not warning, but ....
			run: pre-fetch hook
				# BAD: ...oh oh, now a hook is fetching some
                                # entirely unrelated repo
				run: git pull
			# OK: Doesn't warn either
			run: remote-curl (now not warning, otherwise would)
                        # BAD: our "warned already" has infected a
                        # submodule, and we downgrade "die" to "allow"
			user config = die
			run: git fetch <submodule>
				...

But, and maybe I'm still not getting it, but the "use a different env
var" isn't actually the important part, i.e. these would behave the
same after the initial "warn":

	-c transfer.credentialsInUrlWarnedAlready=true

And:

	GIT_CHECKED_AND_WARNED_ALREADY=1

But not what I was suggesting:

	# conflated with a later "die"
	-c transfer.credentialsInUrl=allow

But that only goes for e.g. a "pull" where we "warn" followed by
submodule whose config is "die".

But all suggested variants of this (mine and yours) are going to get
e.g. the case where the submodule also wants "warn". I.e. it's not
enough that we're saying "warned already".

That gets us past conflating an existing "warn" with a "die", but now we
can't tell a submodule "warn" from a superproject "warn".

For that we'd basically need to do:

	-c transfer.$(<make path to .git, or other "unique repo id>).credentialsInUrl=allow

Or another similar mechanism, of course the most sane one to be to not
be playing this game at all, but to just ferry this state e.g. with
"--do-not-warn-about-this-repo" to our own children, which we'd not add
to the command-lines when we know we're spawning a hook, or doing the
submodule "pull/fetch".

Does that all seem right?

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

[Jeff King]

On Fri, Nov 04, 2022 at 10:01:00AM +0100, Ævar Arnfjörð Bjarmason wrote:

> > But you mentioned submodules in your other mail. And you're right that
> > the patch I showed doesn't handle that (it would need to add the new
> > variable to local_repo_env). It seems like yours should work because
> > CONFIG_DATA_ENVIRONMENT as part of local_repo_env. But I don't think it
> > actually does; in prepare_other_repo_env(), we retain the variables for
> > config in the environment, so that:
> >
> >   git -c foo.bar=whatever fetch
> >
> > will override variables in both the superproject and in submodules.
> 
> Replying to your main point below, just an aside on this:
> 
> To be clear I'm not saying it would handle it sensibly now, but just
> that if we're using env vars to communicate to sub-processes then using
> CONFIG_DATA_ENVIRONMENT seems better to me.
> 
> Because even if we're getting it wrong now, then surely that's something
> we're probably getting wrong in more than one place.
> 
> So e.g. if we set an env var "for ourselves", i.e. "pull->fetch" then we
> could detect that condition in run_command(), and if we then spot that
> we're carrying an env variable we set earlier up the chain reset it
> before we spawn a submodule.
> 
> Whereas if it's all custom env variables here & there we'll need to add
> all that special-casing in.

The idea is that local_repo_env carries that information, and everybody
uses it. So yes, it would have to know about the custom env variables,
but then any place which enters another repo learns about it "for free".
But using CONFIG_DATA_ENVIRONMENT for this doesn't work, because we
don't actually clear it when moving between repositories.

And in the example I showed, I used a config variable that was specific
to this particular problem. But if there were a lot of these, it really
could be:

  GIT_CHECKED_AND_WARNED='remotes some-other-subsytem etc'

so that all of them were shoved into one variable, and parsed. But we
can do the simplest dumb thing for now, because none of this is a public
interface we need to support across versions. We could move from one to
the other later.

> But, and maybe I'm still not getting it, but the "use a different env
> var" isn't actually the important part, i.e. these would behave the
> same after the initial "warn":
> 
> 	-c transfer.credentialsInUrlWarnedAlready=true
> 
> And:
> 
> 	GIT_CHECKED_AND_WARNED_ALREADY=1
> 
> But not what I was suggesting:
> 
> 	# conflated with a later "die"
> 	-c transfer.credentialsInUrl=allow

Right. The important thing is not that it's in a different env variable
in particular, but that it _isn't_ using the same config variable that
the user would set.

But once you are not using that same config variable, the question is:
do you want it put in with the config data or not? And I think we have
seen that putting it with the config data is not going to work. We don't
clear those environment variables when moving between repositories.

Now obviously if the env-clearing code knew which config variables were
to be cleared and which not, it could do so. But why introduce that
complexity, when you can just stick all the stuff that should be cleared
into a new variable (and again, that can be _one_ variable for all of
this stuff).

> But all suggested variants of this (mine and yours) are going to get
> e.g. the case where the submodule also wants "warn". I.e. it's not
> enough that we're saying "warned already".
> 
> That gets us past conflating an existing "warn" with a "die", but now we
> can't tell a submodule "warn" from a superproject "warn".
> 
> For that we'd basically need to do:
> 
> 	-c transfer.$(<make path to .git, or other "unique repo id>).credentialsInUrl=allow
> 
> Or another similar mechanism, of course the most sane one to be to not
> be playing this game at all, but to just ferry this state e.g. with
> "--do-not-warn-about-this-repo" to our own children, which we'd not add
> to the command-lines when we know we're spawning a hook, or doing the
> submodule "pull/fetch".
> 
> Does that all seem right?

That seems much more complicated. Again, why bother stuffing this extra
context information into a config variable, just to work around the fact
that config isn't cleared between repositories, when there is already a
simple mechanism for clearing state when switching repositories?

All that said, the implementation of this whole warning seems really
weird to me. If we want to warn about using such a URL, then we should
do it at the point of use. From the original commit message, the only
reason it was put where it is was to avoid sigpipe when the remote
helper dies. The solution there seems to be "have the helper die in a
less abrupt way".

And TBH, I'm skeptical of the whole feature. I don't see the point in
even warning somebody about:

  git fetch https://username:password@example.com

Maybe it's bad practice (though with password stand-ins like
restricted-access tokens, it isn't always), but at the point the user
has invoked git, there's nothing else bad that happens by doing what
they asked for.

What _is_ potentially bad is using that URL for cloning, because we then
write the credential in plaintext to the config file. And a better
solution there is probably to issue a warning, clone anyway, and then
omit the password from what we write into config. A follow-on fetch
would fail, but that is not any worse than the "die" mode of this
current config option.

There were even patches we discussed a few years ago:

  https://lore.kernel.org/git/20190517222031.GA17966@sigill.intra.peff.net/

I think the only really contentious part was the third one, which tried
to do so automagic with credential helpers. If we skipped that, then the
mode given by patches 1+2 seems strictly better than anything this
transfer.credentialsInUrl option provides.

-Peff