Re: [PATCH v2] http: support sending custom HTTP headers

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> We introduce a way to send custom HTTP headers with all requests.
>
> This allows us, for example, to send an extra token from build agents
> for temporary access to private repositories. (This is the use case that
> triggered this patch.)
>
> This feature can be used like this:
>
> 	git -c http.extraheader='Secret: sssh!' fetch $URL $REF
>
> As `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` overrides previous
> calls' headers (instead of appending the headers, as this unsuspecting
> developer thought initially), we piggyback onto the `Pragma:` setting by
> default, and introduce the global helper `http_copy_default_headers()`
> to help functions that want to specify HTTP headers themselves.

My reading stuttered at "we piggyback onto the `Pragma:` setting by
default", which made me stop and wonder if a description about a
knob that changes this behaviour and makes us piggyback onto
something else follows.

I guess "by default" you meant that the majority of codepaths set
the headers using no_pragma_header or pragma_header variables, and
by [no_]pragma_header to mean more than just about "Pragma:" by
adding the extra headers to them, you did not have to touch them.
Other codepaths that do not use these two variables but start from
NULL you made them start from these extra headers.

Which makes sense.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 42d2b50..37b9af7 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1655,6 +1655,12 @@ http.emptyAuth::
>  	a username in the URL, as libcurl normally requires a username for
>  	authentication.
>  
> +http.extraHeader::
> +	Pass an additional HTTP header when communicating with a server.  If
> +	more than one such entry exists, all of them are added as extra headers.
> +	This feature is useful e.g. to increase security, or to allow
> +	time-limited access based on expiring tokens.
> +

I think one-time/short-lived use case does not want to have this in
a configuration file, and instead want to do the command line thing
you illustrated in the proposed log message.  I however wonder if
there are other use cases where having this in $GIT_DIR/config for
repeated use is useful.  If there is, not being able to override a
configured value per invocation would become a problem.

Peff, what do you think?  I vaguely recollect that you did a hack to
one variable that declares "an empty value means discard accumulated
values so far" or something like that, and this variable deserves a
mechanism like that, too.

> diff --git a/http.c b/http.c
> index 4304b80..3d662bb 100644
> --- a/http.c
> +++ b/http.c
> @@ -1163,6 +1175,16 @@ int run_one_slot(struct active_request_slot *slot,
>  	return handle_curl_result(results);
>  }
>  
> +struct curl_slist *http_copy_default_headers()

struct curl_slist *http_copy_default_headers(void)

> +{
> +	struct curl_slist *headers = NULL, *h;
> +
> +	for (h = extra_http_headers; h; h = h->next)
> +		headers = curl_slist_append(headers, h->data);
> +
> +	return headers;
> +}
> +
> diff --git a/http.h b/http.h
> index 4ef4bbd..5f13695 100644
> --- a/http.h
> +++ b/http.h
> @@ -106,6 +106,7 @@ extern void step_active_slots(void);
>  extern void http_init(struct remote *remote, const char *url,
>  		      int proactive_auth);
>  extern void http_cleanup(void);
> +extern struct curl_slist *http_copy_default_headers();

extern struct curl_slist *http_copy_default_headers(void);