Scott Chacon <schacon@gmail.com> writes:

quoted text
> When an HTTP request returns a 401, Git will currently fail with a
> confusing message saying that it got a 401.  This changes
> http_request to prompt for the username and password, then return
> HTTP_REAUTH so http_get_strbuf can try again.  If it gets a 401 even
> when a user/pass is supplied, http_request will now return HTTP_NOAUTH
> which remote_curl can then use to display a more intelligent error
> message that is less confusing.

I'd like another sentence after "that it got a 401.", explaining why it is
sometimes OK for us to get 401 and continue, in order to justify that it
is a good idea to retry after asking for authentication credentials to the
end user when it happens.  I am guessing that it might be something like
this:

    The repository owner might have given out an HTTP URL as if it were a
    public resource (e.g. "http://github.com/myrepo.git/"), and the end
    user may find out that the URL is not valid and he needs to supply a
    username (e.g. "http://me@github.com/myrepo.git") in the URL to
    trigger authentication.  Retrying by asking for username and password
    would help users in such a case.

I said "something like this" because I do not think what I wrote above is
the whole story.  A natural question it begs is "why didn't the repository
owner give the right URL to begin with?"

Also, earlier I said "sometimes OK", because I don't know if it always OK
for us to get 401 and continue.  If the end user got a 401 and then does
not have a good username or password (e.g. he realizes that the URL he
accessed was incorrect), he used to see "you are not allowed to access
this repository" with a clean failure, but now he would have to get out of
"who are you?" interaction (and how would he do that?).  Would that be a
problem?

If that is not a problem, then the patch looks like a good solution to the
problem, and an obvious enhancement that may want to happen would be to
add a boolean parameter to git_getpass() in order to control if we want to
hide what the user types, as we would probably prefer the Username to be
echoed.  But that is an independent issue to be addressed as a separate
follow-up patch.

quoted text
> Signed-off-by: Scott Chacon <schacon@gmail.com>
> ---
>
> Here is the fourth version of this patch - now incorporating the
> GIT_ASKPASS stuff.
>
>  http.c        |   20 ++++++++++++++++++--
>  http.h        |    2 ++
>  remote-curl.c |    2 ++
>  3 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/http.c b/http.c
> index 4814217..6027546 100644
> --- a/http.c
> +++ b/http.c
> @@ -815,7 +815,19 @@ static int http_request(const char *url, void
> *result, int target, int options)
>  			ret = HTTP_OK;
>  		else if (missing_target(&results))
>  			ret = HTTP_MISSING_TARGET;
> -		else
> +		else if (results.http_code == 401) {
> +			if (user_name) {
> +				ret = HTTP_NOAUTH;
> +			} else {
> +				// git_getpass is needed here because its very likely stdin/stdout are
> +				// pipes to our parent process.  So we instead need to use /dev/tty,
> +				// but that is non-portable.  Using git_getpass() can at least be stubbed
> +				// on other platforms with a different implementation if/when necessary.
> +				user_name = xstrdup(git_getpass("Username: "));

No C99/C++ "//" comments.

	/*
         * We format multi-line
         * comments like
         * this.
         */

Thanks.  Tentatively I'll queue this version _without_ any touch-up to
'pu'.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html