[RFC PATCH 0/5] fix issues in transfer.credentialsInUrl

[RFC PATCH 0/5] fix issues in transfer.credentialsInUrl

[Ævar Arnfjörð Bjarmason]

This is on top of [1], and given the "rc" phase is an RFC. This:

 * Fixes the issue of the transfer.credentialsInUrl (now renamed) not
   finding passwords in "pushurl" URLs (in my case, the only place
   where I'd actually put a password in a URL in a config...)

 * 1/5 fixes a bug in an existing test, but I didn't think it was
   worth bothering with for 2.37.0.

 * Adds missing test coverage for reading the config from a file, not
   the CLI.

 * 3/5 is a WIP CI target to spot the type of issue I fixed in [2],
   it's not the first time where we have a NO_CURL=Y breakage land on
   master...

 * 4/5 attemps to "really" fix the duplicate warnings we emit, I think
   the approach there is good, especially the part where we shouldn't
   emit it twice in-process.

   But this currently misses e.g. "git ls-remote". I wonder if we
   should just stick that git_config_push_parameter() condition into
   packet_trace_identity() and call it a day.

 * 5/5 fixes the (major) blind spot of the warning missing "pushurl" config.

I think this is all non-RFC quality, except the "ls-remote" case, and
us missing tests for that & other transport users that aren't
clone/fetch/push.

Derrick: Are you interested in picking this up & pursuing it after the
release, with whatever fix-ups/rewrites etc. that you find
appropriate?

1. https://lore.kernel.org/git/cover-0.2-00000000000-20220615T103852Z-avarab@gmail.com/
2. https://lore.kernel.org/git/cover-0.1-00000000000-20220615T103609Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (5):
  push tests: add a missing "test_line_count"
  fetch+push tests: add missing coverage for 6dcbdc0d661
  CI: add a linux-BUILD-vars job
  fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings
  transport: check remote.<name>pushurl with transfer.credentialsInUrl

 .github/workflows/main.yml |  3 ++
 builtin/clone.c            |  5 ++-
 builtin/fetch.c            |  4 ++
 builtin/push.c             |  6 ++-
 ci/run-build-and-tests.sh  | 30 ++++++++++++++
 remote.c                   | 82 +++++++++++++++++++++++++++-----------
 remote.h                   | 14 +++++++
 t/t5516-fetch-push.sh      | 46 ++++++++++++++++++++-
 t/t5601-clone.sh           |  2 +-
 9 files changed, 164 insertions(+), 28 deletions(-)

-- 
2.36.1.1239.gfba91521d90

[RFC PATCH 1/5] push tests: add a missing "test_line_count"

[Ævar Arnfjörð Bjarmason]

Add a "test_line_count" missing from 6dcbdc0d661, we'd clobber
"warnings" here, but never test its contents.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 79d8a7b3675..4b32ae39a39 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1860,6 +1860,8 @@ test_expect_success 'push warns or fails when using username:password' '
 
 	test_must_fail git -c transfer.credentialsInUrl=warn push https://username:password@localhost 2>err &&
 	grep "warning: $message" err >warnings &&
+	test_line_count = 1 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
-- 
2.36.1.1239.gfba91521d90

[RFC PATCH 2/5] fetch+push tests: add missing coverage for 6dcbdc0d661

[Ævar Arnfjörð Bjarmason]

Add tests that were missing from 6dcbdc0d661 (remote: create
fetch.credentialsInUrl config, 2022-06-06), we want to test how we
handle cases where the config comes from a file, and that we handle
"pushURL" correctly.

Currently the "pushURL" case isn't handled at all, i.e. URLs aren't
warned about in "remote.*pushurl" , only for "remote.*.url".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5516-fetch-push.sh | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4b32ae39a39..51d695e475a 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1852,6 +1852,26 @@ test_expect_success 'fetch warns or fails when using username:password' '
 	test_line_count = 1 warnings
 '
 
+test_expect_success CURL 'fetch warns or fails when using username:password in config' '
+	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
+
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	test_commit -C repo A &&
+	git -C repo remote add pwd-url https://username:password@localhost &&
+	test_must_fail git -C repo -c transfer.credentialsInUrl=allow fetch pwd-url 2>err &&
+	! grep "$message" err &&
+
+	test_must_fail git -C repo -c transfer.credentialsInUrl=warn fetch pwd-url 2>err &&
+	grep "warning: $message" err >warnings &&
+	test_line_count = 3 warnings &&
+
+	git -C repo remote set-url --push pwd-url https://username:password@localhost &&
+	git -C repo remote set-url pwd-url https://localhost &&
+
+	test_must_fail git -C repo -c transfer.credentialsInUrl=warn fetch pwd-url 2>err &&
+	! grep "fatal: $message" err
+'
 
 test_expect_success 'push warns or fails when using username:password' '
 	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
@@ -1867,4 +1887,25 @@ test_expect_success 'push warns or fails when using username:password' '
 	test_line_count = 1 warnings
 '
 
+test_expect_success CURL 'push warns or fails when using username:password in config' '
+	message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
+
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	test_commit -C repo A &&
+	git -C repo remote add pwd-url https://username:password@localhost &&
+	test_must_fail git -C repo -c transfer.credentialsInUrl=allow push pwd-url HEAD:refs/heads/branch 2>err &&
+	! grep "$message" err &&
+
+	test_must_fail git -C repo -c transfer.credentialsInUrl=warn push pwd-url HEAD:refs/heads/branch 2>err &&
+	grep "warning: $message" err >warnings &&
+	test_line_count = 2 warnings &&
+
+	git -C repo remote set-url --push pwd-url https://username:password@localhost &&
+	git -C repo remote set-url pwd-url https://localhost &&
+
+	test_must_fail git -C repo -c transfer.credentialsInUrl=warn push pwd-url HEAD:refs/heads/branch 2>err &&
+	! grep "warning: $message" err
+'
+
 test_done
-- 
2.36.1.1239.gfba91521d90

[RFC PATCH 3/5] CI: add a linux-BUILD-vars job

[Ævar Arnfjörð Bjarmason]

Add a linux-BUILD-vars job, in a preceding commit we fixed a bug that
would have been spotted by testing under NO_CURL=Y.

This CI job attempts to unset various settings in config.mak.uname and
the Makefile, so that we'll stress our fallbacks and conditionally
compiled code as much as possible.

If there is a missing setting here that we can enable under
"ubuntu-latest" the omission isn't intentional, this list came from a
quick skimming of the relevant parts of the Makefile and
config.mak.uname.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .github/workflows/main.yml |  3 +++
 ci/run-build-and-tests.sh  | 30 ++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 3fa88b78b6d..25263c6b17a 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -229,6 +229,9 @@ jobs:
             cc: gcc
             os: ubuntu
             cc_package: gcc-8
+          - jobname: linux-BUILD-vars
+            cc: gcc
+            os: ubuntu
             pool: ubuntu-latest
           - jobname: osx-clang
             cc: clang
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 8ebff425967..786285c5071 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -31,6 +31,36 @@ linux-TEST-vars)
 	export GIT_TEST_WRITE_REV_INDEX=1
 	export GIT_TEST_CHECKOUT_WORKERS=2
 	;;
+linux-BUILD-vars)
+	export NO_CURL=Y
+	export NO_PTHREADS=Y
+	export NO_GETTEXT=Y
+
+	# Undo settings in config.mak.uname
+	export HAVE_ALLOCA_H=
+
+	# Various compat/ fallbacks, with "FAIL" omitted if faking it
+	# doesn't work on Linux.
+	export NO_REGEX=Y
+	export NO_MEMMEM=Y
+	export INTERNAL_QSORT=Y
+	export SNPRINTF_RETURNS_BOGUS=Y
+	export FREAD_READS_DIRECTORIES=Y
+	export OPEN_RETURNS_EINTR=Y
+	export NO_HSTRERROR= # compat/hstrerror.c FAIL
+	export NO_POLL=Y
+	export NO_STRCASESTR=Y
+	export NO_STRTOUMAX=Y
+	export NO_SETENV=Y
+	export NO_UNSETENV=Y
+	export NO_MMAP=Y
+	export NO_PREAD=Y
+	export NEEDS_MODE_TRANSLATION= # compat/stat.c FAIL
+	export NO_IPV6=Y
+	export NO_INET_NTOP=Y
+	export NO_INET_PTON=Y
+	export NO_UNIX_SOCKETS=Y
+	;;
 linux-clang)
 	export GIT_TEST_DEFAULT_HASH=sha1
 	;;
-- 
2.36.1.1239.gfba91521d90

[RFC PATCH 4/5] 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>
---
 builtin/clone.c       |  5 ++++-
 builtin/fetch.c       |  4 ++++
 builtin/push.c        |  6 ++++-
 remote.c              | 51 +++++++++++++++++++++++++++++++++----------
 remote.h              | 14 ++++++++++++
 t/t5516-fetch-push.sh |  6 ++---
 t/t5601-clone.sh      |  2 +-
 7 files changed, 70 insertions(+), 18 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 89a91b00177..96a94621a09 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -886,12 +886,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 ac29c2b1ae3..bf67ef8c3d8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -2101,9 +2101,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 86b44f8aa71..6dd1b20dda0 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 42c891d44fd..61add35be2f 100644
--- a/remote.c
+++ b/remote.c
@@ -616,25 +616,43 @@ 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);
 
+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 };
 
@@ -648,16 +666,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 dd4402436f1..4ef359e4d4a 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.
@@ -441,4 +443,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 51d695e475a..c7a21d7cfb5 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1841,7 +1841,7 @@ test_expect_success '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 &&
@@ -1864,7 +1864,7 @@ test_expect_success CURL 'fetch warns or fails when using username:password in c
 
 	test_must_fail git -C repo -c transfer.credentialsInUrl=warn fetch pwd-url 2>err &&
 	grep "warning: $message" err >warnings &&
-	test_line_count = 3 warnings &&
+	test_line_count = 1 warnings &&
 
 	git -C repo remote set-url --push pwd-url https://username:password@localhost &&
 	git -C repo remote set-url pwd-url https://localhost &&
@@ -1899,7 +1899,7 @@ test_expect_success CURL 'push warns or fails when using username:password in co
 
 	test_must_fail git -C repo -c transfer.credentialsInUrl=warn push pwd-url HEAD:refs/heads/branch 2>err &&
 	grep "warning: $message" err >warnings &&
-	test_line_count = 2 warnings &&
+	test_line_count = 1 warnings &&
 
 	git -C repo remote set-url --push pwd-url https://username:password@localhost &&
 	git -C repo remote set-url pwd-url https://localhost &&
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index e6a248bbdcc..e449ccb54e3 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -78,7 +78,7 @@ test_expect_success '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.36.1.1239.gfba91521d90

[RFC PATCH 5/5] transport: check remote.<name>pushurl with transfer.credentialsInUrl

[Ævar Arnfjörð Bjarmason]

Expand the checks added in 6dcbdc0d661 (remote: create
fetch.credentialsInUrl config, 2022-06-06) to also check the
remote.<name>.pushurl setting. Before this it would only check the
remote.<name>.url setting, and would thus miss potential passwords in
the config.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 remote.c              | 63 ++++++++++++++++++++++++-------------------
 t/t5516-fetch-push.sh |  3 ++-
 2 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/remote.c b/remote.c
index 61add35be2f..d4dcc02a827 100644
--- a/remote.c
+++ b/remote.c
@@ -642,6 +642,37 @@ enum credentials_in_url get_credentials_in_url(void)
 	return cred_in_url;
 }
 
+static void validate_one_remote_url(enum credentials_in_url cfg,
+				    const char *url, struct strbuf *redacted)
+{
+	struct url_info url_info = { 0 };
+
+	if (!url_normalize(url, &url_info) || !url_info.passwd_off)
+		goto cleanup;
+
+	strbuf_reset(redacted);
+	strbuf_add(redacted, url_info.url, url_info.passwd_off);
+	strbuf_addstr(redacted, "<redacted>");
+	strbuf_addstr(redacted, url_info.url + url_info.passwd_off +
+		      url_info.passwd_len);
+
+	switch (cfg) {
+	case CRED_IN_URL_WARN:
+		warning(_("URL '%s' uses plaintext credentials"), redacted->buf);
+		break;
+	case CRED_IN_URL_DIE:
+		die(_("URL '%s' uses plaintext credentials"), redacted->buf);
+		break;
+	case CRED_IN_URL_ALLOW:
+	case CRED_IN_URL_UNKNOWN:
+		BUG("unreachable");
+		break;
+	}
+cleanup:
+	free(url_info.url);
+}
+
+
 static void validate_remote_url(struct remote *remote)
 {
 	int i;
@@ -653,34 +684,10 @@ static void validate_remote_url(struct remote *remote)
 	if (cfg == CRED_IN_URL_ALLOW)
 		goto done;
 
-	for (i = 0; i < remote->url_nr; i++) {
-		struct url_info url_info = { 0 };
-
-		if (!url_normalize(remote->url[i], &url_info) ||
-		    !url_info.passwd_off)
-			goto loop_cleanup;
-
-		strbuf_reset(&redacted);
-		strbuf_add(&redacted, url_info.url, url_info.passwd_off);
-		strbuf_addstr(&redacted, "<redacted>");
-		strbuf_addstr(&redacted,
-			      url_info.url + url_info.passwd_off + url_info.passwd_len);
-
-		switch (cfg) {
-		case CRED_IN_URL_WARN:
-			warning(_("URL '%s' uses plaintext credentials"), redacted.buf);
-			break;
-		case CRED_IN_URL_DIE:
-			die(_("URL '%s' uses plaintext credentials"), redacted.buf);
-			break;
-		case CRED_IN_URL_ALLOW:
-		case CRED_IN_URL_UNKNOWN:
-			BUG("unreachable");
-			break;
-		}
-	loop_cleanup:
-		free(url_info.url);
-	}
+	for (i = 0; i < remote->url_nr; i++)
+		validate_one_remote_url(cfg, remote->url[i], &redacted);
+	for (i = 0; i < remote->pushurl_nr; i++)
+		validate_one_remote_url(cfg, remote->pushurl[i], &redacted);
 
 	strbuf_release(&redacted);
 done:
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index c7a21d7cfb5..cdecc1c049c 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1905,7 +1905,8 @@ test_expect_success CURL 'push warns or fails when using username:password in co
 	git -C repo remote set-url pwd-url https://localhost &&
 
 	test_must_fail git -C repo -c transfer.credentialsInUrl=warn push pwd-url HEAD:refs/heads/branch 2>err &&
-	! grep "warning: $message" err
+	grep "warning: $message" err >warnings &&
+	test_line_count = 1 warnings
 '
 
 test_done
-- 
2.36.1.1239.gfba91521d90

Re: [RFC PATCH 2/5] fetch+push tests: add missing coverage for 6dcbdc0d661

[Derrick Stolee]

On 6/15/22 6:53 AM, Ævar Arnfjörð Bjarmason wrote:
> Add tests that were missing from 6dcbdc0d661 (remote: create
> fetch.credentialsInUrl config, 2022-06-06), we want to test how we
> handle cases where the config comes from a file, and that we handle
> "pushURL" correctly.

The tests use your renamed transfer.credentialsInUrl, but you haven't
renamed at this point.

> Currently the "pushURL" case isn't handled at all, i.e. URLs aren't
> warned about in "remote.*pushurl" , only for "remote.*.url".

Good find. I suppose that the second test_expect_success will _fail_
at the point of this patch?

Thanks,
-Stolee

Re: [RFC PATCH 3/5] CI: add a linux-BUILD-vars job

[Derrick Stolee]

On 6/15/22 6:53 AM, Ævar Arnfjörð Bjarmason wrote:
> Add a linux-BUILD-vars job, in a preceding commit we fixed a bug that
> would have been spotted by testing under NO_CURL=Y.

You'll want to double-check patch order before sending a full version
for review.

Also, why capitalize "BUILD"? I suppose the variables are all-caps,
but our job names are all lower case to this point, right?

> This CI job attempts to unset various settings in config.mak.uname and
> the Makefile, so that we'll stress our fallbacks and conditionally
> compiled code as much as possible.

I think it is a good idea to include a job that tries these "as
minimal as possible" builds.

> If there is a missing setting here that we can enable under
> "ubuntu-latest" the omission isn't intentional, this list came from a
> quick skimming of the relevant parts of the Makefile and
> config.mak.uname.

We can extend this in the future if we discover something new to add.
The important thing is that this moves us in the right direction.

Thanks,
-Stolee

Re: [RFC PATCH 0/5] fix issues in transfer.credentialsInUrl

[Derrick Stolee]

On 6/15/22 6:53 AM, Ævar Arnfjörð Bjarmason wrote:
> This is on top of [1], and given the "rc" phase is an RFC. This:
> 
>  * Fixes the issue of the transfer.credentialsInUrl (now renamed)

You are burying the lede here (and I had to look hard to see where
you renamed it in patch 4). I think this rename makes sense, as long
as we do that replacement before the release. Having a patch that
does only that rename as the first patch (and updates all docs, too)
would be helpful.

The doc updates should include the reference in the release notes,
too.

> not
>    finding passwords in "pushurl" URLs (in my case, the only place
>    where I'd actually put a password in a URL in a config...)
...
>  * 5/5 fixes the (major) blind spot of the warning missing "pushurl" config.

I think this is a valuable extension of the feature, and justifies
the rename. I'm mixed on whether we should add this before the
release or save it for the next cycle.

>  * 1/5 fixes a bug in an existing test, but I didn't think it was
>    worth bothering with for 2.37.0.

It's a good find, and a test fix is easy to do during the release
cycle, I think.

>  * Adds missing test coverage for reading the config from a file, not
>    the CLI.

Again, test coverage is good. No functionality needs to change.
However, one test added requires the pushurl change.

>  * 3/5 is a WIP CI target to spot the type of issue I fixed in [2],
>    it's not the first time where we have a NO_CURL=Y breakage land on
>    master...

I think that we should use CI to prevent these kinds of issues, so
I support adding this as well as leaving room for it to be changed
in the future if we notice other build issues that we can group into
this build.

>  * 4/5 attemps to "really" fix the duplicate warnings we emit, I think
>    the approach there is good, especially the part where we shouldn't
>    emit it twice in-process.
> 
>    But this currently misses e.g. "git ls-remote". I wonder if we
>    should just stick that git_config_push_parameter() condition into
>    packet_trace_identity() and call it a day.

I think these duplicate warning things should absolutely be left
until after the release. We do not have "warn" on by default, so it
will not disturb users who don't opt-in. We _should_ pursue this in
the next cycle, but with the time we can take to really be sure it
is the right approach to solving that problem.

> I think this is all non-RFC quality, except the "ls-remote" case, and
> us missing tests for that & other transport users that aren't
> clone/fetch/push.

There are some patch-order things that need to be cleaned up. I
agree that most of the code looks right.

> Derrick: Are you interested in picking this up & pursuing it after the
> release, with whatever fix-ups/rewrites etc. that you find
> appropriate?

I'm happy to review a new version of this series during the release
window that takes the high-priority items (renaming the config,
fixing tests, adding the CI build). I'd also be happy to review the
other changes as a follow-up.

Thanks,
-Stolee