Re: [PATCH] http: Support sending custom HTTP headers
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Apr 26, 2016, 15:37 UTC
- Message-ID
- <alpine.DEB.2.20.1604261733410.2896@virtualbox>
- In-Reply-To
- <20160425170326.GA1551@sigill.intra.peff.net>
Hi Peff,
On Mon, 25 Apr 2016, Jeff King wrote:
Show 26 quoted lines
> On Mon, Apr 25, 2016 at 03:13:08PM +0200, Johannes Schindelin wrote:
>
> > diff --git a/http.c b/http.c
> > index 4304b80..02d7147 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;
> >
> > static struct curl_slist *pragma_header;
> > static struct curl_slist *no_pragma_header;
> > +static struct curl_slist *extra_http_headers;
> >
> > static struct active_request_slot *active_queue_head;
> >
> > @@ -323,6 +324,12 @@ static int http_options(const char *var, const char *value, void *cb)
> > #endif
> > }
> >
> > + if (!strcmp("http.extraheader", var)) {
> > + extra_http_headers =
> > + curl_slist_append(extra_http_headers, value);
> > + return 0;
> > + }
>
> I wondered if this would trigger for "http.*.extraheader", too. And it
> should, as that is all handled in the caller of http_options. Good.Yes, I was surprised about that, too, but all the other http.* settings are handled via the urlmatch mechanism (which rewrites the matching http.<URL>.* settings).
Show 20 quoted lines
> > @@ -678,8 +685,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth) > > if (remote) > > var_override(&http_proxy_authmethod, remote->http_proxy_authmethod); > > > > - pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache"); > > - no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:"); > > + pragma_header = curl_slist_append(http_get_default_headers(), > > + "Pragma: no-cache"); > > + no_pragma_header = curl_slist_append(http_get_default_headers(), > > + "Pragma:"); > > This looked wrong to me at first, because we are appending to the > default header list in each case. But the secret sauce is that calling > http_get_default_headers() actually creates a _new_ list that is a copy > of the default headers (and the caller can do what they will with it, > and must free it). > > I think that's really the only sane way to do it because of curl's > interfaces. But maybe it is worth a comment either here, or along with > http_get_default_headers(), or both.
I chose to rename it to http_copy_default_headers(); That should make it easier to understand.
Ciao, Dscho