git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Michael J Gruber <git@drmicha.warpmail.net>
Cc: git@vger.kernel.org, "Shawn O. Pearce" <spearce@spearce.org>,
	Tay Ray Chuan <rctay89@gmail.com>
Subject: Re: [RFC/PATCH] remote-curl: Obey passed URL
Date: Thu, 6 Oct 2011 09:25:00 -0400	[thread overview]
Message-ID: <20111006132500.GA1792@sigill.intra.peff.net> (raw)
In-Reply-To: <2f1eccfa3fa9e732e9bea344fd69dfd9b16697a9.1317906388.git.git@drmicha.warpmail.net>

On Thu, Oct 06, 2011 at 03:15:59PM +0200, Michael J Gruber wrote:

> When the curl remote helper is called, e.g., as
> 
> git-remote-https foo https://john@doe.com
> 
> it looks up remote.foo.url rather than taking the provided url, at least
> as far as http_init() is concerned (authentication). (It does use the
> provided url at later stages.)
> [...]
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -846,6 +846,7 @@ int main(int argc, const char **argv)
>  
>  	if (argc > 2) {
>  		end_url_with_slash(&buf, argv[2]);
> +		remote->url[0] = xstrdup(argv[2]);
>  	} else {
>  		end_url_with_slash(&buf, remote->url[0]);
>  	}

Your analysis is correct, but tweaking the remote object seems kind
of ugly. I think a nicer solution would be to pass the URL in
separately to http_init. Of the three existing callers:

  1. http-fetch.c just passes a NULL remote. Which means we don't even
     look at the URL at all for grabbing the auth information. Passing
     the URL would be an improvement.

  2. http-push.c creates a fake remote just to stick the URL in. That
     ugly code could just go away.

  3. remote-curl.c needs to pass in the alternate URL anyway, as you
     described.

So it seems like all callsites would benefit.

That being said, I wonder if http_init is the right place to pull the
auth information out. It's where we've always done it, and it makes
sense if you are hitting one base URL. But what happens if we get a
redirect to some other site? Shouldn't we be deciding at the time of
making the request what the context of the http request is?

And then http_init can stop caring entirely what the URL is.

-Peff

  reply	other threads:[~2011-10-06 13:25 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-10-04 10:19 Git ksshaskpass to play nice with https and kwallet Michael J Gruber
2011-10-04 10:50 ` Jeff King
2011-10-04 11:27   ` Michael J Gruber
2011-10-04 11:37     ` Jeff King
2011-10-04 12:12       ` Michael J Gruber
2011-10-04 12:43         ` Jeff King
2011-10-04 18:49           ` Michael J Gruber
2011-10-05 17:55             ` Jeff King
2011-10-05 18:01               ` Jeff King
2011-10-06  6:33                 ` Michael J Gruber
2011-10-06 13:15                   ` [RFC/PATCH] remote-curl: Obey passed URL Michael J Gruber
2011-10-06 13:25                     ` Jeff King [this message]
2011-10-06 13:37                       ` Jeff King
2011-10-12 20:51                         ` Michael J Gruber
2011-10-12 21:43                           ` [PATCH] http_init: accept separate URL parameter Jeff King
2011-10-12 21:46                             ` Jeff King
2011-10-12 22:38                               ` Junio C Hamano
2011-10-12 22:46                                 ` Jeff King
2011-10-13  7:26                                   ` Michael J Gruber
2011-10-14  7:40                                     ` [PATCH 0/6] http-auth-early Michael J Gruber
2011-10-14  7:40                                       ` [PATCH 1/6] url: decode buffers that are not NUL-terminated Michael J Gruber
2011-10-14  7:40                                       ` [PATCH 2/6] improve httpd auth tests Michael J Gruber
2011-10-14  7:40                                       ` [PATCH 3/6] remote-curl: don't retry auth failures with dumb protocol Michael J Gruber
2011-10-14  7:40                                       ` [PATCH 4/6] http: retry authentication failures for all http requests Michael J Gruber
2011-10-14  7:40                                       ` [PATCH 5/6] http: use hostname in credential description Michael J Gruber
2011-10-14  7:40                                       ` [PATCH 6/6] http_init: accept separate URL parameter Michael J Gruber
2011-10-14 13:19                                       ` [PATCH 0/6] http-auth-early Jeff King
2011-10-14 13:24                                         ` Michael J Gruber
2011-10-14 18:59                                         ` Junio C Hamano
2011-10-13  2:06                             ` [PATCH] http_init: accept separate URL parameter Tay Ray Chuan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20111006132500.GA1792@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@drmicha.warpmail.net \
    --cc=git@vger.kernel.org \
    --cc=rctay89@gmail.com \
    --cc=spearce@spearce.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).