* [PATCH 0/2] t5516/t5601: avoid using localhost for failing HTTPS requests @ 2022-10-31 19:47 Johannes Schindelin via GitGitGadget 2022-10-31 19:47 ` [PATCH 1/2] t5516/t5601: avoid using `localhost` " Johannes Schindelin via GitGitGadget ` (2 more replies) 0 siblings, 3 replies; 45+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2022-10-31 19:47 UTC (permalink / raw) To: git; +Cc: Derrick Stolee, Johannes Schindelin While trying to figure out why the current CI builds fail in the Windows jobs [https://github.com/git/git/actions/runs/3358829078#annotations], I noticed that I cannot really run the offending test cases on my box because the supposedly failing connection to https://localhost never seems to time out as intended. This patch fixes that, and incidentally also seems to fix the CI failures. Johannes Schindelin (2): t5516/t5601: avoid using `localhost` for failing HTTPS requests t5516/t5601: be less strict about the number of credential warnings t/t5516-fetch-push.sh | 26 +++++++++++++------------- t/t5601-clone.sh | 18 +++++++++--------- 2 files changed, 22 insertions(+), 22 deletions(-) base-commit: c03801e19cb8ab36e9c0d17ff3d5e0c3b0f24193 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1399%2Fdscho%2Favoid-using-localhost-in-url-tests-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1399/dscho/avoid-using-localhost-in-url-tests-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1399 -- gitgitgadget ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests 2022-10-31 19:47 [PATCH 0/2] t5516/t5601: avoid using localhost for failing HTTPS requests Johannes Schindelin via GitGitGadget @ 2022-10-31 19:47 ` Johannes Schindelin via GitGitGadget 2022-10-31 20:49 ` Ævar Arnfjörð Bjarmason 2022-10-31 23:20 ` Jeff King 2022-10-31 19:47 ` Johannes Schindelin via GitGitGadget 2022-10-31 20:47 ` [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings Ævar Arnfjörð Bjarmason 2 siblings, 2 replies; 45+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2022-10-31 19:47 UTC (permalink / raw) To: git; +Cc: Derrick Stolee, Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> In 6dcbdc0d6616 (remote: create fetch.credentialsInUrl config, 2022-06-06), we added four test cases that validate various behavior around passing credentials as part of the URL (which is considered unsafe in general). These tests do not _actually_ try to connect anywhere, but have to use the https:// protocol in order to validate the intended code paths. However, using `localhost` for such a connection causes several problems: - There might be a web server running on localhost, and we do not actually want to connect to that. - The DNS resolver, or the local firewall, might take a substantial amount of time (or forever, whichever comes first) to fail to connect, slowing down the test cases unnecessarily. Let's instead use an IPv4 address that is guaranteed never to offer a web server: 224.0.0.1 (which is part of the IP multicast range). Incidentally, this seems to fix an issue where the tests fail in the Windows jobs of Git's CI builds. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- t/t5516-fetch-push.sh | 18 +++++++++--------- t/t5601-clone.sh | 10 +++++----- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 79dc470c014..8dd4610a8c2 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1854,32 +1854,32 @@ test_expect_success 'refuse to push a hidden ref, and make sure do not pollute t ' test_expect_success LIBCURL 'fetch warns or fails when using username:password' ' - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && - test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err && + message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" && + test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err && ! grep "$message" err && - test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@224.0.0.1 2>err && grep "warning: $message" err >warnings && test_line_count = 3 warnings && - test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@224.0.0.1 2>err && grep "fatal: $message" err >warnings && test_line_count = 1 warnings && - test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@224.0.0.1 2>err && grep "fatal: $message" err >warnings && test_line_count = 1 warnings ' test_expect_success LIBCURL 'push warns or fails when using username:password' ' - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && - test_must_fail git -c transfer.credentialsInUrl=allow push https://username:password@localhost 2>err && + message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" && + test_must_fail git -c transfer.credentialsInUrl=allow push https://username:password@224.0.0.1 2>err && ! grep "$message" err && - test_must_fail git -c transfer.credentialsInUrl=warn push https://username:password@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=warn push https://username:password@224.0.0.1 2>err && grep "warning: $message" err >warnings && - test_must_fail git -c transfer.credentialsInUrl=die push https://username:password@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=die push https://username:password@224.0.0.1 2>err && grep "fatal: $message" err >warnings && test_line_count = 1 warnings ' diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 45f0803ed4d..0b386c74818 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -72,19 +72,19 @@ test_expect_success 'clone respects GIT_WORK_TREE' ' ' test_expect_success LIBCURL 'clone warns or fails when using username:password' ' - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && - test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err && + message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" && + test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@224.0.0.1 attempt1 2>err && ! grep "$message" err && - test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err && + test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@224.0.0.1 attempt2 2>err && grep "warning: $message" err >warnings && test_line_count = 2 warnings && - test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err && + test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@224.0.0.1 attempt3 2>err && grep "fatal: $message" err >warnings && test_line_count = 1 warnings && - test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@localhost attempt3 2>err && + test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@224.0.0.1 attempt3 2>err && grep "fatal: $message" err >warnings && test_line_count = 1 warnings ' -- gitgitgadget ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests 2022-10-31 19:47 ` [PATCH 1/2] t5516/t5601: avoid using `localhost` " Johannes Schindelin via GitGitGadget @ 2022-10-31 20:49 ` Ævar Arnfjörð Bjarmason 2022-10-31 23:20 ` Jeff King 1 sibling, 0 replies; 45+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-10-31 20:49 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Derrick Stolee, Johannes Schindelin On Mon, Oct 31 2022, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > In 6dcbdc0d6616 (remote: create fetch.credentialsInUrl config, > 2022-06-06), we added four test cases that validate various behavior > around passing credentials as part of the URL (which is considered > unsafe in general). > > These tests do not _actually_ try to connect anywhere, but have to use > the https:// protocol in order to validate the intended code paths. > > However, using `localhost` for such a connection causes several > problems: > > - There might be a web server running on localhost, and we do not > actually want to connect to that. > > - The DNS resolver, or the local firewall, might take a substantial > amount of time (or forever, whichever comes first) to fail to connect, > slowing down the test cases unnecessarily. > > Let's instead use an IPv4 address that is guaranteed never to offer a > web server: 224.0.0.1 (which is part of the IP multicast range). > > Incidentally, this seems to fix an issue where the tests fail in the > Windows jobs of Git's CI builds. > [...] > diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh > index 45f0803ed4d..0b386c74818 100755 > --- a/t/t5601-clone.sh > +++ b/t/t5601-clone.sh > @@ -72,19 +72,19 @@ test_expect_success 'clone respects GIT_WORK_TREE' ' > ' > > test_expect_success LIBCURL 'clone warns or fails when using username:password' ' > - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && > - test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err && > + message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" && > + test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@224.0.0.1 attempt1 2>err && > ! grep "$message" err && > > - test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err && > + test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@224.0.0.1 attempt2 2>err && > grep "warning: $message" err >warnings && > test_line_count = 2 warnings && > > - test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err && > + test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@224.0.0.1 attempt3 2>err && > grep "fatal: $message" err >warnings && > test_line_count = 1 warnings && > > - test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@localhost attempt3 2>err && > + test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@224.0.0.1 attempt3 2>err && > grep "fatal: $message" err >warnings && > test_line_count = 1 warnings > ' For this one one at least, it eventually gets around to setting up an actual httpd server with cloning etc. from $HTTPD_URL. Can't we just do that for both of these tests rather than the the 224.0.0.0 hack? I.e. the root cause is that we're cleverly faking a not-a-server here, and now we're going to add another somewhat clever hack on top. but since the test coverage is for https:// anyway, and we have other https tests against an actual server... ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests 2022-10-31 19:47 ` [PATCH 1/2] t5516/t5601: avoid using `localhost` " Johannes Schindelin via GitGitGadget 2022-10-31 20:49 ` Ævar Arnfjörð Bjarmason @ 2022-10-31 23:20 ` Jeff King 2022-11-01 0:59 ` Taylor Blau 2022-11-01 2:03 ` Jeff King 1 sibling, 2 replies; 45+ messages in thread From: Jeff King @ 2022-10-31 23:20 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Derrick Stolee, Johannes Schindelin On Mon, Oct 31, 2022 at 07:47:17PM +0000, Johannes Schindelin via GitGitGadget wrote: > In 6dcbdc0d6616 (remote: create fetch.credentialsInUrl config, > 2022-06-06), we added four test cases that validate various behavior > around passing credentials as part of the URL (which is considered > unsafe in general). > > These tests do not _actually_ try to connect anywhere, but have to use > the https:// protocol in order to validate the intended code paths. By "actually" here, I assume you mean "they do not expect to succeed". But I think the first one (with credentialsInUrl=allow), does try to make a connection. > However, using `localhost` for such a connection causes several > problems: > > - There might be a web server running on localhost, and we do not > actually want to connect to that. > > - The DNS resolver, or the local firewall, might take a substantial > amount of time (or forever, whichever comes first) to fail to connect, > slowing down the test cases unnecessarily. Right. I think we assume that DNS resolution of localhost is fast-ish, as we use it in other https tests. But I could certainly imagine a local firewall causing issues (especially as this is real port 443, whereas our other tests are usually high ports). > Let's instead use an IPv4 address that is guaranteed never to offer a > web server: 224.0.0.1 (which is part of the IP multicast range). This feels pretty magical. I think it would be pretty unlikely for it to have a web server, but I wouldn't be surprised if there are systems where we get similar IP-routing hangs. Is there a reason not to move all of these tests into t5550 or t5551, where we have a real http server? That would be less magical, and then this first test: > test_expect_success LIBCURL 'fetch warns or fails when using username:password' ' > - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && > - test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err && > + message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" && > + test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err && > ! grep "$message" err && could be more robust. It would actually check that we succeeded in using the URL. -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests 2022-10-31 23:20 ` Jeff King @ 2022-11-01 0:59 ` Taylor Blau 2022-11-01 2:28 ` Jeff King 2022-11-01 2:03 ` Jeff King 1 sibling, 1 reply; 45+ messages in thread From: Taylor Blau @ 2022-11-01 0:59 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin via GitGitGadget, git, Derrick Stolee, Johannes Schindelin On Mon, Oct 31, 2022 at 07:20:11PM -0400, Jeff King wrote: > > Let's instead use an IPv4 address that is guaranteed never to offer a > > web server: 224.0.0.1 (which is part of the IP multicast range). > > This feels pretty magical. I think it would be pretty unlikely for it to > have a web server, but I wouldn't be surprised if there are systems > where we get similar IP-routing hangs. > > Is there a reason not to move all of these tests into t5550 or t5551, > where we have a real http server? That would be less magical, and then > this first test: > > > test_expect_success LIBCURL 'fetch warns or fails when using username:password' ' > > - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && > > - test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err && > > + message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" && > > + test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err && > > ! grep "$message" err && > > could be more robust. It would actually check that we succeeded in using > the URL. It is magical, indeed. If it's significantly more difficult to move these into t5550 or t5551, then I'm OK with this in the meantime (since GitHub Actions really is our primary CI target that we care about this not hanging on). But assuming that moving these around isn't that difficult, I would be slightly happier to see these tests in one of the aforementioned spots. Thanks, Taylor ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests 2022-11-01 0:59 ` Taylor Blau @ 2022-11-01 2:28 ` Jeff King 0 siblings, 0 replies; 45+ messages in thread From: Jeff King @ 2022-11-01 2:28 UTC (permalink / raw) To: Taylor Blau Cc: Johannes Schindelin via GitGitGadget, git, Derrick Stolee, Johannes Schindelin On Mon, Oct 31, 2022 at 08:59:21PM -0400, Taylor Blau wrote: > > > test_expect_success LIBCURL 'fetch warns or fails when using username:password' ' > > > - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && > > > - test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err && > > > + message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" && > > > + test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err && > > > ! grep "$message" err && > > > > could be more robust. It would actually check that we succeeded in using > > the URL. > > It is magical, indeed. If it's significantly more difficult to move > these into t5550 or t5551, then I'm OK with this in the meantime (since > GitHub Actions really is our primary CI target that we care about this > not hanging on). > > But assuming that moving these around isn't that difficult, I would be > slightly happier to see these tests in one of the aforementioned spots. I don't think it was too difficult to move them. I just sent in patches (which I just realized you were not cc'd on). -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests 2022-10-31 23:20 ` Jeff King 2022-11-01 0:59 ` Taylor Blau @ 2022-11-01 2:03 ` Jeff King 2022-11-01 2:25 ` Jeff King 1 sibling, 1 reply; 45+ messages in thread From: Jeff King @ 2022-11-01 2:03 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Derrick Stolee, Johannes Schindelin On Mon, Oct 31, 2022 at 07:20:11PM -0400, Jeff King wrote: > > - The DNS resolver, or the local firewall, might take a substantial > > amount of time (or forever, whichever comes first) to fail to connect, > > slowing down the test cases unnecessarily. > > Right. I think we assume that DNS resolution of localhost is fast-ish, > as we use it in other https tests. But I could certainly imagine a local > firewall causing issues (especially as this is real port 443, whereas > our other tests are usually high ports). Actually, I am wrong about DNS here. We use a bare 127.0.0.1 in lib-httpd.sh, so DNS may indeed be the source of the issue here. That _might_ mean that using the bare IP would be safe here, but I would not want to bet on it. Using different port numbers, plus not expecting a listener on the other end, might cause different outcomes than we see in the other tests. I do think moving these tests to use a real http server is a better solution, though. I'll provide a patch in a moment. -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 1/2] t5516/t5601: avoid using `localhost` for failing HTTPS requests 2022-11-01 2:03 ` Jeff King @ 2022-11-01 2:25 ` Jeff King 2022-11-01 2:26 ` [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516 Jeff King 2022-11-01 2:26 ` [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings Jeff King 0 siblings, 2 replies; 45+ messages in thread From: Jeff King @ 2022-11-01 2:25 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Derrick Stolee, Johannes Schindelin On Mon, Oct 31, 2022 at 10:03:58PM -0400, Jeff King wrote: > I do think moving these tests to use a real http server is a better > solution, though. I'll provide a patch in a moment. Here that is. I hope this isn't co-opting your series; my goal was to do the work so that you wouldn't have to, but I admit it is assuming you agree with my approach. ;) The first patch is the interesting one. The second one is just your 2/2 rebased onto the new location. [1/2]: t5516: move plaintext-password tests from t5601 and t5516 [2/2]: t5516/t5601: be less strict about the number of credential warnings t/t5516-fetch-push.sh | 31 --------------- t/t5551-http-fetch-smart.sh | 77 +++++++++++++++++++++++++++++++++++++ t/t5601-clone.sh | 23 ----------- 3 files changed, 77 insertions(+), 54 deletions(-) -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516 2022-11-01 2:25 ` Jeff King @ 2022-11-01 2:26 ` Jeff King 2022-11-01 3:18 ` Ævar Arnfjörð Bjarmason 2022-11-01 2:26 ` [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings Jeff King 1 sibling, 1 reply; 45+ messages in thread From: Jeff King @ 2022-11-01 2:26 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Derrick Stolee, Johannes Schindelin Commit 6dcbdc0d66 (remote: create fetch.credentialsInUrl config, 2022-06-06) added tests for our handling of passwords in URLs. Since the obvious URL to be affected is git-over-http, the tests use http. However they don't set up a test server; they just try to access https://localhost, assuming it will fail (because the nothing is listening there). This causes some possible problems: - There might be a web server running on localhost, and we do not actually want to connect to that. - The DNS resolver, or the local firewall, might take a substantial amount of time (or forever, whichever comes first) to fail to connect, slowing down the tests cases unnecessarily. - Since there's no server, our tests for "allow" and "warn" still expect the clone/fetch/push operations to fail, even though in the real world we'd expect these to succeed. We scrape stderr to see what happened, but it's not as robust as a more realistic test. Let's instead move these to t5551, which is all about testing http and where we have a real server. That eliminates any issues with contacting a strange URL, and lets the "allow" and "warn" tests confirm that the operation actually succeeds. It's not quite a verbatim move for a few reasons: - we can drop the LIBCURL dependency; it's already part of lib-httpd.sh - we'll use HTTPD_URL_USER_PASS, etc, instead of our fake URL. To avoid repetition, we'll add a few extra variables. - the "https://username:@localhost" test uses a funny URL that lib-httpd.sh doesn't provide. We'll similarly construct it in a variable. Note that we're hard-coding the lib-httpd username here, but t5551 already does that everywhere. - for the "domain:port" test, the URL provided by lib-httpd is fine, since our test server will always be on an exotic port. But we'll confirm in the test that this is so. - since our message-matching is done via grep, I simplified it to use a regex, rather than trying to massage lib-httpd's variables. Arguably this makes it more readable, too, while retaining the bits we care about: the fatal/warning distinction, the "uses plaintext" message, and the fact that the password was redacted. - we'll use the /auth/ path for the repo, which shows that we are indeed making use of the auth information when needed. - we'll also use /smart/; most of these tests could be done via /dumb/ in t5550, but setting up pushes there requires extra effort and dependencies. The smart protocol is what most everyone is using these days anyway. This patch is my own, but I stole the analysis and a few bits of the commit message from a patch by Johannes Schindelin. Signed-off-by: Jeff King <peff@peff.net> --- t/t5516-fetch-push.sh | 31 --------------- t/t5551-http-fetch-smart.sh | 77 +++++++++++++++++++++++++++++++++++++ t/t5601-clone.sh | 23 ----------- 3 files changed, 77 insertions(+), 54 deletions(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 79dc470c01..4f2bfaf005 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1853,37 +1853,6 @@ test_expect_success 'refuse to push a hidden ref, and make sure do not pollute t test_dir_is_empty testrepo/.git/objects/pack ' -test_expect_success LIBCURL 'fetch warns or fails when using username:password' ' - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && - test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err && - ! grep "$message" err && - - test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@localhost 2>err && - grep "warning: $message" err >warnings && - test_line_count = 3 warnings && - - test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@localhost 2>err && - grep "fatal: $message" err >warnings && - test_line_count = 1 warnings && - - test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@localhost 2>err && - grep "fatal: $message" err >warnings && - test_line_count = 1 warnings -' - - -test_expect_success LIBCURL 'push warns or fails when using username:password' ' - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && - test_must_fail git -c transfer.credentialsInUrl=allow push https://username:password@localhost 2>err && - ! grep "$message" err && - - test_must_fail git -c transfer.credentialsInUrl=warn push https://username:password@localhost 2>err && - grep "warning: $message" err >warnings && - test_must_fail git -c transfer.credentialsInUrl=die push https://username:password@localhost 2>err && - grep "fatal: $message" err >warnings && - test_line_count = 1 warnings -' - test_expect_success 'push with config push.useBitmaps' ' mk_test testrepo heads/main && git checkout main && diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 6a38294a47..bbe3d5721f 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -580,4 +580,81 @@ test_expect_success 'passing hostname resolution information works' ' git -c "http.curloptResolve=$BOGUS_HOST:$LIB_HTTPD_PORT:127.0.0.1" ls-remote "$BOGUS_HTTPD_URL/smart/repo.git" >/dev/null ' +# here user%40host is the URL-encoded version of user@host, +# which is our intentionally-odd username to catch parsing errors +url_user=$HTTPD_URL_USER/auth/smart/repo.git +url_userpass=$HTTPD_URL_USER_PASS/auth/smart/repo.git +url_userblank=$HTTPD_PROTO://user%40host:@$HTTPD_DEST/auth/smart/repo.git +message="URL .*:<redacted>@.* uses plaintext credentials" + +test_expect_success 'clone warns or fails when using username:password' ' + test_when_finished "rm -rf attempt*" && + + git -c transfer.credentialsInUrl=allow \ + clone $url_userpass attempt1 2>err && + ! grep "$message" err && + + git -c transfer.credentialsInUrl=warn \ + clone $url_userpass attempt2 2>err && + grep "warning: $message" err >warnings && + test_line_count = 2 warnings && + + test_must_fail git -c transfer.credentialsInUrl=die \ + clone $url_userpass attempt3 2>err && + grep "fatal: $message" err >warnings && + test_line_count = 1 warnings && + + test_must_fail git -c transfer.credentialsInUrl=die \ + clone $url_userblank attempt4 2>err && + grep "fatal: $message" err >warnings && + test_line_count = 1 warnings +' + +test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' ' + test_when_finished "rm -rf attempt1" && + + # we are relying on lib-httpd for url construction, so document our + # assumptions + case "$HTTPD_URL_USER" in + *:[0-9]*) : ok ;; + *) BUG "httpd url does not have port: $HTTPD_URL_USER" + esac && + + git -c transfer.credentialsInUrl=warn clone $url_user attempt1 2>err && + ! grep "uses plaintext credentials" err +' + +test_expect_success 'fetch warns or fails when using username:password' ' + git -c transfer.credentialsInUrl=allow fetch $url_userpass 2>err && + ! grep "$message" err && + + git -c transfer.credentialsInUrl=warn fetch $url_userpass 2>err && + grep "warning: $message" err >warnings && + test_line_count = 3 warnings && + + test_must_fail git -c transfer.credentialsInUrl=die \ + fetch $url_userpass 2>err && + grep "fatal: $message" err >warnings && + test_line_count = 1 warnings && + + test_must_fail git -c transfer.credentialsInUrl=die \ + fetch $url_userblank 2>err && + grep "fatal: $message" err >warnings && + test_line_count = 1 warnings +' + + +test_expect_success 'push warns or fails when using username:password' ' + git -c transfer.credentialsInUrl=allow push $url_userpass 2>err && + ! grep "$message" err && + + git -c transfer.credentialsInUrl=warn push $url_userpass 2>err && + grep "warning: $message" err >warnings && + + test_must_fail git -c transfer.credentialsInUrl=die \ + push $url_userpass 2>err && + grep "fatal: $message" err >warnings && + test_line_count = 1 warnings +' + test_done diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 45f0803ed4..b2524a24c2 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -71,29 +71,6 @@ test_expect_success 'clone respects GIT_WORK_TREE' ' ' -test_expect_success LIBCURL 'clone warns or fails when using username:password' ' - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && - test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err && - ! grep "$message" err && - - test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err && - grep "warning: $message" err >warnings && - test_line_count = 2 warnings && - - test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err && - grep "fatal: $message" err >warnings && - test_line_count = 1 warnings && - - test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@localhost attempt3 2>err && - grep "fatal: $message" err >warnings && - test_line_count = 1 warnings -' - -test_expect_success LIBCURL 'clone does not detect username:password when it is https://username@domain:port/' ' - test_must_fail git -c transfer.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err && - ! grep "uses plaintext credentials" err -' - test_expect_success 'clone from hooks' ' test_create_repo r0 && -- 2.38.1.669.g2ee9a5b0e3 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516 2022-11-01 2:26 ` [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516 Jeff King @ 2022-11-01 3:18 ` Ævar Arnfjörð Bjarmason 2022-11-01 7:32 ` Jeff King 0 siblings, 1 reply; 45+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-11-01 3:18 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin via GitGitGadget, git, Derrick Stolee, Johannes Schindelin On Mon, Oct 31 2022, Jeff King wrote: > Commit 6dcbdc0d66 (remote: create fetch.credentialsInUrl config, > 2022-06-06) added tests for our handling of passwords in URLs. Since the > obvious URL to be affected is git-over-http, the tests use http. However > they don't set up a test server; they just try to access > https://localhost, assuming it will fail (because the nothing is > listening there). > > This causes some possible problems: > > - There might be a web server running on localhost, and we do not > actually want to connect to that. > > - The DNS resolver, or the local firewall, might take a substantial > amount of time (or forever, whichever comes first) to fail to > connect, slowing down the tests cases unnecessarily. > > - Since there's no server, our tests for "allow" and "warn" still > expect the clone/fetch/push operations to fail, even though in the > real world we'd expect these to succeed. We scrape stderr to see > what happened, but it's not as robust as a more realistic test. > > Let's instead move these to t5551, which is all about testing http and > where we have a real server. That eliminates any issues with contacting > a strange URL, and lets the "allow" and "warn" tests confirm that the > operation actually succeeds. > > It's not quite a verbatim move for a few reasons: > > - we can drop the LIBCURL dependency; it's already part of > lib-httpd.sh > > - we'll use HTTPD_URL_USER_PASS, etc, instead of our fake URL. To > avoid repetition, we'll add a few extra variables. > > - the "https://username:@localhost" test uses a funny URL that > lib-httpd.sh doesn't provide. We'll similarly construct it in a > variable. Note that we're hard-coding the lib-httpd username here, > but t5551 already does that everywhere. > > - for the "domain:port" test, the URL provided by lib-httpd is fine, > since our test server will always be on an exotic port. But we'll > confirm in the test that this is so. > > - since our message-matching is done via grep, I simplified it to use > a regex, rather than trying to massage lib-httpd's variables. > Arguably this makes it more readable, too, while retaining the bits > we care about: the fatal/warning distinction, the "uses plaintext" > message, and the fact that the password was redacted. > > - we'll use the /auth/ path for the repo, which shows that we are > indeed making use of the auth information when needed. > > - we'll also use /smart/; most of these tests could be done via /dumb/ > in t5550, but setting up pushes there requires extra effort and > dependencies. The smart protocol is what most everyone is using > these days anyway. > > This patch is my own, but I stole the analysis and a few bits of the > commit message from a patch by Johannes Schindelin. Did you consider just using git://; on the WIP branch I linked to where I fixed these tests quite a bit already I left them in their own file in anticipation of having to test that (but didn't finish that yet). I.e.: + git -C /home/avar/g/git/t/trash directory.t5700-protocol-v1/repo/parent/ tag two + GIT_TRACE_PACKET=1 git -C daemon_child -c protocol.version=1 -c transfer.credentialsInUrl=die fetch git://foo:bar@127.0.0.1:5700/parent fatal: URL 'git://foo:<redacted>@127.0.0.1:5700/parent' uses plaintext credentials I can't remember if anything can do anything sensible with user:passwords over non-http(s), but at the point where we emit this error in remote.c we don't care, so perhaps we could just test it that way. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516 2022-11-01 3:18 ` Ævar Arnfjörð Bjarmason @ 2022-11-01 7:32 ` Jeff King 2022-11-01 20:37 ` Taylor Blau 0 siblings, 1 reply; 45+ messages in thread From: Jeff King @ 2022-11-01 7:32 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Johannes Schindelin via GitGitGadget, git, Derrick Stolee, Johannes Schindelin On Tue, Nov 01, 2022 at 04:18:32AM +0100, Ævar Arnfjörð Bjarmason wrote: > Did you consider just using git://; on the WIP branch I linked to where > I fixed these tests quite a bit already I left them in their own file in > anticipation of having to test that (but didn't finish that yet). I.e.: No, I didn't really consider that. Mostly because I was trying to stick with the original intent of 6dcbdc0d66 that created them. If we toss that out, then in theory that widens the options. And in some ways it's nice to use git://, because it has fewer dependencies and so is run on more platforms. But it strikes me as a pretty unrealistic test, just because credentials in git:// URLs make no sense and are not something anybody would do. As you note, since the error comes from remote.c, it would still trigger. But it's a bit more "white box" testing than I think makes sense here. I prefer the original tests' method of trying to create plausible real-world scenarios and seeing how they behave (and I think my patch even improves that, since having an actual server on the other end is the usual case). -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516 2022-11-01 7:32 ` Jeff King @ 2022-11-01 20:37 ` Taylor Blau 0 siblings, 0 replies; 45+ messages in thread From: Taylor Blau @ 2022-11-01 20:37 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, Johannes Schindelin via GitGitGadget, git, Derrick Stolee, Johannes Schindelin On Tue, Nov 01, 2022 at 03:32:06AM -0400, Jeff King wrote: > If we toss that out, then in theory that widens the options. And in some > ways it's nice to use git://, because it has fewer dependencies and so > is run on more platforms. But it strikes me as a pretty unrealistic > test, just because credentials in git:// URLs make no sense and are not > something anybody would do. > > As you note, since the error comes from remote.c, it would still > trigger. But it's a bit more "white box" testing than I think makes > sense here. I prefer the original tests' method of trying to create > plausible real-world scenarios and seeing how they behave (and I think > my patch even improves that, since having an actual server on the other > end is the usual case). Agreed. Despite some of the niceties you mention, I concur that testing http(s) is more realistic and worth doing. Thanks, Taylor ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings 2022-11-01 2:25 ` Jeff King 2022-11-01 2:26 ` [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516 Jeff King @ 2022-11-01 2:26 ` Jeff King 2022-11-01 3:29 ` Ævar Arnfjörð Bjarmason 2022-11-01 4:54 ` Junio C Hamano 1 sibling, 2 replies; 45+ messages in thread From: Jeff King @ 2022-11-01 2:26 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Derrick Stolee, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> It is unclear as to _why_, but under certain circumstances the warning about credentials being passed as part of the URL seems to be swallowed by the `git remote-https` helper in the Windows jobs of Git's CI builds. Since it is not actually important how many times Git prints the warning/error message, as long as it prints it at least once, let's just make the test a bit more lenient and test for the latter instead of the former, which works around these CI issues. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Jeff King <peff@peff.net> --- t/t5551-http-fetch-smart.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index bbe3d5721f..64c6c9f59e 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -597,17 +597,17 @@ test_expect_success 'clone warns or fails when using username:password' ' git -c transfer.credentialsInUrl=warn \ clone $url_userpass attempt2 2>err && grep "warning: $message" err >warnings && - test_line_count = 2 warnings && + test_line_count -ge 1 warnings && test_must_fail git -c transfer.credentialsInUrl=die \ clone $url_userpass attempt3 2>err && grep "fatal: $message" err >warnings && - test_line_count = 1 warnings && + test_line_count -ge 1 warnings && test_must_fail git -c transfer.credentialsInUrl=die \ clone $url_userblank attempt4 2>err && grep "fatal: $message" err >warnings && - test_line_count = 1 warnings + test_line_count -ge 1 warnings ' test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' ' @@ -630,17 +630,17 @@ test_expect_success 'fetch warns or fails when using username:password' ' git -c transfer.credentialsInUrl=warn fetch $url_userpass 2>err && grep "warning: $message" err >warnings && - test_line_count = 3 warnings && + test_line_count -ge 1 warnings && test_must_fail git -c transfer.credentialsInUrl=die \ fetch $url_userpass 2>err && grep "fatal: $message" err >warnings && - test_line_count = 1 warnings && + test_line_count -ge 1 warnings && test_must_fail git -c transfer.credentialsInUrl=die \ fetch $url_userblank 2>err && grep "fatal: $message" err >warnings && - test_line_count = 1 warnings + test_line_count -ge 1 warnings ' @@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' ' test_must_fail git -c transfer.credentialsInUrl=die \ push $url_userpass 2>err && grep "fatal: $message" err >warnings && - test_line_count = 1 warnings + test_line_count -ge 1 warnings ' test_done -- 2.38.1.669.g2ee9a5b0e3 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings 2022-11-01 2:26 ` [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings Jeff King @ 2022-11-01 3:29 ` Ævar Arnfjörð Bjarmason 2022-11-01 7:39 ` Jeff King 2022-11-01 4:54 ` Junio C Hamano 1 sibling, 1 reply; 45+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-11-01 3:29 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin via GitGitGadget, git, Derrick Stolee, Johannes Schindelin On Mon, Oct 31 2022, Jeff King wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> Whatever our difference of opinion about the usefulness of not asserting that a thing doesn't get worse, even if it isn't perfect (c.f.: https://lore.kernel.org/git/221101.86a65b5q9q.gmgdl@evledraar.gmail.com/) I think that... > It is unclear as to _why_, but under certain circumstances the warning > about credentials being passed as part of the URL seems to be swallowed > by the `git remote-https` helper in the Windows jobs of Git's CI builds. ..this description dosen't match what the patch is doing, okey, so there's a case where the remote helper swallows the warnings, i.e. we'll emit fewer than we expected... > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > Signed-off-by: Jeff King <peff@peff.net> > --- > t/t5551-http-fetch-smart.sh | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh > index bbe3d5721f..64c6c9f59e 100755 > --- a/t/t5551-http-fetch-smart.sh > +++ b/t/t5551-http-fetch-smart.sh > @@ -597,17 +597,17 @@ test_expect_success 'clone warns or fails when using username:password' ' > git -c transfer.credentialsInUrl=warn \ > clone $url_userpass attempt2 2>err && > grep "warning: $message" err >warnings && > - test_line_count = 2 warnings && > + test_line_count -ge 1 warnings && > > test_must_fail git -c transfer.credentialsInUrl=die \ > clone $url_userpass attempt3 2>err && > grep "fatal: $message" err >warnings && > - test_line_count = 1 warnings && > + test_line_count -ge 1 warnings && > > test_must_fail git -c transfer.credentialsInUrl=die \ > clone $url_userblank attempt4 2>err && > grep "fatal: $message" err >warnings && > - test_line_count = 1 warnings > + test_line_count -ge 1 warnings > ' > > test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' ' > @@ -630,17 +630,17 @@ test_expect_success 'fetch warns or fails when using username:password' ' > > git -c transfer.credentialsInUrl=warn fetch $url_userpass 2>err && > grep "warning: $message" err >warnings && > - test_line_count = 3 warnings && > + test_line_count -ge 1 warnings && > > test_must_fail git -c transfer.credentialsInUrl=die \ > fetch $url_userpass 2>err && > grep "fatal: $message" err >warnings && > - test_line_count = 1 warnings && > + test_line_count -ge 1 warnings && > > test_must_fail git -c transfer.credentialsInUrl=die \ > fetch $url_userblank 2>err && > grep "fatal: $message" err >warnings && > - test_line_count = 1 warnings > + test_line_count -ge 1 warnings > ' > > > @@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' ' > test_must_fail git -c transfer.credentialsInUrl=die \ > push $url_userpass 2>err && > grep "fatal: $message" err >warnings && > - test_line_count = 1 warnings > + test_line_count -ge 1 warnings > ' ...but then why are we modifying these codepaths that have nothing to do with invoking the remote helper, i.e. where we die early before we get to that? And even if some of this was invoking that remote helper and we were modifying it blindly, then presumably the helper swallowing it would result in 0 some of the time, so "-ge 1" would be wrong. (That's not the case, but it's why I think the patch doesn't make much sense). ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings 2022-11-01 3:29 ` Ævar Arnfjörð Bjarmason @ 2022-11-01 7:39 ` Jeff King 2022-11-01 8:15 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 45+ messages in thread From: Jeff King @ 2022-11-01 7:39 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Johannes Schindelin via GitGitGadget, git, Derrick Stolee, Johannes Schindelin On Tue, Nov 01, 2022 at 04:29:31AM +0100, Ævar Arnfjörð Bjarmason wrote: > > It is unclear as to _why_, but under certain circumstances the warning > > about credentials being passed as part of the URL seems to be swallowed > > by the `git remote-https` helper in the Windows jobs of Git's CI builds. > > ..this description dosen't match what the patch is doing, okey, so > there's a case where the remote helper swallows the warnings, i.e. we'll > emit fewer than we expected... So I really didn't revisit this commit much at all, and was just trying to save Dscho (or Taylor) the work of having to rebase it, if we go with my patch 1. IMHO it is OK enough as it is, but if I were writing it from scratch I probably would have given the rationale that the tests are insiting on a dumb, sub-optimal behavior. And flakes or inconsistencies aside, they should be asserting only the presence or absence of the message. And probably would have left each at "grep" and dropped the test_line_count totally. It is not even clear to me that the remote-https is the one being swallowed (at least, I have not seen an argument or evidence that this is so; it does seem plausible). > > @@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' ' > > test_must_fail git -c transfer.credentialsInUrl=die \ > > push $url_userpass 2>err && > > grep "fatal: $message" err >warnings && > > - test_line_count = 1 warnings > > + test_line_count -ge 1 warnings > > ' > > ...but then why are we modifying these codepaths that have nothing to do > with invoking the remote helper, i.e. where we die early before we get > to that? If you're arguing that we should only do s/= 3/-ge 1/ for the test that is flaking, I could buy that. Though like I said, I consider the value here to be focusing the purpose of the tests as much as dealing with the flake. I really don't care that much either way, though. I'd also be fine with a separate test (marked expect_failure) that checks the number of messages. > And even if some of this was invoking that remote helper and we were > modifying it blindly, then presumably the helper swallowing it would > result in 0 some of the time, so "-ge 1" would be wrong. > > (That's not the case, but it's why I think the patch doesn't make much > sense). I thought the point is that the outer program calling the helper would consistently produce the error, always yielding at least one instance. The helper one is generally "extra" and undesired. -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings 2022-11-01 7:39 ` Jeff King @ 2022-11-01 8:15 ` Ævar Arnfjörð Bjarmason 2022-11-01 9:12 ` Jeff King 0 siblings, 1 reply; 45+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-11-01 8:15 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin via GitGitGadget, git, Derrick Stolee, Johannes Schindelin On Tue, Nov 01 2022, Jeff King wrote: > On Tue, Nov 01, 2022 at 04:29:31AM +0100, Ævar Arnfjörð Bjarmason wrote: > >> > It is unclear as to _why_, but under certain circumstances the warning >> > about credentials being passed as part of the URL seems to be swallowed >> > by the `git remote-https` helper in the Windows jobs of Git's CI builds. >> >> ..this description dosen't match what the patch is doing, okey, so >> there's a case where the remote helper swallows the warnings, i.e. we'll >> emit fewer than we expected... > > So I really didn't revisit this commit much at all, and was just trying > to save Dscho (or Taylor) the work of having to rebase it, if we go with > my patch 1. > > IMHO it is OK enough as it is, but if I were writing it from scratch I > probably would have given the rationale that the tests are insiting on a > dumb, sub-optimal behavior. And flakes or inconsistencies aside, they > should be asserting only the presence or absence of the message. And > probably would have left each at "grep" and dropped the test_line_count > totally. Do you mean that even if we fix the bug and consistently emit one and only one such message you'd like to have the tests not assert that that's the case? I do think that UX is important enough to test for, particularly if we've had a bug related to that that we've fixed. I.e. if something in the direction of my [1] goes in. > It is not even clear to me that the remote-https is the one being > swallowed (at least, I have not seen an argument or evidence that this > is so; it does seem plausible). It is the case, the only ones that are going to be duplicated are the "warn" ones, because for "die" we'll die right away in the parent process. Which is what I'm trying to get across here, and why I'm a bit confused. I.e. I thought you'd agree that we should test that we emit exactly one warning if & when we've fixed the underlying issue. That issue is already fixed for "die", so even if you want to loosen up the test your [2] should only keep the first line removal/addition in the first two hunks, and drop the 3rd one. >> > @@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' ' >> > test_must_fail git -c transfer.credentialsInUrl=die \ >> > push $url_userpass 2>err && >> > grep "fatal: $message" err >warnings && >> > - test_line_count = 1 warnings >> > + test_line_count -ge 1 warnings >> > ' >> >> ...but then why are we modifying these codepaths that have nothing to do >> with invoking the remote helper, i.e. where we die early before we get >> to that? > > If you're arguing that we should only do s/= 3/-ge 1/ for the test that > is flaking, I could buy that. I'm saying that if we're doing a handwaivy-fix and saying "sometimes the message gets swallowed" and fixing this blindly without checking how it works, then changing "= 1" to "-ge 1" doesn't make sense. It should be "-ge 0", i.e. surely that "one warning" can get swallowed too? Now, I know that never happens, because we'll always get at least one. I'm just saying that as soon as you stop to think about that you must also come to the conclusion that the "die" ones are OK as-is. That's because the reason we always get at least one is the same as we're always guaranteed to emit just one in the "die" case: The parent process emits it, then dies. >> And even if some of this was invoking that remote helper and we were >> modifying it blindly, then presumably the helper swallowing it would >> result in 0 some of the time, so "-ge 1" would be wrong. >> >> (That's not the case, but it's why I think the patch doesn't make much >> sense). > > I thought the point is that the outer program calling the helper would > consistently produce the error, always yielding at least one instance. > The helper one is generally "extra" and undesired. Yes, exactly. Which is what my fix[1] the root cause addresses. Anyway, I'm just trying to help here. If you/Johannes/others want to go with the "hotfix" as-is that's fine my me. I just don't see what the hurry is, it's been this way for two releases, if it's flaky that's been the case for months, I'd think we could just fix the root cause. 1. http://lore.kernel.org/git/RFC-patch-1.1-0266485bc6c-20221031T204149Z-avarab@gmail.com 2. https://lore.kernel.org/git/Y2CD6GBl6ORqKsug@coredump.intra.peff.net/ ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings 2022-11-01 8:15 ` Ævar Arnfjörð Bjarmason @ 2022-11-01 9:12 ` Jeff King 2022-11-01 14:05 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 45+ messages in thread From: Jeff King @ 2022-11-01 9:12 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Johannes Schindelin via GitGitGadget, git, Derrick Stolee, Johannes Schindelin On Tue, Nov 01, 2022 at 09:15:00AM +0100, Ævar Arnfjörð Bjarmason wrote: > > So I really didn't revisit this commit much at all, and was just trying > > to save Dscho (or Taylor) the work of having to rebase it, if we go with > > my patch 1. > > > > IMHO it is OK enough as it is, but if I were writing it from scratch I > > probably would have given the rationale that the tests are insiting on a > > dumb, sub-optimal behavior. And flakes or inconsistencies aside, they > > should be asserting only the presence or absence of the message. And > > probably would have left each at "grep" and dropped the test_line_count > > totally. > > Do you mean that even if we fix the bug and consistently emit one and > only one such message you'd like to have the tests not assert that > that's the case? No, I wouldn't mind it, if that is a bug we've fixed. I just mean that the tests as written never wanted to say "3 is the absolute right number of times to write this message". They only put "3" there because it made things pass. > I do think that UX is important enough to test for, particularly if > we've had a bug related to that that we've fixed. I.e. if something in > the direction of my [1] goes in. Sure, I don't mind at all a test for it. In the short-term, if you want a test that fails, I'd prefer it be separate so that we can test the useful existing behavior that _does_ work. If the multiple-messages bug is fixed, I don't mind folding them together into a single test that passes. > > It is not even clear to me that the remote-https is the one being > > swallowed (at least, I have not seen an argument or evidence that this > > is so; it does seem plausible). > > It is the case, the only ones that are going to be duplicated are the > "warn" ones, because for "die" we'll die right away in the parent > process. Right, I understand why "die" produces only one. My question was when we produce 2 on Windows (sometimes?) but 3 elsewhere, are we sure it is the one from remote-https that is eaten, or could it ever be one of the others? In a sense we do not need to worry about "why is it sometimes eaten" if the bug is fixed to produce only the one message. But it may point to a separate and interesting problem (e.g., is stderr from remote-https not reliable?). > >> > @@ -654,7 +654,7 @@ test_expect_success 'push warns or fails when using username:password' ' > >> > test_must_fail git -c transfer.credentialsInUrl=die \ > >> > push $url_userpass 2>err && > >> > grep "fatal: $message" err >warnings && > >> > - test_line_count = 1 warnings > >> > + test_line_count -ge 1 warnings > >> > ' > >> > >> ...but then why are we modifying these codepaths that have nothing to do > >> with invoking the remote helper, i.e. where we die early before we get > >> to that? > > > > If you're arguing that we should only do s/= 3/-ge 1/ for the test that > > is flaking, I could buy that. > > I'm saying that if we're doing a handwaivy-fix and saying "sometimes the > message gets swallowed" and fixing this blindly without checking how it > works, then changing "= 1" to "-ge 1" doesn't make sense. Right, I'm fine with that (I perhaps should have said something stronger than "I could buy that"). As I said, I was mostly just rebasing Dscho's patch and I think it was good enough in the sense that it was hand-waving away the whole "there may be more than one" problem. But I do agree that we'll never see more in the "die" cases, and there is no need to change them. > > I thought the point is that the outer program calling the helper would > > consistently produce the error, always yielding at least one instance. > > The helper one is generally "extra" and undesired. > > Yes, exactly. Which is what my fix[1] the root cause addresses. > > Anyway, I'm just trying to help here. If you/Johannes/others want to go > with the "hotfix" as-is that's fine my me. > > I just don't see what the hurry is, it's been this way for two releases, > if it's flaky that's been the case for months, I'd think we could just > fix the root cause. It recently bit me twice, so maybe I am giving it more urgency than it deserves (or maybe something changed in CI to make it more likely). I do think it would be nice to fix it. I don't love your patch for the reasons I replied there (not your fault; it's inherently a crappy and complicated problem). In the meantime, I'd like to see CI fixed, as it is wasting developer's time. And that's why I called Dscho's loosening "good enough". It is hopefully a temporary state anyway. But I would be just as happy to see a similar patch which just changed the 2/3 lines to "-ge 1" (or just a straight grep). -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings 2022-11-01 9:12 ` Jeff King @ 2022-11-01 14:05 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 45+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-11-01 14:05 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin via GitGitGadget, git, Derrick Stolee, Johannes Schindelin On Tue, Nov 01 2022, Jeff King wrote: > On Tue, Nov 01, 2022 at 09:15:00AM +0100, Ævar Arnfjörð Bjarmason wrote: > >> > So I really didn't revisit this commit much at all, and was just trying >> > to save Dscho (or Taylor) the work of having to rebase it, if we go with >> > my patch 1. >> > >> > IMHO it is OK enough as it is, but if I were writing it from scratch I >> > probably would have given the rationale that the tests are insiting on a >> > dumb, sub-optimal behavior. And flakes or inconsistencies aside, they >> > should be asserting only the presence or absence of the message. And >> > probably would have left each at "grep" and dropped the test_line_count >> > totally. >> >> Do you mean that even if we fix the bug and consistently emit one and >> only one such message you'd like to have the tests not assert that >> that's the case? > > No, I wouldn't mind it, if that is a bug we've fixed. I just mean that > the tests as written never wanted to say "3 is the absolute right number > of times to write this message". They only put "3" there because it made > things pass. That's one reason, another is to assert current behavior, and to be able to answer questions like "does this patch change behavior" without having to recursively diff the trash directory output of a test run, because everything's so fuzzy. If and when it's 1 instead of 3, great, adjusting the test isn't a big deal. Anyway, we're off into general testing philosophy again, which I think is off topic here. >> I do think that UX is important enough to test for, particularly if >> we've had a bug related to that that we've fixed. I.e. if something in >> the direction of my [1] goes in. > > Sure, I don't mind at all a test for it. In the short-term, if you want > a test that fails, I'd prefer it be separate so that we can test the > useful existing behavior that _does_ work. If the multiple-messages bug > is fixed, I don't mind folding them together into a single test that > passes. Right, I'm not saying "keep the flaky test", I'm saying let's keep the ones we know aren't flaky. >> > It is not even clear to me that the remote-https is the one being >> > swallowed (at least, I have not seen an argument or evidence that this >> > is so; it does seem plausible). >> >> It is the case, the only ones that are going to be duplicated are the >> "warn" ones, because for "die" we'll die right away in the parent >> process. > > Right, I understand why "die" produces only one. My question was when we > produce 2 on Windows (sometimes?) but 3 elsewhere, are we sure it is the > one from remote-https that is eaten, or could it ever be one of the > others? I don't have a test case in front of me, and Johannes didn't provide one (or even a link to CI output). But from his description and being familiar with the code I'm pretty certain isn't not the "die" cases, those are all in-process, and it happens before we spawn sub-processes, I don't see how that would be different on Windows. >> > I thought the point is that the outer program calling the helper would >> > consistently produce the error, always yielding at least one instance. >> > The helper one is generally "extra" and undesired. >> >> Yes, exactly. Which is what my fix[1] the root cause addresses. >> >> Anyway, I'm just trying to help here. If you/Johannes/others want to go >> with the "hotfix" as-is that's fine my me. >> >> I just don't see what the hurry is, it's been this way for two releases, >> if it's flaky that's been the case for months, I'd think we could just >> fix the root cause. > > It recently bit me twice, so maybe I am giving it more urgency than it > deserves (or maybe something changed in CI to make it more likely). Bit you in GitHub Windows CI? > I do think it would be nice to fix it. I don't love your patch for the > reasons I replied there (not your fault; it's inherently a crappy and > complicated problem). In the meantime, I'd like to see CI fixed, as > it is wasting developer's time. And that's why I called Dscho's > loosening "good enough". It is hopefully a temporary state anyway. > > But I would be just as happy to see a similar patch which just changed > the 2/3 lines to "-ge 1" (or just a straight grep). Sure, if we're deciding not to care about tests that are unrelated to the flakyness problem at hand being loosened. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings 2022-11-01 2:26 ` [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings Jeff King 2022-11-01 3:29 ` Ævar Arnfjörð Bjarmason @ 2022-11-01 4:54 ` Junio C Hamano 2022-11-01 7:42 ` Jeff King 1 sibling, 1 reply; 45+ messages in thread From: Junio C Hamano @ 2022-11-01 4:54 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin via GitGitGadget, git, Derrick Stolee, Johannes Schindelin Jeff King <peff@peff.net> writes: > It is unclear as to _why_, but under certain circumstances the warning > about credentials being passed as part of the URL seems to be swallowed > by the `git remote-https` helper in the Windows jobs of Git's CI builds. > > Since it is not actually important how many times Git prints the > warning/error message, as long as it prints it at least once, ... Sorry, but I do not quite see the value of keeping the test to expect success in a weakend form. If we think we are emitting three warnings, whether we do so by mistake or by design, and some of them are lost and not shown for an unknown reason, is there a guarantee that at least one would survive? And when all three are lost, even the test in the weakened form would fail and stop the CI builds, no? If we do not know why we are losing some messages, and if we do not have resources to track down why, that is perfectly fine. Fixes can be prioritised. But wouldn't test_expect_failure be a better way to express "we think we ought to get 3 but for some reason we may not get all of them and we haven't figured it out"? ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings 2022-11-01 4:54 ` Junio C Hamano @ 2022-11-01 7:42 ` Jeff King 2022-11-01 20:50 ` Taylor Blau 0 siblings, 1 reply; 45+ messages in thread From: Jeff King @ 2022-11-01 7:42 UTC (permalink / raw) To: Junio C Hamano Cc: Johannes Schindelin via GitGitGadget, git, Derrick Stolee, Johannes Schindelin On Mon, Oct 31, 2022 at 09:54:23PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > It is unclear as to _why_, but under certain circumstances the warning > > about credentials being passed as part of the URL seems to be swallowed > > by the `git remote-https` helper in the Windows jobs of Git's CI builds. > > > > Since it is not actually important how many times Git prints the > > warning/error message, as long as it prints it at least once, ... > > Sorry, but I do not quite see the value of keeping the test to > expect success in a weakend form. If we think we are emitting three > warnings, whether we do so by mistake or by design, and some of them > are lost and not shown for an unknown reason, is there a guarantee > that at least one would survive? And when all three are lost, even > the test in the weakened form would fail and stop the CI builds, no? Without understanding the cause of the loss, I agree that things are a little hand-wavy. But the assumption _does_ seem to hold that we consistently produce at least one (presumably from the parent clone/fetch/push process). And if we can rely on that, there's value in the tests asserting that the message was shown to the user at least once. > If we do not know why we are losing some messages, and if we do not > have resources to track down why, that is perfectly fine. Fixes can > be prioritised. But wouldn't test_expect_failure be a better way to > express "we think we ought to get 3 but for some reason we may not > get all of them and we haven't figured it out"? Marking it as expect_failure throws out the main point of the test, though, which is that the user sees _some_ message (and that the "die" form aborts the operation). It might make sense to add a separate test in the meantime that documents the "oops, we get the wrong number" sometimes state (and eventually, if fixed, that could be folded back into the main test for efficiency / simplicity). -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings 2022-11-01 7:42 ` Jeff King @ 2022-11-01 20:50 ` Taylor Blau 0 siblings, 0 replies; 45+ messages in thread From: Taylor Blau @ 2022-11-01 20:50 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Johannes Schindelin via GitGitGadget, git, Derrick Stolee, Johannes Schindelin On Tue, Nov 01, 2022 at 03:42:59AM -0400, Jeff King wrote: > On Mon, Oct 31, 2022 at 09:54:23PM -0700, Junio C Hamano wrote: > > > Jeff King <peff@peff.net> writes: > > > > > It is unclear as to _why_, but under certain circumstances the warning > > > about credentials being passed as part of the URL seems to be swallowed > > > by the `git remote-https` helper in the Windows jobs of Git's CI builds. > > > > > > Since it is not actually important how many times Git prints the > > > warning/error message, as long as it prints it at least once, ... > > > > Sorry, but I do not quite see the value of keeping the test to > > expect success in a weakend form. If we think we are emitting three > > warnings, whether we do so by mistake or by design, and some of them > > are lost and not shown for an unknown reason, is there a guarantee > > that at least one would survive? And when all three are lost, even > > the test in the weakened form would fail and stop the CI builds, no? > > Without understanding the cause of the loss, I agree that things are a > little hand-wavy. But the assumption _does_ seem to hold that we > consistently produce at least one (presumably from the parent > clone/fetch/push process). And if we can rely on that, there's value in > the tests asserting that the message was shown to the user at least > once. Part of me wonders whether the local DNS-resolution issue you fixed in the first patch would be sufficient to get us to produce the wrong number of warnings consistently. I.e., if I queue just the first patch and drop Johannes's, would that be sufficient to get CI working consistently again? I don't know. It's frustrating to rely so much on an external environment that our feedback loop can only be as short as "push out some combination of these patches and wait for CI". That's disappointing, and TBH I would rather spend time focusing on other patches than play games with CI. The pair of patches look good to me. Perhaps we could avoid the weakened assumption, but I do not mind too much in the meantime. Especially since we already have a series[1] in the works that resolves the issue for good. Thanks, Taylor [1]: https://lore.kernel.org/git/RFC-patch-1.1-0266485bc6c-20221031T204149Z-avarab@gmail.com/ ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings 2022-10-31 19:47 [PATCH 0/2] t5516/t5601: avoid using localhost for failing HTTPS requests Johannes Schindelin via GitGitGadget 2022-10-31 19:47 ` [PATCH 1/2] t5516/t5601: avoid using `localhost` " Johannes Schindelin via GitGitGadget @ 2022-10-31 19:47 ` Johannes Schindelin via GitGitGadget 2022-10-31 23:22 ` Jeff King 2022-11-01 2:27 ` Jeff King 2022-10-31 20:47 ` [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings Ævar Arnfjörð Bjarmason 2 siblings, 2 replies; 45+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2022-10-31 19:47 UTC (permalink / raw) To: git; +Cc: Derrick Stolee, Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> It is unclear as to _why_, but under certain circumstances the warning about credentials being passed as part of the URL seems to be swallowed by the `git remote-https` helper in the Windows jobs of Git's CI builds. Since it is not actually important how many times Git prints the warning/error message, as long as it prints it at least once, let's just make the test a bit more lenient and test for the latter instead of the former, which works around these CI issues. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- t/t5516-fetch-push.sh | 8 ++++---- t/t5601-clone.sh | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 8dd4610a8c2..980c594940b 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1860,15 +1860,15 @@ test_expect_success LIBCURL 'fetch warns or fails when using username:password' test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@224.0.0.1 2>err && grep "warning: $message" err >warnings && - test_line_count = 3 warnings && + test_line_count -ge 1 warnings && test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@224.0.0.1 2>err && grep "fatal: $message" err >warnings && - test_line_count = 1 warnings && + test_line_count -ge 1 warnings && test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@224.0.0.1 2>err && grep "fatal: $message" err >warnings && - test_line_count = 1 warnings + test_line_count -ge 1 warnings ' @@ -1881,7 +1881,7 @@ test_expect_success LIBCURL 'push warns or fails when using username:password' ' grep "warning: $message" err >warnings && test_must_fail git -c transfer.credentialsInUrl=die push https://username:password@224.0.0.1 2>err && grep "fatal: $message" err >warnings && - test_line_count = 1 warnings + test_line_count -ge 1 warnings ' test_expect_success 'push with config push.useBitmaps' ' diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 0b386c74818..b72cdeb6243 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -78,19 +78,19 @@ test_expect_success LIBCURL 'clone warns or fails when using username:password' test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@224.0.0.1 attempt2 2>err && grep "warning: $message" err >warnings && - test_line_count = 2 warnings && + test_line_count -ge 1 warnings && test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@224.0.0.1 attempt3 2>err && grep "fatal: $message" err >warnings && - test_line_count = 1 warnings && + test_line_count -ge 1 warnings && test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@224.0.0.1 attempt3 2>err && grep "fatal: $message" err >warnings && - test_line_count = 1 warnings + test_line_count -ge 1 warnings ' test_expect_success LIBCURL 'clone does not detect username:password when it is https://username@domain:port/' ' - test_must_fail git -c transfer.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err && + test_must_fail git -c transfer.credentialsInUrl=warn clone https://username@224.0.0.1:8080 attempt3 2>err && ! grep "uses plaintext credentials" err ' -- gitgitgadget ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings 2022-10-31 19:47 ` Johannes Schindelin via GitGitGadget @ 2022-10-31 23:22 ` Jeff King 2022-11-01 0:57 ` Taylor Blau 2022-11-01 2:27 ` Jeff King 1 sibling, 1 reply; 45+ messages in thread From: Jeff King @ 2022-10-31 23:22 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Derrick Stolee, Johannes Schindelin On Mon, Oct 31, 2022 at 07:47:18PM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > It is unclear as to _why_, but under certain circumstances the warning > about credentials being passed as part of the URL seems to be swallowed > by the `git remote-https` helper in the Windows jobs of Git's CI builds. > > Since it is not actually important how many times Git prints the > warning/error message, as long as it prints it at least once, let's just > make the test a bit more lenient and test for the latter instead of the > former, which works around these CI issues. This makes sense to me. Regardless of whether we actually fix the multiple outputs (which are user-visible and kind of ugly), I don't think there's any reason for our tests to assert the somewhat undesirable output. So this can proceed independently of any fix. > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index 8dd4610a8c2..980c594940b 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -1860,15 +1860,15 @@ test_expect_success LIBCURL 'fetch warns or fails when using username:password' > > test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@224.0.0.1 2>err && > grep "warning: $message" err >warnings && > - test_line_count = 3 warnings && > + test_line_count -ge 1 warnings && I think this test_line_count (and all the others) is now superfluous; the exit code of grep will tell us if we found anything. I don't mind it too much, though, if we're planning to fix the duplicate messages, at which point it becomes s/-ge/=/. -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings 2022-10-31 23:22 ` Jeff King @ 2022-11-01 0:57 ` Taylor Blau 0 siblings, 0 replies; 45+ messages in thread From: Taylor Blau @ 2022-11-01 0:57 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin via GitGitGadget, git, Derrick Stolee, Johannes Schindelin On Mon, Oct 31, 2022 at 07:22:48PM -0400, Jeff King wrote: > > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > > index 8dd4610a8c2..980c594940b 100755 > > --- a/t/t5516-fetch-push.sh > > +++ b/t/t5516-fetch-push.sh > > @@ -1860,15 +1860,15 @@ test_expect_success LIBCURL 'fetch warns or fails when using username:password' > > > > test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@224.0.0.1 2>err && > > grep "warning: $message" err >warnings && > > - test_line_count = 3 warnings && > > + test_line_count -ge 1 warnings && > > I think this test_line_count (and all the others) is now superfluous; > the exit code of grep will tell us if we found anything. > > I don't mind it too much, though, if we're planning to fix the duplicate > messages, at which point it becomes s/-ge/=/. Yeah, agreed. Let's leave it alone, unless somebody else feels strongly. Thanks, Taylor ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings 2022-10-31 19:47 ` Johannes Schindelin via GitGitGadget 2022-10-31 23:22 ` Jeff King @ 2022-11-01 2:27 ` Jeff King 1 sibling, 0 replies; 45+ messages in thread From: Jeff King @ 2022-11-01 2:27 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Derrick Stolee, Johannes Schindelin On Mon, Oct 31, 2022 at 07:47:18PM +0000, Johannes Schindelin via GitGitGadget wrote: > diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh > index 0b386c74818..b72cdeb6243 100755 > --- a/t/t5601-clone.sh > +++ b/t/t5601-clone.sh > @@ -78,19 +78,19 @@ test_expect_success LIBCURL 'clone warns or fails when using username:password' > > test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@224.0.0.1 attempt2 2>err && > grep "warning: $message" err >warnings && > - test_line_count = 2 warnings && > + test_line_count -ge 1 warnings && > > test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@224.0.0.1 attempt3 2>err && > grep "fatal: $message" err >warnings && > - test_line_count = 1 warnings && > + test_line_count -ge 1 warnings && > > test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@224.0.0.1 attempt3 2>err && > grep "fatal: $message" err >warnings && > - test_line_count = 1 warnings > + test_line_count -ge 1 warnings > ' > > test_expect_success LIBCURL 'clone does not detect username:password when it is https://username@domain:port/' ' > - test_must_fail git -c transfer.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err && > + test_must_fail git -c transfer.credentialsInUrl=warn clone https://username@224.0.0.1:8080 attempt3 2>err && > ! grep "uses plaintext credentials" err > ' By the way, this last part of the hunk obviously belongs in the first patch. I adjusted as appropriate in the patch I just sent. -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
* [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings 2022-10-31 19:47 [PATCH 0/2] t5516/t5601: avoid using localhost for failing HTTPS requests Johannes Schindelin via GitGitGadget 2022-10-31 19:47 ` [PATCH 1/2] t5516/t5601: avoid using `localhost` " Johannes Schindelin via GitGitGadget 2022-10-31 19:47 ` Johannes Schindelin via GitGitGadget @ 2022-10-31 20:47 ` Ævar Arnfjörð Bjarmason 2022-11-01 1:06 ` Taylor Blau 2022-11-01 2:32 ` Jeff King 2 siblings, 2 replies; 45+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-10-31 20:47 UTC (permalink / raw) To: git Cc: Junio C Hamano, Taylor Blau, Derrick Stolee, Johannes Schindelin, Ævar Arnfjörð Bjarmason Improve on 6dcbdc0d661 (remote: create fetch.credentialsInUrl config, 2022-06-06) by fixing the cases where we emit duplicate warnings, which were: * In the same process, as we'd get the same "struct remote *" again. We could probably save ourselves more work in those scenarios, but adding a flag to the "struct remote" indicating that we've validated the URLs will fix those issues. * When we invoke e.g. "git-remote-https" (aka. "git-remote-curl") from "git fetch". For those cases let's pass down the equivalent of a "-c transfer.credentialsInUrl=allow", since we know that we've already inspected our remotes in the parent process. See 7390f05a3c6 (fetch: after refetch, encourage auto gc repacking, 2022-03-28) for prior use of git_config_push_parameter() for this purpose, i.e. to push config parameters before invoking a sub-process. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- [Grabbing a quote from https://lore.kernel.org/git/98fa5270cb720f2f038c4bb9571c7d306ff5d759.1667245639.git.gitgitgadget@gmail.com/ for a reply] > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > It is unclear as to _why_, but under certain circumstances the warning > about credentials being passed as part of the URL seems to be swallowed > by the `git remote-https` helper in the Windows jobs of Git's CI builds. I think this should fix the root cause of the issue you're seeing. I have a larger local branch to fix various issues with this credentialsInUrl facility that I hadn't gotten around to submitting: https://github.com/git/git/compare/master...avar:avar/fix-cred-in-url-warnings-2 This is a cherry-pick of 7/* of that (the first were doc changes, missing test coverage etc). > Since it is not actually important how many times Git prints the > warning/error message, as long as it prints it at least once, let's just > make the test a bit more lenient and test for the latter instead of the > former, which works around these CI issues. That being said your change is obviously smaller, so if we'd prefer that in first as a band-aid I'm fine with that. But I'd really like to object to the "it is not actually important how many...", it's crappy UX to spew duplicate output at the user, and we should avoid it. So it does matter, and we get it wrong not just in this but also some other cases. builtin/clone.c | 5 +++- builtin/fetch.c | 4 ++++ builtin/push.c | 6 ++++- remote.c | 53 ++++++++++++++++++++++++++++++++----------- remote.h | 14 ++++++++++++ t/t5516-fetch-push.sh | 2 +- t/t5601-clone.sh | 2 +- 7 files changed, 69 insertions(+), 17 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 547d6464b3c..da219b74e43 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -903,12 +903,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix) int err = 0, complete_refs_before_fetch = 1; int submodule_progress; int filter_submodules = 0; - + enum credentials_in_url cred_in_url_cfg = get_credentials_in_url(); struct transport_ls_refs_options transport_ls_refs_options = TRANSPORT_LS_REFS_OPTIONS_INIT; packet_trace_identity("clone"); + if (cred_in_url_cfg == CRED_IN_URL_WARN) + git_config_push_parameter("transfer.credentialsInUrl=allow"); + git_config(git_clone_config, NULL); argc = parse_options(argc, argv, prefix, builtin_clone_options, diff --git a/builtin/fetch.c b/builtin/fetch.c index b06e454cbdd..34a10e1927f 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -2110,9 +2110,13 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) struct remote *remote = NULL; int result = 0; int prune_tags_ok = 1; + enum credentials_in_url cred_in_url_cfg = get_credentials_in_url(); packet_trace_identity("fetch"); + if (cred_in_url_cfg == CRED_IN_URL_WARN) + git_config_push_parameter("transfer.credentialsInUrl=allow"); + /* Record the command line for the reflog */ strbuf_addstr(&default_rla, "fetch"); for (i = 1; i < argc; i++) { diff --git a/builtin/push.c b/builtin/push.c index f0329c62a2d..a4890b1586d 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -576,7 +576,7 @@ int cmd_push(int argc, const char **argv, const char *prefix) struct string_list *push_options; const struct string_list_item *item; struct remote *remote; - + enum credentials_in_url cred_in_url_cfg = get_credentials_in_url(); struct option options[] = { OPT__VERBOSITY(&verbosity), OPT_STRING( 0 , "repo", &repo, N_("repository"), N_("repository")), @@ -619,6 +619,10 @@ int cmd_push(int argc, const char **argv, const char *prefix) }; packet_trace_identity("push"); + + if (cred_in_url_cfg == CRED_IN_URL_WARN) + git_config_push_parameter("transfer.credentialsInUrl=allow"); + git_config(git_push_config, &flags); argc = parse_options(argc, argv, prefix, options, push_usage, 0); push_options = (push_options_cmdline.nr diff --git a/remote.c b/remote.c index 60869beebe7..a35da349629 100644 --- a/remote.c +++ b/remote.c @@ -615,24 +615,42 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push) return NULL; } -static void validate_remote_url(struct remote *remote) +static enum credentials_in_url cred_in_url = CRED_IN_URL_UNKNOWN; +enum credentials_in_url get_credentials_in_url(void) { - int i; + enum credentials_in_url new = CRED_IN_URL_ALLOW; const char *value; - struct strbuf redacted = STRBUF_INIT; - int warn_not_die; + + if (cred_in_url != CRED_IN_URL_UNKNOWN) + return cred_in_url; if (git_config_get_string_tmp("transfer.credentialsinurl", &value)) - return; + goto done; if (!strcmp("warn", value)) - warn_not_die = 1; + new = CRED_IN_URL_WARN; else if (!strcmp("die", value)) - warn_not_die = 0; + new = CRED_IN_URL_DIE; else if (!strcmp("allow", value)) - return; + goto done; else - die(_("unrecognized value transfer.credentialsInUrl: '%s'"), value); + die(_("unrecognized value transfer.credentialsInURL: '%s'"), value); + +done: + cred_in_url = new; + return cred_in_url; +} + +static void validate_remote_url(struct remote *remote) +{ + int i; + struct strbuf redacted = STRBUF_INIT; + enum credentials_in_url cfg = get_credentials_in_url(); + + if (remote->validated_urls) + goto done; + if (cfg == CRED_IN_URL_ALLOW) + goto done; for (i = 0; i < remote->url_nr; i++) { struct url_info url_info = { 0 }; @@ -647,16 +665,25 @@ static void validate_remote_url(struct remote *remote) strbuf_addstr(&redacted, url_info.url + url_info.passwd_off + url_info.passwd_len); - if (warn_not_die) + switch (cfg) { + case CRED_IN_URL_WARN: warning(_("URL '%s' uses plaintext credentials"), redacted.buf); - else + break; + case CRED_IN_URL_DIE: die(_("URL '%s' uses plaintext credentials"), redacted.buf); - -loop_cleanup: + break; + case CRED_IN_URL_ALLOW: + case CRED_IN_URL_UNKNOWN: + BUG("unreachable"); + break; + } + loop_cleanup: free(url_info.url); } strbuf_release(&redacted); +done: + remote->validated_urls = 1; } static struct remote * diff --git a/remote.h b/remote.h index 1c4621b414b..5a2da13b4cb 100644 --- a/remote.h +++ b/remote.h @@ -98,6 +98,8 @@ struct remote { int prune; int prune_tags; + int validated_urls; + /** * The configured helper programs to run on the remote side, for * Git-native protocols. @@ -445,4 +447,16 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *); char *relative_url(const char *remote_url, const char *url, const char *up_path); +enum credentials_in_url { + CRED_IN_URL_UNKNOWN, + CRED_IN_URL_ALLOW, + CRED_IN_URL_WARN, + CRED_IN_URL_DIE, +}; + +/** + * Get the transfer.credentialsInUrl config setting as an "enum + * credentials_in_url". + */ +enum credentials_in_url get_credentials_in_url(void); #endif diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 79dc470c014..d01f5cd349f 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1860,7 +1860,7 @@ test_expect_success LIBCURL 'fetch warns or fails when using username:password' test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@localhost 2>err && grep "warning: $message" err >warnings && - test_line_count = 3 warnings && + test_line_count = 1 warnings && test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@localhost 2>err && grep "fatal: $message" err >warnings && diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 45f0803ed4d..37adfd9f83b 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -78,7 +78,7 @@ test_expect_success LIBCURL 'clone warns or fails when using username:password' test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err && grep "warning: $message" err >warnings && - test_line_count = 2 warnings && + test_line_count = 1 warnings && test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err && grep "fatal: $message" err >warnings && -- 2.38.0.1280.g8136eb6fab2 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings 2022-10-31 20:47 ` [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings Ævar Arnfjörð Bjarmason @ 2022-11-01 1:06 ` Taylor Blau 2022-11-01 2:32 ` Jeff King 1 sibling, 0 replies; 45+ messages in thread From: Taylor Blau @ 2022-11-01 1:06 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Derrick Stolee, Johannes Schindelin On Mon, Oct 31, 2022 at 09:47:08PM +0100, Ævar Arnfjörð Bjarmason wrote: > Improve on 6dcbdc0d661 (remote: create fetch.credentialsInUrl config, > 2022-06-06) by fixing the cases where we emit duplicate warnings, > which were: > > * In the same process, as we'd get the same "struct remote *" > again. We could probably save ourselves more work in those scenarios, > but adding a flag to the "struct remote" indicating that we've validated > the URLs will fix those issues. > > * When we invoke e.g. "git-remote-https" (aka. "git-remote-curl") > from "git fetch". For those cases let's pass down the equivalent of a > "-c transfer.credentialsInUrl=allow", since we know that we've already > inspected our remotes in the parent process. > > See 7390f05a3c6 (fetch: after refetch, encourage auto gc repacking, > 2022-03-28) for prior use of git_config_push_parameter() for this > purpose, i.e. to push config parameters before invoking a > sub-process. Both make sense. I was skimming the diff before I read the patch message here and was scratching my head at the calls to git_config_push_parameter(). But for crossing process boundaries, that makes sense. Thanks, Taylor ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings 2022-10-31 20:47 ` [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings Ævar Arnfjörð Bjarmason 2022-11-01 1:06 ` Taylor Blau @ 2022-11-01 2:32 ` Jeff King 2022-11-01 3:01 ` Ævar Arnfjörð Bjarmason 2022-11-01 9:35 ` [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings Jeff King 1 sibling, 2 replies; 45+ messages in thread From: Jeff King @ 2022-11-01 2:32 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Taylor Blau, Derrick Stolee, Johannes Schindelin On Mon, Oct 31, 2022 at 09:47:08PM +0100, Ævar Arnfjörð Bjarmason wrote: > * When we invoke e.g. "git-remote-https" (aka. "git-remote-curl") > from "git fetch". For those cases let's pass down the equivalent of a > "-c transfer.credentialsInUrl=allow", since we know that we've already > inspected our remotes in the parent process. > > See 7390f05a3c6 (fetch: after refetch, encourage auto gc repacking, > 2022-03-28) for prior use of git_config_push_parameter() for this > purpose, i.e. to push config parameters before invoking a > sub-process. So I guess I don't have any _specific_ thing that goes wrong with this approach, but it really feels sketchy to me. We are lying to sub-processes about the config the user asked for. And again, I see how it works, but I wonder if this kind of approach would ever backfire on us. For example, if transfer.credentialsInUrl=warn ever ended up having any side effects besides the warning, and the sub-processes ended up skipping those effects. I know that's a hypothetical, and probably not even a likely one, but it just gets my spider sense tingling. > > Since it is not actually important how many times Git prints the > > warning/error message, as long as it prints it at least once, let's just > > make the test a bit more lenient and test for the latter instead of the > > former, which works around these CI issues. > > That being said your change is obviously smaller, so if we'd prefer > that in first as a band-aid I'm fine with that. > > But I'd really like to object to the "it is not actually important how > many...", it's crappy UX to spew duplicate output at the user, and we > should avoid it. > > So it does matter, and we get it wrong not just in this but also some > other cases. Yeah, I think it is crappy UX, too. It's just that I think the tests should not _asserting_ the bad behavior. At most, they should tolerate the bad behavior as a band-aid. So I think Dscho's patch is doing the right thing (and I do agree that we should fix the immediate CI pain by adjusting the tests, and letting the user-visible fix proceed independently). -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings 2022-11-01 2:32 ` Jeff King @ 2022-11-01 3:01 ` Ævar Arnfjörð Bjarmason 2022-11-01 20:54 ` Taylor Blau 2022-11-01 9:35 ` [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings Jeff King 1 sibling, 1 reply; 45+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-11-01 3:01 UTC (permalink / raw) To: Jeff King Cc: git, Junio C Hamano, Taylor Blau, Derrick Stolee, Johannes Schindelin On Mon, Oct 31 2022, Jeff King wrote: > On Mon, Oct 31, 2022 at 09:47:08PM +0100, Ævar Arnfjörð Bjarmason wrote: > [...] >> That being said your change is obviously smaller, so if we'd prefer >> that in first as a band-aid I'm fine with that. >> >> But I'd really like to object to the "it is not actually important how >> many...", it's crappy UX to spew duplicate output at the user, and we >> should avoid it. >> >> So it does matter, and we get it wrong not just in this but also some >> other cases. > > Yeah, I think it is crappy UX, too. It's just that I think the tests > should not _asserting_ the bad behavior. At most, they should tolerate > the bad behavior as a band-aid. So I think Dscho's patch is doing the > right thing (and I do agree that we should fix the immediate CI pain by > adjusting the tests, and letting the user-visible fix proceed > independently). The tests aren't just asserting the bad behavior, they're also ensuring that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but tolerable, but if we start emitting 500 of these it would be nice to know. I've found a couple of regressions like that in other areas, i.e. where our tests didn't spot some output change because we were selectively grepping things, but where it would have been nice to spot it at the time. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings 2022-11-01 3:01 ` Ævar Arnfjörð Bjarmason @ 2022-11-01 20:54 ` Taylor Blau 2022-11-01 22:17 ` Ævar Arnfjörð Bjarmason 2022-11-02 8:42 ` [PATCH v3 2/2] t5551: be less strict about the number of credential warnings Jeff King 0 siblings, 2 replies; 45+ messages in thread From: Taylor Blau @ 2022-11-01 20:54 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Jeff King, git, Junio C Hamano, Taylor Blau, Derrick Stolee, Johannes Schindelin On Tue, Nov 01, 2022 at 04:01:18AM +0100, Ævar Arnfjörð Bjarmason wrote: > > Yeah, I think it is crappy UX, too. It's just that I think the tests > > should not _asserting_ the bad behavior. At most, they should tolerate > > the bad behavior as a band-aid. So I think Dscho's patch is doing the > > right thing (and I do agree that we should fix the immediate CI pain by > > adjusting the tests, and letting the user-visible fix proceed > > independently). > > The tests aren't just asserting the bad behavior, they're also ensuring > that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but > tolerable, but if we start emitting 500 of these it would be nice to > know. I admit that this kind of argument does not sway me. Is it likely that we would suddenly start spewing 500 such warnings? If we did, are there no other tests that would catch it? And even if *that* were the case, would nobody happen to notice it in the meantime either during development or when we queue an affected topic onto 'next' for wider testing? I guess the answer is that it's possible that we'd miss such a regression in all of those above places, but to me it seems extremely unlikely that we'd let such a regression through without noticing. Thanks, Taylor ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings 2022-11-01 20:54 ` Taylor Blau @ 2022-11-01 22:17 ` Ævar Arnfjörð Bjarmason 2022-11-02 0:53 ` Taylor Blau 2022-11-02 8:42 ` [PATCH v3 2/2] t5551: be less strict about the number of credential warnings Jeff King 1 sibling, 1 reply; 45+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-11-01 22:17 UTC (permalink / raw) To: Taylor Blau Cc: Jeff King, git, Junio C Hamano, Derrick Stolee, Johannes Schindelin On Tue, Nov 01 2022, Taylor Blau wrote: > On Tue, Nov 01, 2022 at 04:01:18AM +0100, Ævar Arnfjörð Bjarmason wrote: >> > Yeah, I think it is crappy UX, too. It's just that I think the tests >> > should not _asserting_ the bad behavior. At most, they should tolerate >> > the bad behavior as a band-aid. So I think Dscho's patch is doing the >> > right thing (and I do agree that we should fix the immediate CI pain by >> > adjusting the tests, and letting the user-visible fix proceed >> > independently). >> >> The tests aren't just asserting the bad behavior, they're also ensuring >> that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but >> tolerable, but if we start emitting 500 of these it would be nice to >> know. > > I admit that this kind of argument does not sway me. > > Is it likely that we would suddenly start spewing 500 such warnings? If > we did, are there no other tests that would catch it? And even if *that* > were the case, would nobody happen to notice it in the meantime either > during development or when we queue an affected topic onto 'next' for > wider testing? > > I guess the answer is that it's possible that we'd miss such a > regression in all of those above places, but to me it seems extremely > unlikely that we'd let such a regression through without noticing. Literally 500? Probably not, that was hyperbole to make a point, but several, low tens? Yeah, I know of at least a couple in-tree off the top of my head. The point, which I assumed was clear is that we literally wouldn't notice if it were 500, and that sort of thing is a common pattern in our tests. I.e. in most cases we'd ideally test_cmp known output (at least to the extent of assuring ourselves that we're getting it right). Instead we often just grep it, or don't test it at all. Sometimes for a good reason (e.g. the output containing absolute paths), but more often than not for no good reason. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings 2022-11-01 22:17 ` Ævar Arnfjörð Bjarmason @ 2022-11-02 0:53 ` Taylor Blau 0 siblings, 0 replies; 45+ messages in thread From: Taylor Blau @ 2022-11-02 0:53 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Taylor Blau, Jeff King, git, Junio C Hamano, Derrick Stolee, Johannes Schindelin On Tue, Nov 01, 2022 at 11:17:42PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> The tests aren't just asserting the bad behavior, they're also ensuring > >> that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but > >> tolerable, but if we start emitting 500 of these it would be nice to > >> know. > > > > I admit that this kind of argument does not sway me. > > > > Is it likely that we would suddenly start spewing 500 such warnings? If > > we did, are there no other tests that would catch it? And even if *that* > > were the case, would nobody happen to notice it in the meantime either > > during development or when we queue an affected topic onto 'next' for > > wider testing? > > > > I guess the answer is that it's possible that we'd miss such a > > regression in all of those above places, but to me it seems extremely > > unlikely that we'd let such a regression through without noticing. > > Literally 500? Probably not, that was hyperbole to make a point, but > several, low tens? Yeah, I know of at least a couple in-tree off the top > of my head. Still, I find it suspect to assume that a developer working in this area wouldn't notice even a minor regression (say, adding an additional warning). Thanks, Taylor ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v3 2/2] t5551: be less strict about the number of credential warnings 2022-11-01 20:54 ` Taylor Blau 2022-11-01 22:17 ` Ævar Arnfjörð Bjarmason @ 2022-11-02 8:42 ` Jeff King 2022-11-02 8:49 ` Eric Sunshine 2022-11-02 9:18 ` Jeff King 1 sibling, 2 replies; 45+ messages in thread From: Jeff King @ 2022-11-02 8:42 UTC (permalink / raw) To: Taylor Blau Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano, Derrick Stolee, Johannes Schindelin On Tue, Nov 01, 2022 at 04:54:38PM -0400, Taylor Blau wrote: > > The tests aren't just asserting the bad behavior, they're also ensuring > > that it doesn't get worse. 1 warning is ideal, 2-3 is bad, but > > tolerable, but if we start emitting 500 of these it would be nice to > > know. > > I admit that this kind of argument does not sway me. > > Is it likely that we would suddenly start spewing 500 such warnings? If > we did, are there no other tests that would catch it? And even if *that* > were the case, would nobody happen to notice it in the meantime either > during development or when we queue an affected topic onto 'next' for > wider testing? > > I guess the answer is that it's possible that we'd miss such a > regression in all of those above places, but to me it seems extremely > unlikely that we'd let such a regression through without noticing. Like you, I don't find much value in asserting "2 or 3, but not 500". But it is easy enough to at least only loosen the few cases that need it. So here's a replacement for 2/2 that does the minimal thing. I rewrote the commit message to explain my view (incidentally, it also fixes the subject line, which mentioned the wrong test number after the rebase). As I said, I had tried to mostly leave patch 2 alone to avoid derailing Dscho's attempt to fix things. But somehow things got derailed anyway, so maybe we can just all agree on this patch and move on with our lives? I can't over-emphasize how little I care about this credentialsInUrl feature in the first place, and somehow it has consumed hours of my life now. -- >8 -- Subject: t5551: be less strict about the number of credential warnings It is unclear as to _why_, but under certain circumstances the warning about credentials being passed as part of the URL seems to be swallowed by the `git remote-https` helper in the Windows jobs of Git's CI builds. This causes the tests to fail, because they assert that in a few cases we should print the warning 2 or even 3 times. The reason for that is given in 6dcbdc0d66 (remote: create fetch.credentialsInUrl config, 2022-06-06), which is that multiple processes are involved, and each warns. In an ideal world, the user would just see the message once per logical operation; they don't care how many underlying processes are involved. And we may fix that eventually. But in the meantime, let's loosen the tests to just assert that the user sees the message _at least_ once. This won't catch a case where we accidentally start producing it 500 times, but that's not a likely regression. A more likely thing is that we'd start producing it four times because the underlying code changes to use a new process. But that's exactly the kind of thing we'd prefer to be ignoring in the tests. Note that the tests for the "die" mode don't need adjusted. They die immediately in the first process that sees the problem, so they consistently show the error once. It's only the "warn" mode which must be loose. If we eventually fix it, then we can tighten its assertion. In the meantime, this works around the CI issues. Based on a patch by Johannes Schindelin. Signed-off-by: Jeff King <peff@peff.net> --- t/t5551-http-fetch-smart.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index bbe3d5721f..4f559722f4 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -597,7 +597,7 @@ test_expect_success 'clone warns or fails when using username:password' ' git -c transfer.credentialsInUrl=warn \ clone $url_userpass attempt2 2>err && grep "warning: $message" err >warnings && - test_line_count = 2 warnings && + test_line_count -ge 1 warnings && test_must_fail git -c transfer.credentialsInUrl=die \ clone $url_userpass attempt3 2>err && @@ -630,7 +630,7 @@ test_expect_success 'fetch warns or fails when using username:password' ' git -c transfer.credentialsInUrl=warn fetch $url_userpass 2>err && grep "warning: $message" err >warnings && - test_line_count = 3 warnings && + test_line_count -ge 1 warnings && test_must_fail git -c transfer.credentialsInUrl=die \ fetch $url_userpass 2>err && -- 2.38.1.677.g9b1428ac2e ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings 2022-11-02 8:42 ` [PATCH v3 2/2] t5551: be less strict about the number of credential warnings Jeff King @ 2022-11-02 8:49 ` Eric Sunshine 2022-11-02 9:15 ` Jeff King 2022-11-02 9:18 ` Jeff King 1 sibling, 1 reply; 45+ messages in thread From: Eric Sunshine @ 2022-11-02 8:49 UTC (permalink / raw) To: Jeff King Cc: Taylor Blau, Ævar Arnfjörð Bjarmason, git, Junio C Hamano, Derrick Stolee, Johannes Schindelin On Wed, Nov 2, 2022 at 4:46 AM Jeff King <peff@peff.net> wrote: > Subject: t5551: be less strict about the number of credential warnings > > It is unclear as to _why_, but under certain circumstances the warning > about credentials being passed as part of the URL seems to be swallowed > by the `git remote-https` helper in the Windows jobs of Git's CI builds. > > This causes the tests to fail, because they assert that in a few cases > we should print the warning 2 or even 3 times. The reason for that is > given in 6dcbdc0d66 (remote: create fetch.credentialsInUrl config, > 2022-06-06), which is that multiple processes are involved, and each > warns. > > In an ideal world, the user would just see the message once per logical > operation; they don't care how many underlying processes are involved. > And we may fix that eventually. But in the meantime, let's loosen the > tests to just assert that the user sees the message _at least_ once. > > This won't catch a case where we accidentally start producing it 500 > times, but that's not a likely regression. A more likely thing is that > we'd start producing it four times because the underlying code changes > to use a new process. But that's exactly the kind of thing we'd prefer > to be ignoring in the tests. > > Note that the tests for the "die" mode don't need adjusted. They die s/adjusted/adjustment --or -- s/need/& to be/ > immediately in the first process that sees the problem, so they > consistently show the error once. It's only the "warn" mode which must > be loose. If we eventually fix it, then we can tighten its assertion. In > the meantime, this works around the CI issues. > > Based on a patch by Johannes Schindelin. > > Signed-off-by: Jeff King <peff@peff.net> ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings 2022-11-02 8:49 ` Eric Sunshine @ 2022-11-02 9:15 ` Jeff King 2022-11-02 9:31 ` Eric Sunshine 0 siblings, 1 reply; 45+ messages in thread From: Jeff King @ 2022-11-02 9:15 UTC (permalink / raw) To: Eric Sunshine Cc: Taylor Blau, Ævar Arnfjörð Bjarmason, git, Junio C Hamano, Derrick Stolee, Johannes Schindelin On Wed, Nov 02, 2022 at 04:49:37AM -0400, Eric Sunshine wrote: > > Note that the tests for the "die" mode don't need adjusted. They die > > s/adjusted/adjustment --or -- s/need/& to be/ https://english.stackexchange.com/questions/5407/central-pennsylvanian-english-speakers-what-are-the-limitations-on-the-needs-w Don't stomp on my linguistic heritage. :) -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings 2022-11-02 9:15 ` Jeff King @ 2022-11-02 9:31 ` Eric Sunshine 0 siblings, 0 replies; 45+ messages in thread From: Eric Sunshine @ 2022-11-02 9:31 UTC (permalink / raw) To: Jeff King Cc: Taylor Blau, Ævar Arnfjörð Bjarmason, git, Junio C Hamano, Derrick Stolee, Johannes Schindelin On Wed, Nov 2, 2022 at 5:15 AM Jeff King <peff@peff.net> wrote: > On Wed, Nov 02, 2022 at 04:49:37AM -0400, Eric Sunshine wrote: > > > Note that the tests for the "die" mode don't need adjusted. They die > > > > s/adjusted/adjustment --or -- s/need/& to be/ > > https://english.stackexchange.com/questions/5407/central-pennsylvanian-english-speakers-what-are-the-limitations-on-the-needs-w > > Don't stomp on my linguistic heritage. :) Sorry. My head needs hanged in shame. I forgot that I can't grammar. (I can't math either.) ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings 2022-11-02 8:42 ` [PATCH v3 2/2] t5551: be less strict about the number of credential warnings Jeff King 2022-11-02 8:49 ` Eric Sunshine @ 2022-11-02 9:18 ` Jeff King 2022-11-03 1:31 ` Taylor Blau 1 sibling, 1 reply; 45+ messages in thread From: Jeff King @ 2022-11-02 9:18 UTC (permalink / raw) To: Taylor Blau Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano, Derrick Stolee, Johannes Schindelin On Wed, Nov 02, 2022 at 04:42:13AM -0400, Jeff King wrote: > As I said, I had tried to mostly leave patch 2 alone to avoid derailing > Dscho's attempt to fix things. But somehow things got derailed anyway, > so maybe we can just all agree on this patch and move on with our lives? By the way, I think you (or somebody?) mentioned elsewhere in the thread that it's possible the first patch fixes things by itself. I would also be content to just apply the first one and see if CI improves. Of course, when I just pushed all this out to CI, it flaked independently on both osx (looks like racy p4 stuff) and fedora (network dropout failed to set up the container). Sigh. -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 2/2] t5551: be less strict about the number of credential warnings 2022-11-02 9:18 ` Jeff King @ 2022-11-03 1:31 ` Taylor Blau 0 siblings, 0 replies; 45+ messages in thread From: Taylor Blau @ 2022-11-03 1:31 UTC (permalink / raw) To: Jeff King Cc: Taylor Blau, Ævar Arnfjörð Bjarmason, git, Junio C Hamano, Derrick Stolee, Johannes Schindelin On Wed, Nov 02, 2022 at 05:18:29AM -0400, Jeff King wrote: > On Wed, Nov 02, 2022 at 04:42:13AM -0400, Jeff King wrote: > > > As I said, I had tried to mostly leave patch 2 alone to avoid derailing > > Dscho's attempt to fix things. But somehow things got derailed anyway, > > so maybe we can just all agree on this patch and move on with our lives? > > By the way, I think you (or somebody?) mentioned elsewhere in the thread > that it's possible the first patch fixes things by itself. I would also > be content to just apply the first one and see if CI improves. > > Of course, when I just pushed all this out to CI, it flaked > independently on both osx (looks like racy p4 stuff) and fedora (network > dropout failed to set up the container). Sigh. Dreams are free, eh? Thanks, Taylor ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings 2022-11-01 2:32 ` Jeff King 2022-11-01 3:01 ` Ævar Arnfjörð Bjarmason @ 2022-11-01 9:35 ` Jeff King 2022-11-01 13:07 ` Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 45+ messages in thread From: Jeff King @ 2022-11-01 9:35 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Taylor Blau, Derrick Stolee, Johannes Schindelin On Mon, Oct 31, 2022 at 10:32:36PM -0400, Jeff King wrote: > On Mon, Oct 31, 2022 at 09:47:08PM +0100, Ævar Arnfjörð Bjarmason wrote: > > > * When we invoke e.g. "git-remote-https" (aka. "git-remote-curl") > > from "git fetch". For those cases let's pass down the equivalent of a > > "-c transfer.credentialsInUrl=allow", since we know that we've already > > inspected our remotes in the parent process. > > > > See 7390f05a3c6 (fetch: after refetch, encourage auto gc repacking, > > 2022-03-28) for prior use of git_config_push_parameter() for this > > purpose, i.e. to push config parameters before invoking a > > sub-process. > > So I guess I don't have any _specific_ thing that goes wrong with this > approach, but it really feels sketchy to me. We are lying to > sub-processes about the config the user asked for. And again, I see how > it works, but I wonder if this kind of approach would ever backfire on > us. For example, if transfer.credentialsInUrl=warn ever ended up having > any side effects besides the warning, and the sub-processes ended up > skipping those effects. > > I know that's a hypothetical, and probably not even a likely one, but it > just gets my spider sense tingling. So inherently this is going to be ugly because it's crossing process boundaries. But the more minimal fix I was thinking of would be something like this: diff --git a/remote.c b/remote.c index 60869beebe..af5f95c719 100644 --- a/remote.c +++ b/remote.c @@ -615,6 +615,14 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push) return NULL; } +static int test_and_set_env(const char *var) +{ + int ret = git_env_bool(var, 0); + if (!ret) + setenv(var, "1", 0); + return ret; +} + static void validate_remote_url(struct remote *remote) { int i; @@ -634,6 +642,9 @@ static void validate_remote_url(struct remote *remote) else die(_("unrecognized value transfer.credentialsInUrl: '%s'"), value); + if (test_and_set_env("GIT_CHECKED_CREDENTIALS_IN_URL")) + return; + for (i = 0; i < remote->url_nr; i++) { struct url_info url_info = { 0 }; You can also put it lower in the function, when we actually warn, which saves adding to the environment when there is nothing to warn about (though this way, we avoid doing the redundant work). Compared to munging the config, this seems shorter and simpler, and there's no chance of us ever getting confused between the fake "suppress" value and something the user actually asked for. -Peff ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings 2022-11-01 9:35 ` [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings Jeff King @ 2022-11-01 13:07 ` Ævar Arnfjörð Bjarmason 2022-11-01 21:00 ` Taylor Blau 0 siblings, 1 reply; 45+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-11-01 13:07 UTC (permalink / raw) To: Jeff King Cc: git, Junio C Hamano, Taylor Blau, Derrick Stolee, Johannes Schindelin On Tue, Nov 01 2022, Jeff King wrote: > On Mon, Oct 31, 2022 at 10:32:36PM -0400, Jeff King wrote: > >> On Mon, Oct 31, 2022 at 09:47:08PM +0100, Ævar Arnfjörð Bjarmason wrote: >> >> > * When we invoke e.g. "git-remote-https" (aka. "git-remote-curl") >> > from "git fetch". For those cases let's pass down the equivalent of a >> > "-c transfer.credentialsInUrl=allow", since we know that we've already >> > inspected our remotes in the parent process. >> > >> > See 7390f05a3c6 (fetch: after refetch, encourage auto gc repacking, >> > 2022-03-28) for prior use of git_config_push_parameter() for this >> > purpose, i.e. to push config parameters before invoking a >> > sub-process. >> >> So I guess I don't have any _specific_ thing that goes wrong with this >> approach, but it really feels sketchy to me. We are lying to >> sub-processes about the config the user asked for. And again, I see how >> it works, but I wonder if this kind of approach would ever backfire on >> us. For example, if transfer.credentialsInUrl=warn ever ended up having >> any side effects besides the warning, and the sub-processes ended up >> skipping those effects. >> >> I know that's a hypothetical, and probably not even a likely one, but it >> just gets my spider sense tingling. > > So inherently this is going to be ugly because it's crossing process > boundaries. But the more minimal fix I was thinking of would be > something like this: > > diff --git a/remote.c b/remote.c > index 60869beebe..af5f95c719 100644 > --- a/remote.c > +++ b/remote.c > @@ -615,6 +615,14 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push) > return NULL; > } > > +static int test_and_set_env(const char *var) > +{ > + int ret = git_env_bool(var, 0); > + if (!ret) > + setenv(var, "1", 0); > + return ret; > +} > + > static void validate_remote_url(struct remote *remote) > { > int i; > @@ -634,6 +642,9 @@ static void validate_remote_url(struct remote *remote) > else > die(_("unrecognized value transfer.credentialsInUrl: '%s'"), value); > > + if (test_and_set_env("GIT_CHECKED_CREDENTIALS_IN_URL")) > + return; > + > for (i = 0; i < remote->url_nr; i++) { > struct url_info url_info = { 0 }; > > > You can also put it lower in the function, when we actually warn, which > saves adding to the environment when there is nothing to warn about > (though this way, we avoid doing the redundant work). > > Compared to munging the config, this seems shorter and simpler, and > there's no chance of us ever getting confused between the fake > "suppress" value and something the user actually asked for. Sure, we can do it with an environment variable, in the end that's all git_config_push_parameter() is doing too. It's just setting things in "GIT_CONFIG_PARAMETERS". And yes, we can set this in the low-level function instead of with git_config_push_parameter() in builtin/*.c as I did. I was aiming for something demonstrably narrow, at the cost of some verbosity. But I don't get how other things being equal you think sticking this in "GIT_CHECKED_CREDENTIALS_IN_URL" instead of "GIT_CONFIG_PARAMETERS" helps. We already pass config to ourselves like that (and via "-c") in other places. Can you think of a case where these would be different? The only ones I can think of are e.g. because we know about "GIT_CONFIG_PARAMETERS", and not this new custom variable, e.g. in "prepare_other_repo_env()", but those seem like exactly the reason to use the existing variable. I can think of potential pitfalls here, e.g. how does it interact with submodules? That's one reason I submitted it as an RFC, the tests need to be better (with or without this change). E.g. "git ls-remote" is also not covered by the upthread patch. But that's all separate from what the environment variable is named, or if it lives in the config space. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings 2022-11-01 13:07 ` Ævar Arnfjörð Bjarmason @ 2022-11-01 21:00 ` Taylor Blau 2022-11-01 21:57 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 45+ messages in thread From: Taylor Blau @ 2022-11-01 21:00 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Jeff King, git, Junio C Hamano, Taylor Blau, Derrick Stolee, Johannes Schindelin On Tue, Nov 01, 2022 at 02:07:39PM +0100, Ævar Arnfjörð Bjarmason wrote: > > You can also put it lower in the function, when we actually warn, which > > saves adding to the environment when there is nothing to warn about > > (though this way, we avoid doing the redundant work). > > > > Compared to munging the config, this seems shorter and simpler, and > > there's no chance of us ever getting confused between the fake > > "suppress" value and something the user actually asked for. > > Sure, we can do it with an environment variable, in the end that's all > git_config_push_parameter() is doing too. It's just setting things in > "GIT_CONFIG_PARAMETERS". > > And yes, we can set this in the low-level function instead of with > git_config_push_parameter() in builtin/*.c as I did. I was aiming for > something demonstrably narrow, at the cost of some verbosity. > > But I don't get how other things being equal you think sticking this in > "GIT_CHECKED_CREDENTIALS_IN_URL" instead of "GIT_CONFIG_PARAMETERS" > helps. I vaguely prefer calling this GIT_CHECKED_CREDENTIALS_IN_URL instead of stuffing it in the configuration. All other things *aren't* equal here, since we're not lying to sub-processes about configuration values set by the user. Instead, we're saying: "regardless of what value the user assigned transfer.credentialsInUrl, we can avoid looking at it because we have already checked for the presence of credentials in the URL". Thanks, Taylor ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings 2022-11-01 21:00 ` Taylor Blau @ 2022-11-01 21:57 ` Ævar Arnfjörð Bjarmason 2022-11-02 8:19 ` Jeff King 0 siblings, 1 reply; 45+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-11-01 21:57 UTC (permalink / raw) To: Taylor Blau Cc: Jeff King, git, Junio C Hamano, Derrick Stolee, Johannes Schindelin On Tue, Nov 01 2022, Taylor Blau wrote: > On Tue, Nov 01, 2022 at 02:07:39PM +0100, Ævar Arnfjörð Bjarmason wrote: >> > You can also put it lower in the function, when we actually warn, which >> > saves adding to the environment when there is nothing to warn about >> > (though this way, we avoid doing the redundant work). >> > >> > Compared to munging the config, this seems shorter and simpler, and >> > there's no chance of us ever getting confused between the fake >> > "suppress" value and something the user actually asked for. >> >> Sure, we can do it with an environment variable, in the end that's all >> git_config_push_parameter() is doing too. It's just setting things in >> "GIT_CONFIG_PARAMETERS". >> >> And yes, we can set this in the low-level function instead of with >> git_config_push_parameter() in builtin/*.c as I did. I was aiming for >> something demonstrably narrow, at the cost of some verbosity. >> >> But I don't get how other things being equal you think sticking this in >> "GIT_CHECKED_CREDENTIALS_IN_URL" instead of "GIT_CONFIG_PARAMETERS" >> helps. > > I vaguely prefer calling this GIT_CHECKED_CREDENTIALS_IN_URL instead of > stuffing it in the configuration.[...] To be clear, I'm asking if there's cases where we think one method or the other produces different results, which I understood Jeff hinting at. > Instead, we're saying: "regardless of what value the user assigned > transfer.credentialsInUrl, we can avoid looking at it because we have > already checked for the presence of credentials in the URL".[...] All > other things *aren't* equal here, since we're not lying to > sub-processes about configuration values set by the user. The user is asking us to warn about storing certain things in config, we know we already warned, so we're looking to flip that value to the "quiet" setting. If you consider that overriding I don't get how doing so via a different environment variable changes anything. It would be doing the same thing, just making it less obvious. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings 2022-11-01 21:57 ` Ævar Arnfjörð Bjarmason @ 2022-11-02 8:19 ` Jeff King 2022-11-04 9:01 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 45+ messages in thread From: Jeff King @ 2022-11-02 8:19 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Taylor Blau, git, Junio C Hamano, Derrick Stolee, Johannes Schindelin On Tue, Nov 01, 2022 at 10:57:46PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> Sure, we can do it with an environment variable, in the end that's all > >> git_config_push_parameter() is doing too. It's just setting things in > >> "GIT_CONFIG_PARAMETERS". > >> > >> And yes, we can set this in the low-level function instead of with > >> git_config_push_parameter() in builtin/*.c as I did. I was aiming for > >> something demonstrably narrow, at the cost of some verbosity. > >> > >> But I don't get how other things being equal you think sticking this in > >> "GIT_CHECKED_CREDENTIALS_IN_URL" instead of "GIT_CONFIG_PARAMETERS" > >> helps. > > > > I vaguely prefer calling this GIT_CHECKED_CREDENTIALS_IN_URL instead of > > stuffing it in the configuration.[...] > > To be clear, I'm asking if there's cases where we think one method or > the other produces different results, which I understood Jeff hinting > at. What I was hinting before was not that I knew of a particular bug in your patch, but that I think the technique of munging GIT_CONFIG_PARAMETERS is fragile in error-prone in the general case, because the sub-programs can't differentiate between the config the user asked for, and what was set by the suppression mechanism. For this variable, there's no need to differentiate between "the user asked us to be quiet" and "the calling program asked us to be quiet", but I could imagine cases where there are subtle distinctions. Imagine if there was a setting for "warn and rewrite the URL". We'd need to change that to "don't warn, but just rewrite the URL", which otherwise is a mode that doesn't need to exist. Keeping it in a separate variable keeps the concerns orthogonal. The code still gets to see what the user actually wants (via the config), but has extra information from the calling process about how noisy/quiet to be. But you mentioned submodules in your other mail. And you're right that the patch I showed doesn't handle that (it would need to add the new variable to local_repo_env). It seems like yours should work because CONFIG_DATA_ENVIRONMENT as part of local_repo_env. But I don't think it actually does; in prepare_other_repo_env(), we retain the variables for config in the environment, so that: git -c foo.bar=whatever fetch will override variables in both the superproject and in submodules. I didn't try it, but I suspect with your patch that a superproject with "warn" and a submodule with "die" (both in their on-disk config files) would misbehave. The superproject process will warn and say "yes, I've checked everything" by munging the in-environment config to "allow". Then the submodule process will see that config, and will override the on-disk setting (in the usual last-one-wins config way). I.e., the problem is that it cannot tell the difference between "the user asked to override this" and the suppression mechanism. -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings 2022-11-02 8:19 ` Jeff King @ 2022-11-04 9:01 ` Ævar Arnfjörð Bjarmason 2022-11-04 13:16 ` Jeff King 0 siblings, 1 reply; 45+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-11-04 9:01 UTC (permalink / raw) To: Jeff King Cc: Taylor Blau, git, Junio C Hamano, Derrick Stolee, Johannes Schindelin On Wed, Nov 02 2022, Jeff King wrote: > On Tue, Nov 01, 2022 at 10:57:46PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> >> Sure, we can do it with an environment variable, in the end that's all >> >> git_config_push_parameter() is doing too. It's just setting things in >> >> "GIT_CONFIG_PARAMETERS". >> >> >> >> And yes, we can set this in the low-level function instead of with >> >> git_config_push_parameter() in builtin/*.c as I did. I was aiming for >> >> something demonstrably narrow, at the cost of some verbosity. >> >> >> >> But I don't get how other things being equal you think sticking this in >> >> "GIT_CHECKED_CREDENTIALS_IN_URL" instead of "GIT_CONFIG_PARAMETERS" >> >> helps. >> > >> > I vaguely prefer calling this GIT_CHECKED_CREDENTIALS_IN_URL instead of >> > stuffing it in the configuration.[...] >> >> To be clear, I'm asking if there's cases where we think one method or >> the other produces different results, which I understood Jeff hinting >> at. > > What I was hinting before was not that I knew of a particular bug in > your patch, but that I think the technique of munging > GIT_CONFIG_PARAMETERS is fragile in error-prone in the general case, > because the sub-programs can't differentiate between the config the user > asked for, and what was set by the suppression mechanism. > > For this variable, there's no need to differentiate between "the user > asked us to be quiet" and "the calling program asked us to be quiet", > but I could imagine cases where there are subtle distinctions. Imagine > if there was a setting for "warn and rewrite the URL". We'd need to > change that to "don't warn, but just rewrite the URL", which otherwise > is a mode that doesn't need to exist. > > Keeping it in a separate variable keeps the concerns orthogonal. The > code still gets to see what the user actually wants (via the config), > but has extra information from the calling process about how noisy/quiet > to be. ... (replied below) ... > But you mentioned submodules in your other mail. And you're right that > the patch I showed doesn't handle that (it would need to add the new > variable to local_repo_env). It seems like yours should work because > CONFIG_DATA_ENVIRONMENT as part of local_repo_env. But I don't think it > actually does; in prepare_other_repo_env(), we retain the variables for > config in the environment, so that: > > git -c foo.bar=whatever fetch > > will override variables in both the superproject and in submodules. Replying to your main point below, just an aside on this: To be clear I'm not saying it would handle it sensibly now, but just that if we're using env vars to communicate to sub-processes then using CONFIG_DATA_ENVIRONMENT seems better to me. Because even if we're getting it wrong now, then surely that's something we're probably getting wrong in more than one place. So e.g. if we set an env var "for ourselves", i.e. "pull->fetch" then we could detect that condition in run_command(), and if we then spot that we're carrying an env variable we set earlier up the chain reset it before we spawn a submodule. Whereas if it's all custom env variables here & there we'll need to add all that special-casing in. > I didn't try it, but I suspect with your patch that a superproject with > "warn" and a submodule with "die" (both in their on-disk config files) > would misbehave. The superproject process will warn and say "yes, I've > checked everything" by munging the in-environment config to "allow". > Then the submodule process will see that config, and will override the > on-disk setting (in the usual last-one-wins config way). I.e., the > problem is that it cannot tell the difference between "the user asked to > override this" and the suppression mechanism. Yes, definitely, and now I see what you're saying. I.e. imagine a chain like this (not actual process chains, but let's go with the example); user config = warn run: pull our config = allow # OK: doesn't "warn" now run: fetch # Not warning, but .... run: pre-fetch hook # BAD: ...oh oh, now a hook is fetching some # entirely unrelated repo run: git pull # OK: Doesn't warn either run: remote-curl (now not warning, otherwise would) # BAD: our "warned already" has infected a # submodule, and we downgrade "die" to "allow" user config = die run: git fetch <submodule> ... But, and maybe I'm still not getting it, but the "use a different env var" isn't actually the important part, i.e. these would behave the same after the initial "warn": -c transfer.credentialsInUrlWarnedAlready=true And: GIT_CHECKED_AND_WARNED_ALREADY=1 But not what I was suggesting: # conflated with a later "die" -c transfer.credentialsInUrl=allow But that only goes for e.g. a "pull" where we "warn" followed by submodule whose config is "die". But all suggested variants of this (mine and yours) are going to get e.g. the case where the submodule also wants "warn". I.e. it's not enough that we're saying "warned already". That gets us past conflating an existing "warn" with a "die", but now we can't tell a submodule "warn" from a superproject "warn". For that we'd basically need to do: -c transfer.$(<make path to .git, or other "unique repo id>).credentialsInUrl=allow Or another similar mechanism, of course the most sane one to be to not be playing this game at all, but to just ferry this state e.g. with "--do-not-warn-about-this-repo" to our own children, which we'd not add to the command-lines when we know we're spawning a hook, or doing the submodule "pull/fetch". Does that all seem right? ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings 2022-11-04 9:01 ` Ævar Arnfjörð Bjarmason @ 2022-11-04 13:16 ` Jeff King 0 siblings, 0 replies; 45+ messages in thread From: Jeff King @ 2022-11-04 13:16 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Taylor Blau, git, Junio C Hamano, Derrick Stolee, Johannes Schindelin On Fri, Nov 04, 2022 at 10:01:00AM +0100, Ævar Arnfjörð Bjarmason wrote: > > But you mentioned submodules in your other mail. And you're right that > > the patch I showed doesn't handle that (it would need to add the new > > variable to local_repo_env). It seems like yours should work because > > CONFIG_DATA_ENVIRONMENT as part of local_repo_env. But I don't think it > > actually does; in prepare_other_repo_env(), we retain the variables for > > config in the environment, so that: > > > > git -c foo.bar=whatever fetch > > > > will override variables in both the superproject and in submodules. > > Replying to your main point below, just an aside on this: > > To be clear I'm not saying it would handle it sensibly now, but just > that if we're using env vars to communicate to sub-processes then using > CONFIG_DATA_ENVIRONMENT seems better to me. > > Because even if we're getting it wrong now, then surely that's something > we're probably getting wrong in more than one place. > > So e.g. if we set an env var "for ourselves", i.e. "pull->fetch" then we > could detect that condition in run_command(), and if we then spot that > we're carrying an env variable we set earlier up the chain reset it > before we spawn a submodule. > > Whereas if it's all custom env variables here & there we'll need to add > all that special-casing in. The idea is that local_repo_env carries that information, and everybody uses it. So yes, it would have to know about the custom env variables, but then any place which enters another repo learns about it "for free". But using CONFIG_DATA_ENVIRONMENT for this doesn't work, because we don't actually clear it when moving between repositories. And in the example I showed, I used a config variable that was specific to this particular problem. But if there were a lot of these, it really could be: GIT_CHECKED_AND_WARNED='remotes some-other-subsytem etc' so that all of them were shoved into one variable, and parsed. But we can do the simplest dumb thing for now, because none of this is a public interface we need to support across versions. We could move from one to the other later. > But, and maybe I'm still not getting it, but the "use a different env > var" isn't actually the important part, i.e. these would behave the > same after the initial "warn": > > -c transfer.credentialsInUrlWarnedAlready=true > > And: > > GIT_CHECKED_AND_WARNED_ALREADY=1 > > But not what I was suggesting: > > # conflated with a later "die" > -c transfer.credentialsInUrl=allow Right. The important thing is not that it's in a different env variable in particular, but that it _isn't_ using the same config variable that the user would set. But once you are not using that same config variable, the question is: do you want it put in with the config data or not? And I think we have seen that putting it with the config data is not going to work. We don't clear those environment variables when moving between repositories. Now obviously if the env-clearing code knew which config variables were to be cleared and which not, it could do so. But why introduce that complexity, when you can just stick all the stuff that should be cleared into a new variable (and again, that can be _one_ variable for all of this stuff). > But all suggested variants of this (mine and yours) are going to get > e.g. the case where the submodule also wants "warn". I.e. it's not > enough that we're saying "warned already". > > That gets us past conflating an existing "warn" with a "die", but now we > can't tell a submodule "warn" from a superproject "warn". > > For that we'd basically need to do: > > -c transfer.$(<make path to .git, or other "unique repo id>).credentialsInUrl=allow > > Or another similar mechanism, of course the most sane one to be to not > be playing this game at all, but to just ferry this state e.g. with > "--do-not-warn-about-this-repo" to our own children, which we'd not add > to the command-lines when we know we're spawning a hook, or doing the > submodule "pull/fetch". > > Does that all seem right? That seems much more complicated. Again, why bother stuffing this extra context information into a config variable, just to work around the fact that config isn't cleared between repositories, when there is already a simple mechanism for clearing state when switching repositories? All that said, the implementation of this whole warning seems really weird to me. If we want to warn about using such a URL, then we should do it at the point of use. From the original commit message, the only reason it was put where it is was to avoid sigpipe when the remote helper dies. The solution there seems to be "have the helper die in a less abrupt way". And TBH, I'm skeptical of the whole feature. I don't see the point in even warning somebody about: git fetch https://username:password@example.com Maybe it's bad practice (though with password stand-ins like restricted-access tokens, it isn't always), but at the point the user has invoked git, there's nothing else bad that happens by doing what they asked for. What _is_ potentially bad is using that URL for cloning, because we then write the credential in plaintext to the config file. And a better solution there is probably to issue a warning, clone anyway, and then omit the password from what we write into config. A follow-on fetch would fail, but that is not any worse than the "die" mode of this current config option. There were even patches we discussed a few years ago: https://lore.kernel.org/git/20190517222031.GA17966@sigill.intra.peff.net/ I think the only really contentious part was the third one, which tried to do so automagic with credential helpers. If we skipped that, then the mode given by patches 1+2 seems strictly better than anything this transfer.credentialsInUrl option provides. -Peff ^ permalink raw reply [flat|nested] 45+ messages in thread
end of thread, other threads:[~2022-11-04 13:17 UTC | newest] Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-31 19:47 [PATCH 0/2] t5516/t5601: avoid using localhost for failing HTTPS requests Johannes Schindelin via GitGitGadget 2022-10-31 19:47 ` [PATCH 1/2] t5516/t5601: avoid using `localhost` " Johannes Schindelin via GitGitGadget 2022-10-31 20:49 ` Ævar Arnfjörð Bjarmason 2022-10-31 23:20 ` Jeff King 2022-11-01 0:59 ` Taylor Blau 2022-11-01 2:28 ` Jeff King 2022-11-01 2:03 ` Jeff King 2022-11-01 2:25 ` Jeff King 2022-11-01 2:26 ` [PATCH 1/2] t5516: move plaintext-password tests from t5601 and t5516 Jeff King 2022-11-01 3:18 ` Ævar Arnfjörð Bjarmason 2022-11-01 7:32 ` Jeff King 2022-11-01 20:37 ` Taylor Blau 2022-11-01 2:26 ` [PATCH 2/2] t5516/t5601: be less strict about the number of credential warnings Jeff King 2022-11-01 3:29 ` Ævar Arnfjörð Bjarmason 2022-11-01 7:39 ` Jeff King 2022-11-01 8:15 ` Ævar Arnfjörð Bjarmason 2022-11-01 9:12 ` Jeff King 2022-11-01 14:05 ` Ævar Arnfjörð Bjarmason 2022-11-01 4:54 ` Junio C Hamano 2022-11-01 7:42 ` Jeff King 2022-11-01 20:50 ` Taylor Blau 2022-10-31 19:47 ` Johannes Schindelin via GitGitGadget 2022-10-31 23:22 ` Jeff King 2022-11-01 0:57 ` Taylor Blau 2022-11-01 2:27 ` Jeff King 2022-10-31 20:47 ` [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings Ævar Arnfjörð Bjarmason 2022-11-01 1:06 ` Taylor Blau 2022-11-01 2:32 ` Jeff King 2022-11-01 3:01 ` Ævar Arnfjörð Bjarmason 2022-11-01 20:54 ` Taylor Blau 2022-11-01 22:17 ` Ævar Arnfjörð Bjarmason 2022-11-02 0:53 ` Taylor Blau 2022-11-02 8:42 ` [PATCH v3 2/2] t5551: be less strict about the number of credential warnings Jeff King 2022-11-02 8:49 ` Eric Sunshine 2022-11-02 9:15 ` Jeff King 2022-11-02 9:31 ` Eric Sunshine 2022-11-02 9:18 ` Jeff King 2022-11-03 1:31 ` Taylor Blau 2022-11-01 9:35 ` [RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings Jeff King 2022-11-01 13:07 ` Ævar Arnfjörð Bjarmason 2022-11-01 21:00 ` Taylor Blau 2022-11-01 21:57 ` Ævar Arnfjörð Bjarmason 2022-11-02 8:19 ` Jeff King 2022-11-04 9:01 ` Ævar Arnfjörð Bjarmason 2022-11-04 13:16 ` Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.