Git 2.23.0-rc0 HTTP authentication failure - error message change

Git 2.23.0-rc0 HTTP authentication failure - error message change

[Ben Humphreys]

Hi,

I've run 2.32.0-rc0 through the Bitbucket Server test matrix and it picked up a
change in error message that perhaps is unintentional.

On 2.31.1:
  $ git ls-remote http://notauser@localhost:7990/bitbucket/scm/project_1/rep_1.git
  Password for 'http://notauser@localhost:7990':
  fatal: Authentication failed for 'http://localhost:7990/bitbucket/scm/project_1/rep_1.git/'

and on 2.32.0-rc0:
  $ git ls-remote http://notauser@localhost:7990/bitbucket/scm/project_1/rep_1.git
  Password for 'http://notauser@localhost:7990':
  fatal: unable to access 'http://localhost:7990/bitbucket/scm/project_1/rep_1.git/': The requested URL returned error: 401

This appears to be a side effect of 1b0d9545bb8, if I revert that commit the old
behavior returns. Certainly we can update our pattern matching to match the new message,
but I wonder if the old message is preferred for folks who are not familiar with HTTP
status codes.

Best Regards,
Ben Humphreys

Re: Git 2.23.0-rc0 HTTP authentication failure - error message change

[Jeff King]

On Tue, May 18, 2021 at 01:07:36PM +1000, Ben Humphreys wrote:

> I've run 2.32.0-rc0 through the Bitbucket Server test matrix and it picked up a
> change in error message that perhaps is unintentional.
> 
> On 2.31.1:
>   $ git ls-remote http://notauser@localhost:7990/bitbucket/scm/project_1/rep_1.git
>   Password for 'http://notauser@localhost:7990':
>   fatal: Authentication failed for 'http://localhost:7990/bitbucket/scm/project_1/rep_1.git/'
> 
> and on 2.32.0-rc0:
>   $ git ls-remote http://notauser@localhost:7990/bitbucket/scm/project_1/rep_1.git
>   Password for 'http://notauser@localhost:7990':
>   fatal: unable to access 'http://localhost:7990/bitbucket/scm/project_1/rep_1.git/': The requested URL returned error: 401
> 
> This appears to be a side effect of 1b0d9545bb8, if I revert that commit the old
> behavior returns. Certainly we can update our pattern matching to match the new message,
> but I wonder if the old message is preferred for folks who are not familiar with HTTP
> status codes.

Thanks for reporting; I've added the author of that commit to the cc.

I think this is definitely a regression in the output we're giving, but
it also points to a bug in the behavior (we fail to forget about bad
credentials; see below).

The motivation from the commit is:

    When the username and password are supplied in a url like this
    https://myuser:secret@git.exampe/myrepo.git and the server supports the
    negotiate authenticaten method, git does not fall back to basic auth and
    libcurl hardly tries to authenticate with the negotiate method.

    Stop using the Negotiate authentication method after the first failure
    because if it fails on the first try it will never succeed.

So it is focused on the case when the credentials came in the URL,
before the first contact with the server (where we'd get an HTTP 401).
And the diff moves the negotiate check earlier in the function, before
we see if we already have credentials:

diff --git a/http.c b/http.c
index 0e31fc21bc..19c203d0ca 100644
--- a/http.c
+++ b/http.c
@@ -1641,17 +1641,18 @@ static int handle_curl_result(struct slot_results *results)
 	} else if (missing_target(results))
 		return HTTP_MISSING_TARGET;
 	else if (results->http_code == 401) {
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+		http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
+		if (results->auth_avail) {
+			http_auth_methods &= results->auth_avail;
+			http_auth_methods_restricted = 1;
+			return HTTP_REAUTH;
+		}
+#endif
 		if (http_auth.username && http_auth.password) {
 			credential_reject(&http_auth);
 			return HTTP_NOAUTH;
 		} else {
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
-			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
-			if (results->auth_avail) {
-				http_auth_methods &= results->auth_avail;
-				http_auth_methods_restricted = 1;
-			}
-#endif
 			return HTTP_REAUTH;
 		}
 	} else {

So in that case, we'd clear the GSSNEGOTIATE bit and return HTTP_REAUTH,
and the caller will try again. Makes sense for the use case described.

But imagine we didn't get a username/password in the URL. The first
request will return REAUTH because of this moved code path (just as it
would have before, because http.auth.{username,password} are not set).
And then we'll get a credential from the user or from a helper and try
again. But this time, if we fail, we'll return HTTP_REAUTH again! We
never hit the "if (http_auth.username && http_auth.password)" check at
all. And hence we never return HTTP_NOAUTH (which gives us the more
useful "authentication failed" message), nor the credential_reject()
line (which informs helpers to stop caching a known-bad password).

We can see it like this. First, seed the cache with a bad password (in
this case a bogus token to elicit a 401 response from GitHub). In real
life, this would be a password stored from a previous successful
attempt, but which was invalidated in the meantime. Note the empty value
for credential.helper clears the list of any regular helpers you'd use
for github.com.

  echo url=https://peff:ghp_000000000000000000000000000000000000@github.com |
  git -c credential.helper= \
      -c credential.helper=cache \
      credential approve

Now make a request that requires auth; we expect it to fail since our
credential is bad.

  git -c credential.helper= \
        -c credential.helper=cache \
        ls-remote https://github.com/peff/foo

which yields:

  remote: Invalid username or password.
  fatal: unable to access 'https://github.com/peff/foo/': The requested URL returned error: 401

as expected (except for the error message). But we also expect it to
clear the bogus credential from the helper, so that if we run ls-remote
again, it will prompt us. But it doesn't! With git v2.32.0-rc0, you can
run that ls-remote as many times as you want, and it will always fail.

Whereas if you switch to 1b0d9545bb8^, on the second run it will
correctly prompt you for an updated password.

I think for this to work, we would need to figure out from libcurl's
response that GSSNEGOTIATE was the problem for our particular request,
and only trigger the bit-clearing and HTTP_REAUTH if that was true. I'm
not sure if that's possible, though.

I suspect we could hack around it by pessimistically guessing that
GSSNEGOTIATE was the problem. But I'm worried that making that work
would require up to three requests (one to find out we need auth, one to
remove the GSSNEGOTIATE bit, and one to retry with a username/password).
That seems like punishing people with servers that don't even care about
Negotiate for no reason.

So perhaps somebody can come up with something clever, but I suspect we
may need to just revert this for the v2.32 release, and re-break the
case that 1b0d9545bb8 was trying to solve.

We probably should beef up the tests around http's credential-rejection,
too, to catch this regression.

-Peff

[PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH

[Jeff King]

On Tue, May 18, 2021 at 01:50:27AM -0400, Jeff King wrote:

> So perhaps somebody can come up with something clever, but I suspect we
> may need to just revert this for the v2.32 release, and re-break the
> case that 1b0d9545bb8 was trying to solve.
> 
> We probably should beef up the tests around http's credential-rejection,
> too, to catch this regression.

Here are some patches. The first one adds the tests, and I think is an
obvious improvement.

The second one does the revert. I'd be quite happy if somebody wants to
figure out how to fix it in a way that addresses the original problem,
and then we can replace that. But in the meantime, I think it makes
sense to prepare the revert, as we wouldn't want to release v2.32.0 with
the bug.

  [1/2]: t5551: test http interaction with credential helpers
  [2/2]: Revert "remote-curl: fall back to basic auth if Negotiate fails"

 http.c                      | 15 +++++++-------
 t/t5551-http-fetch-smart.sh | 41 +++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 8 deletions(-)

-Peff

[PATCH 1/2] t5551: test http interaction with credential helpers

[Jeff King]

We test authentication with http, and we independently test that
credential helpers work, but we don't have any tests that cover the
two features working together. Let's add two:

  1. Make sure that a successful request asks the helper to save the
     credential. This works as expected.

  2. Make sure that a failed request asks the helper to forget the
     credential. This is marked as expect_failure, as it was recently
     regressed by 1b0d9545bb (remote-curl: fall back to basic auth if
     Negotiate fails, 2021-03-22). The symptom here is that the second
     request should prompt the user, but doesn't.

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

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 984dba22af..1de87e4ffe 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -517,4 +517,45 @@ test_expect_success 'server-side error detected' '
 	test_i18ngrep "server-side error" actual
 '
 
+test_expect_success 'http auth remembers successful credentials' '
+	rm -f .git-credentials &&
+	test_config credential.helper store &&
+
+	# the first request prompts the user...
+	set_askpass user@host pass@host &&
+	git ls-remote "$HTTPD_URL/auth/smart/repo.git" >/dev/null &&
+	expect_askpass both user@host &&
+
+	# ...and the second one uses the stored value rather than
+	# prompting the user.
+	set_askpass bogus-user bogus-pass &&
+	git ls-remote "$HTTPD_URL/auth/smart/repo.git" >/dev/null &&
+	expect_askpass none
+'
+
+test_expect_failure 'http auth forgets bogus credentials' '
+	# seed credential store with bogus values. In real life,
+	# this would probably come from a password which worked
+	# for a previous request.
+	rm -f .git-credentials &&
+	test_config credential.helper store &&
+	{
+		echo "url=$HTTPD_URL" &&
+		echo "username=bogus" &&
+		echo "password=bogus"
+	} | git credential approve &&
+
+	# we expect this to use the bogus values and fail, never even
+	# prompting the user...
+	set_askpass user@host pass@host &&
+	test_must_fail git ls-remote "$HTTPD_URL/auth/smart/repo.git" >/dev/null &&
+	expect_askpass none &&
+
+	# ...but now we should have forgotten the bad value, causing
+	# us to prompt the user again.
+	set_askpass user@host pass@host &&
+	git ls-remote "$HTTPD_URL/auth/smart/repo.git" >/dev/null &&
+	expect_askpass both user@host
+'
+
 test_done
-- 
2.32.0.rc0.421.g64c9147932

[PATCH 2/2] Revert "remote-curl: fall back to basic auth if Negotiate fails"

[Jeff King]

This reverts commit 1b0d9545bb85912a16b367229d414f55d140d3be.

That commit does fix the situation it intended to (avoiding Negotiate
even when the credentials were provided in the URL), but it creates a
more serious regression: we now never hit the conditional for "we had a
username and password, tried them, but the server still gave us a 401".
That has two bad effects:

 1. we never call credential_reject(), and thus a bogus credential
    stored by a helper will live on forever

 2. we never return HTTP_NOAUTH, so the error message the user gets is
    "The requested URL returned error: 401", instead of "Authentication
    failed".

Doing this correctly seems non-trivial, as we don't know whether the
Negotiate auth was a problem. Since this is a regression in the upcoming
v2.23.0 release (for which we're in -rc0), let's revert for now and work
on a fix separately.

(Note that this isn't a pure revert; the previous commit added a test
showing the regression, so we can now flip it to expect_success).

Reported-by: Ben Humphreys <behumphreys@atlassian.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 http.c                      | 15 +++++++--------
 t/t5551-http-fetch-smart.sh |  2 +-
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/http.c b/http.c
index c83bc33a5f..8119247149 100644
--- a/http.c
+++ b/http.c
@@ -1650,18 +1650,17 @@ static int handle_curl_result(struct slot_results *results)
 	} else if (missing_target(results))
 		return HTTP_MISSING_TARGET;
 	else if (results->http_code == 401) {
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
-		http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
-		if (results->auth_avail) {
-			http_auth_methods &= results->auth_avail;
-			http_auth_methods_restricted = 1;
-			return HTTP_REAUTH;
-		}
-#endif
 		if (http_auth.username && http_auth.password) {
 			credential_reject(&http_auth);
 			return HTTP_NOAUTH;
 		} else {
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
+			if (results->auth_avail) {
+				http_auth_methods &= results->auth_avail;
+				http_auth_methods_restricted = 1;
+			}
+#endif
 			return HTTP_REAUTH;
 		}
 	} else {
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 1de87e4ffe..4f87d90c5b 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -533,7 +533,7 @@ test_expect_success 'http auth remembers successful credentials' '
 	expect_askpass none
 '
 
-test_expect_failure 'http auth forgets bogus credentials' '
+test_expect_success 'http auth forgets bogus credentials' '
 	# seed credential store with bogus values. In real life,
 	# this would probably come from a password which worked
 	# for a previous request.
-- 
2.32.0.rc0.421.g64c9147932

Re: Git 2.23.0-rc0 HTTP authentication failure - error message change

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 4775 bytes --]

On 2021-05-18 at 05:50:26, Jeff King wrote:
> So it is focused on the case when the credentials came in the URL,
> before the first contact with the server (where we'd get an HTTP 401).
> And the diff moves the negotiate check earlier in the function, before
> we see if we already have credentials:
> 
> diff --git a/http.c b/http.c
> index 0e31fc21bc..19c203d0ca 100644
> --- a/http.c
> +++ b/http.c
> @@ -1641,17 +1641,18 @@ static int handle_curl_result(struct slot_results *results)
>  	} else if (missing_target(results))
>  		return HTTP_MISSING_TARGET;
>  	else if (results->http_code == 401) {
> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> +		http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
> +		if (results->auth_avail) {
> +			http_auth_methods &= results->auth_avail;
> +			http_auth_methods_restricted = 1;
> +			return HTTP_REAUTH;
> +		}
> +#endif
>  		if (http_auth.username && http_auth.password) {
>  			credential_reject(&http_auth);
>  			return HTTP_NOAUTH;
>  		} else {
> -#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> -			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
> -			if (results->auth_avail) {
> -				http_auth_methods &= results->auth_avail;
> -				http_auth_methods_restricted = 1;
> -			}
> -#endif
>  			return HTTP_REAUTH;
>  		}
>  	} else {
> 
> So in that case, we'd clear the GSSNEGOTIATE bit and return HTTP_REAUTH,
> and the caller will try again. Makes sense for the use case described.
> 
> But imagine we didn't get a username/password in the URL. The first
> request will return REAUTH because of this moved code path (just as it
> would have before, because http.auth.{username,password} are not set).
> And then we'll get a credential from the user or from a helper and try
> again. But this time, if we fail, we'll return HTTP_REAUTH again! We
> never hit the "if (http_auth.username && http_auth.password)" check at
> all. And hence we never return HTTP_NOAUTH (which gives us the more
> useful "authentication failed" message), nor the credential_reject()
> line (which informs helpers to stop caching a known-bad password).

I think what we'd want to do in this case is to only call HTTP_REAUTH if
we actually cleared CURLAUTH_GSSNEGOTIATE.  Maybe something like this:

diff --git a/http.c b/http.c
index c83bc33a5f..e9fead8cd8 100644
--- a/http.c
+++ b/http.c
@@ -1650,18 +1650,18 @@ static int handle_curl_result(struct slot_results *results)
 	} else if (missing_target(results))
 		return HTTP_MISSING_TARGET;
 	else if (results->http_code == 401) {
+		int used_negotiate = 0;
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+		if (http_auth_methods & CURLAUTH_GSSNEGOTIATE)
+			used_negotiate = 1;
 		http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
-		if (results->auth_avail) {
-			http_auth_methods &= results->auth_avail;
-			http_auth_methods_restricted = 1;
-			return HTTP_REAUTH;
-		}
 #endif
-		if (http_auth.username && http_auth.password) {
+		if (!used_negotiate && http_auth.username && http_auth.password) {
 			credential_reject(&http_auth);
 			return HTTP_NOAUTH;
 		} else {
+			http_auth_methods &= results->auth_avail;
+			http_auth_methods_restricted = 1;
 			return HTTP_REAUTH;
 		}
 	} else {

That, of course, is totally untested, and I don't have Basic auth
fallback set up on my server with Kerberos, so I can't test it.

> I suspect we could hack around it by pessimistically guessing that
> GSSNEGOTIATE was the problem. But I'm worried that making that work
> would require up to three requests (one to find out we need auth, one to
> remove the GSSNEGOTIATE bit, and one to retry with a username/password).
> That seems like punishing people with servers that don't even care about
> Negotiate for no reason.

I think my proposal above does that, but I'm not sure.  If Negotiate
wasn't set, we won't need to make a third request, since we'll have
known the supported mechanisms as part of the original 401.  If they do
support both, then three requests will be required if they have to fall
back to Basic auth, but then they're only paying the price for the
environment they have.

If we aren't already reading the supported mechanisms out of the initial
401, then we'll need the third request, but that would be silly and we
should just avoid doing that.

> So perhaps somebody can come up with something clever, but I suspect we
> may need to just revert this for the v2.32 release, and re-break the
> case that 1b0d9545bb8 was trying to solve.

Yeah, I think this is the right solution for the problem until somebody
with a suitable mixed auth environment shows up and can test.  Your
patches seemed reasonable and, as always, well explained.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 0/2] fix v2.32.0-rc0 bug with Negotiate / HTTP_NOAUTH

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here are some patches. The first one adds the tests, and I think is an
> obvious improvement.
>
> The second one does the revert. I'd be quite happy if somebody wants to
> figure out how to fix it in a way that addresses the original problem,
> and then we can replace that. But in the meantime, I think it makes
> sense to prepare the revert, as we wouldn't want to release v2.32.0 with
> the bug.

I'll queue these directly on top of the original topic, so that even
a merge of the branch by mistake down to maint won't break.

I saw brian has already floated an idea to solve the original issue,
and I'd encourage people to help finding a proper solution, but
let's leave the actual application of such a change for the next
cycle.

Thanks.

Re: Git 2.23.0-rc0 HTTP authentication failure - error message change

[Jeff King]

On Wed, May 19, 2021 at 12:12:52AM +0000, brian m. carlson wrote:

> > But imagine we didn't get a username/password in the URL. The first
> > request will return REAUTH because of this moved code path (just as it
> > would have before, because http.auth.{username,password} are not set).
> > And then we'll get a credential from the user or from a helper and try
> > again. But this time, if we fail, we'll return HTTP_REAUTH again! We
> > never hit the "if (http_auth.username && http_auth.password)" check at
> > all. And hence we never return HTTP_NOAUTH (which gives us the more
> > useful "authentication failed" message), nor the credential_reject()
> > line (which informs helpers to stop caching a known-bad password).
> 
> I think what we'd want to do in this case is to only call HTTP_REAUTH if
> we actually cleared CURLAUTH_GSSNEGOTIATE.  Maybe something like this:
> [...]

Yeah, that was my instinct, too, but...

> > I suspect we could hack around it by pessimistically guessing that
> > GSSNEGOTIATE was the problem. But I'm worried that making that work
> > would require up to three requests (one to find out we need auth, one to
> > remove the GSSNEGOTIATE bit, and one to retry with a username/password).
> > That seems like punishing people with servers that don't even care about
> > Negotiate for no reason.
> 
> I think my proposal above does that, but I'm not sure.  If Negotiate
> wasn't set, we won't need to make a third request, since we'll have
> known the supported mechanisms as part of the original 401.  If they do
> support both, then three requests will be required if they have to fall
> back to Basic auth, but then they're only paying the price for the
> environment they have.
> 
> If we aren't already reading the supported mechanisms out of the initial
> 401, then we'll need the third request, but that would be silly and we
> should just avoid doing that.

Yeah, I was worried that just clearing the bit results in the extra
round-trip. I think we do clear bits based on what the other side showed
us. That's the:

  http_auth_methods &= results->auth_avail;

in the code being discussed. But it seems like we'd want to do that as
part of setting the "used negotiate" flag in your sample patch. I.e.,:

  if (http_auth_methods & results->auth_avail & CURLAUTH_GSSNEGOTIATE)
          used_negotiate = 1;

But it's entirely possible I don't understand the subtleties around
unsetting GSSNEGOTIATE in the first place (it's not something I've ever
used myself).

> > So perhaps somebody can come up with something clever, but I suspect we
> > may need to just revert this for the v2.32 release, and re-break the
> > case that 1b0d9545bb8 was trying to solve.
> 
> Yeah, I think this is the right solution for the problem until somebody
> with a suitable mixed auth environment shows up and can test.  Your
> patches seemed reasonable and, as always, well explained.

Thanks for taking a look!

-Peff

Re: [PATCH 2/2] Revert "remote-curl: fall back to basic auth if Negotiate fails"

[Derrick Stolee]

On 5/18/2021 2:27 AM, Jeff King wrote:
> This reverts commit 1b0d9545bb85912a16b367229d414f55d140d3be.
> 
> That commit does fix the situation it intended to (avoiding Negotiate
> even when the credentials were provided in the URL), but it creates a
> more serious regression: we now never hit the conditional for "we had a
> username and password, tried them, but the server still gave us a 401".
> That has two bad effects:
> 
>  1. we never call credential_reject(), and thus a bogus credential
>     stored by a helper will live on forever
> 
>  2. we never return HTTP_NOAUTH, so the error message the user gets is
>     "The requested URL returned error: 401", instead of "Authentication
>     failed".
> 
> Doing this correctly seems non-trivial, as we don't know whether the
> Negotiate auth was a problem. Since this is a regression in the upcoming
> v2.23.0 release (for which we're in -rc0), let's revert for now and work
> on a fix separately.

I think the revert is sufficiently justified here.
 
> (Note that this isn't a pure revert; the previous commit added a test
> showing the regression, so we can now flip it to expect_success).

Keeping the test is excellent, because it gives us a way to confirm
that a second attempt at a fix is at least as good as the first.

The only thing that could improve this situation is to add a test
that checks the bug that the previous version introduced, so that
the next round doesn't repeat the mistake. That can be deferred
because it is more important that we get this fix in time for the
next release candidate.

Thanks,
-Stolee

Re: [PATCH 2/2] Revert "remote-curl: fall back to basic auth if Negotiate fails"

[Jeff King]

On Wed, May 19, 2021 at 09:58:50AM -0400, Derrick Stolee wrote:

> > (Note that this isn't a pure revert; the previous commit added a test
> > showing the regression, so we can now flip it to expect_success).
> 
> Keeping the test is excellent, because it gives us a way to confirm
> that a second attempt at a fix is at least as good as the first.
> 
> The only thing that could improve this situation is to add a test
> that checks the bug that the previous version introduced, so that
> the next round doesn't repeat the mistake. That can be deferred
> because it is more important that we get this fix in time for the
> next release candidate.

Re-reading what I wrote, I think "the previous commit" may be ambiguous.
The original commit which introduced the bug (and which we're reverting
here) didn't include a test at all. In patch 1/2 of this series (what
I'm calling "the previous commit"), I provided a test which shows the
regression. And now this revert shows that we fixed it (by flipping from
expect_failure to expect_success).

So I think I've already done what you're asking (if I understand it
correctly).

It probably would be a little easier to follow by reverting first, and
then adding in the expect_success test on top to future-proof us. But by
doing it in the other order, it was easy to see the test demonstrate the
behavior before and after the revert.

-Peff

Re: [PATCH 2/2] Revert "remote-curl: fall back to basic auth if Negotiate fails"

[Derrick Stolee]

On 5/19/2021 10:14 AM, Jeff King wrote:
> On Wed, May 19, 2021 at 09:58:50AM -0400, Derrick Stolee wrote:
> 
>>> (Note that this isn't a pure revert; the previous commit added a test
>>> showing the regression, so we can now flip it to expect_success).
>>
>> Keeping the test is excellent, because it gives us a way to confirm
>> that a second attempt at a fix is at least as good as the first.
>>
>> The only thing that could improve this situation is to add a test
>> that checks the bug that the previous version introduced, so that
>> the next round doesn't repeat the mistake. That can be deferred
>> because it is more important that we get this fix in time for the
>> next release candidate.
> 
> Re-reading what I wrote, I think "the previous commit" may be ambiguous.
> The original commit which introduced the bug (and which we're reverting
> here) didn't include a test at all. In patch 1/2 of this series (what
> I'm calling "the previous commit"), I provided a test which shows the
> regression. And now this revert shows that we fixed it (by flipping from
> expect_failure to expect_success).
> 
> So I think I've already done what you're asking (if I understand it
> correctly).

Ah. For some reason my email client didn't thread your messages
together, so I saw this as a one-off patch (ignoring the 2/2 part
of the message, of course).

Thanks,
-Stolee