{"thread":{"id":"58726","subject":"[PATCH 0/2] t5516/t5601: avoid using localhost for failing HTTPS requests","startedAt":"2022-10-31T19:47:26Z","lastAt":"2022-11-04T13:17:00Z","messageCount":45,"participants":["Johannes Schindelin via GitGitGadget","Ævar Arnfjörð Bjarmason","Jeff King","Taylor Blau","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"466108","messageId":"pull.1399.git.1667245638.gitgitgadget@gmail.com","threadId":"58726","inReplyTo":null,"subject":"[PATCH 0/2] t5516/t5601: avoid using localhost for failing HTTPS requests","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-31T19:47:16Z","receivedAt":"2022-10-31T19:47:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"While trying to figure out why the current CI builds fail in the Windows\njobs [https://github.com/git/git/actions/runs/3358829078#annotations], I\nnoticed that I cannot really run the offending test cases on my box because\nthe supposedly failing connection to https://localhost never seems to time\nout as intended.\n\nThis patch fixes that, and incidentally also seems to fix the CI failures.\n\nJohannes Schindelin (2):\n t5516/t5601: avoid using `localhost` for failing HTTPS requests\n t5516/t5601: be less strict about the number of credential warnings\n\n t/t5516-fetch-push.sh | 26 +++++++++++++-------------\n t/t5601-clone.sh | 18 +++++++++---------\n 2 files changed, 22 insertions(+), 22 deletions(-)\n\n\nbase-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1399%2Fdscho%2Favoid-using-localhost-in-url-tests-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1399/dscho/avoid-using-localhost-in-url-tests-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1399\n-- \ngitgitgadget\n"},{"id":"466109","messageId":"25cc0f6d91a9d23eb1b755e1463d672e4958a4e9.1667245639.git.gitgitgadget@gmail.com","threadId":"58726","inReplyTo":"pull.1399.git.1667245638.gitgitgadget@gmail.com","subject":"[PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-31T19:47:17Z","receivedAt":"2022-10-31T19:47:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 6dcbdc0d6616 (remote: create fetch.credentialsInUrl config,\n2022-06-06), we added four test cases that validate various behavior\naround passing credentials as part of the URL (which is considered\nunsafe in general).\n\nThese tests do not _actually_ try to connect anywhere, but have to use\nthe https:// protocol in order to validate the intended code paths.\n\nHowever, using `localhost` for such a connection causes several\nproblems:\n\n- There might be a web server running on localhost, and we do not\n actually want to connect to that.\n\n- The DNS resolver, or the local firewall, might take a substantial\n amount of time (or forever, whichever comes first) to fail to connect,\n slowing down the test cases unnecessarily.\n\nLet's instead use an IPv4 address that is guaranteed never to offer a\nweb server: 224.0.0.1 (which is part of the IP multicast range).\n\nIncidentally, this seems to fix an issue where the tests fail in the\nWindows jobs of Git's CI builds.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5516-fetch-push.sh | 18 +++++++++---------\n t/t5601-clone.sh | 10 +++++-----\n 2 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 79dc470c014..8dd4610a8c2 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1854,32 +1854,32 @@ test_expect_success 'refuse to push a hidden ref, and make sure do not pollute t\n '\n \n test_expect_success LIBCURL 'fetch warns or fails when using username:password' '\n-\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n-\ttest_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&\n+\tmessage=\"URL '\\''https://username:@224.0.0.1/'\\'' uses plaintext credentials\" &&\n+\ttest_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err &&\n \t! grep \"$message\" err &&\n \n-\ttest_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@localhost 2>err &&\n+\ttest_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@224.0.0.1 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n \ttest_line_count = 3 warnings &&\n \n-\ttest_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@localhost 2>err &&\n+\ttest_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@224.0.0.1 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n \ttest_line_count = 1 warnings &&\n \n-\ttest_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@localhost 2>err &&\n+\ttest_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@224.0.0.1 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n \ttest_line_count = 1 warnings\n '\n \n \n test_expect_success LIBCURL 'push warns or fails when using username:password' '\n-\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n-\ttest_must_fail git -c transfer.credentialsInUrl=allow push https://username:password@localhost 2>err &&\n+\tmessage=\"URL '\\''https://username:@224.0.0.1/'\\'' uses plaintext credentials\" &&\n+\ttest_must_fail git -c transfer.credentialsInUrl=allow push https://username:password@224.0.0.1 2>err &&\n \t! grep \"$message\" err &&\n \n-\ttest_must_fail git -c transfer.credentialsInUrl=warn push https://username:password@localhost 2>err &&\n+\ttest_must_fail git -c transfer.credentialsInUrl=warn push https://username:password@224.0.0.1 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n-\ttest_must_fail git -c transfer.credentialsInUrl=die push https://username:password@localhost 2>err &&\n+\ttest_must_fail git -c transfer.credentialsInUrl=die push https://username:password@224.0.0.1 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n \ttest_line_count = 1 warnings\n '\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 45f0803ed4d..0b386c74818 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -72,19 +72,19 @@ test_expect_success 'clone respects GIT_WORK_TREE' '\n '\n \n test_expect_success LIBCURL 'clone warns or fails when using username:password' '\n-\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n-\ttest_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&\n+\tmessage=\"URL '\\''https://username:@224.0.0.1/'\\'' uses plaintext credentials\" &&\n+\ttest_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@224.0.0.1 attempt1 2>err &&\n \t! grep \"$message\" err &&\n \n-\ttest_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&\n+\ttest_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@224.0.0.1 attempt2 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n \ttest_line_count = 2 warnings &&\n \n-\ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&\n+\ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@224.0.0.1 attempt3 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n \ttest_line_count = 1 warnings &&\n \n-\ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:@localhost attempt3 2>err &&\n+\ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:@224.0.0.1 attempt3 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n \ttest_line_count = 1 warnings\n '\n-- \ngitgitgadget\n\n"},{"id":"466110","messageId":"98fa5270cb720f2f038c4bb9571c7d306ff5d759.1667245639.git.gitgitgadget@gmail.com","threadId":"58726","inReplyTo":"pull.1399.git.1667245638.gitgitgadget@gmail.com","subject":"[PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-31T19:47:18Z","receivedAt":"2022-10-31T19:47:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt is unclear as to _why_, but under certain circumstances the warning\nabout credentials being passed as part of the URL seems to be swallowed\nby the `git remote-https` helper in the Windows jobs of Git's CI builds.\n\nSince it is not actually important how many times Git prints the\nwarning/error message, as long as it prints it at least once, let's just\nmake the test a bit more lenient and test for the latter instead of the\nformer, which works around these CI issues.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5516-fetch-push.sh | 8 ++++----\n t/t5601-clone.sh | 8 ++++----\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 8dd4610a8c2..980c594940b 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1860,15 +1860,15 @@ test_expect_success LIBCURL 'fetch warns or fails when using username:password'\n \n \ttest_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@224.0.0.1 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n-\ttest_line_count = 3 warnings &&\n+\ttest_line_count -ge 1 warnings &&\n \n \ttest_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@224.0.0.1 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings &&\n+\ttest_line_count -ge 1 warnings &&\n \n \ttest_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@224.0.0.1 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings\n+\ttest_line_count -ge 1 warnings\n '\n \n \n@@ -1881,7 +1881,7 @@ test_expect_success LIBCURL 'push warns or fails when using username:password' '\n \tgrep \"warning: $message\" err >warnings &&\n \ttest_must_fail git -c transfer.credentialsInUrl=die push https://username:password@224.0.0.1 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings\n+\ttest_line_count -ge 1 warnings\n '\n \n test_expect_success 'push with config push.useBitmaps' '\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 0b386c74818..b72cdeb6243 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -78,19 +78,19 @@ test_expect_success LIBCURL 'clone warns or fails when using username:password'\n \n \ttest_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@224.0.0.1 attempt2 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n-\ttest_line_count = 2 warnings &&\n+\ttest_line_count -ge 1 warnings &&\n \n \ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@224.0.0.1 attempt3 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings &&\n+\ttest_line_count -ge 1 warnings &&\n \n \ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:@224.0.0.1 attempt3 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings\n+\ttest_line_count -ge 1 warnings\n '\n \n test_expect_success LIBCURL 'clone does not detect username:password when it is https://username@domain:port/' '\n-\ttest_must_fail git -c transfer.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&\n+\ttest_must_fail git -c transfer.credentialsInUrl=warn clone https://username@224.0.0.1:8080 attempt3 2>err &&\n \t! grep \"uses plaintext credentials\" err\n '\n \n-- \ngitgitgadget\n"},{"id":"466117","messageId":"RFC-patch-1.1-0266485bc6c-20221031T204149Z-avarab@gmail.com","threadId":"58726","inReplyTo":"pull.1399.git.1667245638.gitgitgadget@gmail.com","subject":"[RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-31T20:47:08Z","receivedAt":"2022-10-31T20:47:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Improve on 6dcbdc0d661 (remote: create fetch.credentialsInUrl config,\n2022-06-06) by fixing the cases where we emit duplicate warnings,\nwhich were:\n\n * In the same process, as we'd get the same \"struct remote *\"\n again. We could probably save ourselves more work in those scenarios,\n but adding a flag to the \"struct remote\" indicating that we've validated\n the URLs will fix those issues.\n\n * When we invoke e.g. \"git-remote-https\" (aka. \"git-remote-curl\")\n from \"git fetch\". For those cases let's pass down the equivalent of a\n \"-c transfer.credentialsInUrl=allow\", since we know that we've already\n inspected our remotes in the parent process.\n\n See 7390f05a3c6 (fetch: after refetch, encourage auto gc repacking,\n 2022-03-28) for prior use of git_config_push_parameter() for this\n purpose, i.e. to push config parameters before invoking a\n sub-process.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\n[Grabbing a quote from\nhttps://lore.kernel.org/git/98fa5270cb720f2f038c4bb9571c7d306ff5d759.1667245639.git.gitgitgadget@gmail.com/\nfor a reply]\n\n> From: Johannes Schindelin \n>\n> It is unclear as to _why_, but under certain circumstances the warning\n> about credentials being passed as part of the URL seems to be swallowed\n> by the `git remote-https` helper in the Windows jobs of Git's CI builds.\n\nI think this should fix the root cause of the issue you're seeing. I\nhave a larger local branch to fix various issues with this\ncredentialsInUrl facility that I hadn't gotten around to submitting:\n\n\thttps://github.com/git/git/compare/master...avar:avar/fix-cred-in-url-warnings-2\n\nThis is a cherry-pick of 7/* of that (the first were doc changes,\nmissing test coverage etc).\n\n> Since it is not actually important how many times Git prints the\n> warning/error message, as long as it prints it at least once, let's just\n> make the test a bit more lenient and test for the latter instead of the\n> former, which works around these CI issues.\n\nThat being said your change is obviously smaller, so if we'd prefer\nthat in first as a band-aid I'm fine with that.\n\nBut I'd really like to object to the \"it is not actually important how\nmany...\", it's crappy UX to spew duplicate output at the user, and we\nshould avoid it.\n\nSo it does matter, and we get it wrong not just in this but also some\nother cases.\n\n builtin/clone.c | 5 +++-\n builtin/fetch.c | 4 ++++\n builtin/push.c | 6 ++++-\n remote.c | 53 ++++++++++++++++++++++++++++++++-----------\n remote.h | 14 ++++++++++++\n t/t5516-fetch-push.sh | 2 +-\n t/t5601-clone.sh | 2 +-\n 7 files changed, 69 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 547d6464b3c..da219b74e43 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -903,12 +903,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tint err = 0, complete_refs_before_fetch = 1;\n \tint submodule_progress;\n \tint filter_submodules = 0;\n-\n+\tenum credentials_in_url cred_in_url_cfg = get_credentials_in_url();\n \tstruct transport_ls_refs_options transport_ls_refs_options =\n \t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n \n \tpacket_trace_identity(\"clone\");\n \n+\tif (cred_in_url_cfg == CRED_IN_URL_WARN)\n+\t\tgit_config_push_parameter(\"transfer.credentialsInUrl=allow\");\n+\n \tgit_config(git_clone_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, builtin_clone_options,\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b06e454cbdd..34a10e1927f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2110,9 +2110,13 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tstruct remote *remote = NULL;\n \tint result = 0;\n \tint prune_tags_ok = 1;\n+\tenum credentials_in_url cred_in_url_cfg = get_credentials_in_url();\n \n \tpacket_trace_identity(\"fetch\");\n \n+\tif (cred_in_url_cfg == CRED_IN_URL_WARN)\n+\t\tgit_config_push_parameter(\"transfer.credentialsInUrl=allow\");\n+\n \t/* Record the command line for the reflog */\n \tstrbuf_addstr(&default_rla, \"fetch\");\n \tfor (i = 1; i < argc; i++) {\ndiff --git a/builtin/push.c b/builtin/push.c\nindex f0329c62a2d..a4890b1586d 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -576,7 +576,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tstruct string_list *push_options;\n \tconst struct string_list_item *item;\n \tstruct remote *remote;\n-\n+\tenum credentials_in_url cred_in_url_cfg = get_credentials_in_url();\n \tstruct option options[] = {\n \t\tOPT__VERBOSITY(&verbosity),\n \t\tOPT_STRING( 0 , \"repo\", &repo, N_(\"repository\"), N_(\"repository\")),\n@@ -619,6 +619,10 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t};\n \n \tpacket_trace_identity(\"push\");\n+\n+\tif (cred_in_url_cfg == CRED_IN_URL_WARN)\n+\t\tgit_config_push_parameter(\"transfer.credentialsInUrl=allow\");\n+\n \tgit_config(git_push_config, &flags);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n \tpush_options = (push_options_cmdline.nr\ndiff --git a/remote.c b/remote.c\nindex 60869beebe7..a35da349629 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -615,24 +615,42 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)\n \treturn NULL;\n }\n \n-static void validate_remote_url(struct remote *remote)\n+static enum credentials_in_url cred_in_url = CRED_IN_URL_UNKNOWN;\n+enum credentials_in_url get_credentials_in_url(void)\n {\n-\tint i;\n+\tenum credentials_in_url new = CRED_IN_URL_ALLOW;\n \tconst char *value;\n-\tstruct strbuf redacted = STRBUF_INIT;\n-\tint warn_not_die;\n+\n+\tif (cred_in_url != CRED_IN_URL_UNKNOWN)\n+\t\treturn cred_in_url;\n \n \tif (git_config_get_string_tmp(\"transfer.credentialsinurl\", &value))\n-\t\treturn;\n+\t\tgoto done;\n \n \tif (!strcmp(\"warn\", value))\n-\t\twarn_not_die = 1;\n+\t\tnew = CRED_IN_URL_WARN;\n \telse if (!strcmp(\"die\", value))\n-\t\twarn_not_die = 0;\n+\t\tnew = CRED_IN_URL_DIE;\n \telse if (!strcmp(\"allow\", value))\n-\t\treturn;\n+\t\tgoto done;\n \telse\n-\t\tdie(_(\"unrecognized value transfer.credentialsInUrl: '%s'\"), value);\n+\t\tdie(_(\"unrecognized value transfer.credentialsInURL: '%s'\"), value);\n+\n+done:\n+\tcred_in_url = new;\n+\treturn cred_in_url;\n+}\n+\n+static void validate_remote_url(struct remote *remote)\n+{\n+\tint i;\n+\tstruct strbuf redacted = STRBUF_INIT;\n+\tenum credentials_in_url cfg = get_credentials_in_url();\n+\n+\tif (remote->validated_urls)\n+\t\tgoto done;\n+\tif (cfg == CRED_IN_URL_ALLOW)\n+\t\tgoto done;\n \n \tfor (i = 0; i < remote->url_nr; i++) {\n \t\tstruct url_info url_info = { 0 };\n@@ -647,16 +665,25 @@ static void validate_remote_url(struct remote *remote)\n \t\tstrbuf_addstr(&redacted,\n \t\t\t url_info.url + url_info.passwd_off + url_info.passwd_len);\n \n-\t\tif (warn_not_die)\n+\t\tswitch (cfg) {\n+\t\tcase CRED_IN_URL_WARN:\n \t\t\twarning(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n-\t\telse\n+\t\t\tbreak;\n+\t\tcase CRED_IN_URL_DIE:\n \t\t\tdie(_(\"URL '%s' uses plaintext credentials\"), redacted.buf);\n-\n-loop_cleanup:\n+\t\t\tbreak;\n+\t\tcase CRED_IN_URL_ALLOW:\n+\t\tcase CRED_IN_URL_UNKNOWN:\n+\t\t\tBUG(\"unreachable\");\n+\t\t\tbreak;\n+\t\t}\n+\tloop_cleanup:\n \t\tfree(url_info.url);\n \t}\n \n \tstrbuf_release(&redacted);\n+done:\n+\tremote->validated_urls = 1;\n }\n \n static struct remote *\ndiff --git a/remote.h b/remote.h\nindex 1c4621b414b..5a2da13b4cb 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -98,6 +98,8 @@ struct remote {\n \tint prune;\n \tint prune_tags;\n \n+\tint validated_urls;\n+\n \t/**\n \t * The configured helper programs to run on the remote side, for\n \t * Git-native protocols.\n@@ -445,4 +447,16 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);\n char *relative_url(const char *remote_url, const char *url,\n \t\t const char *up_path);\n \n+enum credentials_in_url {\n+\tCRED_IN_URL_UNKNOWN,\n+\tCRED_IN_URL_ALLOW,\n+\tCRED_IN_URL_WARN,\n+\tCRED_IN_URL_DIE,\n+};\n+\n+/**\n+ * Get the transfer.credentialsInUrl config setting as an \"enum\n+ * credentials_in_url\".\n+ */\n+enum credentials_in_url get_credentials_in_url(void);\n #endif\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 79dc470c014..d01f5cd349f 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1860,7 +1860,7 @@ test_expect_success LIBCURL 'fetch warns or fails when using username:password'\n \n \ttest_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@localhost 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n-\ttest_line_count = 3 warnings &&\n+\ttest_line_count = 1 warnings &&\n \n \ttest_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@localhost 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 45f0803ed4d..37adfd9f83b 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -78,7 +78,7 @@ test_expect_success LIBCURL 'clone warns or fails when using username:password'\n \n \ttest_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n-\ttest_line_count = 2 warnings &&\n+\ttest_line_count = 1 warnings &&\n \n \ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n-- \n2.38.0.1280.g8136eb6fab2\n\n"},{"id":"466118","messageId":"221031.86v8nz67jn.gmgdl@evledraar.gmail.com","threadId":"58726","inReplyTo":"25cc0f6d91a9d23eb1b755e1463d672e4958a4e9.1667245639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-31T20:49:55Z","receivedAt":"2022-10-31T20:51:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 31 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> In 6dcbdc0d6616 (remote: create fetch.credentialsInUrl config,\n> 2022-06-06), we added four test cases that validate various behavior\n> around passing credentials as part of the URL (which is considered\n> unsafe in general).\n>\n> These tests do not _actually_ try to connect anywhere, but have to use\n> the https:// protocol in order to validate the intended code paths.\n>\n> However, using `localhost` for such a connection causes several\n> problems:\n>\n> - There might be a web server running on localhost, and we do not\n> actually want to connect to that.\n>\n> - The DNS resolver, or the local firewall, might take a substantial\n> amount of time (or forever, whichever comes first) to fail to connect,\n> slowing down the test cases unnecessarily.\n>\n> Let's instead use an IPv4 address that is guaranteed never to offer a\n> web server: 224.0.0.1 (which is part of the IP multicast range).\n>\n> Incidentally, this seems to fix an issue where the tests fail in the\n> Windows jobs of Git's CI builds.\n> [...]\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 45f0803ed4d..0b386c74818 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -72,19 +72,19 @@ test_expect_success 'clone respects GIT_WORK_TREE' '\n> '\n> \n> test_expect_success LIBCURL 'clone warns or fails when using username:password' '\n> -\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n> -\ttest_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&\n> +\tmessage=\"URL '\\''https://username:@224.0.0.1/'\\'' uses plaintext credentials\" &&\n> +\ttest_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@224.0.0.1 attempt1 2>err &&\n> \t! grep \"$message\" err &&\n> \n> -\ttest_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&\n> +\ttest_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@224.0.0.1 attempt2 2>err &&\n> \tgrep \"warning: $message\" err >warnings &&\n> \ttest_line_count = 2 warnings &&\n> \n> -\ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&\n> +\ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@224.0.0.1 attempt3 2>err &&\n> \tgrep \"fatal: $message\" err >warnings &&\n> \ttest_line_count = 1 warnings &&\n> \n> -\ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:@localhost attempt3 2>err &&\n> +\ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:@224.0.0.1 attempt3 2>err &&\n> \tgrep \"fatal: $message\" err >warnings &&\n> \ttest_line_count = 1 warnings\n> '\n\nFor this one one at least, it eventually gets around to setting up an\nactual httpd server with cloning etc. from $HTTPD_URL.\n\nCan't we just do that for both of these tests rather than the the\n224.0.0.0 hack? I.e. the root cause is that we're cleverly faking a\nnot-a-server here, and now we're going to add another somewhat clever\nhack on top.\n\nbut since the test coverage is for https:// anyway, and we have other\nhttps tests against an actual server...\n"},{"id":"466132","messageId":"Y2BYKxxkG57XAV/1@coredump.intra.peff.net","threadId":"58726","inReplyTo":"25cc0f6d91a9d23eb1b755e1463d672e4958a4e9.1667245639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-31T23:20:11Z","receivedAt":"2022-10-31T23:20:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 31, 2022 at 07:47:17PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> In 6dcbdc0d6616 (remote: create fetch.credentialsInUrl config,\n> 2022-06-06), we added four test cases that validate various behavior\n> around passing credentials as part of the URL (which is considered\n> unsafe in general).\n> \n> These tests do not _actually_ try to connect anywhere, but have to use\n> the https:// protocol in order to validate the intended code paths.\n\nBy \"actually\" here, I assume you mean \"they do not expect to succeed\".\nBut I think the first one (with credentialsInUrl=allow), does try to\nmake a connection.\n\n> However, using `localhost` for such a connection causes several\n> problems:\n> \n> - There might be a web server running on localhost, and we do not\n> actually want to connect to that.\n> \n> - The DNS resolver, or the local firewall, might take a substantial\n> amount of time (or forever, whichever comes first) to fail to connect,\n> slowing down the test cases unnecessarily.\n\nRight. I think we assume that DNS resolution of localhost is fast-ish,\nas we use it in other https tests. But I could certainly imagine a local\nfirewall causing issues (especially as this is real port 443, whereas\nour other tests are usually high ports).\n\n> Let's instead use an IPv4 address that is guaranteed never to offer a\n> web server: 224.0.0.1 (which is part of the IP multicast range).\n\nThis feels pretty magical. I think it would be pretty unlikely for it to\nhave a web server, but I wouldn't be surprised if there are systems\nwhere we get similar IP-routing hangs.\n\nIs there a reason not to move all of these tests into t5550 or t5551,\nwhere we have a real http server? That would be less magical, and then\nthis first test:\n\n> test_expect_success LIBCURL 'fetch warns or fails when using username:password' '\n> -\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n> -\ttest_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&\n> +\tmessage=\"URL '\\''https://username:@224.0.0.1/'\\'' uses plaintext credentials\" &&\n> +\ttest_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err &&\n> \t! grep \"$message\" err &&\n\ncould be more robust. It would actually check that we succeeded in using\nthe URL.\n\n-Peff\n"},{"id":"466133","messageId":"Y2BYyB3H9n2LDo/E@coredump.intra.peff.net","threadId":"58726","inReplyTo":"98fa5270cb720f2f038c4bb9571c7d306ff5d759.1667245639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-31T23:22:48Z","receivedAt":"2022-10-31T23:23:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 31, 2022 at 07:47:18PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> It is unclear as to _why_, but under certain circumstances the warning\n> about credentials being passed as part of the URL seems to be swallowed\n> by the `git remote-https` helper in the Windows jobs of Git's CI builds.\n> \n> Since it is not actually important how many times Git prints the\n> warning/error message, as long as it prints it at least once, let's just\n> make the test a bit more lenient and test for the latter instead of the\n> former, which works around these CI issues.\n\nThis makes sense to me. Regardless of whether we actually fix the\nmultiple outputs (which are user-visible and kind of ugly), I don't\nthink there's any reason for our tests to assert the somewhat\nundesirable output. So this can proceed independently of any fix.\n\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 8dd4610a8c2..980c594940b 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1860,15 +1860,15 @@ test_expect_success LIBCURL 'fetch warns or fails when using username:password'\n> \n> \ttest_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@224.0.0.1 2>err &&\n> \tgrep \"warning: $message\" err >warnings &&\n> -\ttest_line_count = 3 warnings &&\n> +\ttest_line_count -ge 1 warnings &&\n\nI think this test_line_count (and all the others) is now superfluous;\nthe exit code of grep will tell us if we found anything.\n\nI don't mind it too much, though, if we're planning to fix the duplicate\nmessages, at which point it becomes s/-ge/=/.\n\n-Peff\n"},{"id":"466144","messageId":"Y2BvCMlWNKnKR6OE@nand.local","threadId":"58726","inReplyTo":"Y2BYyB3H9n2LDo/E@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-01T00:57:44Z","receivedAt":"2022-11-01T00:57:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 31, 2022 at 07:22:48PM -0400, Jeff King wrote:\n> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> > index 8dd4610a8c2..980c594940b 100755\n> > --- a/t/t5516-fetch-push.sh\n> > +++ b/t/t5516-fetch-push.sh\n> > @@ -1860,15 +1860,15 @@ test_expect_success LIBCURL 'fetch warns or fails when using username:password'\n> >\n> > \ttest_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@224.0.0.1 2>err &&\n> > \tgrep \"warning: $message\" err >warnings &&\n> > -\ttest_line_count = 3 warnings &&\n> > +\ttest_line_count -ge 1 warnings &&\n>\n> I think this test_line_count (and all the others) is now superfluous;\n> the exit code of grep will tell us if we found anything.\n>\n> I don't mind it too much, though, if we're planning to fix the duplicate\n> messages, at which point it becomes s/-ge/=/.\n\nYeah, agreed. Let's leave it alone, unless somebody else feels strongly.\n\nThanks,\nTaylor\n"},{"id":"466145","messageId":"Y2BvaXN2C325YUsz@nand.local","threadId":"58726","inReplyTo":"Y2BYKxxkG57XAV/1@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-01T00:59:21Z","receivedAt":"2022-11-01T00:59:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 31, 2022 at 07:20:11PM -0400, Jeff King wrote:\n> > Let's instead use an IPv4 address that is guaranteed never to offer a\n> > web server: 224.0.0.1 (which is part of the IP multicast range).\n>\n> This feels pretty magical. I think it would be pretty unlikely for it to\n> have a web server, but I wouldn't be surprised if there are systems\n> where we get similar IP-routing hangs.\n>\n> Is there a reason not to move all of these tests into t5550 or t5551,\n> where we have a real http server? That would be less magical, and then\n> this first test:\n>\n> > test_expect_success LIBCURL 'fetch warns or fails when using username:password' '\n> > -\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n> > -\ttest_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&\n> > +\tmessage=\"URL '\\''https://username:@224.0.0.1/'\\'' uses plaintext credentials\" &&\n> > +\ttest_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err &&\n> > \t! grep \"$message\" err &&\n>\n> could be more robust. It would actually check that we succeeded in using\n> the URL.\n\nIt is magical, indeed. If it's significantly more difficult to move\nthese into t5550 or t5551, then I'm OK with this in the meantime (since\nGitHub Actions really is our primary CI target that we care about this\nnot hanging on).\n\nBut assuming that moving these around isn't that difficult, I would be\nslightly happier to see these tests in one of the aforementioned spots.\n\nThanks,\nTaylor\n"},{"id":"466146","messageId":"Y2BxFxZqA9GA40AB@nand.local","threadId":"58726","inReplyTo":"RFC-patch-1.1-0266485bc6c-20221031T204149Z-avarab@gmail.com","subject":"Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-01T01:06:31Z","receivedAt":"2022-11-01T01:06:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 31, 2022 at 09:47:08PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> Improve on 6dcbdc0d661 (remote: create fetch.credentialsInUrl config,\n> 2022-06-06) by fixing the cases where we emit duplicate warnings,\n> which were:\n>\n> * In the same process, as we'd get the same \"struct remote *\"\n> again. We could probably save ourselves more work in those scenarios,\n> but adding a flag to the \"struct remote\" indicating that we've validated\n> the URLs will fix those issues.\n>\n> * When we invoke e.g. \"git-remote-https\" (aka. \"git-remote-curl\")\n> from \"git fetch\". For those cases let's pass down the equivalent of a\n> \"-c transfer.credentialsInUrl=allow\", since we know that we've already\n> inspected our remotes in the parent process.\n>\n> See 7390f05a3c6 (fetch: after refetch, encourage auto gc repacking,\n> 2022-03-28) for prior use of git_config_push_parameter() for this\n> purpose, i.e. to push config parameters before invoking a\n> sub-process.\n\nBoth make sense. I was skimming the diff before I read the patch message\nhere and was scratching my head at the calls to\ngit_config_push_parameter().\n\nBut for crossing process boundaries, that makes sense.\n\nThanks,\nTaylor\n"},{"id":"466165","messageId":"Y2B+jn4goN4OIcwL@coredump.intra.peff.net","threadId":"58726","inReplyTo":"Y2BYKxxkG57XAV/1@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T02:03:58Z","receivedAt":"2022-11-01T02:04:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 31, 2022 at 07:20:11PM -0400, Jeff King wrote:\n\n> > - The DNS resolver, or the local firewall, might take a substantial\n> > amount of time (or forever, whichever comes first) to fail to connect,\n> > slowing down the test cases unnecessarily.\n> \n> Right. I think we assume that DNS resolution of localhost is fast-ish,\n> as we use it in other https tests. But I could certainly imagine a local\n> firewall causing issues (especially as this is real port 443, whereas\n> our other tests are usually high ports).\n\nActually, I am wrong about DNS here. We use a bare 127.0.0.1 in\nlib-httpd.sh, so DNS may indeed be the source of the issue here. That\n_might_ mean that using the bare IP would be safe here, but I would not\nwant to bet on it. Using different port numbers, plus not expecting a\nlistener on the other end, might cause different outcomes than we see in\nthe other tests.\n\nI do think moving these tests to use a real http server is a better\nsolution, though. I'll provide a patch in a moment.\n\n-Peff\n"},{"id":"466167","messageId":"Y2CDrZNgNZ+flJBK@coredump.intra.peff.net","threadId":"58726","inReplyTo":"Y2B+jn4goN4OIcwL@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T02:25:49Z","receivedAt":"2022-11-01T02:25:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 31, 2022 at 10:03:58PM -0400, Jeff King wrote:\n\n> I do think moving these tests to use a real http server is a better\n> solution, though. I'll provide a patch in a moment.\n\nHere that is. I hope this isn't co-opting your series; my goal was to do\nthe work so that you wouldn't have to, but I admit it is assuming you\nagree with my approach. ;)\n\nThe first patch is the interesting one. The second one is just your 2/2\nrebased onto the new location.\n\n [1/2]: t5516: move plaintext-password tests from t5601 and t5516\n [2/2]: t5516/t5601: be less strict about the number of credential warnings\n\n t/t5516-fetch-push.sh | 31 ---------------\n t/t5551-http-fetch-smart.sh | 77 +++++++++++++++++++++++++++++++++++++\n t/t5601-clone.sh | 23 -----------\n 3 files changed, 77 insertions(+), 54 deletions(-)\n\n-Peff\n"},{"id":"466168","messageId":"Y2CD4g9z2yVFA06l@coredump.intra.peff.net","threadId":"58726","inReplyTo":"Y2CDrZNgNZ+flJBK@coredump.intra.peff.net","subject":"[PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T02:26:42Z","receivedAt":"2022-11-01T02:26:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 6dcbdc0d66 (remote: create fetch.credentialsInUrl config,\n2022-06-06) added tests for our handling of passwords in URLs. Since the\nobvious URL to be affected is git-over-http, the tests use http. However\nthey don't set up a test server; they just try to access\nhttps://localhost, assuming it will fail (because the nothing is\nlistening there).\n\nThis causes some possible problems:\n\n - There might be a web server running on localhost, and we do not\n actually want to connect to that.\n\n - The DNS resolver, or the local firewall, might take a substantial\n amount of time (or forever, whichever comes first) to fail to\n connect, slowing down the tests cases unnecessarily.\n\n - Since there's no server, our tests for \"allow\" and \"warn\" still\n expect the clone/fetch/push operations to fail, even though in the\n real world we'd expect these to succeed. We scrape stderr to see\n what happened, but it's not as robust as a more realistic test.\n\nLet's instead move these to t5551, which is all about testing http and\nwhere we have a real server. That eliminates any issues with contacting\na strange URL, and lets the \"allow\" and \"warn\" tests confirm that the\noperation actually succeeds.\n\nIt's not quite a verbatim move for a few reasons:\n\n - we can drop the LIBCURL dependency; it's already part of\n lib-httpd.sh\n\n - we'll use HTTPD_URL_USER_PASS, etc, instead of our fake URL. To\n avoid repetition, we'll add a few extra variables.\n\n - the \"https://username:@localhost\" test uses a funny URL that\n lib-httpd.sh doesn't provide. We'll similarly construct it in a\n variable. Note that we're hard-coding the lib-httpd username here,\n but t5551 already does that everywhere.\n\n - for the \"domain:port\" test, the URL provided by lib-httpd is fine,\n since our test server will always be on an exotic port. But we'll\n confirm in the test that this is so.\n\n - since our message-matching is done via grep, I simplified it to use\n a regex, rather than trying to massage lib-httpd's variables.\n Arguably this makes it more readable, too, while retaining the bits\n we care about: the fatal/warning distinction, the \"uses plaintext\"\n message, and the fact that the password was redacted.\n\n - we'll use the /auth/ path for the repo, which shows that we are\n indeed making use of the auth information when needed.\n\n - we'll also use /smart/; most of these tests could be done via /dumb/\n in t5550, but setting up pushes there requires extra effort and\n dependencies. The smart protocol is what most everyone is using\n these days anyway.\n\nThis patch is my own, but I stole the analysis and a few bits of the\ncommit message from a patch by Johannes Schindelin.\n\nSigned-off-by: Jeff King \n---\n t/t5516-fetch-push.sh | 31 ---------------\n t/t5551-http-fetch-smart.sh | 77 +++++++++++++++++++++++++++++++++++++\n t/t5601-clone.sh | 23 -----------\n 3 files changed, 77 insertions(+), 54 deletions(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 79dc470c01..4f2bfaf005 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1853,37 +1853,6 @@ test_expect_success 'refuse to push a hidden ref, and make sure do not pollute t\n \ttest_dir_is_empty testrepo/.git/objects/pack\n '\n \n-test_expect_success LIBCURL 'fetch warns or fails when using username:password' '\n-\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n-\ttest_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&\n-\t! grep \"$message\" err &&\n-\n-\ttest_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@localhost 2>err &&\n-\tgrep \"warning: $message\" err >warnings &&\n-\ttest_line_count = 3 warnings &&\n-\n-\ttest_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@localhost 2>err &&\n-\tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings &&\n-\n-\ttest_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@localhost 2>err &&\n-\tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings\n-'\n-\n-\n-test_expect_success LIBCURL 'push warns or fails when using username:password' '\n-\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n-\ttest_must_fail git -c transfer.credentialsInUrl=allow push https://username:password@localhost 2>err &&\n-\t! grep \"$message\" err &&\n-\n-\ttest_must_fail git -c transfer.credentialsInUrl=warn push https://username:password@localhost 2>err &&\n-\tgrep \"warning: $message\" err >warnings &&\n-\ttest_must_fail git -c transfer.credentialsInUrl=die push https://username:password@localhost 2>err &&\n-\tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings\n-'\n-\n test_expect_success 'push with config push.useBitmaps' '\n \tmk_test testrepo heads/main &&\n \tgit checkout main &&\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 6a38294a47..bbe3d5721f 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -580,4 +580,81 @@ test_expect_success 'passing hostname resolution information works' '\n \tgit -c \"http.curloptResolve=$BOGUS_HOST:$LIB_HTTPD_PORT:127.0.0.1\" ls-remote \"$BOGUS_HTTPD_URL/smart/repo.git\" >/dev/null\n '\n \n+# here user%40host is the URL-encoded version of user@host,\n+# which is our intentionally-odd username to catch parsing errors\n+url_user=$HTTPD_URL_USER/auth/smart/repo.git\n+url_userpass=$HTTPD_URL_USER_PASS/auth/smart/repo.git\n+url_userblank=$HTTPD_PROTO://user%40host:@$HTTPD_DEST/auth/smart/repo.git\n+message=\"URL .*:@.* uses plaintext credentials\"\n+\n+test_expect_success 'clone warns or fails when using username:password' '\n+\ttest_when_finished \"rm -rf attempt*\" &&\n+\n+\tgit -c transfer.credentialsInUrl=allow \\\n+\t\tclone $url_userpass attempt1 2>err &&\n+\t! grep \"$message\" err &&\n+\n+\tgit -c transfer.credentialsInUrl=warn \\\n+\t\tclone $url_userpass attempt2 2>err &&\n+\tgrep \"warning: $message\" err >warnings &&\n+\ttest_line_count = 2 warnings &&\n+\n+\ttest_must_fail git -c transfer.credentialsInUrl=die \\\n+\t\tclone $url_userpass attempt3 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings &&\n+\n+\ttest_must_fail git -c transfer.credentialsInUrl=die \\\n+\t\tclone $url_userblank attempt4 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings\n+'\n+\n+test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '\n+\ttest_when_finished \"rm -rf attempt1\" &&\n+\n+\t# we are relying on lib-httpd for url construction, so document our\n+\t# assumptions\n+\tcase \"$HTTPD_URL_USER\" in\n+\t*:[0-9]*) : ok ;;\n+\t*) BUG \"httpd url does not have port: $HTTPD_URL_USER\"\n+\tesac &&\n+\n+\tgit -c transfer.credentialsInUrl=warn clone $url_user attempt1 2>err &&\n+\t! grep \"uses plaintext credentials\" err\n+'\n+\n+test_expect_success 'fetch warns or fails when using username:password' '\n+\tgit -c transfer.credentialsInUrl=allow fetch $url_userpass 2>err &&\n+\t! grep \"$message\" err &&\n+\n+\tgit -c transfer.credentialsInUrl=warn fetch $url_userpass 2>err &&\n+\tgrep \"warning: $message\" err >warnings &&\n+\ttest_line_count = 3 warnings &&\n+\n+\ttest_must_fail git -c transfer.credentialsInUrl=die \\\n+\t\tfetch $url_userpass 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings &&\n+\n+\ttest_must_fail git -c transfer.credentialsInUrl=die \\\n+\t\tfetch $url_userblank 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings\n+'\n+\n+\n+test_expect_success 'push warns or fails when using username:password' '\n+\tgit -c transfer.credentialsInUrl=allow push $url_userpass 2>err &&\n+\t! grep \"$message\" err &&\n+\n+\tgit -c transfer.credentialsInUrl=warn push $url_userpass 2>err &&\n+\tgrep \"warning: $message\" err >warnings &&\n+\n+\ttest_must_fail git -c transfer.credentialsInUrl=die \\\n+\t\tpush $url_userpass 2>err &&\n+\tgrep \"fatal: $message\" err >warnings &&\n+\ttest_line_count = 1 warnings\n+'\n+\n test_done\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 45f0803ed4..b2524a24c2 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -71,29 +71,6 @@ test_expect_success 'clone respects GIT_WORK_TREE' '\n \n '\n \n-test_expect_success LIBCURL 'clone warns or fails when using username:password' '\n-\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n-\ttest_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&\n-\t! grep \"$message\" err &&\n-\n-\ttest_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&\n-\tgrep \"warning: $message\" err >warnings &&\n-\ttest_line_count = 2 warnings &&\n-\n-\ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&\n-\tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings &&\n-\n-\ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:@localhost attempt3 2>err &&\n-\tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings\n-'\n-\n-test_expect_success LIBCURL 'clone does not detect username:password when it is https://username@domain:port/' '\n-\ttest_must_fail git -c transfer.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&\n-\t! grep \"uses plaintext credentials\" err\n-'\n-\n test_expect_success 'clone from hooks' '\n \n \ttest_create_repo r0 &&\n-- \n2.38.1.669.g2ee9a5b0e3\n\n"},{"id":"466169","messageId":"Y2CD6GBl6ORqKsug@coredump.intra.peff.net","threadId":"58726","inReplyTo":"Y2CDrZNgNZ+flJBK@coredump.intra.peff.net","subject":"[PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T02:26:48Z","receivedAt":"2022-11-01T02:26:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Johannes Schindelin \n\nIt is unclear as to _why_, but under certain circumstances the warning\nabout credentials being passed as part of the URL seems to be swallowed\nby the `git remote-https` helper in the Windows jobs of Git's CI builds.\n\nSince it is not actually important how many times Git prints the\nwarning/error message, as long as it prints it at least once, let's just\nmake the test a bit more lenient and test for the latter instead of the\nformer, which works around these CI issues.\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Jeff King \n---\n t/t5551-http-fetch-smart.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex bbe3d5721f..64c6c9f59e 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -597,17 +597,17 @@ test_expect_success 'clone warns or fails when using username:password' '\n \tgit -c transfer.credentialsInUrl=warn \\\n \t\tclone $url_userpass attempt2 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n-\ttest_line_count = 2 warnings &&\n+\ttest_line_count -ge 1 warnings &&\n \n \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n \t\tclone $url_userpass attempt3 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings &&\n+\ttest_line_count -ge 1 warnings &&\n \n \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n \t\tclone $url_userblank attempt4 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings\n+\ttest_line_count -ge 1 warnings\n '\n \n test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '\n@@ -630,17 +630,17 @@ test_expect_success 'fetch warns or fails when using username:password' '\n \n \tgit -c transfer.credentialsInUrl=warn fetch $url_userpass 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n-\ttest_line_count = 3 warnings &&\n+\ttest_line_count -ge 1 warnings &&\n \n \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n \t\tfetch $url_userpass 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings &&\n+\ttest_line_count -ge 1 warnings &&\n \n \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n \t\tfetch $url_userblank 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings\n+\ttest_line_count -ge 1 warnings\n '\n \n \n@@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' '\n \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n \t\tpush $url_userpass 2>err &&\n \tgrep \"fatal: $message\" err >warnings &&\n-\ttest_line_count = 1 warnings\n+\ttest_line_count -ge 1 warnings\n '\n \n test_done\n-- \n2.38.1.669.g2ee9a5b0e3\n"},{"id":"466170","messageId":"Y2CEHvo0WyttNfVk@coredump.intra.peff.net","threadId":"58726","inReplyTo":"98fa5270cb720f2f038c4bb9571c7d306ff5d759.1667245639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T02:27:42Z","receivedAt":"2022-11-01T02:27:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 31, 2022 at 07:47:18PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 0b386c74818..b72cdeb6243 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -78,19 +78,19 @@ test_expect_success LIBCURL 'clone warns or fails when using username:password'\n> \n> \ttest_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@224.0.0.1 attempt2 2>err &&\n> \tgrep \"warning: $message\" err >warnings &&\n> -\ttest_line_count = 2 warnings &&\n> +\ttest_line_count -ge 1 warnings &&\n> \n> \ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@224.0.0.1 attempt3 2>err &&\n> \tgrep \"fatal: $message\" err >warnings &&\n> -\ttest_line_count = 1 warnings &&\n> +\ttest_line_count -ge 1 warnings &&\n> \n> \ttest_must_fail git -c transfer.credentialsInUrl=die clone https://username:@224.0.0.1 attempt3 2>err &&\n> \tgrep \"fatal: $message\" err >warnings &&\n> -\ttest_line_count = 1 warnings\n> +\ttest_line_count -ge 1 warnings\n> '\n> \n> test_expect_success LIBCURL 'clone does not detect username:password when it is https://username@domain:port/' '\n> -\ttest_must_fail git -c transfer.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&\n> +\ttest_must_fail git -c transfer.credentialsInUrl=warn clone https://username@224.0.0.1:8080 attempt3 2>err &&\n> \t! grep \"uses plaintext credentials\" err\n> '\n\nBy the way, this last part of the hunk obviously belongs in the first\npatch. I adjusted as appropriate in the patch I just sent.\n\n-Peff\n"},{"id":"466171","messageId":"Y2CEQwzz+nGR7PLf@coredump.intra.peff.net","threadId":"58726","inReplyTo":"Y2BvaXN2C325YUsz@nand.local","subject":"Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T02:28:19Z","receivedAt":"2022-11-01T02:28:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 31, 2022 at 08:59:21PM -0400, Taylor Blau wrote:\n\n> > > test_expect_success LIBCURL 'fetch warns or fails when using username:password' '\n> > > -\tmessage=\"URL '\\''https://username:@localhost/'\\'' uses plaintext credentials\" &&\n> > > -\ttest_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&\n> > > +\tmessage=\"URL '\\''https://username:@224.0.0.1/'\\'' uses plaintext credentials\" &&\n> > > +\ttest_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err &&\n> > > \t! grep \"$message\" err &&\n> >\n> > could be more robust. It would actually check that we succeeded in using\n> > the URL.\n> \n> It is magical, indeed. If it's significantly more difficult to move\n> these into t5550 or t5551, then I'm OK with this in the meantime (since\n> GitHub Actions really is our primary CI target that we care about this\n> not hanging on).\n> \n> But assuming that moving these around isn't that difficult, I would be\n> slightly happier to see these tests in one of the aforementioned spots.\n\nI don't think it was too difficult to move them. I just sent in patches\n(which I just realized you were not cc'd on).\n\n-Peff\n"},{"id":"466172","messageId":"Y2CFRJLFRXvGwFBC@coredump.intra.peff.net","threadId":"58726","inReplyTo":"RFC-patch-1.1-0266485bc6c-20221031T204149Z-avarab@gmail.com","subject":"Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T02:32:36Z","receivedAt":"2022-11-01T02:32:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 31, 2022 at 09:47:08PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> * When we invoke e.g. \"git-remote-https\" (aka. \"git-remote-curl\")\n> from \"git fetch\". For those cases let's pass down the equivalent of a\n> \"-c transfer.credentialsInUrl=allow\", since we know that we've already\n> inspected our remotes in the parent process.\n> \n> See 7390f05a3c6 (fetch: after refetch, encourage auto gc repacking,\n> 2022-03-28) for prior use of git_config_push_parameter() for this\n> purpose, i.e. to push config parameters before invoking a\n> sub-process.\n\nSo I guess I don't have any _specific_ thing that goes wrong with this\napproach, but it really feels sketchy to me. We are lying to\nsub-processes about the config the user asked for. And again, I see how\nit works, but I wonder if this kind of approach would ever backfire on\nus. For example, if transfer.credentialsInUrl=warn ever ended up having\nany side effects besides the warning, and the sub-processes ended up\nskipping those effects.\n\nI know that's a hypothetical, and probably not even a likely one, but it\njust gets my spider sense tingling.\n\n> > Since it is not actually important how many times Git prints the\n> > warning/error message, as long as it prints it at least once, let's just\n> > make the test a bit more lenient and test for the latter instead of the\n> > former, which works around these CI issues.\n> \n> That being said your change is obviously smaller, so if we'd prefer\n> that in first as a band-aid I'm fine with that.\n> \n> But I'd really like to object to the \"it is not actually important how\n> many...\", it's crappy UX to spew duplicate output at the user, and we\n> should avoid it.\n> \n> So it does matter, and we get it wrong not just in this but also some\n> other cases.\n\nYeah, I think it is crappy UX, too. It's just that I think the tests\nshould not _asserting_ the bad behavior. At most, they should tolerate\nthe bad behavior as a band-aid. So I think Dscho's patch is doing the\nright thing (and I do agree that we should fix the immediate CI pain by\nadjusting the tests, and letting the user-visible fix proceed\nindependently).\n\n-Peff\n"},{"id":"466173","messageId":"221101.86a65b5q9q.gmgdl@evledraar.gmail.com","threadId":"58726","inReplyTo":"Y2CFRJLFRXvGwFBC@coredump.intra.peff.net","subject":"Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-01T03:01:18Z","receivedAt":"2022-11-01T03:04:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 31 2022, Jeff King wrote:\n\n> On Mon, Oct 31, 2022 at 09:47:08PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> [...]\n>> That being said your change is obviously smaller, so if we'd prefer\n>> that in first as a band-aid I'm fine with that.\n>> \n>> But I'd really like to object to the \"it is not actually important how\n>> many...\", it's crappy UX to spew duplicate output at the user, and we\n>> should avoid it.\n>> \n>> So it does matter, and we get it wrong not just in this but also some\n>> other cases.\n>\n> Yeah, I think it is crappy UX, too. It's just that I think the tests\n> should not _asserting_ the bad behavior. At most, they should tolerate\n> the bad behavior as a band-aid. So I think Dscho's patch is doing the\n> right thing (and I do agree that we should fix the immediate CI pain by\n> adjusting the tests, and letting the user-visible fix proceed\n> independently).\n\nThe tests aren't just asserting the bad behavior, they're also ensuring\nthat it doesn't get worse. 1 warning is ideal, 2-3 is bad, but\ntolerable, but if we start emitting 500 of these it would be nice to\nknow.\n\nI've found a couple of regressions like that in other areas, i.e. where\nour tests didn't spot some output change because we were selectively\ngrepping things, but where it would have been nice to spot it at the\ntime.\n\n"},{"id":"466174","messageId":"221101.865yfz5pjp.gmgdl@evledraar.gmail.com","threadId":"58726","inReplyTo":"Y2CD4g9z2yVFA06l@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-01T03:18:32Z","receivedAt":"2022-11-01T03:20:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 31 2022, Jeff King wrote:\n\n> Commit 6dcbdc0d66 (remote: create fetch.credentialsInUrl config,\n> 2022-06-06) added tests for our handling of passwords in URLs. Since the\n> obvious URL to be affected is git-over-http, the tests use http. However\n> they don't set up a test server; they just try to access\n> https://localhost, assuming it will fail (because the nothing is\n> listening there).\n>\n> This causes some possible problems:\n>\n> - There might be a web server running on localhost, and we do not\n> actually want to connect to that.\n>\n> - The DNS resolver, or the local firewall, might take a substantial\n> amount of time (or forever, whichever comes first) to fail to\n> connect, slowing down the tests cases unnecessarily.\n>\n> - Since there's no server, our tests for \"allow\" and \"warn\" still\n> expect the clone/fetch/push operations to fail, even though in the\n> real world we'd expect these to succeed. We scrape stderr to see\n> what happened, but it's not as robust as a more realistic test.\n>\n> Let's instead move these to t5551, which is all about testing http and\n> where we have a real server. That eliminates any issues with contacting\n> a strange URL, and lets the \"allow\" and \"warn\" tests confirm that the\n> operation actually succeeds.\n>\n> It's not quite a verbatim move for a few reasons:\n>\n> - we can drop the LIBCURL dependency; it's already part of\n> lib-httpd.sh\n>\n> - we'll use HTTPD_URL_USER_PASS, etc, instead of our fake URL. To\n> avoid repetition, we'll add a few extra variables.\n>\n> - the \"https://username:@localhost\" test uses a funny URL that\n> lib-httpd.sh doesn't provide. We'll similarly construct it in a\n> variable. Note that we're hard-coding the lib-httpd username here,\n> but t5551 already does that everywhere.\n>\n> - for the \"domain:port\" test, the URL provided by lib-httpd is fine,\n> since our test server will always be on an exotic port. But we'll\n> confirm in the test that this is so.\n>\n> - since our message-matching is done via grep, I simplified it to use\n> a regex, rather than trying to massage lib-httpd's variables.\n> Arguably this makes it more readable, too, while retaining the bits\n> we care about: the fatal/warning distinction, the \"uses plaintext\"\n> message, and the fact that the password was redacted.\n>\n> - we'll use the /auth/ path for the repo, which shows that we are\n> indeed making use of the auth information when needed.\n>\n> - we'll also use /smart/; most of these tests could be done via /dumb/\n> in t5550, but setting up pushes there requires extra effort and\n> dependencies. The smart protocol is what most everyone is using\n> these days anyway.\n>\n> This patch is my own, but I stole the analysis and a few bits of the\n> commit message from a patch by Johannes Schindelin.\n\nDid you consider just using git://; on the WIP branch I linked to where\nI fixed these tests quite a bit already I left them in their own file in\nanticipation of having to test that (but didn't finish that yet). I.e.:\n\n\t+ git -C /home/avar/g/git/t/trash directory.t5700-protocol-v1/repo/parent/ tag two\n\t+ 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\n\tfatal: URL 'git://foo:@127.0.0.1:5700/parent' uses plaintext credentials\n\nI can't remember if anything can do anything sensible with\nuser:passwords over non-http(s), but at the point where we emit this\nerror in remote.c we don't care, so perhaps we could just test it that\nway.\n"},{"id":"466175","messageId":"221101.861qqn5ovf.gmgdl@evledraar.gmail.com","threadId":"58726","inReplyTo":"Y2CD6GBl6ORqKsug@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-01T03:29:31Z","receivedAt":"2022-11-01T03:34:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 31 2022, Jeff King wrote:\n\n> From: Johannes Schindelin \n\nWhatever our difference of opinion about the usefulness of not asserting\nthat a thing doesn't get worse, even if it isn't perfect (c.f.:\nhttps://lore.kernel.org/git/221101.86a65b5q9q.gmgdl@evledraar.gmail.com/)\nI think that...\n\n> It is unclear as to _why_, but under certain circumstances the warning\n> about credentials being passed as part of the URL seems to be swallowed\n> by the `git remote-https` helper in the Windows jobs of Git's CI builds.\n\n..this description dosen't match what the patch is doing, okey, so\nthere's a case where the remote helper swallows the warnings, i.e. we'll\nemit fewer than we expected...\n\n> Signed-off-by: Johannes Schindelin \n> Signed-off-by: Jeff King \n> ---\n> t/t5551-http-fetch-smart.sh | 14 +++++++-------\n> 1 file changed, 7 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index bbe3d5721f..64c6c9f59e 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -597,17 +597,17 @@ test_expect_success 'clone warns or fails when using username:password' '\n> \tgit -c transfer.credentialsInUrl=warn \\\n> \t\tclone $url_userpass attempt2 2>err &&\n> \tgrep \"warning: $message\" err >warnings &&\n> -\ttest_line_count = 2 warnings &&\n> +\ttest_line_count -ge 1 warnings &&\n> \n> \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n> \t\tclone $url_userpass attempt3 2>err &&\n> \tgrep \"fatal: $message\" err >warnings &&\n> -\ttest_line_count = 1 warnings &&\n> +\ttest_line_count -ge 1 warnings &&\n> \n> \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n> \t\tclone $url_userblank attempt4 2>err &&\n> \tgrep \"fatal: $message\" err >warnings &&\n> -\ttest_line_count = 1 warnings\n> +\ttest_line_count -ge 1 warnings\n> '\n> \n> test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '\n> @@ -630,17 +630,17 @@ test_expect_success 'fetch warns or fails when using username:password' '\n> \n> \tgit -c transfer.credentialsInUrl=warn fetch $url_userpass 2>err &&\n> \tgrep \"warning: $message\" err >warnings &&\n> -\ttest_line_count = 3 warnings &&\n> +\ttest_line_count -ge 1 warnings &&\n> \n> \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n> \t\tfetch $url_userpass 2>err &&\n> \tgrep \"fatal: $message\" err >warnings &&\n> -\ttest_line_count = 1 warnings &&\n> +\ttest_line_count -ge 1 warnings &&\n> \n> \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n> \t\tfetch $url_userblank 2>err &&\n> \tgrep \"fatal: $message\" err >warnings &&\n> -\ttest_line_count = 1 warnings\n> +\ttest_line_count -ge 1 warnings\n> '\n> \n> \n> @@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' '\n> \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n> \t\tpush $url_userpass 2>err &&\n> \tgrep \"fatal: $message\" err >warnings &&\n> -\ttest_line_count = 1 warnings\n> +\ttest_line_count -ge 1 warnings\n> '\n\n...but then why are we modifying these codepaths that have nothing to do\nwith invoking the remote helper, i.e. where we die early before we get\nto that?\n\nAnd even if some of this was invoking that remote helper and we were\nmodifying it blindly, then presumably the helper swallowing it would\nresult in 0 some of the time, so \"-ge 1\" would be wrong.\n\n(That's not the case, but it's why I think the patch doesn't make much\nsense).\n"},{"id":"466177","messageId":"xmqq5yfze0lc.fsf@gitster.g","threadId":"58726","inReplyTo":"Y2CD6GBl6ORqKsug@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-01T04:54:23Z","receivedAt":"2022-11-01T04:54:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It is unclear as to _why_, but under certain circumstances the warning\n> about credentials being passed as part of the URL seems to be swallowed\n> by the `git remote-https` helper in the Windows jobs of Git's CI builds.\n>\n> Since it is not actually important how many times Git prints the\n> warning/error message, as long as it prints it at least once, ...\n\nSorry, but I do not quite see the value of keeping the test to\nexpect success in a weakend form. If we think we are emitting three\nwarnings, whether we do so by mistake or by design, and some of them\nare lost and not shown for an unknown reason, is there a guarantee\nthat at least one would survive? And when all three are lost, even\nthe test in the weakened form would fail and stop the CI builds, no?\n\nIf we do not know why we are losing some messages, and if we do not\nhave resources to track down why, that is perfectly fine. Fixes can\nbe prioritised. But wouldn't test_expect_failure be a better way to\nexpress \"we think we ought to get 3 but for some reason we may not\nget all of them and we haven't figured it out\"?\n"},{"id":"466181","messageId":"Y2DLdsM2CYY3Z2Sc@coredump.intra.peff.net","threadId":"58726","inReplyTo":"221101.865yfz5pjp.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T07:32:06Z","receivedAt":"2022-11-01T07:32:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 01, 2022 at 04:18:32AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Did you consider just using git://; on the WIP branch I linked to where\n> I fixed these tests quite a bit already I left them in their own file in\n> anticipation of having to test that (but didn't finish that yet). I.e.:\n\nNo, I didn't really consider that. Mostly because I was trying to stick\nwith the original intent of 6dcbdc0d66 that created them.\n\nIf we toss that out, then in theory that widens the options. And in some\nways it's nice to use git://, because it has fewer dependencies and so\nis run on more platforms. But it strikes me as a pretty unrealistic\ntest, just because credentials in git:// URLs make no sense and are not\nsomething anybody would do.\n\nAs you note, since the error comes from remote.c, it would still\ntrigger. But it's a bit more \"white box\" testing than I think makes\nsense here. I prefer the original tests' method of trying to create\nplausible real-world scenarios and seeing how they behave (and I think\nmy patch even improves that, since having an actual server on the other\nend is the usual case).\n\n-Peff\n"},{"id":"466182","messageId":"Y2DNS0W5vgk2Q3qJ@coredump.intra.peff.net","threadId":"58726","inReplyTo":"221101.861qqn5ovf.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T07:39:55Z","receivedAt":"2022-11-01T07:40:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 01, 2022 at 04:29:31AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > It is unclear as to _why_, but under certain circumstances the warning\n> > about credentials being passed as part of the URL seems to be swallowed\n> > by the `git remote-https` helper in the Windows jobs of Git's CI builds.\n> \n> ..this description dosen't match what the patch is doing, okey, so\n> there's a case where the remote helper swallows the warnings, i.e. we'll\n> emit fewer than we expected...\n\nSo I really didn't revisit this commit much at all, and was just trying\nto save Dscho (or Taylor) the work of having to rebase it, if we go with\nmy patch 1.\n\nIMHO it is OK enough as it is, but if I were writing it from scratch I\nprobably would have given the rationale that the tests are insiting on a\ndumb, sub-optimal behavior. And flakes or inconsistencies aside, they\nshould be asserting only the presence or absence of the message. And\nprobably would have left each at \"grep\" and dropped the test_line_count\ntotally.\n\nIt is not even clear to me that the remote-https is the one being\nswallowed (at least, I have not seen an argument or evidence that this\nis so; it does seem plausible).\n\n> > @@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' '\n> > \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n> > \t\tpush $url_userpass 2>err &&\n> > \tgrep \"fatal: $message\" err >warnings &&\n> > -\ttest_line_count = 1 warnings\n> > +\ttest_line_count -ge 1 warnings\n> > '\n> \n> ...but then why are we modifying these codepaths that have nothing to do\n> with invoking the remote helper, i.e. where we die early before we get\n> to that?\n\nIf you're arguing that we should only do s/= 3/-ge 1/ for the test that\nis flaking, I could buy that. Though like I said, I consider the value\nhere to be focusing the purpose of the tests as much as dealing with the\nflake.\n\nI really don't care that much either way, though.\n\nI'd also be fine with a separate test (marked expect_failure) that\nchecks the number of messages.\n\n> And even if some of this was invoking that remote helper and we were\n> modifying it blindly, then presumably the helper swallowing it would\n> result in 0 some of the time, so \"-ge 1\" would be wrong.\n> \n> (That's not the case, but it's why I think the patch doesn't make much\n> sense).\n\nI thought the point is that the outer program calling the helper would\nconsistently produce the error, always yielding at least one instance.\nThe helper one is generally \"extra\" and undesired.\n\n-Peff\n"},{"id":"466183","messageId":"Y2DOA7uYfkFveAU9@coredump.intra.peff.net","threadId":"58726","inReplyTo":"xmqq5yfze0lc.fsf@gitster.g","subject":"Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T07:42:59Z","receivedAt":"2022-11-01T07:43:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 31, 2022 at 09:54:23PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > It is unclear as to _why_, but under certain circumstances the warning\n> > about credentials being passed as part of the URL seems to be swallowed\n> > by the `git remote-https` helper in the Windows jobs of Git's CI builds.\n> >\n> > Since it is not actually important how many times Git prints the\n> > warning/error message, as long as it prints it at least once, ...\n> \n> Sorry, but I do not quite see the value of keeping the test to\n> expect success in a weakend form. If we think we are emitting three\n> warnings, whether we do so by mistake or by design, and some of them\n> are lost and not shown for an unknown reason, is there a guarantee\n> that at least one would survive? And when all three are lost, even\n> the test in the weakened form would fail and stop the CI builds, no?\n\nWithout understanding the cause of the loss, I agree that things are a\nlittle hand-wavy. But the assumption _does_ seem to hold that we\nconsistently produce at least one (presumably from the parent\nclone/fetch/push process). And if we can rely on that, there's value in\nthe tests asserting that the message was shown to the user at least\nonce.\n\n> If we do not know why we are losing some messages, and if we do not\n> have resources to track down why, that is perfectly fine. Fixes can\n> be prioritised. But wouldn't test_expect_failure be a better way to\n> express \"we think we ought to get 3 but for some reason we may not\n> get all of them and we haven't figured it out\"?\n\nMarking it as expect_failure throws out the main point of the test,\nthough, which is that the user sees _some_ message (and that the \"die\"\nform aborts the operation).\n\nIt might make sense to add a separate test in the meantime that\ndocuments the \"oops, we get the wrong number\" sometimes state (and\neventually, if fixed, that could be folded back into the main test for\nefficiency / simplicity).\n\n-Peff\n"},{"id":"466187","messageId":"221101.86sfj33wmg.gmgdl@evledraar.gmail.com","threadId":"58726","inReplyTo":"Y2DNS0W5vgk2Q3qJ@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-01T08:15:00Z","receivedAt":"2022-11-01T08:30:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 01 2022, Jeff King wrote:\n\n> On Tue, Nov 01, 2022 at 04:29:31AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > It is unclear as to _why_, but under certain circumstances the warning\n>> > about credentials being passed as part of the URL seems to be swallowed\n>> > by the `git remote-https` helper in the Windows jobs of Git's CI builds.\n>> \n>> ..this description dosen't match what the patch is doing, okey, so\n>> there's a case where the remote helper swallows the warnings, i.e. we'll\n>> emit fewer than we expected...\n>\n> So I really didn't revisit this commit much at all, and was just trying\n> to save Dscho (or Taylor) the work of having to rebase it, if we go with\n> my patch 1.\n>\n> IMHO it is OK enough as it is, but if I were writing it from scratch I\n> probably would have given the rationale that the tests are insiting on a\n> dumb, sub-optimal behavior. And flakes or inconsistencies aside, they\n> should be asserting only the presence or absence of the message. And\n> probably would have left each at \"grep\" and dropped the test_line_count\n> totally.\n\nDo you mean that even if we fix the bug and consistently emit one and\nonly one such message you'd like to have the tests not assert that\nthat's the case?\n\nI do think that UX is important enough to test for, particularly if\nwe've had a bug related to that that we've fixed. I.e. if something in\nthe direction of my [1] goes in.\n\n> It is not even clear to me that the remote-https is the one being\n> swallowed (at least, I have not seen an argument or evidence that this\n> is so; it does seem plausible).\n\nIt is the case, the only ones that are going to be duplicated are the\n\"warn\" ones, because for \"die\" we'll die right away in the parent\nprocess.\n\nWhich is what I'm trying to get across here, and why I'm a bit\nconfused. I.e. I thought you'd agree that we should test that we emit\nexactly one warning if & when we've fixed the underlying issue.\n\nThat issue is already fixed for \"die\", so even if you want to loosen up\nthe test your [2] should only keep the first line removal/addition in\nthe first two hunks, and drop the 3rd one.\n\n>> > @@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' '\n>> > \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n>> > \t\tpush $url_userpass 2>err &&\n>> > \tgrep \"fatal: $message\" err >warnings &&\n>> > -\ttest_line_count = 1 warnings\n>> > +\ttest_line_count -ge 1 warnings\n>> > '\n>> \n>> ...but then why are we modifying these codepaths that have nothing to do\n>> with invoking the remote helper, i.e. where we die early before we get\n>> to that?\n>\n> If you're arguing that we should only do s/= 3/-ge 1/ for the test that\n> is flaking, I could buy that.\n\nI'm saying that if we're doing a handwaivy-fix and saying \"sometimes the\nmessage gets swallowed\" and fixing this blindly without checking how it\nworks, then changing \"= 1\" to \"-ge 1\" doesn't make sense.\n\nIt should be \"-ge 0\", i.e. surely that \"one warning\" can get swallowed\ntoo?\n\nNow, I know that never happens, because we'll always get at least\none.\n\nI'm just saying that as soon as you stop to think about that you must\nalso come to the conclusion that the \"die\" ones are OK as-is.\n\nThat's because the reason we always get at least one is the same as\nwe're always guaranteed to emit just one in the \"die\" case: The parent\nprocess emits it, then dies.\n\n>> And even if some of this was invoking that remote helper and we were\n>> modifying it blindly, then presumably the helper swallowing it would\n>> result in 0 some of the time, so \"-ge 1\" would be wrong.\n>> \n>> (That's not the case, but it's why I think the patch doesn't make much\n>> sense).\n>\n> I thought the point is that the outer program calling the helper would\n> consistently produce the error, always yielding at least one instance.\n> The helper one is generally \"extra\" and undesired.\n\nYes, exactly. Which is what my fix[1] the root cause addresses.\n\nAnyway, I'm just trying to help here. If you/Johannes/others want to go\nwith the \"hotfix\" as-is that's fine my me.\n\nI just don't see what the hurry is, it's been this way for two releases,\nif it's flaky that's been the case for months, I'd think we could just\nfix the root cause.\n\n1. http://lore.kernel.org/git/RFC-patch-1.1-0266485bc6c-20221031T204149Z-avarab@gmail.com\n2. https://lore.kernel.org/git/Y2CD6GBl6ORqKsug@coredump.intra.peff.net/\n"},{"id":"466192","messageId":"Y2Di/5Hgp6IdSuuk@coredump.intra.peff.net","threadId":"58726","inReplyTo":"221101.86sfj33wmg.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T09:12:31Z","receivedAt":"2022-11-01T09:12:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 01, 2022 at 09:15:00AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > So I really didn't revisit this commit much at all, and was just trying\n> > to save Dscho (or Taylor) the work of having to rebase it, if we go with\n> > my patch 1.\n> >\n> > IMHO it is OK enough as it is, but if I were writing it from scratch I\n> > probably would have given the rationale that the tests are insiting on a\n> > dumb, sub-optimal behavior. And flakes or inconsistencies aside, they\n> > should be asserting only the presence or absence of the message. And\n> > probably would have left each at \"grep\" and dropped the test_line_count\n> > totally.\n> \n> Do you mean that even if we fix the bug and consistently emit one and\n> only one such message you'd like to have the tests not assert that\n> that's the case?\n\nNo, I wouldn't mind it, if that is a bug we've fixed. I just mean that\nthe tests as written never wanted to say \"3 is the absolute right number\nof times to write this message\". They only put \"3\" there because it made\nthings pass.\n\n> I do think that UX is important enough to test for, particularly if\n> we've had a bug related to that that we've fixed. I.e. if something in\n> the direction of my [1] goes in.\n\nSure, I don't mind at all a test for it. In the short-term, if you want\na test that fails, I'd prefer it be separate so that we can test the\nuseful existing behavior that _does_ work. If the multiple-messages bug\nis fixed, I don't mind folding them together into a single test that\npasses.\n\n> > It is not even clear to me that the remote-https is the one being\n> > swallowed (at least, I have not seen an argument or evidence that this\n> > is so; it does seem plausible).\n> \n> It is the case, the only ones that are going to be duplicated are the\n> \"warn\" ones, because for \"die\" we'll die right away in the parent\n> process.\n\nRight, I understand why \"die\" produces only one. My question was when we\nproduce 2 on Windows (sometimes?) but 3 elsewhere, are we sure it is the\none from remote-https that is eaten, or could it ever be one of the\nothers?\n\nIn a sense we do not need to worry about \"why is it sometimes eaten\" if\nthe bug is fixed to produce only the one message. But it may point to a\nseparate and interesting problem (e.g., is stderr from remote-https not\nreliable?).\n\n> >> > @@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' '\n> >> > \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n> >> > \t\tpush $url_userpass 2>err &&\n> >> > \tgrep \"fatal: $message\" err >warnings &&\n> >> > -\ttest_line_count = 1 warnings\n> >> > +\ttest_line_count -ge 1 warnings\n> >> > '\n> >> \n> >> ...but then why are we modifying these codepaths that have nothing to do\n> >> with invoking the remote helper, i.e. where we die early before we get\n> >> to that?\n> >\n> > If you're arguing that we should only do s/= 3/-ge 1/ for the test that\n> > is flaking, I could buy that.\n> \n> I'm saying that if we're doing a handwaivy-fix and saying \"sometimes the\n> message gets swallowed\" and fixing this blindly without checking how it\n> works, then changing \"= 1\" to \"-ge 1\" doesn't make sense.\n\nRight, I'm fine with that (I perhaps should have said something stronger\nthan \"I could buy that\"). As I said, I was mostly just rebasing Dscho's\npatch and I think it was good enough in the sense that it was\nhand-waving away the whole \"there may be more than one\" problem.\n\nBut I do agree that we'll never see more in the \"die\" cases, and there\nis no need to change them.\n\n> > I thought the point is that the outer program calling the helper would\n> > consistently produce the error, always yielding at least one instance.\n> > The helper one is generally \"extra\" and undesired.\n> \n> Yes, exactly. Which is what my fix[1] the root cause addresses.\n> \n> Anyway, I'm just trying to help here. If you/Johannes/others want to go\n> with the \"hotfix\" as-is that's fine my me.\n> \n> I just don't see what the hurry is, it's been this way for two releases,\n> if it's flaky that's been the case for months, I'd think we could just\n> fix the root cause.\n\nIt recently bit me twice, so maybe I am giving it more urgency than it\ndeserves (or maybe something changed in CI to make it more likely).\n\nI do think it would be nice to fix it. I don't love your patch for the\nreasons I replied there (not your fault; it's inherently a crappy and\ncomplicated problem). In the meantime, I'd like to see CI fixed, as\nit is wasting developer's time. And that's why I called Dscho's\nloosening \"good enough\". It is hopefully a temporary state anyway.\n\nBut I would be just as happy to see a similar patch which just changed\nthe 2/3 lines to \"-ge 1\" (or just a straight grep).\n\n-Peff\n"},{"id":"466193","messageId":"Y2Doe0ZGb3Zmmmen@coredump.intra.peff.net","threadId":"58726","inReplyTo":"Y2CFRJLFRXvGwFBC@coredump.intra.peff.net","subject":"Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T09:35:55Z","receivedAt":"2022-11-01T09:38:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 31, 2022 at 10:32:36PM -0400, Jeff King wrote:\n\n> On Mon, Oct 31, 2022 at 09:47:08PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> > * When we invoke e.g. \"git-remote-https\" (aka. \"git-remote-curl\")\n> > from \"git fetch\". For those cases let's pass down the equivalent of a\n> > \"-c transfer.credentialsInUrl=allow\", since we know that we've already\n> > inspected our remotes in the parent process.\n> > \n> > See 7390f05a3c6 (fetch: after refetch, encourage auto gc repacking,\n> > 2022-03-28) for prior use of git_config_push_parameter() for this\n> > purpose, i.e. to push config parameters before invoking a\n> > sub-process.\n> \n> So I guess I don't have any _specific_ thing that goes wrong with this\n> approach, but it really feels sketchy to me. We are lying to\n> sub-processes about the config the user asked for. And again, I see how\n> it works, but I wonder if this kind of approach would ever backfire on\n> us. For example, if transfer.credentialsInUrl=warn ever ended up having\n> any side effects besides the warning, and the sub-processes ended up\n> skipping those effects.\n> \n> I know that's a hypothetical, and probably not even a likely one, but it\n> just gets my spider sense tingling.\n\nSo inherently this is going to be ugly because it's crossing process\nboundaries. But the more minimal fix I was thinking of would be\nsomething like this:\n\ndiff --git a/remote.c b/remote.c\nindex 60869beebe..af5f95c719 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -615,6 +615,14 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)\n \treturn NULL;\n }\n \n+static int test_and_set_env(const char *var)\n+{\n+\tint ret = git_env_bool(var, 0);\n+\tif (!ret)\n+\t\tsetenv(var, \"1\", 0);\n+\treturn ret;\n+}\n+\n static void validate_remote_url(struct remote *remote)\n {\n \tint i;\n@@ -634,6 +642,9 @@ static void validate_remote_url(struct remote *remote)\n \telse\n \t\tdie(_(\"unrecognized value transfer.credentialsInUrl: '%s'\"), value);\n \n+\tif (test_and_set_env(\"GIT_CHECKED_CREDENTIALS_IN_URL\"))\n+\t\treturn;\n+\n \tfor (i = 0; i < remote->url_nr; i++) {\n \t\tstruct url_info url_info = { 0 };\n \n\nYou can also put it lower in the function, when we actually warn, which\nsaves adding to the environment when there is nothing to warn about\n(though this way, we avoid doing the redundant work).\n\nCompared to munging the config, this seems shorter and simpler, and\nthere's no chance of us ever getting confused between the fake\n\"suppress\" value and something the user actually asked for.\n\n-Peff\n"},{"id":"466200","messageId":"221101.86o7tq4vsn.gmgdl@evledraar.gmail.com","threadId":"58726","inReplyTo":"Y2Doe0ZGb3Zmmmen@coredump.intra.peff.net","subject":"Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-01T13:07:39Z","receivedAt":"2022-11-01T14:02:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 01 2022, Jeff King wrote:\n\n> On Mon, Oct 31, 2022 at 10:32:36PM -0400, Jeff King wrote:\n>\n>> On Mon, Oct 31, 2022 at 09:47:08PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> > * When we invoke e.g. \"git-remote-https\" (aka. \"git-remote-curl\")\n>> > from \"git fetch\". For those cases let's pass down the equivalent of a\n>> > \"-c transfer.credentialsInUrl=allow\", since we know that we've already\n>> > inspected our remotes in the parent process.\n>> > \n>> > See 7390f05a3c6 (fetch: after refetch, encourage auto gc repacking,\n>> > 2022-03-28) for prior use of git_config_push_parameter() for this\n>> > purpose, i.e. to push config parameters before invoking a\n>> > sub-process.\n>> \n>> So I guess I don't have any _specific_ thing that goes wrong with this\n>> approach, but it really feels sketchy to me. We are lying to\n>> sub-processes about the config the user asked for. And again, I see how\n>> it works, but I wonder if this kind of approach would ever backfire on\n>> us. For example, if transfer.credentialsInUrl=warn ever ended up having\n>> any side effects besides the warning, and the sub-processes ended up\n>> skipping those effects.\n>> \n>> I know that's a hypothetical, and probably not even a likely one, but it\n>> just gets my spider sense tingling.\n>\n> So inherently this is going to be ugly because it's crossing process\n> boundaries. But the more minimal fix I was thinking of would be\n> something like this:\n>\n> diff --git a/remote.c b/remote.c\n> index 60869beebe..af5f95c719 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -615,6 +615,14 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)\n> \treturn NULL;\n> }\n> \n> +static int test_and_set_env(const char *var)\n> +{\n> +\tint ret = git_env_bool(var, 0);\n> +\tif (!ret)\n> +\t\tsetenv(var, \"1\", 0);\n> +\treturn ret;\n> +}\n> +\n> static void validate_remote_url(struct remote *remote)\n> {\n> \tint i;\n> @@ -634,6 +642,9 @@ static void validate_remote_url(struct remote *remote)\n> \telse\n> \t\tdie(_(\"unrecognized value transfer.credentialsInUrl: '%s'\"), value);\n> \n> +\tif (test_and_set_env(\"GIT_CHECKED_CREDENTIALS_IN_URL\"))\n> +\t\treturn;\n> +\n> \tfor (i = 0; i < remote->url_nr; i++) {\n> \t\tstruct url_info url_info = { 0 };\n> \n>\n> You can also put it lower in the function, when we actually warn, which\n> saves adding to the environment when there is nothing to warn about\n> (though this way, we avoid doing the redundant work).\n>\n> Compared to munging the config, this seems shorter and simpler, and\n> there's no chance of us ever getting confused between the fake\n> \"suppress\" value and something the user actually asked for.\n\nSure, we can do it with an environment variable, in the end that's all\ngit_config_push_parameter() is doing too. It's just setting things in\n\"GIT_CONFIG_PARAMETERS\".\n\nAnd yes, we can set this in the low-level function instead of with\ngit_config_push_parameter() in builtin/*.c as I did. I was aiming for\nsomething demonstrably narrow, at the cost of some verbosity.\n\nBut I don't get how other things being equal you think sticking this in\n\"GIT_CHECKED_CREDENTIALS_IN_URL\" instead of \"GIT_CONFIG_PARAMETERS\"\nhelps.\n\nWe already pass config to ourselves like that (and via \"-c\") in other\nplaces. Can you think of a case where these would be different?\n\nThe only ones I can think of are e.g. because we know about\n\"GIT_CONFIG_PARAMETERS\", and not this new custom variable, e.g. in\n\"prepare_other_repo_env()\", but those seem like exactly the reason to\nuse the existing variable.\n\nI can think of potential pitfalls here, e.g. how does it interact with\nsubmodules? That's one reason I submitted it as an RFC, the tests need\nto be better (with or without this change). E.g. \"git ls-remote\" is also\nnot covered by the upthread patch.\n\nBut that's all separate from what the environment variable is named, or\nif it lives in the config space.\n"},{"id":"466201","messageId":"221101.86k04e4ve9.gmgdl@evledraar.gmail.com","threadId":"58726","inReplyTo":"Y2Di/5Hgp6IdSuuk@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-01T14:05:23Z","receivedAt":"2022-11-01T14:11:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 01 2022, Jeff King wrote:\n\n> On Tue, Nov 01, 2022 at 09:15:00AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > So I really didn't revisit this commit much at all, and was just trying\n>> > to save Dscho (or Taylor) the work of having to rebase it, if we go with\n>> > my patch 1.\n>> >\n>> > IMHO it is OK enough as it is, but if I were writing it from scratch I\n>> > probably would have given the rationale that the tests are insiting on a\n>> > dumb, sub-optimal behavior. And flakes or inconsistencies aside, they\n>> > should be asserting only the presence or absence of the message. And\n>> > probably would have left each at \"grep\" and dropped the test_line_count\n>> > totally.\n>> \n>> Do you mean that even if we fix the bug and consistently emit one and\n>> only one such message you'd like to have the tests not assert that\n>> that's the case?\n>\n> No, I wouldn't mind it, if that is a bug we've fixed. I just mean that\n> the tests as written never wanted to say \"3 is the absolute right number\n> of times to write this message\". They only put \"3\" there because it made\n> things pass.\n\nThat's one reason, another is to assert current behavior, and to be able\nto answer questions like \"does this patch change behavior\" without\nhaving to recursively diff the trash directory output of a test run,\nbecause everything's so fuzzy.\n\nIf and when it's 1 instead of 3, great, adjusting the test isn't a big\ndeal.\n\nAnyway, we're off into general testing philosophy again, which I think\nis off topic here.\n\n>> I do think that UX is important enough to test for, particularly if\n>> we've had a bug related to that that we've fixed. I.e. if something in\n>> the direction of my [1] goes in.\n>\n> Sure, I don't mind at all a test for it. In the short-term, if you want\n> a test that fails, I'd prefer it be separate so that we can test the\n> useful existing behavior that _does_ work. If the multiple-messages bug\n> is fixed, I don't mind folding them together into a single test that\n> passes.\n\nRight, I'm not saying \"keep the flaky test\", I'm saying let's keep the\nones we know aren't flaky.\n\n>> > It is not even clear to me that the remote-https is the one being\n>> > swallowed (at least, I have not seen an argument or evidence that this\n>> > is so; it does seem plausible).\n>> \n>> It is the case, the only ones that are going to be duplicated are the\n>> \"warn\" ones, because for \"die\" we'll die right away in the parent\n>> process.\n>\n> Right, I understand why \"die\" produces only one. My question was when we\n> produce 2 on Windows (sometimes?) but 3 elsewhere, are we sure it is the\n> one from remote-https that is eaten, or could it ever be one of the\n> others?\n\nI don't have a test case in front of me, and Johannes didn't provide one\n(or even a link to CI output).\n\nBut from his description and being familiar with the code I'm pretty\ncertain isn't not the \"die\" cases, those are all in-process, and it\nhappens before we spawn sub-processes, I don't see how that would be\ndifferent on Windows.\n\n>> > I thought the point is that the outer program calling the helper would\n>> > consistently produce the error, always yielding at least one instance.\n>> > The helper one is generally \"extra\" and undesired.\n>> \n>> Yes, exactly. Which is what my fix[1] the root cause addresses.\n>> \n>> Anyway, I'm just trying to help here. If you/Johannes/others want to go\n>> with the \"hotfix\" as-is that's fine my me.\n>> \n>> I just don't see what the hurry is, it's been this way for two releases,\n>> if it's flaky that's been the case for months, I'd think we could just\n>> fix the root cause.\n>\n> It recently bit me twice, so maybe I am giving it more urgency than it\n> deserves (or maybe something changed in CI to make it more likely).\n\nBit you in GitHub Windows CI?\n\n> I do think it would be nice to fix it. I don't love your patch for the\n> reasons I replied there (not your fault; it's inherently a crappy and\n> complicated problem). In the meantime, I'd like to see CI fixed, as\n> it is wasting developer's time. And that's why I called Dscho's\n> loosening \"good enough\". It is hopefully a temporary state anyway.\n>\n> But I would be just as happy to see a similar patch which just changed\n> the 2/3 lines to \"-ge 1\" (or just a straight grep).\n\nSure, if we're deciding not to care about tests that are unrelated to\nthe flakyness problem at hand being loosened.\n"},{"id":"466214","messageId":"Y2GDlZBzZqR+lwyU@nand.local","threadId":"58726","inReplyTo":"Y2DLdsM2CYY3Z2Sc@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-01T20:37:41Z","receivedAt":"2022-11-01T20:37:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 01, 2022 at 03:32:06AM -0400, Jeff King wrote:\n> If we toss that out, then in theory that widens the options. And in some\n> ways it's nice to use git://, because it has fewer dependencies and so\n> is run on more platforms. But it strikes me as a pretty unrealistic\n> test, just because credentials in git:// URLs make no sense and are not\n> something anybody would do.\n>\n> As you note, since the error comes from remote.c, it would still\n> trigger. But it's a bit more \"white box\" testing than I think makes\n> sense here. I prefer the original tests' method of trying to create\n> plausible real-world scenarios and seeing how they behave (and I think\n> my patch even improves that, since having an actual server on the other\n> end is the usual case).\n\nAgreed. Despite some of the niceties you mention, I concur that testing\nhttp(s) is more realistic and worth doing.\n\nThanks,\nTaylor\n"},{"id":"466215","messageId":"Y2GGofvR0ii2pI1x@nand.local","threadId":"58726","inReplyTo":"Y2DOA7uYfkFveAU9@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-01T20:50:41Z","receivedAt":"2022-11-01T20:50:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 01, 2022 at 03:42:59AM -0400, Jeff King wrote:\n> On Mon, Oct 31, 2022 at 09:54:23PM -0700, Junio C Hamano wrote:\n>\n> > Jeff King writes:\n> >\n> > > It is unclear as to _why_, but under certain circumstances the warning\n> > > about credentials being passed as part of the URL seems to be swallowed\n> > > by the `git remote-https` helper in the Windows jobs of Git's CI builds.\n> > >\n> > > Since it is not actually important how many times Git prints the\n> > > warning/error message, as long as it prints it at least once, ...\n> >\n> > Sorry, but I do not quite see the value of keeping the test to\n> > expect success in a weakend form. If we think we are emitting three\n> > warnings, whether we do so by mistake or by design, and some of them\n> > are lost and not shown for an unknown reason, is there a guarantee\n> > that at least one would survive? And when all three are lost, even\n> > the test in the weakened form would fail and stop the CI builds, no?\n>\n> Without understanding the cause of the loss, I agree that things are a\n> little hand-wavy. But the assumption _does_ seem to hold that we\n> consistently produce at least one (presumably from the parent\n> clone/fetch/push process). And if we can rely on that, there's value in\n> the tests asserting that the message was shown to the user at least\n> once.\n\nPart of me wonders whether the local DNS-resolution issue you fixed in\nthe first patch would be sufficient to get us to produce the wrong\nnumber of warnings consistently.\n\nI.e., if I queue just the first patch and drop Johannes's, would that be\nsufficient to get CI working consistently again?\n\nI don't know. It's frustrating to rely so much on an external\nenvironment that our feedback loop can only be as short as \"push out\nsome combination of these patches and wait for CI\". That's\ndisappointing, and TBH I would rather spend time focusing on other\npatches than play games with CI.\n\nThe pair of patches look good to me. Perhaps we could avoid the weakened\nassumption, but I do not mind too much in the meantime. Especially since\nwe already have a series[1] in the works that resolves the issue for\ngood.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/RFC-patch-1.1-0266485bc6c-20221031T204149Z-avarab@gmail.com/\n"},{"id":"466216","messageId":"Y2GHjnuyuwGpY3II@nand.local","threadId":"58726","inReplyTo":"221101.86a65b5q9q.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-01T20:54:38Z","receivedAt":"2022-11-01T20:54:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 01, 2022 at 04:01:18AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> > Yeah, I think it is crappy UX, too. It's just that I think the tests\n> > should not _asserting_ the bad behavior. At most, they should tolerate\n> > the bad behavior as a band-aid. So I think Dscho's patch is doing the\n> > right thing (and I do agree that we should fix the immediate CI pain by\n> > adjusting the tests, and letting the user-visible fix proceed\n> > independently).\n>\n> The tests aren't just asserting the bad behavior, they're also ensuring\n> that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but\n> tolerable, but if we start emitting 500 of these it would be nice to\n> know.\n\nI admit that this kind of argument does not sway me.\n\nIs it likely that we would suddenly start spewing 500 such warnings? If\nwe did, are there no other tests that would catch it? And even if *that*\nwere the case, would nobody happen to notice it in the meantime either\nduring development or when we queue an affected topic onto 'next' for\nwider testing?\n\nI guess the answer is that it's possible that we'd miss such a\nregression in all of those above places, but to me it seems extremely\nunlikely that we'd let such a regression through without noticing.\n\nThanks,\nTaylor\n"},{"id":"466218","messageId":"Y2GI0R6pJmdZNgHn@nand.local","threadId":"58726","inReplyTo":"221101.86o7tq4vsn.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-01T21:00:01Z","receivedAt":"2022-11-01T21:00:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 01, 2022 at 02:07:39PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> > You can also put it lower in the function, when we actually warn, which\n> > saves adding to the environment when there is nothing to warn about\n> > (though this way, we avoid doing the redundant work).\n> >\n> > Compared to munging the config, this seems shorter and simpler, and\n> > there's no chance of us ever getting confused between the fake\n> > \"suppress\" value and something the user actually asked for.\n>\n> Sure, we can do it with an environment variable, in the end that's all\n> git_config_push_parameter() is doing too. It's just setting things in\n> \"GIT_CONFIG_PARAMETERS\".\n>\n> And yes, we can set this in the low-level function instead of with\n> git_config_push_parameter() in builtin/*.c as I did. I was aiming for\n> something demonstrably narrow, at the cost of some verbosity.\n>\n> But I don't get how other things being equal you think sticking this in\n> \"GIT_CHECKED_CREDENTIALS_IN_URL\" instead of \"GIT_CONFIG_PARAMETERS\"\n> helps.\n\nI vaguely prefer calling this GIT_CHECKED_CREDENTIALS_IN_URL instead of\nstuffing it in the configuration. All other things *aren't* equal here,\nsince we're not lying to sub-processes about configuration values set\nby the user.\n\nInstead, we're saying: \"regardless of what value the user assigned\ntransfer.credentialsInUrl, we can avoid looking at it because we have\nalready checked for the presence of credentials in the URL\".\n\nThanks,\nTaylor\n"},{"id":"466223","messageId":"221101.8635b24959.gmgdl@evledraar.gmail.com","threadId":"58726","inReplyTo":"Y2GI0R6pJmdZNgHn@nand.local","subject":"Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-01T21:57:46Z","receivedAt":"2022-11-01T22:12:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 01 2022, Taylor Blau wrote:\n\n> On Tue, Nov 01, 2022 at 02:07:39PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> > You can also put it lower in the function, when we actually warn, which\n>> > saves adding to the environment when there is nothing to warn about\n>> > (though this way, we avoid doing the redundant work).\n>> >\n>> > Compared to munging the config, this seems shorter and simpler, and\n>> > there's no chance of us ever getting confused between the fake\n>> > \"suppress\" value and something the user actually asked for.\n>>\n>> Sure, we can do it with an environment variable, in the end that's all\n>> git_config_push_parameter() is doing too. It's just setting things in\n>> \"GIT_CONFIG_PARAMETERS\".\n>>\n>> And yes, we can set this in the low-level function instead of with\n>> git_config_push_parameter() in builtin/*.c as I did. I was aiming for\n>> something demonstrably narrow, at the cost of some verbosity.\n>>\n>> But I don't get how other things being equal you think sticking this in\n>> \"GIT_CHECKED_CREDENTIALS_IN_URL\" instead of \"GIT_CONFIG_PARAMETERS\"\n>> helps.\n>\n> I vaguely prefer calling this GIT_CHECKED_CREDENTIALS_IN_URL instead of\n> stuffing it in the configuration.[...]\n\nTo be clear, I'm asking if there's cases where we think one method or\nthe other produces different results, which I understood Jeff hinting\nat.\n\n> Instead, we're saying: \"regardless of what value the user assigned\n> transfer.credentialsInUrl, we can avoid looking at it because we have\n> already checked for the presence of credentials in the URL\".[...] All\n> other things *aren't* equal here, since we're not lying to\n> sub-processes about configuration values set by the user.\n\nThe user is asking us to warn about storing certain things in config, we\nknow we already warned, so we're looking to flip that value to the\n\"quiet\" setting.\n\nIf you consider that overriding I don't get how doing so via a different\nenvironment variable changes anything. It would be doing the same thing,\njust making it less obvious.\n"},{"id":"466224","messageId":"221101.86y1su2u2p.gmgdl@evledraar.gmail.com","threadId":"58726","inReplyTo":"Y2GHjnuyuwGpY3II@nand.local","subject":"Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-01T22:17:42Z","receivedAt":"2022-11-01T22:23:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 01 2022, Taylor Blau wrote:\n\n> On Tue, Nov 01, 2022 at 04:01:18AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> > Yeah, I think it is crappy UX, too. It's just that I think the tests\n>> > should not _asserting_ the bad behavior. At most, they should tolerate\n>> > the bad behavior as a band-aid. So I think Dscho's patch is doing the\n>> > right thing (and I do agree that we should fix the immediate CI pain by\n>> > adjusting the tests, and letting the user-visible fix proceed\n>> > independently).\n>>\n>> The tests aren't just asserting the bad behavior, they're also ensuring\n>> that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but\n>> tolerable, but if we start emitting 500 of these it would be nice to\n>> know.\n>\n> I admit that this kind of argument does not sway me.\n>\n> Is it likely that we would suddenly start spewing 500 such warnings? If\n> we did, are there no other tests that would catch it? And even if *that*\n> were the case, would nobody happen to notice it in the meantime either\n> during development or when we queue an affected topic onto 'next' for\n> wider testing?\n>\n> I guess the answer is that it's possible that we'd miss such a\n> regression in all of those above places, but to me it seems extremely\n> unlikely that we'd let such a regression through without noticing.\n\nLiterally 500? Probably not, that was hyperbole to make a point, but\nseveral, low tens? Yeah, I know of at least a couple in-tree off the top\nof my head.\n\nThe point, which I assumed was clear is that we literally wouldn't\nnotice if it were 500, and that sort of thing is a common pattern in our\ntests. I.e. in most cases we'd ideally test_cmp known output (at least\nto the extent of assuring ourselves that we're getting it right).\n\nInstead we often just grep it, or don't test it at all. Sometimes for a\ngood reason (e.g. the output containing absolute paths), but more often\nthan not for no good reason.\n"},{"id":"466275","messageId":"Y2G/b5L65+Kaw/WD@nand.local","threadId":"58726","inReplyTo":"221101.86y1su2u2p.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-02T00:53:03Z","receivedAt":"2022-11-02T00:53:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 01, 2022 at 11:17:42PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> >> The tests aren't just asserting the bad behavior, they're also ensuring\n> >> that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but\n> >> tolerable, but if we start emitting 500 of these it would be nice to\n> >> know.\n> >\n> > I admit that this kind of argument does not sway me.\n> >\n> > Is it likely that we would suddenly start spewing 500 such warnings? If\n> > we did, are there no other tests that would catch it? And even if *that*\n> > were the case, would nobody happen to notice it in the meantime either\n> > during development or when we queue an affected topic onto 'next' for\n> > wider testing?\n> >\n> > I guess the answer is that it's possible that we'd miss such a\n> > regression in all of those above places, but to me it seems extremely\n> > unlikely that we'd let such a regression through without noticing.\n>\n> Literally 500? Probably not, that was hyperbole to make a point, but\n> several, low tens? Yeah, I know of at least a couple in-tree off the top\n> of my head.\n\nStill, I find it suspect to assume that a developer working in this area\nwouldn't notice even a minor regression (say, adding an additional\nwarning).\n\nThanks,\nTaylor\n"},{"id":"466295","messageId":"Y2IoEN6NHqj2Qisa@coredump.intra.peff.net","threadId":"58726","inReplyTo":"221101.8635b24959.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-02T08:19:28Z","receivedAt":"2022-11-02T08:19:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 01, 2022 at 10:57:46PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> >> Sure, we can do it with an environment variable, in the end that's all\n> >> git_config_push_parameter() is doing too. It's just setting things in\n> >> \"GIT_CONFIG_PARAMETERS\".\n> >>\n> >> And yes, we can set this in the low-level function instead of with\n> >> git_config_push_parameter() in builtin/*.c as I did. I was aiming for\n> >> something demonstrably narrow, at the cost of some verbosity.\n> >>\n> >> But I don't get how other things being equal you think sticking this in\n> >> \"GIT_CHECKED_CREDENTIALS_IN_URL\" instead of \"GIT_CONFIG_PARAMETERS\"\n> >> helps.\n> >\n> > I vaguely prefer calling this GIT_CHECKED_CREDENTIALS_IN_URL instead of\n> > stuffing it in the configuration.[...]\n> \n> To be clear, I'm asking if there's cases where we think one method or\n> the other produces different results, which I understood Jeff hinting\n> at.\n\nWhat I was hinting before was not that I knew of a particular bug in\nyour patch, but that I think the technique of munging\nGIT_CONFIG_PARAMETERS is fragile in error-prone in the general case,\nbecause the sub-programs can't differentiate between the config the user\nasked for, and what was set by the suppression mechanism.\n\nFor this variable, there's no need to differentiate between \"the user\nasked us to be quiet\" and \"the calling program asked us to be quiet\",\nbut I could imagine cases where there are subtle distinctions. Imagine\nif there was a setting for \"warn and rewrite the URL\". We'd need to\nchange that to \"don't warn, but just rewrite the URL\", which otherwise\nis a mode that doesn't need to exist.\n\nKeeping it in a separate variable keeps the concerns orthogonal. The\ncode still gets to see what the user actually wants (via the config),\nbut has extra information from the calling process about how noisy/quiet\nto be.\n\nBut you mentioned submodules in your other mail. And you're right that\nthe patch I showed doesn't handle that (it would need to add the new\nvariable to local_repo_env). It seems like yours should work because\nCONFIG_DATA_ENVIRONMENT as part of local_repo_env. But I don't think it\nactually does; in prepare_other_repo_env(), we retain the variables for\nconfig in the environment, so that:\n\n git -c foo.bar=whatever fetch\n\nwill override variables in both the superproject and in submodules.\n\nI didn't try it, but I suspect with your patch that a superproject with\n\"warn\" and a submodule with \"die\" (both in their on-disk config files)\nwould misbehave. The superproject process will warn and say \"yes, I've\nchecked everything\" by munging the in-environment config to \"allow\".\nThen the submodule process will see that config, and will override the\non-disk setting (in the usual last-one-wins config way). I.e., the\nproblem is that it cannot tell the difference between \"the user asked to\noverride this\" and the suppression mechanism.\n\n-Peff\n"},{"id":"466297","messageId":"Y2ItZWx+kBmTreGQ@coredump.intra.peff.net","threadId":"58726","inReplyTo":"Y2GHjnuyuwGpY3II@nand.local","subject":"[PATCH v3 2/2] t5551: be less strict about the number of credential warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-02T08:42:13Z","receivedAt":"2022-11-02T08:42:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 01, 2022 at 04:54:38PM -0400, Taylor Blau wrote:\n\n> > The tests aren't just asserting the bad behavior, they're also ensuring\n> > that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but\n> > tolerable, but if we start emitting 500 of these it would be nice to\n> > know.\n> \n> I admit that this kind of argument does not sway me.\n> \n> Is it likely that we would suddenly start spewing 500 such warnings? If\n> we did, are there no other tests that would catch it? And even if *that*\n> were the case, would nobody happen to notice it in the meantime either\n> during development or when we queue an affected topic onto 'next' for\n> wider testing?\n> \n> I guess the answer is that it's possible that we'd miss such a\n> regression in all of those above places, but to me it seems extremely\n> unlikely that we'd let such a regression through without noticing.\n\nLike you, I don't find much value in asserting \"2 or 3, but not 500\".\nBut it is easy enough to at least only loosen the few cases that need\nit.\n\nSo here's a replacement for 2/2 that does the minimal thing. I rewrote\nthe commit message to explain my view (incidentally, it also fixes the\nsubject line, which mentioned the wrong test number after the rebase).\n\nAs I said, I had tried to mostly leave patch 2 alone to avoid derailing\nDscho's attempt to fix things. But somehow things got derailed anyway,\nso maybe we can just all agree on this patch and move on with our lives?\nI can't over-emphasize how little I care about this credentialsInUrl\nfeature in the first place, and somehow it has consumed hours of my life\nnow.\n\n-- >8 --\nSubject: t5551: be less strict about the number of credential warnings\n\nIt is unclear as to _why_, but under certain circumstances the warning\nabout credentials being passed as part of the URL seems to be swallowed\nby the `git remote-https` helper in the Windows jobs of Git's CI builds.\n\nThis causes the tests to fail, because they assert that in a few cases\nwe should print the warning 2 or even 3 times. The reason for that is\ngiven in 6dcbdc0d66 (remote: create fetch.credentialsInUrl config,\n2022-06-06), which is that multiple processes are involved, and each\nwarns.\n\nIn an ideal world, the user would just see the message once per logical\noperation; they don't care how many underlying processes are involved.\nAnd we may fix that eventually. But in the meantime, let's loosen the\ntests to just assert that the user sees the message _at least_ once.\n\nThis won't catch a case where we accidentally start producing it 500\ntimes, but that's not a likely regression. A more likely thing is that\nwe'd start producing it four times because the underlying code changes\nto use a new process. But that's exactly the kind of thing we'd prefer\nto be ignoring in the tests.\n\nNote that the tests for the \"die\" mode don't need adjusted. They die\nimmediately in the first process that sees the problem, so they\nconsistently show the error once. It's only the \"warn\" mode which must\nbe loose. If we eventually fix it, then we can tighten its assertion. In\nthe meantime, this works around the CI issues.\n\nBased on a patch by Johannes Schindelin.\n\nSigned-off-by: Jeff King \n---\n t/t5551-http-fetch-smart.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex bbe3d5721f..4f559722f4 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -597,7 +597,7 @@ test_expect_success 'clone warns or fails when using username:password' '\n \tgit -c transfer.credentialsInUrl=warn \\\n \t\tclone $url_userpass attempt2 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n-\ttest_line_count = 2 warnings &&\n+\ttest_line_count -ge 1 warnings &&\n \n \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n \t\tclone $url_userpass attempt3 2>err &&\n@@ -630,7 +630,7 @@ test_expect_success 'fetch warns or fails when using username:password' '\n \n \tgit -c transfer.credentialsInUrl=warn fetch $url_userpass 2>err &&\n \tgrep \"warning: $message\" err >warnings &&\n-\ttest_line_count = 3 warnings &&\n+\ttest_line_count -ge 1 warnings &&\n \n \ttest_must_fail git -c transfer.credentialsInUrl=die \\\n \t\tfetch $url_userpass 2>err &&\n-- \n2.38.1.677.g9b1428ac2e\n\n"},{"id":"466298","messageId":"CAPig+cSj78Mh8v6mN3BtyHJ+NxOvT2H_NW6SPFSAce0op2DfjA@mail.gmail.com","threadId":"58726","inReplyTo":"Y2ItZWx+kBmTreGQ@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-02T08:49:37Z","receivedAt":"2022-11-02T08:49:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 2, 2022 at 4:46 AM Jeff King wrote:\n> Subject: t5551: be less strict about the number of credential warnings\n>\n> It is unclear as to _why_, but under certain circumstances the warning\n> about credentials being passed as part of the URL seems to be swallowed\n> by the `git remote-https` helper in the Windows jobs of Git's CI builds.\n>\n> This causes the tests to fail, because they assert that in a few cases\n> we should print the warning 2 or even 3 times. The reason for that is\n> given in 6dcbdc0d66 (remote: create fetch.credentialsInUrl config,\n> 2022-06-06), which is that multiple processes are involved, and each\n> warns.\n>\n> In an ideal world, the user would just see the message once per logical\n> operation; they don't care how many underlying processes are involved.\n> And we may fix that eventually. But in the meantime, let's loosen the\n> tests to just assert that the user sees the message _at least_ once.\n>\n> This won't catch a case where we accidentally start producing it 500\n> times, but that's not a likely regression. A more likely thing is that\n> we'd start producing it four times because the underlying code changes\n> to use a new process. But that's exactly the kind of thing we'd prefer\n> to be ignoring in the tests.\n>\n> Note that the tests for the \"die\" mode don't need adjusted. They die\n\ns/adjusted/adjustment --or -- s/need/& to be/\n\n> immediately in the first process that sees the problem, so they\n> consistently show the error once. It's only the \"warn\" mode which must\n> be loose. If we eventually fix it, then we can tighten its assertion. In\n> the meantime, this works around the CI issues.\n>\n> Based on a patch by Johannes Schindelin.\n>\n> Signed-off-by: Jeff King \n"},{"id":"466300","messageId":"Y2I1FsYyIRCCwh30@coredump.intra.peff.net","threadId":"58726","inReplyTo":"CAPig+cSj78Mh8v6mN3BtyHJ+NxOvT2H_NW6SPFSAce0op2DfjA@mail.gmail.com","subject":"Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-02T09:15:02Z","receivedAt":"2022-11-02T09:16:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 02, 2022 at 04:49:37AM -0400, Eric Sunshine wrote:\n\n> > Note that the tests for the \"die\" mode don't need adjusted. They die\n> \n> s/adjusted/adjustment --or -- s/need/& to be/\n\nhttps://english.stackexchange.com/questions/5407/central-pennsylvanian-english-speakers-what-are-the-limitations-on-the-needs-w\n\nDon't stomp on my linguistic heritage. :)\n\n-Peff\n"},{"id":"466301","messageId":"Y2I15UY/a7OAZUtm@coredump.intra.peff.net","threadId":"58726","inReplyTo":"Y2ItZWx+kBmTreGQ@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-02T09:18:29Z","receivedAt":"2022-11-02T09:21:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 02, 2022 at 04:42:13AM -0400, Jeff King wrote:\n\n> As I said, I had tried to mostly leave patch 2 alone to avoid derailing\n> Dscho's attempt to fix things. But somehow things got derailed anyway,\n> so maybe we can just all agree on this patch and move on with our lives?\n\nBy the way, I think you (or somebody?) mentioned elsewhere in the thread\nthat it's possible the first patch fixes things by itself. I would also\nbe content to just apply the first one and see if CI improves.\n\nOf course, when I just pushed all this out to CI, it flaked\nindependently on both osx (looks like racy p4 stuff) and fedora (network\ndropout failed to set up the container). Sigh.\n\n-Peff\n"},{"id":"466303","messageId":"CAPig+cTZfckXjX5=nD=cufhus_m9G8LgPqA2dtzKW0AT2bqLgw@mail.gmail.com","threadId":"58726","inReplyTo":"Y2I1FsYyIRCCwh30@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-02T09:31:02Z","receivedAt":"2022-11-02T09:31:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 2, 2022 at 5:15 AM Jeff King wrote:\n> On Wed, Nov 02, 2022 at 04:49:37AM -0400, Eric Sunshine wrote:\n> > > Note that the tests for the \"die\" mode don't need adjusted. They die\n> >\n> > s/adjusted/adjustment --or -- s/need/& to be/\n>\n> https://english.stackexchange.com/questions/5407/central-pennsylvanian-english-speakers-what-are-the-limitations-on-the-needs-w\n>\n> Don't stomp on my linguistic heritage. :)\n\nSorry. My head needs hanged in shame. I forgot that I can't grammar.\n(I can't math either.)\n"},{"id":"466371","messageId":"Y2MZ46aE/P7jpabG@nand.local","threadId":"58726","inReplyTo":"Y2I15UY/a7OAZUtm@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-03T01:31:15Z","receivedAt":"2022-11-03T01:31:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 02, 2022 at 05:18:29AM -0400, Jeff King wrote:\n> On Wed, Nov 02, 2022 at 04:42:13AM -0400, Jeff King wrote:\n>\n> > As I said, I had tried to mostly leave patch 2 alone to avoid derailing\n> > Dscho's attempt to fix things. But somehow things got derailed anyway,\n> > so maybe we can just all agree on this patch and move on with our lives?\n>\n> By the way, I think you (or somebody?) mentioned elsewhere in the thread\n> that it's possible the first patch fixes things by itself. I would also\n> be content to just apply the first one and see if CI improves.\n>\n> Of course, when I just pushed all this out to CI, it flaked\n> independently on both osx (looks like racy p4 stuff) and fedora (network\n> dropout failed to set up the container). Sigh.\n\nDreams are free, eh?\n\nThanks,\nTaylor\n"},{"id":"466495","messageId":"221104.8635azysw7.gmgdl@evledraar.gmail.com","threadId":"58726","inReplyTo":"Y2IoEN6NHqj2Qisa@coredump.intra.peff.net","subject":"Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-04T09:01:00Z","receivedAt":"2022-11-04T09:51:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 02 2022, Jeff King wrote:\n\n> On Tue, Nov 01, 2022 at 10:57:46PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> >> Sure, we can do it with an environment variable, in the end that's all\n>> >> git_config_push_parameter() is doing too. It's just setting things in\n>> >> \"GIT_CONFIG_PARAMETERS\".\n>> >>\n>> >> And yes, we can set this in the low-level function instead of with\n>> >> git_config_push_parameter() in builtin/*.c as I did. I was aiming for\n>> >> something demonstrably narrow, at the cost of some verbosity.\n>> >>\n>> >> But I don't get how other things being equal you think sticking this in\n>> >> \"GIT_CHECKED_CREDENTIALS_IN_URL\" instead of \"GIT_CONFIG_PARAMETERS\"\n>> >> helps.\n>> >\n>> > I vaguely prefer calling this GIT_CHECKED_CREDENTIALS_IN_URL instead of\n>> > stuffing it in the configuration.[...]\n>> \n>> To be clear, I'm asking if there's cases where we think one method or\n>> the other produces different results, which I understood Jeff hinting\n>> at.\n>\n> What I was hinting before was not that I knew of a particular bug in\n> your patch, but that I think the technique of munging\n> GIT_CONFIG_PARAMETERS is fragile in error-prone in the general case,\n> because the sub-programs can't differentiate between the config the user\n> asked for, and what was set by the suppression mechanism.\n>\n> For this variable, there's no need to differentiate between \"the user\n> asked us to be quiet\" and \"the calling program asked us to be quiet\",\n> but I could imagine cases where there are subtle distinctions. Imagine\n> if there was a setting for \"warn and rewrite the URL\". We'd need to\n> change that to \"don't warn, but just rewrite the URL\", which otherwise\n> is a mode that doesn't need to exist.\n>\n> Keeping it in a separate variable keeps the concerns orthogonal. The\n> code still gets to see what the user actually wants (via the config),\n> but has extra information from the calling process about how noisy/quiet\n> to be.\n\n... (replied below) ...\n\n> But you mentioned submodules in your other mail. And you're right that\n> the patch I showed doesn't handle that (it would need to add the new\n> variable to local_repo_env). It seems like yours should work because\n> CONFIG_DATA_ENVIRONMENT as part of local_repo_env. But I don't think it\n> actually does; in prepare_other_repo_env(), we retain the variables for\n> config in the environment, so that:\n>\n> git -c foo.bar=whatever fetch\n>\n> will override variables in both the superproject and in submodules.\n\nReplying to your main point below, just an aside on this:\n\nTo be clear I'm not saying it would handle it sensibly now, but just\nthat if we're using env vars to communicate to sub-processes then using\nCONFIG_DATA_ENVIRONMENT seems better to me.\n\nBecause even if we're getting it wrong now, then surely that's something\nwe're probably getting wrong in more than one place.\n\nSo e.g. if we set an env var \"for ourselves\", i.e. \"pull->fetch\" then we\ncould detect that condition in run_command(), and if we then spot that\nwe're carrying an env variable we set earlier up the chain reset it\nbefore we spawn a submodule.\n\nWhereas if it's all custom env variables here & there we'll need to add\nall that special-casing in.\n\n> I didn't try it, but I suspect with your patch that a superproject with\n> \"warn\" and a submodule with \"die\" (both in their on-disk config files)\n> would misbehave. The superproject process will warn and say \"yes, I've\n> checked everything\" by munging the in-environment config to \"allow\".\n> Then the submodule process will see that config, and will override the\n> on-disk setting (in the usual last-one-wins config way). I.e., the\n> problem is that it cannot tell the difference between \"the user asked to\n> override this\" and the suppression mechanism.\n\nYes, definitely, and now I see what you're saying. I.e. imagine a chain\nlike this (not actual process chains, but let's go with the example);\n\n\tuser config = warn\n\trun: pull\n\tour config = allow\n\t\t# OK: doesn't \"warn\" now\n\t\trun: fetch\n\t\t\t# Not warning, but ....\n\t\t\trun: pre-fetch hook\n\t\t\t\t# BAD: ...oh oh, now a hook is fetching some\n # entirely unrelated repo\n\t\t\t\trun: git pull\n\t\t\t# OK: Doesn't warn either\n\t\t\trun: remote-curl (now not warning, otherwise would)\n # BAD: our \"warned already\" has infected a\n # submodule, and we downgrade \"die\" to \"allow\"\n\t\t\tuser config = die\n\t\t\trun: git fetch \n\t\t\t\t...\n\nBut, and maybe I'm still not getting it, but the \"use a different env\nvar\" isn't actually the important part, i.e. these would behave the\nsame after the initial \"warn\":\n\n\t-c transfer.credentialsInUrlWarnedAlready=true\n\nAnd:\n\n\tGIT_CHECKED_AND_WARNED_ALREADY=1\n\nBut not what I was suggesting:\n\n\t# conflated with a later \"die\"\n\t-c transfer.credentialsInUrl=allow\n\nBut that only goes for e.g. a \"pull\" where we \"warn\" followed by\nsubmodule whose config is \"die\".\n\nBut all suggested variants of this (mine and yours) are going to get\ne.g. the case where the submodule also wants \"warn\". I.e. it's not\nenough that we're saying \"warned already\".\n\nThat gets us past conflating an existing \"warn\" with a \"die\", but now we\ncan't tell a submodule \"warn\" from a superproject \"warn\".\n\nFor that we'd basically need to do:\n\n\t-c transfer.$().credentialsInUrl=allow\n\nOr another similar mechanism, of course the most sane one to be to not\nbe playing this game at all, but to just ferry this state e.g. with\n\"--do-not-warn-about-this-repo\" to our own children, which we'd not add\nto the command-lines when we know we're spawning a hook, or doing the\nsubmodule \"pull/fetch\".\n\nDoes that all seem right?\n\n\n\n"},{"id":"466514","messageId":"Y2UQxGqE9wXYVrTs@coredump.intra.peff.net","threadId":"58726","inReplyTo":"221104.8635azysw7.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-04T13:16:52Z","receivedAt":"2022-11-04T13:17:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 04, 2022 at 10:01:00AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > But you mentioned submodules in your other mail. And you're right that\n> > the patch I showed doesn't handle that (it would need to add the new\n> > variable to local_repo_env). It seems like yours should work because\n> > CONFIG_DATA_ENVIRONMENT as part of local_repo_env. But I don't think it\n> > actually does; in prepare_other_repo_env(), we retain the variables for\n> > config in the environment, so that:\n> >\n> > git -c foo.bar=whatever fetch\n> >\n> > will override variables in both the superproject and in submodules.\n> \n> Replying to your main point below, just an aside on this:\n> \n> To be clear I'm not saying it would handle it sensibly now, but just\n> that if we're using env vars to communicate to sub-processes then using\n> CONFIG_DATA_ENVIRONMENT seems better to me.\n> \n> Because even if we're getting it wrong now, then surely that's something\n> we're probably getting wrong in more than one place.\n> \n> So e.g. if we set an env var \"for ourselves\", i.e. \"pull->fetch\" then we\n> could detect that condition in run_command(), and if we then spot that\n> we're carrying an env variable we set earlier up the chain reset it\n> before we spawn a submodule.\n> \n> Whereas if it's all custom env variables here & there we'll need to add\n> all that special-casing in.\n\nThe idea is that local_repo_env carries that information, and everybody\nuses it. So yes, it would have to know about the custom env variables,\nbut then any place which enters another repo learns about it \"for free\".\nBut using CONFIG_DATA_ENVIRONMENT for this doesn't work, because we\ndon't actually clear it when moving between repositories.\n\nAnd in the example I showed, I used a config variable that was specific\nto this particular problem. But if there were a lot of these, it really\ncould be:\n\n GIT_CHECKED_AND_WARNED='remotes some-other-subsytem etc'\n\nso that all of them were shoved into one variable, and parsed. But we\ncan do the simplest dumb thing for now, because none of this is a public\ninterface we need to support across versions. We could move from one to\nthe other later.\n\n> But, and maybe I'm still not getting it, but the \"use a different env\n> var\" isn't actually the important part, i.e. these would behave the\n> same after the initial \"warn\":\n> \n> \t-c transfer.credentialsInUrlWarnedAlready=true\n> \n> And:\n> \n> \tGIT_CHECKED_AND_WARNED_ALREADY=1\n> \n> But not what I was suggesting:\n> \n> \t# conflated with a later \"die\"\n> \t-c transfer.credentialsInUrl=allow\n\nRight. The important thing is not that it's in a different env variable\nin particular, but that it _isn't_ using the same config variable that\nthe user would set.\n\nBut once you are not using that same config variable, the question is:\ndo you want it put in with the config data or not? And I think we have\nseen that putting it with the config data is not going to work. We don't\nclear those environment variables when moving between repositories.\n\nNow obviously if the env-clearing code knew which config variables were\nto be cleared and which not, it could do so. But why introduce that\ncomplexity, when you can just stick all the stuff that should be cleared\ninto a new variable (and again, that can be _one_ variable for all of\nthis stuff).\n\n> But all suggested variants of this (mine and yours) are going to get\n> e.g. the case where the submodule also wants \"warn\". I.e. it's not\n> enough that we're saying \"warned already\".\n> \n> That gets us past conflating an existing \"warn\" with a \"die\", but now we\n> can't tell a submodule \"warn\" from a superproject \"warn\".\n> \n> For that we'd basically need to do:\n> \n> \t-c transfer.$().credentialsInUrl=allow\n> \n> Or another similar mechanism, of course the most sane one to be to not\n> be playing this game at all, but to just ferry this state e.g. with\n> \"--do-not-warn-about-this-repo\" to our own children, which we'd not add\n> to the command-lines when we know we're spawning a hook, or doing the\n> submodule \"pull/fetch\".\n> \n> Does that all seem right?\n\nThat seems much more complicated. Again, why bother stuffing this extra\ncontext information into a config variable, just to work around the fact\nthat config isn't cleared between repositories, when there is already a\nsimple mechanism for clearing state when switching repositories?\n\nAll that said, the implementation of this whole warning seems really\nweird to me. If we want to warn about using such a URL, then we should\ndo it at the point of use. From the original commit message, the only\nreason it was put where it is was to avoid sigpipe when the remote\nhelper dies. The solution there seems to be \"have the helper die in a\nless abrupt way\".\n\nAnd TBH, I'm skeptical of the whole feature. I don't see the point in\neven warning somebody about:\n\n git fetch https://username:password@example.com\n\nMaybe it's bad practice (though with password stand-ins like\nrestricted-access tokens, it isn't always), but at the point the user\nhas invoked git, there's nothing else bad that happens by doing what\nthey asked for.\n\nWhat _is_ potentially bad is using that URL for cloning, because we then\nwrite the credential in plaintext to the config file. And a better\nsolution there is probably to issue a warning, clone anyway, and then\nomit the password from what we write into config. A follow-on fetch\nwould fail, but that is not any worse than the \"die\" mode of this\ncurrent config option.\n\nThere were even patches we discussed a few years ago:\n\n https://lore.kernel.org/git/20190517222031.GA17966@sigill.intra.peff.net/\n\nI think the only really contentious part was the third one, which tried\nto do so automagic with credential helpers. If we skipped that, then the\nmode given by patches 1+2 seems strictly better than anything this\ntransfer.credentialsInUrl option provides.\n\n-Peff\n"}]}