[PATCH] http: Support sending custom HTTP headers

[PATCH] http: Support sending custom HTTP headers

[Johannes Schindelin]

To make communication for `git fetch`, `git ls-remote` and friends extra
secure, we introduce a way to send custom HTTP headers with all
requests.

This allows us, for example, to send an extra token that the server
tests for. The server could use this token e.g. to ensure that only
certain operations or refs are allowed, or allow the token to be used
only once.

This feature can be used like this:

	git -c http.extraheader='Secret: sssh!' fetch $URL $REF

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v1
---
 http-push.c   | 10 +++++-----
 http.c        | 28 +++++++++++++++++++++++++---
 http.h        |  1 +
 remote-curl.c |  4 ++--
 4 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/http-push.c b/http-push.c
index bd60668..04eef17 100644
--- a/http-push.c
+++ b/http-push.c
@@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
 static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
 {
 	struct strbuf buf = STRBUF_INIT;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_get_default_headers();
 
 	if (options & DAV_HEADER_IF) {
 		strbuf_addf(&buf, "If: (<%s>)", lock->token);
@@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
 static void start_move(struct transfer_request *request)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_get_default_headers();
 
 	slot = get_active_slot();
 	slot->callback_func = process_response;
@@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	char *ep;
 	char timeout_header[25];
 	struct remote_lock *lock = NULL;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_get_default_headers();
 	struct xml_ctx ctx;
 	char *escaped;
 
@@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_get_default_headers();
 	struct xml_ctx ctx;
 	struct remote_ls_ctx ls;
 
@@ -1204,7 +1204,7 @@ static int locking_available(void)
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_get_default_headers();
 	struct xml_ctx ctx;
 	int lock_flags = 0;
 	char *escaped;
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;
+	}
+
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
 }
@@ -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:");
 
 #ifdef USE_CURL_MULTI
 	{
@@ -765,6 +774,9 @@ void http_cleanup(void)
 #endif
 	curl_global_cleanup();
 
+	curl_slist_free_all(extra_http_headers);
+	extra_http_headers = NULL;
+
 	curl_slist_free_all(pragma_header);
 	pragma_header = NULL;
 
@@ -1163,6 +1175,16 @@ int run_one_slot(struct active_request_slot *slot,
 	return handle_curl_result(results);
 }
 
+struct curl_slist *http_get_default_headers()
+{
+	struct curl_slist *headers = NULL, *h;
+
+	for (h = extra_http_headers; h; h = h->next)
+		headers = curl_slist_append(headers, h->data);
+
+	return headers;
+}
+
 static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
 {
 	char *ptr;
@@ -1380,7 +1402,7 @@ static int http_request(const char *url,
 {
 	struct active_request_slot *slot;
 	struct slot_results results;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_get_default_headers();
 	struct strbuf buf = STRBUF_INIT;
 	const char *accept_language;
 	int ret;
diff --git a/http.h b/http.h
index 4ef4bbd..b0927de 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_get_default_headers();
 
 extern long int git_curl_ipresolve;
 extern int active_requests;
diff --git a/remote-curl.c b/remote-curl.c
index 15e48e2..86ba787 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot,
 static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_get_default_headers();
 	struct strbuf buf = STRBUF_INIT;
 	int err;
 
@@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 static int post_rpc(struct rpc_state *rpc)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_get_default_headers();
 	int use_gzip = rpc->gzip_request;
 	char *gzip_body = NULL;
 	size_t gzip_size = 0;
-- 
2.8.1.306.gff998f2

Re: [PATCH] http: Support sending custom HTTP headers

[Shawn Pearce]

On Mon, Apr 25, 2016 at 6:13 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> To make communication for `git fetch`, `git ls-remote` and friends extra
> secure, we introduce a way to send custom HTTP headers with all
> requests.

Hmm. Its not Apr 1 2016. So I guess you are serious. :)

> This allows us, for example, to send an extra token that the server
> tests for. The server could use this token e.g. to ensure that only
> certain operations or refs are allowed, or allow the token to be used
> only once.
>
> This feature can be used like this:
>
>         git -c http.extraheader='Secret: sssh!' fetch $URL $REF

Its not very secure to be adding secure data to the command line, e.g.
on Linux you can see that data in /proc.

Re: [PATCH] http: Support sending custom HTTP headers

[Jeff King]

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.

> @@ -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.

-Peff

Re: [PATCH] http: Support sending custom HTTP headers

[Johannes Schindelin]

Hi Peff,

On Mon, 25 Apr 2016, Jeff King wrote:

> 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).

> > @@ -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

Re: [PATCH] http: Support sending custom HTTP headers

[Jeff King]

On Tue, Apr 26, 2016 at 05:37:32PM +0200, Johannes Schindelin wrote:

> > 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.

Thanks, I think that makes it clearer.

-Peff

Re: [PATCH] http: Support sending custom HTTP headers

[Junio C Hamano]

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

> To make communication for `git fetch`, `git ls-remote` and friends extra
> secure, we introduce a way to send custom HTTP headers with all
> requests.

I think an ability to send custom headers may be a good addition and
have no problem with it, but I tend to agree with Shawn that its log
message that advertises it as if it has anything to do with security
is probably a bad idea in both ways (i.e. it isn't very secure, and
the usefulness of the feature is not limited to security).

> This allows us, for example, to send an extra token that the server
> tests for. The server could use this token e.g. to ensure that only
> certain operations or refs are allowed, or allow the token to be used
> only once.
>
> This feature can be used like this:
>
> 	git -c http.extraheader='Secret: sssh!' fetch $URL $REF
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>


> Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v1

Move this after "---".

> ---

This obviously needs documentation updates and tests, no?

>  http-push.c   | 10 +++++-----
>  http.c        | 28 +++++++++++++++++++++++++---
>  http.h        |  1 +
>  remote-curl.c |  4 ++--
>  4 files changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/http-push.c b/http-push.c
> index bd60668..04eef17 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
>  static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> -	struct curl_slist *dav_headers = NULL;
> +	struct curl_slist *dav_headers = http_get_default_headers();
>  
>  	if (options & DAV_HEADER_IF) {
>  		strbuf_addf(&buf, "If: (<%s>)", lock->token);
> @@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
>  static void start_move(struct transfer_request *request)
>  {
>  	struct active_request_slot *slot;
> -	struct curl_slist *dav_headers = NULL;
> +	struct curl_slist *dav_headers = http_get_default_headers();
>  
>  	slot = get_active_slot();
>  	slot->callback_func = process_response;
> @@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
>  	char *ep;
>  	char timeout_header[25];
>  	struct remote_lock *lock = NULL;
> -	struct curl_slist *dav_headers = NULL;
> +	struct curl_slist *dav_headers = http_get_default_headers();
>  	struct xml_ctx ctx;
>  	char *escaped;
>  
> @@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
>  	struct slot_results results;
>  	struct strbuf in_buffer = STRBUF_INIT;
>  	struct buffer out_buffer = { STRBUF_INIT, 0 };
> -	struct curl_slist *dav_headers = NULL;
> +	struct curl_slist *dav_headers = http_get_default_headers();
>  	struct xml_ctx ctx;
>  	struct remote_ls_ctx ls;
>  
> @@ -1204,7 +1204,7 @@ static int locking_available(void)
>  	struct slot_results results;
>  	struct strbuf in_buffer = STRBUF_INIT;
>  	struct buffer out_buffer = { STRBUF_INIT, 0 };
> -	struct curl_slist *dav_headers = NULL;
> +	struct curl_slist *dav_headers = http_get_default_headers();
>  	struct xml_ctx ctx;
>  	int lock_flags = 0;
>  	char *escaped;
> 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;
> +	}
> +
>  	/* Fall back on the default ones */
>  	return git_default_config(var, value, cb);
>  }
> @@ -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:");
>  
>  #ifdef USE_CURL_MULTI
>  	{
> @@ -765,6 +774,9 @@ void http_cleanup(void)
>  #endif
>  	curl_global_cleanup();
>  
> +	curl_slist_free_all(extra_http_headers);
> +	extra_http_headers = NULL;
> +
>  	curl_slist_free_all(pragma_header);
>  	pragma_header = NULL;
>  
> @@ -1163,6 +1175,16 @@ int run_one_slot(struct active_request_slot *slot,
>  	return handle_curl_result(results);
>  }
>  
> +struct curl_slist *http_get_default_headers()
> +{
> +	struct curl_slist *headers = NULL, *h;
> +
> +	for (h = extra_http_headers; h; h = h->next)
> +		headers = curl_slist_append(headers, h->data);
> +
> +	return headers;
> +}
> +
>  static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
>  {
>  	char *ptr;
> @@ -1380,7 +1402,7 @@ static int http_request(const char *url,
>  {
>  	struct active_request_slot *slot;
>  	struct slot_results results;
> -	struct curl_slist *headers = NULL;
> +	struct curl_slist *headers = http_get_default_headers();
>  	struct strbuf buf = STRBUF_INIT;
>  	const char *accept_language;
>  	int ret;
> diff --git a/http.h b/http.h
> index 4ef4bbd..b0927de 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_get_default_headers();
>  
>  extern long int git_curl_ipresolve;
>  extern int active_requests;
> diff --git a/remote-curl.c b/remote-curl.c
> index 15e48e2..86ba787 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot,
>  static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
>  {
>  	struct active_request_slot *slot;
> -	struct curl_slist *headers = NULL;
> +	struct curl_slist *headers = http_get_default_headers();
>  	struct strbuf buf = STRBUF_INIT;
>  	int err;
>  
> @@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
>  static int post_rpc(struct rpc_state *rpc)
>  {
>  	struct active_request_slot *slot;
> -	struct curl_slist *headers = NULL;
> +	struct curl_slist *headers = http_get_default_headers();
>  	int use_gzip = rpc->gzip_request;
>  	char *gzip_body = NULL;
>  	size_t gzip_size = 0;

Re: [PATCH] http: Support sending custom HTTP headers

[Johannes Schindelin]

Hi Junio,

On Mon, 25 Apr 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > To make communication for `git fetch`, `git ls-remote` and friends
> > extra secure, we introduce a way to send custom HTTP headers with all
> > requests.
> 
> I think an ability to send custom headers may be a good addition and
> have no problem with it, but I tend to agree with Shawn that its log
> message that advertises it as if it has anything to do with security is
> probably a bad idea in both ways (i.e. it isn't very secure, and the
> usefulness of the feature is not limited to security).

You know, it never occurred to me that anybody could even *think* that I
was talking about the security of the client setup.

You see, it is much easier to read $HOME/.netrc than /proc/, especially if
you are looking outside of Linux, where the proc filesystem does not even
exist.  And it is almost as easy to query the credential helper for a
plain text password as looking at $HOME/.netrc.

So I took it for granted that everybody knows that they have to keep their
own computer safe.

Instead, I was thinking of server side security (with the clear
expectation that the users will keep their client setups secure).

I will rephrase the commit message to describe the actual use case I have
here: build agents need temporary access to private repositories, and I'd
like to do that via sort of One-Time-Passwords, sent as additional HTTP
headers (via HTTPS, I should not need to point out, but now feel I have to
spell out).

> > Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v1
> 
> Move this after "---".

Whoops. That is what I intended, but overlooked. Will fix.

> This obviously needs documentation updates and tests, no?

Documentation, yes. I have that already, but somehow it slipped out of the
patch.

Testing the headers? I dunno, do we have tests for that already? I thought
we did not: it requires an HTTP server (so that the headers are actually
sent) that we can force to check the header...

So I see we have some tests that use Apache, and one that uses our own
http-backend. But is there already anything that logs HTTP requests? I did
not think so, please correct me if I am wrong.

Ciao,
Dscho

Re: [PATCH] http: Support sending custom HTTP headers

[Junio C Hamano]

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

> Testing the headers? I dunno, do we have tests for that already? I thought
> we did not: it requires an HTTP server (so that the headers are actually
> sent) that we can force to check the header...
>
> So I see we have some tests that use Apache, and one that uses our own
> http-backend. But is there already anything that logs HTTP requests? I did
> not think so, please correct me if I am wrong.

I suspect that no codepath in the current system has cared about
what http headers are sent; auth stuff might have but even then I
would imagine that a test for auth would observe the end result
(i.e. it would ask "did the server accept or reject us?") not the
mechanism (i.e. it would not ask "did we correctly send an
Authorization header?").

So I wouldn't be surprised if this topic is the first one that cares
exactly what headers are sent out (eh, rather, "we told Git to send
this and that header, do they appear at the server end?"), in which
case it is very likely that we do not have any existing test that
can be imitated for that purpose X-<.

In other words, the answer to "Do we already have a test so that I
can mimick it instead of thinking of a way to test this?" would
probably be "No".

Do we care about this feature deeply enough to devise a mechanism
to prevent it from getting broken by careless others in the future?

Re: [PATCH] http: Support sending custom HTTP headers

[Jeff King]

On Tue, Apr 26, 2016 at 05:33:33PM +0200, Johannes Schindelin wrote:

> Testing the headers? I dunno, do we have tests for that already? I thought
> we did not: it requires an HTTP server (so that the headers are actually
> sent) that we can force to check the header...
> 
> So I see we have some tests that use Apache, and one that uses our own
> http-backend. But is there already anything that logs HTTP requests? I did
> not think so, please correct me if I am wrong.

You can ask apache to check for specific headers. Like this:

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 9317ba0..de5a8fe 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -102,6 +102,12 @@ Alias /auth/dumb/ www/auth/dumb/
 	SetEnv GIT_HTTP_EXPORT_ALL
 	Header set Set-Cookie name=value
 </LocationMatch>
+<LocationMatch /smart_headers/>
+	Require expr %{HTTP:x-magic-one} == 'abra'
+	Require expr %{HTTP:x-magic-two} == 'cadabra'
+	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+	SetEnv GIT_HTTP_EXPORT_ALL
+</LocationMatch>
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 ScriptAlias /error/ error.sh/
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 58207d8..e44fe72 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -282,5 +282,12 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
 	test_line_count = 100000 tags
 '
 
+test_expect_success 'custom http headers' '
+	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
+	git -c http.extraheader="x-magic-one: abra" \
+	    -c http.extraheader="x-magic-two: cadabra" \
+	    fetch "$HTTPD_URL/smart_headers/repo.git"
+'
+
 stop_httpd
 test_done

Re: [PATCH] http: Support sending custom HTTP headers

[Johannes Schindelin]

Hi Peff,

On Tue, 26 Apr 2016, Jeff King wrote:

> On Tue, Apr 26, 2016 at 05:33:33PM +0200, Johannes Schindelin wrote:
> 
> > Testing the headers? I dunno, do we have tests for that already? I thought
> > we did not: it requires an HTTP server (so that the headers are actually
> > sent) that we can force to check the header...
> > 
> > So I see we have some tests that use Apache, and one that uses our own
> > http-backend. But is there already anything that logs HTTP requests? I did
> > not think so, please correct me if I am wrong.
> 
> You can ask apache to check for specific headers. Like this:
> 
> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index 9317ba0..de5a8fe 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -102,6 +102,12 @@ Alias /auth/dumb/ www/auth/dumb/
>  	SetEnv GIT_HTTP_EXPORT_ALL
>  	Header set Set-Cookie name=value
>  </LocationMatch>
> +<LocationMatch /smart_headers/>
> +	Require expr %{HTTP:x-magic-one} == 'abra'
> +	Require expr %{HTTP:x-magic-two} == 'cadabra'
> +	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
> +	SetEnv GIT_HTTP_EXPORT_ALL
> +</LocationMatch>
>  ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
>  ScriptAlias /broken_smart/ broken-smart-http.sh/
>  ScriptAlias /error/ error.sh/
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 58207d8..e44fe72 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -282,5 +282,12 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
>  	test_line_count = 100000 tags
>  '
>  
> +test_expect_success 'custom http headers' '
> +	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
> +	git -c http.extraheader="x-magic-one: abra" \
> +	    -c http.extraheader="x-magic-two: cadabra" \
> +	    fetch "$HTTPD_URL/smart_headers/repo.git"
> +'
> +
>  stop_httpd
>  test_done

That's pretty easy.

After sleeping over the issue, I realized, though, that the test can use
the very same method *I* used to verify that the headers are sent: using
GIT_CURL_VERBOSE.

I hope you do not mind that I used this method instead.

Thanks,
Dscho

Re: [PATCH] http: Support sending custom HTTP headers

[Jeff King]

On Wed, Apr 27, 2016 at 08:31:50AM +0200, Johannes Schindelin wrote:

> > +test_expect_success 'custom http headers' '
> > +	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
> > +	git -c http.extraheader="x-magic-one: abra" \
> > +	    -c http.extraheader="x-magic-two: cadabra" \
> > +	    fetch "$HTTPD_URL/smart_headers/repo.git"
> > +'
> > +
> >  stop_httpd
> >  test_done
> 
> That's pretty easy.
> 
> After sleeping over the issue, I realized, though, that the test can use
> the very same method *I* used to verify that the headers are sent: using
> GIT_CURL_VERBOSE.
> 
> I hope you do not mind that I used this method instead.

TBH, I think mine is a little more robust, but I don't overly care
either way.

-Peff

Re: [PATCH] http: Support sending custom HTTP headers

[Johannes Schindelin]

Hi Peff,

On Wed, 27 Apr 2016, Jeff King wrote:

> On Wed, Apr 27, 2016 at 08:31:50AM +0200, Johannes Schindelin wrote:
> 
> > > +test_expect_success 'custom http headers' '
> > > +	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
> > > +	git -c http.extraheader="x-magic-one: abra" \
> > > +	    -c http.extraheader="x-magic-two: cadabra" \
> > > +	    fetch "$HTTPD_URL/smart_headers/repo.git"
> > > +'
> > > +
> > >  stop_httpd
> > >  test_done
> > 
> > That's pretty easy.
> > 
> > After sleeping over the issue, I realized, though, that the test can use
> > the very same method *I* used to verify that the headers are sent: using
> > GIT_CURL_VERBOSE.
> > 
> > I hope you do not mind that I used this method instead.
> 
> TBH, I think mine is a little more robust, but I don't overly care
> either way.

Yes, you are correct, I only test that the header is sent twice, assuming
that exactly two HTTP requests are sent. Your test verifies that whatever
request is sent *requires* the header.

Will send out v4 with your test instead of mine in a few moments.

Thanks,
Dscho

[PATCH v2] http: support sending custom HTTP headers

[Johannes Schindelin]

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v2

 Documentation/config.txt |  6 ++++++
 http-push.c              | 10 +++++-----
 http.c                   | 28 +++++++++++++++++++++++++---
 http.h                   |  1 +
 remote-curl.c            |  4 ++--
 5 files changed, 39 insertions(+), 10 deletions(-)

Interdiff vs v1:

 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.
 +
  http.cookieFile::
  	File containing previously stored cookie lines which should be used
  	in the Git http session, if they match the server. The file format
 diff --git a/http-push.c b/http-push.c
 index 04eef17..ae2b7f1 100644
 --- a/http-push.c
 +++ b/http-push.c
 @@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
  static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
  {
  	struct strbuf buf = STRBUF_INIT;
 -	struct curl_slist *dav_headers = http_get_default_headers();
 +	struct curl_slist *dav_headers = http_copy_default_headers();
  
  	if (options & DAV_HEADER_IF) {
  		strbuf_addf(&buf, "If: (<%s>)", lock->token);
 @@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
  static void start_move(struct transfer_request *request)
  {
  	struct active_request_slot *slot;
 -	struct curl_slist *dav_headers = http_get_default_headers();
 +	struct curl_slist *dav_headers = http_copy_default_headers();
  
  	slot = get_active_slot();
  	slot->callback_func = process_response;
 @@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
  	char *ep;
  	char timeout_header[25];
  	struct remote_lock *lock = NULL;
 -	struct curl_slist *dav_headers = http_get_default_headers();
 +	struct curl_slist *dav_headers = http_copy_default_headers();
  	struct xml_ctx ctx;
  	char *escaped;
  
 @@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
  	struct slot_results results;
  	struct strbuf in_buffer = STRBUF_INIT;
  	struct buffer out_buffer = { STRBUF_INIT, 0 };
 -	struct curl_slist *dav_headers = http_get_default_headers();
 +	struct curl_slist *dav_headers = http_copy_default_headers();
  	struct xml_ctx ctx;
  	struct remote_ls_ctx ls;
  
 @@ -1204,7 +1204,7 @@ static int locking_available(void)
  	struct slot_results results;
  	struct strbuf in_buffer = STRBUF_INIT;
  	struct buffer out_buffer = { STRBUF_INIT, 0 };
 -	struct curl_slist *dav_headers = http_get_default_headers();
 +	struct curl_slist *dav_headers = http_copy_default_headers();
  	struct xml_ctx ctx;
  	int lock_flags = 0;
  	char *escaped;
 diff --git a/http.c b/http.c
 index 02d7147..3d662bb 100644
 --- a/http.c
 +++ b/http.c
 @@ -685,9 +685,9 @@ 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(http_get_default_headers(),
 +	pragma_header = curl_slist_append(http_copy_default_headers(),
  		"Pragma: no-cache");
 -	no_pragma_header = curl_slist_append(http_get_default_headers(),
 +	no_pragma_header = curl_slist_append(http_copy_default_headers(),
  		"Pragma:");
  
  #ifdef USE_CURL_MULTI
 @@ -1175,7 +1175,7 @@ int run_one_slot(struct active_request_slot *slot,
  	return handle_curl_result(results);
  }
  
 -struct curl_slist *http_get_default_headers()
 +struct curl_slist *http_copy_default_headers()
  {
  	struct curl_slist *headers = NULL, *h;
  
 @@ -1402,7 +1402,7 @@ static int http_request(const char *url,
  {
  	struct active_request_slot *slot;
  	struct slot_results results;
 -	struct curl_slist *headers = http_get_default_headers();
 +	struct curl_slist *headers = http_copy_default_headers();
  	struct strbuf buf = STRBUF_INIT;
  	const char *accept_language;
  	int ret;
 diff --git a/http.h b/http.h
 index b0927de..5f13695 100644
 --- a/http.h
 +++ b/http.h
 @@ -106,7 +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_get_default_headers();
 +extern struct curl_slist *http_copy_default_headers();
  
  extern long int git_curl_ipresolve;
  extern int active_requests;
 diff --git a/remote-curl.c b/remote-curl.c
 index 86ba787..672b382 100644
 --- a/remote-curl.c
 +++ b/remote-curl.c
 @@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot,
  static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
  {
  	struct active_request_slot *slot;
 -	struct curl_slist *headers = http_get_default_headers();
 +	struct curl_slist *headers = http_copy_default_headers();
  	struct strbuf buf = STRBUF_INIT;
  	int err;
  
 @@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
  static int post_rpc(struct rpc_state *rpc)
  {
  	struct active_request_slot *slot;
 -	struct curl_slist *headers = http_get_default_headers();
 +	struct curl_slist *headers = http_copy_default_headers();
  	int use_gzip = rpc->gzip_request;
  	char *gzip_body = NULL;
  	size_t gzip_size = 0;


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.
+
 http.cookieFile::
 	File containing previously stored cookie lines which should be used
 	in the Git http session, if they match the server. The file format
diff --git a/http-push.c b/http-push.c
index bd60668..ae2b7f1 100644
--- a/http-push.c
+++ b/http-push.c
@@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
 static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
 {
 	struct strbuf buf = STRBUF_INIT;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 
 	if (options & DAV_HEADER_IF) {
 		strbuf_addf(&buf, "If: (<%s>)", lock->token);
@@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
 static void start_move(struct transfer_request *request)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 
 	slot = get_active_slot();
 	slot->callback_func = process_response;
@@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	char *ep;
 	char timeout_header[25];
 	struct remote_lock *lock = NULL;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	char *escaped;
 
@@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	struct remote_ls_ctx ls;
 
@@ -1204,7 +1204,7 @@ static int locking_available(void)
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	int lock_flags = 0;
 	char *escaped;
diff --git a/http.c b/http.c
index 4304b80..3d662bb 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;
+	}
+
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
 }
@@ -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_copy_default_headers(),
+		"Pragma: no-cache");
+	no_pragma_header = curl_slist_append(http_copy_default_headers(),
+		"Pragma:");
 
 #ifdef USE_CURL_MULTI
 	{
@@ -765,6 +774,9 @@ void http_cleanup(void)
 #endif
 	curl_global_cleanup();
 
+	curl_slist_free_all(extra_http_headers);
+	extra_http_headers = NULL;
+
 	curl_slist_free_all(pragma_header);
 	pragma_header = NULL;
 
@@ -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 *headers = NULL, *h;
+
+	for (h = extra_http_headers; h; h = h->next)
+		headers = curl_slist_append(headers, h->data);
+
+	return headers;
+}
+
 static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
 {
 	char *ptr;
@@ -1380,7 +1402,7 @@ static int http_request(const char *url,
 {
 	struct active_request_slot *slot;
 	struct slot_results results;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	struct strbuf buf = STRBUF_INIT;
 	const char *accept_language;
 	int ret;
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 long int git_curl_ipresolve;
 extern int active_requests;
diff --git a/remote-curl.c b/remote-curl.c
index 15e48e2..672b382 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot,
 static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	struct strbuf buf = STRBUF_INIT;
 	int err;
 
@@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 static int post_rpc(struct rpc_state *rpc)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	int use_gzip = rpc->gzip_request;
 	char *gzip_body = NULL;
 	size_t gzip_size = 0;
-- 
2.8.1.306.gff998f2

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

[Junio C Hamano]

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);

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

[Jeff King]

On Tue, Apr 26, 2016 at 10:03:14AM -0700, Junio C Hamano wrote:

> > +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.

Yes, it was for credential.helper. I think the _implementation_ is a
hack (because each callback has to handle it individually), but the
user-facing part is reasonably elegant, given the constraint of not
introducing new syntax.

In this case it would be something like:

diff --git a/http.c b/http.c
index 3d662bb..a7a4be5 100644
--- a/http.c
+++ b/http.c
@@ -325,8 +325,13 @@ static int http_options(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp("http.extraheader", var)) {
-		extra_http_headers =
-			curl_slist_append(extra_http_headers, value);
+		if (*value)
+			extra_http_headers =
+				curl_slist_append(extra_http_headers, value);
+		else {
+			curl_slist_free_all(extra_http_headers);
+			extra_http_headers = NULL;
+		}
 		return 0;
 	}
 

But I think this block (even before my patch) also needs to handle the
case where "value" is NULL (presumably by complaining with
config_error_nonbool).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But I think this block (even before my patch) also needs to handle the
> case where "value" is NULL (presumably by complaining with
> config_error_nonbool).

OK, so squashes found to be necessary so far amounts to the attached
patch.  I still haven't figured out the best way to rephrase the "by
default" in the proposed log message that made me stutter while
reading it, though.


 http.c | 13 ++++++++++---
 http.h |  2 +-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index 6c4d2ed..aae9944 100644
--- a/http.c
+++ b/http.c
@@ -325,8 +325,15 @@ static int http_options(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp("http.extraheader", var)) {
-		extra_http_headers =
-			curl_slist_append(extra_http_headers, value);
+		if (!value) {
+			return config_error_nonbool(var);
+		} else if (!*value) {
+			curl_slist_free_all(extra_http_headers);
+			extra_http_headers = NULL;
+		} else {
+			extra_http_headers =
+				curl_slist_append(extra_http_headers, value);
+		}
 		return 0;
 	}
 
@@ -1172,7 +1179,7 @@ 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;
 
diff --git a/http.h b/http.h
index 5f13695..36f558b 100644
--- a/http.h
+++ b/http.h
@@ -106,7 +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);
 
 extern long int git_curl_ipresolve;
 extern int active_requests;

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

[Jeff King]

On Tue, Apr 26, 2016 at 10:20:19AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > But I think this block (even before my patch) also needs to handle the
> > case where "value" is NULL (presumably by complaining with
> > config_error_nonbool).
> 
> OK, so squashes found to be necessary so far amounts to the attached
> patch.  I still haven't figured out the best way to rephrase the "by
> default" in the proposed log message that made me stutter while
> reading it, though.

I think that part is just trying to explain the implementation hackery.
Maybe something like:

  Note that `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` takes only
  a single list, overriding any previous call. This means we have to
  collect _all_ of the headers we want to use into a single list, and
  feed it to curl in one shot. Since we already unconditionally set a
  "pragma" header when initializing the curl handles, we can add our new
  headers to that list.

  For callers which override the default header list (like probe_rpc),
  we provide `http_copy_default_headers()` so they can do the same
  trick.

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Tue, 26 Apr 2016, Jeff King wrote:

> On Tue, Apr 26, 2016 at 10:20:19AM -0700, Junio C Hamano wrote:
> 
> > Jeff King <peff@peff.net> writes:
> > 
> > > But I think this block (even before my patch) also needs to handle the
> > > case where "value" is NULL (presumably by complaining with
> > > config_error_nonbool).
> > 
> > OK, so squashes found to be necessary so far amounts to the attached
> > patch.  I still haven't figured out the best way to rephrase the "by
> > default" in the proposed log message that made me stutter while
> > reading it, though.
> 
> I think that part is just trying to explain the implementation hackery.
> Maybe something like:
> 
>   Note that `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` takes only
>   a single list, overriding any previous call. This means we have to
>   collect _all_ of the headers we want to use into a single list, and
>   feed it to curl in one shot. Since we already unconditionally set a
>   "pragma" header when initializing the curl handles, we can add our new
>   headers to that list.
> 
>   For callers which override the default header list (like probe_rpc),
>   we provide `http_copy_default_headers()` so they can do the same
>   trick.

Thank you very much for this. I replaced that part of the commit message
with your version.

Ciao,
Johannes

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

[Johannes Schindelin]

Hi Junio,

On Tue, 26 Apr 2016, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > But I think this block (even before my patch) also needs to handle the
> > case where "value" is NULL (presumably by complaining with
> > config_error_nonbool).
> 
> OK, so squashes found to be necessary so far amounts to the attached
> patch.

Thanks.
Dscho

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

[Junio C Hamano]

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

>  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 am unsure about the last two lines.  Is it necessary to have them?

[PATCH v3] http: support sending custom HTTP headers

[Johannes Schindelin]

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

Note that `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` takes only
a single list, overriding any previous call. This means we have to
collect _all_ of the headers we want to use into a single list, and
feed it to cURL in one shot. Since we already unconditionally set a
"pragma" header when initializing the curl handles, we can add our new
headers to that list.

For callers which override the default header list (like probe_rpc),
we provide `http_copy_default_headers()` so they can do the same
trick.

Big thanks to Jeff King and Junio Hamano for their outstanding help and
patient reviews.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

Changes since v2:
	- now using Peff's much improved wording in the commit message
	- skipped the last two lines from the documentation of the feature
	- fixed function declaration/definition by using `(void)`
	- we now allow resetting extra headers with an empty value
	- added a test

Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v3
 Documentation/config.txt   |  6 ++++++
 http-push.c                | 10 +++++-----
 http.c                     | 35 ++++++++++++++++++++++++++++++++---
 http.h                     |  1 +
 remote-curl.c              |  4 ++--
 t/t5550-http-fetch-dumb.sh |  8 ++++++++
 6 files changed, 54 insertions(+), 10 deletions(-)
Interdiff vs v2:

 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index 37b9af7..c7bbe98 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -1657,9 +1657,9 @@ http.emptyAuth::
  
  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.
 +	more than one such entry exists, all of them are added as extra
 +	headers.  To allow overriding the settings inherited from the system
 +	config, an empty value will reset the extra headers to the empty list.
  
  http.cookieFile::
  	File containing previously stored cookie lines which should be used
 diff --git a/http.c b/http.c
 index 3d662bb..985b995 100644
 --- a/http.c
 +++ b/http.c
 @@ -325,8 +325,15 @@ static int http_options(const char *var, const char *value, void *cb)
  	}
  
  	if (!strcmp("http.extraheader", var)) {
 -		extra_http_headers =
 -			curl_slist_append(extra_http_headers, value);
 +		if (!value) {
 +			return config_error_nonbool(var);
 +		} else if (!*value) {
 +			curl_slist_free_all(extra_http_headers);
 +			extra_http_headers = NULL;
 +		} else {
 +			extra_http_headers =
 +				curl_slist_append(extra_http_headers, value);
 +		}
  		return 0;
  	}
  
 @@ -1175,7 +1182,7 @@ 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;
  
 diff --git a/http.h b/http.h
 index 5f13695..36f558b 100644
 --- a/http.h
 +++ b/http.h
 @@ -106,7 +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);
  
  extern long int git_curl_ipresolve;
  extern int active_requests;
 diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
 index 48e2ab6..96425b1 100755
 --- a/t/t5550-http-fetch-dumb.sh
 +++ b/t/t5550-http-fetch-dumb.sh
 @@ -267,5 +267,13 @@ test_expect_success 'git client does not send an empty Accept-Language' '
  	! grep "^Accept-Language:" stderr
  '
  
 +test_expect_success 'extra HTTP headers are sent' '
 +	GIT_CURL_VERBOSE=1 \
 +	git -c http.extraheader="Hello: World" \
 +		ls-remote "$HTTPD_URL/dumb/repo.git" >out 2>err &&
 +	test_i18ngrep "Hello: World" err >hello.txt &&
 +	test_line_count = 2 hello.txt
 +'
 +
  stop_httpd
  test_done


diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50..c7bbe98 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.  To allow overriding the settings inherited from the system
+	config, an empty value will reset the extra headers to the empty list.
+
 http.cookieFile::
 	File containing previously stored cookie lines which should be used
 	in the Git http session, if they match the server. The file format
diff --git a/http-push.c b/http-push.c
index bd60668..ae2b7f1 100644
--- a/http-push.c
+++ b/http-push.c
@@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
 static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
 {
 	struct strbuf buf = STRBUF_INIT;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 
 	if (options & DAV_HEADER_IF) {
 		strbuf_addf(&buf, "If: (<%s>)", lock->token);
@@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
 static void start_move(struct transfer_request *request)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 
 	slot = get_active_slot();
 	slot->callback_func = process_response;
@@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	char *ep;
 	char timeout_header[25];
 	struct remote_lock *lock = NULL;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	char *escaped;
 
@@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	struct remote_ls_ctx ls;
 
@@ -1204,7 +1204,7 @@ static int locking_available(void)
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	int lock_flags = 0;
 	char *escaped;
diff --git a/http.c b/http.c
index 4304b80..985b995 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,19 @@ static int http_options(const char *var, const char *value, void *cb)
 #endif
 	}
 
+	if (!strcmp("http.extraheader", var)) {
+		if (!value) {
+			return config_error_nonbool(var);
+		} else if (!*value) {
+			curl_slist_free_all(extra_http_headers);
+			extra_http_headers = NULL;
+		} else {
+			extra_http_headers =
+				curl_slist_append(extra_http_headers, value);
+		}
+		return 0;
+	}
+
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
 }
@@ -678,8 +692,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_copy_default_headers(),
+		"Pragma: no-cache");
+	no_pragma_header = curl_slist_append(http_copy_default_headers(),
+		"Pragma:");
 
 #ifdef USE_CURL_MULTI
 	{
@@ -765,6 +781,9 @@ void http_cleanup(void)
 #endif
 	curl_global_cleanup();
 
+	curl_slist_free_all(extra_http_headers);
+	extra_http_headers = NULL;
+
 	curl_slist_free_all(pragma_header);
 	pragma_header = NULL;
 
@@ -1163,6 +1182,16 @@ int run_one_slot(struct active_request_slot *slot,
 	return handle_curl_result(results);
 }
 
+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;
+}
+
 static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
 {
 	char *ptr;
@@ -1380,7 +1409,7 @@ static int http_request(const char *url,
 {
 	struct active_request_slot *slot;
 	struct slot_results results;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	struct strbuf buf = STRBUF_INIT;
 	const char *accept_language;
 	int ret;
diff --git a/http.h b/http.h
index 4ef4bbd..36f558b 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(void);
 
 extern long int git_curl_ipresolve;
 extern int active_requests;
diff --git a/remote-curl.c b/remote-curl.c
index 15e48e2..672b382 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot,
 static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	struct strbuf buf = STRBUF_INIT;
 	int err;
 
@@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 static int post_rpc(struct rpc_state *rpc)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	int use_gzip = rpc->gzip_request;
 	char *gzip_body = NULL;
 	size_t gzip_size = 0;
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 48e2ab6..96425b1 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -267,5 +267,13 @@ test_expect_success 'git client does not send an empty Accept-Language' '
 	! grep "^Accept-Language:" stderr
 '
 
+test_expect_success 'extra HTTP headers are sent' '
+	GIT_CURL_VERBOSE=1 \
+	git -c http.extraheader="Hello: World" \
+		ls-remote "$HTTPD_URL/dumb/repo.git" >out 2>err &&
+	test_i18ngrep "Hello: World" err >hello.txt &&
+	test_line_count = 2 hello.txt
+'
+
 stop_httpd
 test_done
-- 
2.8.1.306.gff998f2

[PATCH v4] http: support sending custom HTTP headers

[Johannes Schindelin]

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

Note that `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` takes only
a single list, overriding any previous call. This means we have to
collect _all_ of the headers we want to use into a single list, and
feed it to cURL in one shot. Since we already unconditionally set a
"pragma" header when initializing the curl handles, we can add our new
headers to that list.

For callers which override the default header list (like probe_rpc),
we provide `http_copy_default_headers()` so they can do the same
trick.

Big thanks to Jeff King and Junio Hamano for their outstanding help and
patient reviews.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v4

The only change vs v3 is that I replaced my flimsical test by Peff's (with
*one* change: I realized that we need to group the Require statements in a
<RequireAll> block when I tried to verify that the test fails when I
modify the first header).

 Documentation/config.txt    |  6 ++++++
 http-push.c                 | 10 +++++-----
 http.c                      | 35 ++++++++++++++++++++++++++++++++---
 http.h                      |  1 +
 remote-curl.c               |  4 ++--
 t/lib-httpd/apache.conf     |  8 ++++++++
 t/t5551-http-fetch-smart.sh |  7 +++++++
 7 files changed, 61 insertions(+), 10 deletions(-)
Interdiff vs v3:

 diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
 index 9317ba0..b8ed96f 100644
 --- a/t/lib-httpd/apache.conf
 +++ b/t/lib-httpd/apache.conf
 @@ -102,6 +102,14 @@ Alias /auth/dumb/ www/auth/dumb/
  	SetEnv GIT_HTTP_EXPORT_ALL
  	Header set Set-Cookie name=value
  </LocationMatch>
 +<LocationMatch /smart_headers/>
 +	<RequireAll>
 +		Require expr %{HTTP:x-magic-one} == 'abra'
 +		Require expr %{HTTP:x-magic-two} == 'cadabra'
 +	</RequireAll>
 +	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 +	SetEnv GIT_HTTP_EXPORT_ALL
 +</LocationMatch>
  ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
  ScriptAlias /broken_smart/ broken-smart-http.sh/
  ScriptAlias /error/ error.sh/
 diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
 index 96425b1..48e2ab6 100755
 --- a/t/t5550-http-fetch-dumb.sh
 +++ b/t/t5550-http-fetch-dumb.sh
 @@ -267,13 +267,5 @@ test_expect_success 'git client does not send an empty Accept-Language' '
  	! grep "^Accept-Language:" stderr
  '
  
 -test_expect_success 'extra HTTP headers are sent' '
 -	GIT_CURL_VERBOSE=1 \
 -	git -c http.extraheader="Hello: World" \
 -		ls-remote "$HTTPD_URL/dumb/repo.git" >out 2>err &&
 -	test_i18ngrep "Hello: World" err >hello.txt &&
 -	test_line_count = 2 hello.txt
 -'
 -
  stop_httpd
  test_done
 diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
 index 58207d8..e44fe72 100755
 --- a/t/t5551-http-fetch-smart.sh
 +++ b/t/t5551-http-fetch-smart.sh
 @@ -282,5 +282,12 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
  	test_line_count = 100000 tags
  '
  
 +test_expect_success 'custom http headers' '
 +	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
 +	git -c http.extraheader="x-magic-one: abra" \
 +	    -c http.extraheader="x-magic-two: cadabra" \
 +	    fetch "$HTTPD_URL/smart_headers/repo.git"
 +'
 +
  stop_httpd
  test_done


diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50..c7bbe98 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.  To allow overriding the settings inherited from the system
+	config, an empty value will reset the extra headers to the empty list.
+
 http.cookieFile::
 	File containing previously stored cookie lines which should be used
 	in the Git http session, if they match the server. The file format
diff --git a/http-push.c b/http-push.c
index bd60668..ae2b7f1 100644
--- a/http-push.c
+++ b/http-push.c
@@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
 static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
 {
 	struct strbuf buf = STRBUF_INIT;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 
 	if (options & DAV_HEADER_IF) {
 		strbuf_addf(&buf, "If: (<%s>)", lock->token);
@@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
 static void start_move(struct transfer_request *request)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 
 	slot = get_active_slot();
 	slot->callback_func = process_response;
@@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	char *ep;
 	char timeout_header[25];
 	struct remote_lock *lock = NULL;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	char *escaped;
 
@@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	struct remote_ls_ctx ls;
 
@@ -1204,7 +1204,7 @@ static int locking_available(void)
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	int lock_flags = 0;
 	char *escaped;
diff --git a/http.c b/http.c
index 4304b80..985b995 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,19 @@ static int http_options(const char *var, const char *value, void *cb)
 #endif
 	}
 
+	if (!strcmp("http.extraheader", var)) {
+		if (!value) {
+			return config_error_nonbool(var);
+		} else if (!*value) {
+			curl_slist_free_all(extra_http_headers);
+			extra_http_headers = NULL;
+		} else {
+			extra_http_headers =
+				curl_slist_append(extra_http_headers, value);
+		}
+		return 0;
+	}
+
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
 }
@@ -678,8 +692,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_copy_default_headers(),
+		"Pragma: no-cache");
+	no_pragma_header = curl_slist_append(http_copy_default_headers(),
+		"Pragma:");
 
 #ifdef USE_CURL_MULTI
 	{
@@ -765,6 +781,9 @@ void http_cleanup(void)
 #endif
 	curl_global_cleanup();
 
+	curl_slist_free_all(extra_http_headers);
+	extra_http_headers = NULL;
+
 	curl_slist_free_all(pragma_header);
 	pragma_header = NULL;
 
@@ -1163,6 +1182,16 @@ int run_one_slot(struct active_request_slot *slot,
 	return handle_curl_result(results);
 }
 
+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;
+}
+
 static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
 {
 	char *ptr;
@@ -1380,7 +1409,7 @@ static int http_request(const char *url,
 {
 	struct active_request_slot *slot;
 	struct slot_results results;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	struct strbuf buf = STRBUF_INIT;
 	const char *accept_language;
 	int ret;
diff --git a/http.h b/http.h
index 4ef4bbd..36f558b 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(void);
 
 extern long int git_curl_ipresolve;
 extern int active_requests;
diff --git a/remote-curl.c b/remote-curl.c
index 15e48e2..672b382 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot,
 static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	struct strbuf buf = STRBUF_INIT;
 	int err;
 
@@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 static int post_rpc(struct rpc_state *rpc)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	int use_gzip = rpc->gzip_request;
 	char *gzip_body = NULL;
 	size_t gzip_size = 0;
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 9317ba0..b8ed96f 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -102,6 +102,14 @@ Alias /auth/dumb/ www/auth/dumb/
 	SetEnv GIT_HTTP_EXPORT_ALL
 	Header set Set-Cookie name=value
 </LocationMatch>
+<LocationMatch /smart_headers/>
+	<RequireAll>
+		Require expr %{HTTP:x-magic-one} == 'abra'
+		Require expr %{HTTP:x-magic-two} == 'cadabra'
+	</RequireAll>
+	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+	SetEnv GIT_HTTP_EXPORT_ALL
+</LocationMatch>
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 ScriptAlias /error/ error.sh/
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 58207d8..e44fe72 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -282,5 +282,12 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
 	test_line_count = 100000 tags
 '
 
+test_expect_success 'custom http headers' '
+	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
+	git -c http.extraheader="x-magic-one: abra" \
+	    -c http.extraheader="x-magic-two: cadabra" \
+	    fetch "$HTTPD_URL/smart_headers/repo.git"
+'
+
 stop_httpd
 test_done
-- 
2.8.1.306.gff998f2

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

[Jeff King]

On Wed, Apr 27, 2016 at 02:20:37PM +0200, Johannes Schindelin wrote:

> The only change vs v3 is that I replaced my flimsical test by Peff's (with
> *one* change: I realized that we need to group the Require statements in a
> <RequireAll> block when I tried to verify that the test fails when I
> modify the first header).

Whoops, I didn't actually test that case. Thanks for catching (as you
might guess, I wanted to make sure we handle multiple values correctly).

>  Documentation/config.txt    |  6 ++++++
>  http-push.c                 | 10 +++++-----
>  http.c                      | 35 ++++++++++++++++++++++++++++++++---
>  http.h                      |  1 +
>  remote-curl.c               |  4 ++--
>  t/lib-httpd/apache.conf     |  8 ++++++++
>  t/t5551-http-fetch-smart.sh |  7 +++++++
>  7 files changed, 61 insertions(+), 10 deletions(-)

This version looks good to me.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Apr 27, 2016 at 02:20:37PM +0200, Johannes Schindelin wrote:
>
>> The only change vs v3 is that I replaced my flimsical test by Peff's (with
>> *one* change: I realized that we need to group the Require statements in a
>> <RequireAll> block when I tried to verify that the test fails when I
>> modify the first header).
>
> Whoops, I didn't actually test that case. Thanks for catching (as you
> might guess, I wanted to make sure we handle multiple values correctly).
>
>>  Documentation/config.txt    |  6 ++++++
>>  http-push.c                 | 10 +++++-----
>>  http.c                      | 35 ++++++++++++++++++++++++++++++++---
>>  http.h                      |  1 +
>>  remote-curl.c               |  4 ++--
>>  t/lib-httpd/apache.conf     |  8 ++++++++
>>  t/t5551-http-fetch-smart.sh |  7 +++++++
>>  7 files changed, 61 insertions(+), 10 deletions(-)
>
> This version looks good to me.
>
> -Peff

Thanks, both.

[PATCH v5 0/2] Add support for sending additional HTTP headers

[Johannes Schindelin]

My use case is an army of build agents that need only limited and
selective access to otherwise private repositories.

I apologize for sending out v5 after v4 was already acknowledged: my
initial testing was on simple repositories and I forgot that my build
agents need to handle submodules, too.

So here goes v5, the only change being the addition of the second patch
that adds support for passing the extra headers to git-submodule through
the command-line.


Johannes Schindelin (2):
  http: support sending custom HTTP headers
  submodule: pass on http.extraheader config settings

 Documentation/config.txt    |  6 ++++++
 builtin/submodule--helper.c |  4 +++-
 http-push.c                 | 10 +++++-----
 http.c                      | 35 ++++++++++++++++++++++++++++++++---
 http.h                      |  1 +
 remote-curl.c               |  4 ++--
 t/lib-httpd/apache.conf     |  8 ++++++++
 t/t5551-http-fetch-smart.sh | 16 ++++++++++++++++
 8 files changed, 73 insertions(+), 11 deletions(-)

Interdiff vs v4:

 diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
 index 3bd6883..b338f93 100644
 --- a/builtin/submodule--helper.c
 +++ b/builtin/submodule--helper.c
 @@ -127,7 +127,9 @@ static int module_name(int argc, const char **argv, const char *prefix)
   */
  static int submodule_config_ok(const char *var)
  {
 -	if (starts_with(var, "credential."))
 +	if (starts_with(var, "credential.") ||
 +			(starts_with(var, "http.") &&
 +			 ends_with(var, ".extraheader")))
  		return 1;
  	return 0;
  }
 diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
 index e44fe72..1794168 100755
 --- a/t/t5551-http-fetch-smart.sh
 +++ b/t/t5551-http-fetch-smart.sh
 @@ -286,7 +286,16 @@ test_expect_success 'custom http headers' '
  	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
  	git -c http.extraheader="x-magic-one: abra" \
  	    -c http.extraheader="x-magic-two: cadabra" \
 -	    fetch "$HTTPD_URL/smart_headers/repo.git"
 +	    fetch "$HTTPD_URL/smart_headers/repo.git" &&
 +	git update-index --add --cacheinfo 160000,$(git rev-parse HEAD),sub &&
 +	git config -f .gitmodules submodule.sub.path sub &&
 +	git config -f .gitmodules submodule.sub.url \
 +		"$HTTPD_URL/smart_headers/repo.git" &&
 +	git submodule init sub &&
 +	test_must_fail git submodule update sub &&
 +	git -c http.extraheader="x-magic-one: abra" \
 +	    -c http.extraheader="x-magic-two: cadabra" \
 +		submodule update sub
  '
  
  stop_httpd

-- 
2.8.1.306.gff998f2

[PATCH v5 1/2] http: support sending custom HTTP headers

[Johannes Schindelin]

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

Note that `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` takes only
a single list, overriding any previous call. This means we have to
collect _all_ of the headers we want to use into a single list, and
feed it to cURL in one shot. Since we already unconditionally set a
"pragma" header when initializing the curl handles, we can add our new
headers to that list.

For callers which override the default header list (like probe_rpc),
we provide `http_copy_default_headers()` so they can do the same
trick.

Big thanks to Jeff King and Junio Hamano for their outstanding help and
patient reviews.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt    |  6 ++++++
 http-push.c                 | 10 +++++-----
 http.c                      | 35 ++++++++++++++++++++++++++++++++---
 http.h                      |  1 +
 remote-curl.c               |  4 ++--
 t/lib-httpd/apache.conf     |  8 ++++++++
 t/t5551-http-fetch-smart.sh |  7 +++++++
 7 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50..c7bbe98 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.  To allow overriding the settings inherited from the system
+	config, an empty value will reset the extra headers to the empty list.
+
 http.cookieFile::
 	File containing previously stored cookie lines which should be used
 	in the Git http session, if they match the server. The file format
diff --git a/http-push.c b/http-push.c
index bd60668..ae2b7f1 100644
--- a/http-push.c
+++ b/http-push.c
@@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
 static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
 {
 	struct strbuf buf = STRBUF_INIT;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 
 	if (options & DAV_HEADER_IF) {
 		strbuf_addf(&buf, "If: (<%s>)", lock->token);
@@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
 static void start_move(struct transfer_request *request)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 
 	slot = get_active_slot();
 	slot->callback_func = process_response;
@@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	char *ep;
 	char timeout_header[25];
 	struct remote_lock *lock = NULL;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	char *escaped;
 
@@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	struct remote_ls_ctx ls;
 
@@ -1204,7 +1204,7 @@ static int locking_available(void)
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	int lock_flags = 0;
 	char *escaped;
diff --git a/http.c b/http.c
index 4304b80..985b995 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,19 @@ static int http_options(const char *var, const char *value, void *cb)
 #endif
 	}
 
+	if (!strcmp("http.extraheader", var)) {
+		if (!value) {
+			return config_error_nonbool(var);
+		} else if (!*value) {
+			curl_slist_free_all(extra_http_headers);
+			extra_http_headers = NULL;
+		} else {
+			extra_http_headers =
+				curl_slist_append(extra_http_headers, value);
+		}
+		return 0;
+	}
+
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
 }
@@ -678,8 +692,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_copy_default_headers(),
+		"Pragma: no-cache");
+	no_pragma_header = curl_slist_append(http_copy_default_headers(),
+		"Pragma:");
 
 #ifdef USE_CURL_MULTI
 	{
@@ -765,6 +781,9 @@ void http_cleanup(void)
 #endif
 	curl_global_cleanup();
 
+	curl_slist_free_all(extra_http_headers);
+	extra_http_headers = NULL;
+
 	curl_slist_free_all(pragma_header);
 	pragma_header = NULL;
 
@@ -1163,6 +1182,16 @@ int run_one_slot(struct active_request_slot *slot,
 	return handle_curl_result(results);
 }
 
+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;
+}
+
 static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
 {
 	char *ptr;
@@ -1380,7 +1409,7 @@ static int http_request(const char *url,
 {
 	struct active_request_slot *slot;
 	struct slot_results results;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	struct strbuf buf = STRBUF_INIT;
 	const char *accept_language;
 	int ret;
diff --git a/http.h b/http.h
index 4ef4bbd..36f558b 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(void);
 
 extern long int git_curl_ipresolve;
 extern int active_requests;
diff --git a/remote-curl.c b/remote-curl.c
index 15e48e2..672b382 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot,
 static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	struct strbuf buf = STRBUF_INIT;
 	int err;
 
@@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 static int post_rpc(struct rpc_state *rpc)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	int use_gzip = rpc->gzip_request;
 	char *gzip_body = NULL;
 	size_t gzip_size = 0;
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 9317ba0..b8ed96f 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -102,6 +102,14 @@ Alias /auth/dumb/ www/auth/dumb/
 	SetEnv GIT_HTTP_EXPORT_ALL
 	Header set Set-Cookie name=value
 </LocationMatch>
+<LocationMatch /smart_headers/>
+	<RequireAll>
+		Require expr %{HTTP:x-magic-one} == 'abra'
+		Require expr %{HTTP:x-magic-two} == 'cadabra'
+	</RequireAll>
+	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+	SetEnv GIT_HTTP_EXPORT_ALL
+</LocationMatch>
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 ScriptAlias /error/ error.sh/
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 58207d8..e44fe72 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -282,5 +282,12 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
 	test_line_count = 100000 tags
 '
 
+test_expect_success 'custom http headers' '
+	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
+	git -c http.extraheader="x-magic-one: abra" \
+	    -c http.extraheader="x-magic-two: cadabra" \
+	    fetch "$HTTPD_URL/smart_headers/repo.git"
+'
+
 stop_httpd
 test_done
-- 
2.8.1.306.gff998f2

[PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Johannes Schindelin]

To support this developer's use case of allowing build agents token-based
access to private repositories, we introduced the http.extraheader
feature, allowing extra HTTP headers to be sent along with every HTTP
request.

This patch allows us to configure these extra HTTP headers for use with
`git submodule update`, too. It requires somewhat special handling:
submodules do not share the parent project's config. It would be
incorrect to simply reuse that specific part of the parent's config.
Instead, the config option needs to be specified on the command-line or
in ~/.gitconfig or friends.

Example: git -c http.extraheader="Secret: Sauce" submodule update --init

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/submodule--helper.c |  4 +++-
 t/t5551-http-fetch-smart.sh | 11 ++++++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3bd6883..b338f93 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -127,7 +127,9 @@ static int module_name(int argc, const char **argv, const char *prefix)
  */
 static int submodule_config_ok(const char *var)
 {
-	if (starts_with(var, "credential."))
+	if (starts_with(var, "credential.") ||
+			(starts_with(var, "http.") &&
+			 ends_with(var, ".extraheader")))
 		return 1;
 	return 0;
 }
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index e44fe72..1794168 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -286,7 +286,16 @@ test_expect_success 'custom http headers' '
 	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
 	git -c http.extraheader="x-magic-one: abra" \
 	    -c http.extraheader="x-magic-two: cadabra" \
-	    fetch "$HTTPD_URL/smart_headers/repo.git"
+	    fetch "$HTTPD_URL/smart_headers/repo.git" &&
+	git update-index --add --cacheinfo 160000,$(git rev-parse HEAD),sub &&
+	git config -f .gitmodules submodule.sub.path sub &&
+	git config -f .gitmodules submodule.sub.url \
+		"$HTTPD_URL/smart_headers/repo.git" &&
+	git submodule init sub &&
+	test_must_fail git submodule update sub &&
+	git -c http.extraheader="x-magic-one: abra" \
+	    -c http.extraheader="x-magic-two: cadabra" \
+		submodule update sub
 '
 
 stop_httpd
-- 
2.8.1.306.gff998f2

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Jeff King]

On Thu, Apr 28, 2016 at 12:03:47PM +0200, Johannes Schindelin wrote:

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 3bd6883..b338f93 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -127,7 +127,9 @@ static int module_name(int argc, const char **argv, const char *prefix)
>   */
>  static int submodule_config_ok(const char *var)
>  {
> -	if (starts_with(var, "credential."))
> +	if (starts_with(var, "credential.") ||
> +			(starts_with(var, "http.") &&
> +			 ends_with(var, ".extraheader")))
>  		return 1;
>  	return 0;
>  }

Should we consider just white-listing all of "http.*"?

That would help other cases which have come up, like:

  http://thread.gmane.org/gmane.comp.version-control.git/264840

which wants to turn off http.sslverify. That would mean it turns off for
every submodule, too, but if you want to be choosy about your http
variables, you should be using the "http.$URL.sslverify" form, to only
affect specific servers (whether they are in submodules or not).

-Peff

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Johannes Schindelin]

Hi Peff,

Cc:ing Jacob, the author of the CONFIG_DATA_ENVIRONMENT sanitizing code.

On Thu, 28 Apr 2016, Jeff King wrote:

> On Thu, Apr 28, 2016 at 12:03:47PM +0200, Johannes Schindelin wrote:
> 
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index 3bd6883..b338f93 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -127,7 +127,9 @@ static int module_name(int argc, const char **argv, const char *prefix)
> >   */
> >  static int submodule_config_ok(const char *var)
> >  {
> > -	if (starts_with(var, "credential."))
> > +	if (starts_with(var, "credential.") ||
> > +			(starts_with(var, "http.") &&
> > +			 ends_with(var, ".extraheader")))
> >  		return 1;
> >  	return 0;
> >  }
> 
> Should we consider just white-listing all of "http.*"?
> 
> That would help other cases which have come up, like:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/264840
> 
> which wants to turn off http.sslverify. That would mean it turns off for
> every submodule, too, but if you want to be choosy about your http
> variables, you should be using the "http.$URL.sslverify" form, to only
> affect specific servers (whether they are in submodules or not).

I considered that, and thought that it might be dangerous, what with me
not vetting carefully which http.* variables are safe to pass on to the
submodules' update and which are not.

So I had a look now, and the most prominent potential problem is the
http.cookieFile setting, which could be reused all of a sudden if we
made my patch more general.

But then, we are talking about the code that filters what gets passed via
the *command-line*. And to be quite honest, I am not sure that we should
actually filter out *any* of these settings.

The commit message that introduced this particular filtering has this
rationale to let only credential.* through:

    GIT_CONFIG_PARAMETERS is special, and we actually do want to
    preserve these settings. However, we do not want to preserve all
    configuration as many things should be left specific to the parent
    project.

    Add a git submodule--helper function, sanitize-config, which shall be
    used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs
    except a small subset that are known to be safe and necessary.

Dunno. I tried to err on the side of caution... But this sounds maybe a
bit *too* cautious?

Jacob, Junio?

Ciao,
Dscho

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Jeff King]

On Thu, Apr 28, 2016 at 02:19:37PM +0200, Johannes Schindelin wrote:

> > Should we consider just white-listing all of "http.*"?
> > 
> > That would help other cases which have come up, like:
> > 
> >   http://thread.gmane.org/gmane.comp.version-control.git/264840
> > 
> > which wants to turn off http.sslverify. That would mean it turns off for
> > every submodule, too, but if you want to be choosy about your http
> > variables, you should be using the "http.$URL.sslverify" form, to only
> > affect specific servers (whether they are in submodules or not).
> 
> I considered that, and thought that it might be dangerous, what with me
> not vetting carefully which http.* variables are safe to pass on to the
> submodules' update and which are not.
> 
> So I had a look now, and the most prominent potential problem is the
> http.cookieFile setting, which could be reused all of a sudden if we
> made my patch more general.
> 
> But then, we are talking about the code that filters what gets passed via
> the *command-line*. And to be quite honest, I am not sure that we should
> actually filter out *any* of these settings.

The intent of the whitelist (from my recollection of the discussion) is
to filter out config that must be repo-specific. E.g., core.worktree or
core.bare should definitely _not_ be passed to a submodule.

I don't know if there are others. We started with a whitelist because it
was the smallest and safest change away from the status quo. A blacklist
would also work, with the risk that we might let through nonsense in
some cases (but only if the user triggers us to do so).

> The commit message that introduced this particular filtering has this
> rationale to let only credential.* through:
> 
>     GIT_CONFIG_PARAMETERS is special, and we actually do want to
>     preserve these settings. However, we do not want to preserve all
>     configuration as many things should be left specific to the parent
>     project.
> 
>     Add a git submodule--helper function, sanitize-config, which shall be
>     used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs
>     except a small subset that are known to be safe and necessary.
> 
> Dunno. I tried to err on the side of caution... But this sounds maybe a
> bit *too* cautious?

So if we all agree that the sanitizing is really about preventing
repo-specific variables from leaking, and not any kind of security
boundary, I think we should generally be pretty liberal in whitelisting
things.

I can certainly come up with a pathological case where using it as a
security boundary may have some practical use, but in general I think it
is mostly getting in the way of what users are trying to do.

-Peff

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Jacob Keller]

On Thu, Apr 28, 2016 at 6:49 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Apr 28, 2016 at 02:19:37PM +0200, Johannes Schindelin wrote:
>
>> > Should we consider just white-listing all of "http.*"?
>> >
>> > That would help other cases which have come up, like:
>> >
>> >   http://thread.gmane.org/gmane.comp.version-control.git/264840
>> >
>> > which wants to turn off http.sslverify. That would mean it turns off for
>> > every submodule, too, but if you want to be choosy about your http
>> > variables, you should be using the "http.$URL.sslverify" form, to only
>> > affect specific servers (whether they are in submodules or not).
>>
>> I considered that, and thought that it might be dangerous, what with me
>> not vetting carefully which http.* variables are safe to pass on to the
>> submodules' update and which are not.
>>
>> So I had a look now, and the most prominent potential problem is the
>> http.cookieFile setting, which could be reused all of a sudden if we
>> made my patch more general.
>>
>> But then, we are talking about the code that filters what gets passed via
>> the *command-line*. And to be quite honest, I am not sure that we should
>> actually filter out *any* of these settings.
>
> The intent of the whitelist (from my recollection of the discussion) is
> to filter out config that must be repo-specific. E.g., core.worktree or
> core.bare should definitely _not_ be passed to a submodule.
>
> I don't know if there are others. We started with a whitelist because it
> was the smallest and safest change away from the status quo. A blacklist
> would also work, with the risk that we might let through nonsense in
> some cases (but only if the user triggers us to do so).
>
>> The commit message that introduced this particular filtering has this
>> rationale to let only credential.* through:
>>
>>     GIT_CONFIG_PARAMETERS is special, and we actually do want to
>>     preserve these settings. However, we do not want to preserve all
>>     configuration as many things should be left specific to the parent
>>     project.
>>
>>     Add a git submodule--helper function, sanitize-config, which shall be
>>     used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs
>>     except a small subset that are known to be safe and necessary.
>>
>> Dunno. I tried to err on the side of caution... But this sounds maybe a
>> bit *too* cautious?
>
> So if we all agree that the sanitizing is really about preventing
> repo-specific variables from leaking, and not any kind of security
> boundary, I think we should generally be pretty liberal in whitelisting
> things.
>
> I can certainly come up with a pathological case where using it as a
> security boundary may have some practical use, but in general I think it
> is mostly getting in the way of what users are trying to do.
>
> -Peff

I think I prefer a blacklist approach, since it reduces the need for
future changes, since most cases will either not put config on the
environment or (based on feedback on the mailing list and bug reports)
the user will believe it should be applied.

A black list which only removed configurations we know are harmful
would be easier to maintain but risks new additions forgetting to do
so. A whitelist means we only fix things as they come up but also
means we aren't "breaking" anything that works today, where as a
blacklist could break something that works today.

Thanks,
Jake

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Jeff King]

On Thu, Apr 28, 2016 at 08:37:10AM -0700, Jacob Keller wrote:

> I think I prefer a blacklist approach, since it reduces the need for
> future changes, since most cases will either not put config on the
> environment or (based on feedback on the mailing list and bug reports)
> the user will believe it should be applied.
> 
> A black list which only removed configurations we know are harmful
> would be easier to maintain but risks new additions forgetting to do
> so. A whitelist means we only fix things as they come up but also
> means we aren't "breaking" anything that works today, where as a
> blacklist could break something that works today.

I think the key thing with a blacklist is somebody has to go to the work
to audit the existing keys.

-Peff

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Stefan Beller]

On Thu, Apr 28, 2016 at 8:39 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Apr 28, 2016 at 08:37:10AM -0700, Jacob Keller wrote:
>
>> I think I prefer a blacklist approach, since it reduces the need for
>> future changes, since most cases will either not put config on the
>> environment or (based on feedback on the mailing list and bug reports)
>> the user will believe it should be applied.
>>
>> A black list which only removed configurations we know are harmful
>> would be easier to maintain but risks new additions forgetting to do
>> so. A whitelist means we only fix things as they come up but also
>> means we aren't "breaking" anything that works today, where as a
>> blacklist could break something that works today.
>
> I think the key thing with a blacklist is somebody has to go to the work
> to audit the existing keys.

Would it be sufficient to wait until someone screams at the mailing list
for some key to be blacklisted? (I mean in the short term that would be
of less quality, but relying on the larger community would result in a better
end result? So your going through is just a jump start this process of
listening to the community?)

Thanks,
Stefan

>
> -Peff
> --
> 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

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Jeff King]

On Thu, Apr 28, 2016 at 09:09:44AM -0700, Stefan Beller wrote:

> > I think the key thing with a blacklist is somebody has to go to the work
> > to audit the existing keys.
> 
> Would it be sufficient to wait until someone screams at the mailing list
> for some key to be blacklisted? (I mean in the short term that would be
> of less quality, but relying on the larger community would result in a better
> end result? So your going through is just a jump start this process of
> listening to the community?)

Yeah, I think ultimately we will rely on the community. But I would feel
a lot more comfortable if somebody made at least a single pass.

I'll be curious what Junio says, too. I generally defer to him on how
conservative we want to be in cases like this.

-Peff

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Apr 28, 2016 at 09:09:44AM -0700, Stefan Beller wrote:
>
>> > I think the key thing with a blacklist is somebody has to go to the work
>> > to audit the existing keys.
>> 
>> Would it be sufficient to wait until someone screams at the mailing list
>> for some key to be blacklisted? (I mean in the short term that would be
>> of less quality, but relying on the larger community would result in a better
>> end result? So your going through is just a jump start this process of
>> listening to the community?)
>
> Yeah, I think ultimately we will rely on the community. But I would feel
> a lot more comfortable if somebody made at least a single pass.
>
> I'll be curious what Junio says, too. I generally defer to him on how
> conservative we want to be in cases like this.

Starting from an empty whitelist and waiting for people to scream
with valid use cases would automatically give us the single pass to
identify the set of essential ones that users must be able to pass,
no?

Of course, the screamed proposal to add something to whitelist must
be vetted (i.e. "yeah, we can see passing X in _your_ usecase might
be useful, but here are downsides (e.g. security implications) of
allowing X in other usecases").  And we might even find that it is
insufficient safety to allow/disallow per variable name during that
discussion, in which case choice between whitelist and blacklist
becomes moot.

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Jeff King]

On Thu, Apr 28, 2016 at 12:06:56PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Thu, Apr 28, 2016 at 09:09:44AM -0700, Stefan Beller wrote:
> >
> >> > I think the key thing with a blacklist is somebody has to go to the work
> >> > to audit the existing keys.
> >> 
> >> Would it be sufficient to wait until someone screams at the mailing list
> >> for some key to be blacklisted? (I mean in the short term that would be
> >> of less quality, but relying on the larger community would result in a better
> >> end result? So your going through is just a jump start this process of
> >> listening to the community?)
> >
> > Yeah, I think ultimately we will rely on the community. But I would feel
> > a lot more comfortable if somebody made at least a single pass.
> >
> > I'll be curious what Junio says, too. I generally defer to him on how
> > conservative we want to be in cases like this.
> 
> Starting from an empty whitelist and waiting for people to scream
> with valid use cases would automatically give us the single pass to
> identify the set of essential ones that users must be able to pass,
> no?

It's definitely sufficient, it's just annoying if a user shows up every
week and says "I want X.Y", and then somebody else shows up a week later
and says "I want X.Z".

Are we serving any purpose in vetting each one (and if so, what)?

-Peff

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It's definitely sufficient, it's just annoying if a user shows up every
> week and says "I want X.Y", and then somebody else shows up a week later
> and says "I want X.Z".
>
> Are we serving any purpose in vetting each one (and if so, what)?

Personally I do not think we would need to filter _anything_ if we
can tell that the user directly said

	git -c var1=val1 -c var2=val2 $cmd ...

and "git $cmd" ended up needing to spawn another "git" subcommand,
possibly in some other repository (i.e. "$cmd" in this case is
likely to be "submodule", but in principle it does not have to be).
If the user somehow gives variables like core.worktree that are
inappropriate to be applied across repositories, that's user's
problem, i.e. "don't do it then if it hurts".

If we are doing any filtering, however, it is always hard, if not
impossible, to take away what we originally granted, even by
mistake, for any reason, even for correctness or for security, in a
later release.

We probably could sidestep it by introducing an end-user
configurable "whitelist" somewhere.

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Stefan Beller]

On Thu, Apr 28, 2016 at 12:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> It's definitely sufficient, it's just annoying if a user shows up every
>> week and says "I want X.Y", and then somebody else shows up a week later
>> and says "I want X.Z".
>>
>> Are we serving any purpose in vetting each one (and if so, what)?
>
> Personally I do not think we would need to filter _anything_ if we
> can tell that the user directly said
>
>         git -c var1=val1 -c var2=val2 $cmd ...
>
> and "git $cmd" ended up needing to spawn another "git" subcommand,
> possibly in some other repository (i.e. "$cmd" in this case is
> likely to be "submodule", but in principle it does not have to be).
> If the user somehow gives variables like core.worktree that are
> inappropriate to be applied across repositories, that's user's
> problem, i.e. "don't do it then if it hurts".

So when going with that philosophy, the user might be missing
switches like

    -c-for-this-repo-only core.worktree=... -c
submodule.worktree=align-relative-to-parent

i.e. when shifting the responsibility to the user, we need to have
switches to pass options to the repository or a subset of submodules?

>
> If we are doing any filtering, however, it is always hard, if not
> impossible, to take away what we originally granted, even by
> mistake, for any reason, even for correctness or for security, in a
> later release.
>
> We probably could sidestep it by introducing an end-user
> configurable "whitelist" somewhere.
>
>

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So when going with that philosophy, the user might be missing
> switches like
>
>     -c-for-this-repo-only core.worktree=... -c
> submodule.worktree=align-relative-to-parent
>
> i.e. when shifting the responsibility to the user, we need to have
> switches to pass options to the repository or a subset of submodules?

I think that is an excellent illlustration of the issue by an
example of what we shouldn't do ;-)

"git" is not always about submodules, so "-c-but-not-for-submodules"
option does not belong to "git" wrapper.

Users use "git -c" and hope to affect what happens in submodules,
only because "git submodule" support is still immature and does not
have options to do that.  You certainly smell a linkage between
"pass options to a selected subset of submodules" and your recent
"give labels to submodules so that they can be named with *group
syntax" topic, no?

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Users use "git -c" and hope to affect what happens in submodules,
> only because "git submodule" support is still immature and does not
> have options to do that.  You certainly smell a linkage between
> "pass options to a selected subset of submodules" and your recent
> "give labels to submodules so that they can be named with *group
> syntax" topic, no?

By "options to do that", I meant "options to specify 'these
configurations are passed down to part of this command that operates
on these submodules'".

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Stefan Beller]

On Thu, Apr 28, 2016 at 12:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So when going with that philosophy, the user might be missing
>> switches like
>>
>>     -c-for-this-repo-only core.worktree=... -c
>> submodule.worktree=align-relative-to-parent
>>
>> i.e. when shifting the responsibility to the user, we need to have
>> switches to pass options to the repository or a subset of submodules?
>
> I think that is an excellent illlustration of the issue by an
> example of what we shouldn't do ;-)

So rather have a white / black list?

Could we have a pre curated list with the list easily changed by the user?
So instead of screaming at the mailing list they can work around easily,
and eventually someone fixes the "stupid" default?

Maybe a user would want to do

    git config --global submodule-credentials-filter-white-list "host.*"

instead? (Naming intentionally bad)

>
> "git" is not always about submodules, so "-c-but-not-for-submodules"
> option does not belong to "git" wrapper.
>
> Users use "git -c" and hope to affect what happens in submodules,
> only because "git submodule" support is still immature and does not
> have options to do that.  You certainly smell a linkage between
> "pass options to a selected subset of submodules" and your recent
> "give labels to submodules so that they can be named with *group
> syntax" topic, no?

I thought about that for a split second, but no. I mean it in the
most general  way possible.

It doesn't even have to be a submodule related thing at all.

I can imagine that `git gc` could learn to walk nested repos
(not submodules, just repos on disk inside the work tree).
And for that use case we'd maybe want to have a setting
to pack the nested repos more aggressively than the toplevel repo.

(Not sure if there would be a use case or such a thing, but it is the
first I came up with)

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> It doesn't even have to be a submodule related thing at all.
>
> I can imagine that `git gc` could learn to walk nested repos
> (not submodules, just repos on disk inside the work tree).
> And for that use case we'd maybe want to have a setting
> to pack the nested repos more aggressively than the toplevel repo.
>
> (Not sure if there would be a use case or such a thing, but it is the
> first I came up with)

I do not think we are in the business of mucking with "dump of
unrelated collections of repositories, whose inter-relationship the
user did not tell us anything about".

I however can foresee a value in "git gc --recurse-submodules", and
I could see users may want to gc different submodules in different
ways (which again brings us back to your "submodule group" thing).

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Jeff King]

On Thu, Apr 28, 2016 at 12:52:03PM -0700, Junio C Hamano wrote:

> "git" is not always about submodules, so "-c-but-not-for-submodules"
> option does not belong to "git" wrapper.
> 
> Users use "git -c" and hope to affect what happens in submodules,
> only because "git submodule" support is still immature and does not
> have options to do that.  You certainly smell a linkage between
> "pass options to a selected subset of submodules" and your recent
> "give labels to submodules so that they can be named with *group
> syntax" topic, no?

Keep in mind that submodule interactions may be triggered from other
non-submodule commands. So "git fetch", for instance, may end up caring
about whether you pass "http.*" or "credential.*" down to the
submodules. I do not think "fetch" should grow submodule-specific
options, so that pretty much leaves "git" options as the only place
left.

-Peff

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Stefan Beller]

On Thu, Apr 28, 2016 at 2:03 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Apr 28, 2016 at 12:52:03PM -0700, Junio C Hamano wrote:
>
>> "git" is not always about submodules, so "-c-but-not-for-submodules"
>> option does not belong to "git" wrapper.
>>
>> Users use "git -c" and hope to affect what happens in submodules,
>> only because "git submodule" support is still immature and does not
>> have options to do that.  You certainly smell a linkage between
>> "pass options to a selected subset of submodules" and your recent
>> "give labels to submodules so that they can be named with *group
>> syntax" topic, no?
>
> Keep in mind that submodule interactions may be triggered from other
> non-submodule commands. So "git fetch", for instance, may end up caring
> about whether you pass "http.*" or "credential.*" down to the
> submodules.

Or clone. I recently sent origin/sb/clone-shallow-passthru
which adds more options to a non-submodule command.
I think a command line option there is better than a "git -c $OPTION"

> I do not think "fetch" should grow submodule-specific
> options, so that pretty much leaves "git" options as the only place
> left.

I would rather see it in fetch as in the generic Git?

Stefan

>
> -Peff

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Keep in mind that submodule interactions may be triggered from other
> non-submodule commands. So "git fetch", for instance, may end up caring
> about whether you pass "http.*" or "credential.*" down to the
> submodules.

> I do not think "fetch" should grow submodule-specific
> options,...

The updated "git fetch" needs to grow submodule-specific options to
at least either enable or disable "recurse into submodules", and
that is true even if the default behaviour in the future were to
recurse into submodules in a top-level project repository that has
submodules (i.e. you must have "git fetch --no-recurse-submodules"
option).  "Please use these configuration when you do recurse into
them" options are very much submodule specific in the same way.

If anything, with Stefan's "submodule groups" thing, I would expect
more commands (like "git diff") to become aware of the possibility
of descending into submodules (and even "selected subset of
submodules"), and they need command line options to tell which ones
to descend into.  "Here is the set of submodules I want you to
descend into" and "By the way, I want you to use these settings
while working in them" would go naturally hand in hand, I would
imagine, so I strongly disagree with the statement "fetch should not
grow submodule specific options".

Of course, we can stop teaching --recurse-submodules to non "git
submodule" commands and concentrate on improving "git submodule" as
the end-user facing command, or cover usecases to work on subset of
submodules with "git submodule foreach".  But I do not think that is
what you are advocating for.

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Jeff King]

On Thu, Apr 28, 2016 at 03:44:50PM -0700, Junio C Hamano wrote:

> > I do not think "fetch" should grow submodule-specific
> > options,...
> 
> The updated "git fetch" needs to grow submodule-specific options to
> at least either enable or disable "recurse into submodules", and
> that is true even if the default behaviour in the future were to
> recurse into submodules in a top-level project repository that has
> submodules (i.e. you must have "git fetch --no-recurse-submodules"
> option).  "Please use these configuration when you do recurse into
> them" options are very much submodule specific in the same way.

I suppose so. I actually think it might be useful / convenient to have a
global "do not recurse" environment variable (and possibly a matching
"git --recurse" command-line flag to set it). Otherwise we end up with
annoying propagation problems for command-line options when scripts call
other programs, etc.

It's one of the reasons I think "-c" is so useful. Because it Just Works
for the whole environment of the command you are invoking, without
having to worry about whether it's "git fetch" itself, or "git remote"
calling "git fetch", or a script calling "git fetch", or whatever.

Of course there are downsides, too. If you're a script who really
_doesn't_ want to propagate that option to your children, you have to
take an explicit step to prevent it. But I think it helps more often
than hurts.

I think there are other options that would benefit, too. For example,
the way we handle "--progress", "--verbose", and "--quiet" is a bit of a
mess. All of our programs understand these concepts to some degree, and
there have been numerous bugs involve in propagating their values to
sub-programs. Having "$GIT_VERBOSITY_LEVEL" in the environment would
probably make this simpler and more consistent.

But you'll note that this email isn't accompanied by any patches. This
is philosophical, and I'm not sure it's worth the effort to transition
things like verbosity at this point. So you may take it with the
appropriate grain of salt.  And especially I don't actually care all
that much about submodules myself, so I am fine with whatever people who
_are_ actively using and developing them decide.

-Peff

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Jeff King]

On Thu, Apr 28, 2016 at 12:28:21PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > It's definitely sufficient, it's just annoying if a user shows up every
> > week and says "I want X.Y", and then somebody else shows up a week later
> > and says "I want X.Z".
> >
> > Are we serving any purpose in vetting each one (and if so, what)?
> 
> Personally I do not think we would need to filter _anything_ if we
> can tell that the user directly said
> 
> 	git -c var1=val1 -c var2=val2 $cmd ...
> 
> and "git $cmd" ended up needing to spawn another "git" subcommand,
> possibly in some other repository (i.e. "$cmd" in this case is
> likely to be "submodule", but in principle it does not have to be).
> If the user somehow gives variables like core.worktree that are
> inappropriate to be applied across repositories, that's user's
> problem, i.e. "don't do it then if it hurts".

Right, we are talking about that direct case here. And any time our
filter heuristic lets something through, it is probably "if it hurts
don't do it" as the worst case.

So I think the only two cases worth filtering are:

  1. Ones where we _know_ that the config is nonsense to pass along,
     _and_ where a user might conceivably make use of the
     just-the-top-level version of it (core.worktree
     comes to mind, though of course they are probably better served by
     "--work-tree" in such a case).

  2. An option where we think there may be some security implication.
     Setting "http.sslverify" to false does have some security
     implications ("oops, I only meant to turn off verification for the
     root repo, and I got MiTM-attacked for the submodules!"). But it's
     so obscure and unlikely that I think the benefit outweighs it.

     And I can't think of any other cases whose security implications
     aren't similarly unlikely. But I haven't carefully gone down the
     list (and as I said, I'd be hesitant to support a blacklist until
     _somebody_ takes the time to do so).

> If we are doing any filtering, however, it is always hard, if not
> impossible, to take away what we originally granted, even by
> mistake, for any reason, even for correctness or for security, in a
> later release.

Yep, agreed.

I am OK staying with a whitelist. But I think we should be fairly
lenient in whitelisting hierarchies that people have a use for, and
which do not violate (1) or (2) above.

> We probably could sidestep it by introducing an end-user
> configurable "whitelist" somewhere.

Ugh. Please no. I do not want to have to think about explaining to
somebody that they can accomplish what they want with submodules, but
only by pre-configuring their ~/.gitconfig to allow certain keys so that
they can pass the appropriate config on the command line.

-Peff

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Stefan Beller]

On Thu, Apr 28, 2016 at 2:00 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Apr 28, 2016 at 12:28:21PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > It's definitely sufficient, it's just annoying if a user shows up every
>> > week and says "I want X.Y", and then somebody else shows up a week later
>> > and says "I want X.Z".
>> >
>> > Are we serving any purpose in vetting each one (and if so, what)?
>>
>> Personally I do not think we would need to filter _anything_ if we
>> can tell that the user directly said
>>
>>       git -c var1=val1 -c var2=val2 $cmd ...
>>
>> and "git $cmd" ended up needing to spawn another "git" subcommand,
>> possibly in some other repository (i.e. "$cmd" in this case is
>> likely to be "submodule", but in principle it does not have to be).
>> If the user somehow gives variables like core.worktree that are
>> inappropriate to be applied across repositories, that's user's
>> problem, i.e. "don't do it then if it hurts".
>
> Right, we are talking about that direct case here. And any time our
> filter heuristic lets something through, it is probably "if it hurts
> don't do it" as the worst case.
>
> So I think the only two cases worth filtering are:
>
>   1. Ones where we _know_ that the config is nonsense to pass along,
>      _and_ where a user might conceivably make use of the
>      just-the-top-level version of it (core.worktree
>      comes to mind, though of course they are probably better served by
>      "--work-tree" in such a case).

My gut reaction to this:
In this specific case I would rather error out, as you never want to have
core.worktree to point at the same dir for all of the repo and submodules.

Thinking about it further, I am not so sure any more.
(What if you have multiple submodules tracking the same project
and you want to see each submodule version with the one worktree you point to?
Highly unlikely edge case, but it voids the /never/ assumption of my
gut reaction)


>
>   2. An option where we think there may be some security implication.
>      Setting "http.sslverify" to false does have some security
>      implications ("oops, I only meant to turn off verification for the
>      root repo, and I got MiTM-attacked for the submodules!"). But it's
>      so obscure and unlikely that I think the benefit outweighs it.
>
>      And I can't think of any other cases whose security implications
>      aren't similarly unlikely. But I haven't carefully gone down the
>      list (and as I said, I'd be hesitant to support a blacklist until
>      _somebody_ takes the time to do so).

I think in this case, it is more likely for the user to say:

    I know my ssl is so borked, so I want all traffic with no ssl
(including submodules).

>
>> If we are doing any filtering, however, it is always hard, if not
>> impossible, to take away what we originally granted, even by
>> mistake, for any reason, even for correctness or for security, in a
>> later release.
>
> Yep, agreed.
>
> I am OK staying with a whitelist. But I think we should be fairly
> lenient in whitelisting hierarchies that people have a use for, and
> which do not violate (1) or (2) above.
>
>> We probably could sidestep it by introducing an end-user
>> configurable "whitelist" somewhere.
>
> Ugh. Please no. I do not want to have to think about explaining to
> somebody that they can accomplish what they want with submodules, but
> only by pre-configuring their ~/.gitconfig to allow certain keys so that
> they can pass the appropriate config on the command line.

I view the whitelist more like an "emergency knob to turn, because the
developers did it wrong and I want it now". the general case should be
covered by a mechanism we provide?

Thanks,
Stefan

>
> -Peff

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Jeff King]

On Thu, Apr 28, 2016 at 02:08:43PM -0700, Stefan Beller wrote:

> >   1. Ones where we _know_ that the config is nonsense to pass along,
> >      _and_ where a user might conceivably make use of the
> >      just-the-top-level version of it (core.worktree
> >      comes to mind, though of course they are probably better served by
> >      "--work-tree" in such a case).
> 
> My gut reaction to this:
> In this specific case I would rather error out, as you never want to have
> core.worktree to point at the same dir for all of the repo and submodules.

But then you're erroring out on a case that currently works today (we
apply core.worktree to the root repo, and ignore it for the others),
which I think is worth avoiding.

> Thinking about it further, I am not so sure any more.
> (What if you have multiple submodules tracking the same project
> and you want to see each submodule version with the one worktree you point to?
> Highly unlikely edge case, but it voids the /never/ assumption of my
> gut reaction)

I think this falls in my "nonsense" category. Especially when there are
other ways to handle it (e.g., by asking "git submodule foreach" to look
at each directory).

> I view the whitelist more like an "emergency knob to turn, because the
> developers did it wrong and I want it now". the general case should be
> covered by a mechanism we provide?

I think the emergency knob is "visit each submodule individually if you
have want to do something clever in each one" (or "ask git not to
recurse into submodules" for the opposite effect). The use cases I have
seen discussed are:

  1. Convenience. People expect to set some global-ish config like
     credential.helper, and have it applied uniformly.

  2. Inserting config into awkward parts of the call chain. E.g., I
     think Lars is mostly interested in speeding up clones where the
     submodules have a lot of slow clean/smudge filters. So he wants to
     disable "filter.*" _just_ for the clone, but there's no easy way to
     intercept the clone step of each submodule, stop, and the run the
     individual checkout with "git -c filter.foo.smudge=". I'm sure
     there's probably a way to hack it with plumbing, but it fails in
     "convenience".

-Peff

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Johannes Schindelin]

Hi Peff,

On Thu, 28 Apr 2016, Jeff King wrote:

> On Thu, Apr 28, 2016 at 12:28:21PM -0700, Junio C Hamano wrote:
> 
> > Jeff King <peff@peff.net> writes:
> > 
> > > It's definitely sufficient, it's just annoying if a user shows up
> > > every week and says "I want X.Y", and then somebody else shows up a
> > > week later and says "I want X.Z".
> > >
> > > Are we serving any purpose in vetting each one (and if so, what)?
> > 
> > Personally I do not think we would need to filter _anything_ if we can
> > tell that the user directly said
> > 
> > 	git -c var1=val1 -c var2=val2 $cmd ...
> > 
> > and "git $cmd" ended up needing to spawn another "git" subcommand,
> > possibly in some other repository (i.e. "$cmd" in this case is likely
> > to be "submodule", but in principle it does not have to be).  If the
> > user somehow gives variables like core.worktree that are inappropriate
> > to be applied across repositories, that's user's problem, i.e. "don't
> > do it then if it hurts".
> 
> Right, we are talking about that direct case here. And any time our
> filter heuristic lets something through, it is probably "if it hurts
> don't do it" as the worst case.

The more I think about it, I actually think that we do the user a *really*
great disservice by filtering the CONFIG_DATA_ENVIRONMENT. If I call

	git -c ... $cmd

and that configuration is *not* picked up, it is much worse than letting
users shoot themselves in their own feet by specifying config settings
that are *prone* to wreak havoc.

> So I think the only two cases worth filtering are:
> 
>   1. Ones where we _know_ that the config is nonsense to pass along,
>      _and_ where a user might conceivably make use of the
>      just-the-top-level version of it (core.worktree
>      comes to mind, though of course they are probably better served by
>      "--work-tree" in such a case).
>   2. An option where we think there may be some security implication.
>      Setting "http.sslverify" to false does have some security
>      implications ("oops, I only meant to turn off verification for the
>      root repo, and I got MiTM-attacked for the submodules!"). But it's
>      so obscure and unlikely that I think the benefit outweighs it.

I can see that happening when somebody calls an alias with `git -c ...`
and that alias performs actions in the top-level project as well as all
submodules.

But. Do we really have to be "Big Daddy" for users who do that?

> I am OK staying with a whitelist.

Me, too. But I am even more in favor of abandoning this "we know what is
good for you" approach, i.e. that whitelist that filters
CONFIG_DATA_ENVIRONMENT.

Ciao,
Dscho

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Jeff King]

On Fri, Apr 29, 2016 at 02:29:25PM +0200, Johannes Schindelin wrote:

> The more I think about it, I actually think that we do the user a *really*
> great disservice by filtering the CONFIG_DATA_ENVIRONMENT. If I call
> 
> 	git -c ... $cmd
> 
> and that configuration is *not* picked up, it is much worse than letting
> users shoot themselves in their own feet by specifying config settings
> that are *prone* to wreak havoc.

That's a good point. For every "whoops, I didn't mean for this to kick
in for the submodule!" there is an equal and opposite "whoops, this
needed to kick in for the submodule!" case (e.g., instead of
over-reaching turning http.sslverify off, you might be trying to turn it
_on_ and fail to apply it in all cases).

And making the rule "-c applies to all sub-processes, period" is much
simpler to explain.

(Though of course it is still not entirely true. We clear the config
 when accessing another repository as a remote for fetching or pushing.
 But a good chunk of that is simply for consistency of all remotes, as
 we do not pass the environment across TCP sessions).

> > So I think the only two cases worth filtering are:
> > 
> >   1. Ones where we _know_ that the config is nonsense to pass along,
> >      _and_ where a user might conceivably make use of the
> >      just-the-top-level version of it (core.worktree
> >      comes to mind, though of course they are probably better served by
> >      "--work-tree" in such a case).
> >   2. An option where we think there may be some security implication.
> >      Setting "http.sslverify" to false does have some security
> >      implications ("oops, I only meant to turn off verification for the
> >      root repo, and I got MiTM-attacked for the submodules!"). But it's
> >      so obscure and unlikely that I think the benefit outweighs it.
> 
> I can see that happening when somebody calls an alias with `git -c ...`
> and that alias performs actions in the top-level project as well as all
> submodules.
> 
> But. Do we really have to be "Big Daddy" for users who do that?

I have more sympathy for cases that full under (1) above, just because
they currently work _now_. So it's possible somebody is currently doing
a thing that makes sense under the current rules, but will involve
foot-shooting under the new version.

> > I am OK staying with a whitelist.
> 
> Me, too. But I am even more in favor of abandoning this "we know what is
> good for you" approach, i.e. that whitelist that filters
> CONFIG_DATA_ENVIRONMENT.

I could live with that, too. I've added Jens to the cc; his preference
for not blindly sharing config to submodules is part of what influenced
me in the original discussion.

-Peff

[1] http://thread.gmane.org/gmane.comp.version-control.git/264840

    I think that's the right link, which I dug out of my
    <20160219043019.GA14764@sigill.intra.peff.net> from a few months
    ago. Gmane seems to be down.

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Jeff King]

On Thu, Apr 28, 2016 at 02:19:37PM +0200, Johannes Schindelin wrote:

> > Should we consider just white-listing all of "http.*"?
> > 
> > That would help other cases which have come up, like:
> > 
> >   http://thread.gmane.org/gmane.comp.version-control.git/264840
> > 
> > which wants to turn off http.sslverify. That would mean it turns off for
> > every submodule, too, but if you want to be choosy about your http
> > variables, you should be using the "http.$URL.sslverify" form, to only
> > affect specific servers (whether they are in submodules or not).
> 
> I considered that, and thought that it might be dangerous, what with me
> not vetting carefully which http.* variables are safe to pass on to the
> submodules' update and which are not.

BTW, just in case you or anybody else ends up playing around with this
and finds your tests do not work as expected: the config pass-through
feature is somewhat broken for anything except cloning. I just posted
some fixes in:

  http://thread.gmane.org/gmane.comp.version-control.git/292466/focus=292875

-Peff

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Junio C Hamano]

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

> -     if (starts_with(var, "credential."))
> +     if (starts_with(var, "credential.") ||
> +                     (starts_with(var, "http.") &&
> +                      ends_with(var, ".extraheader")))

I know you are fond of indenting with HT without aligning things,
but this is going too far in the quest of making the code
unreadable.

        if (starts_with(var, "credential.") ||
            (starts_with(var, "http.") && ends_with(var, ".extraheader")))

would make iteasier to see what are the top-level items (there are two)
and how they are related (just one of them needs to be satisfied).

Assuming that we will discover more variables that can be safely
passed, I'd rather see the above written like this, though:

        if (starts_with(var, "credential."))
                return 1;
        if (starts_with(var, "http.") && ends_with(var, ".extraheader"))
                return 1;

        return 0;

Or even something along this line:

        struct whitelist {
                const char *prefix;
                const char *suffix;
        } whitelist[] = {
                { "credential.", NULL },
                { "http.", ".extraheader" },
        };

        for (i = 0; i < ARRAY_SIZE(whitelist); i++) {
                struct whitelist *w = &whitelist[i];
                if ((!w->prefix || starts_with(var, w->prefix)) &&
                    (!w->suffix || ends_with(var, w->suffix)))
                        return 1;
        }
        return 0;

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Johannes Schindelin]

Hi Junio,

On Thu, 28 Apr 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > -     if (starts_with(var, "credential."))
> > +     if (starts_with(var, "credential.") ||
> > +                     (starts_with(var, "http.") &&
> > +                      ends_with(var, ".extraheader")))
> 
> I know you are fond of indenting with HT without aligning things,
> but this is going too far in the quest of making the code
> unreadable.

Hah. I am actually not fond of anything there, but I go with the default
in my vim when selecting lines and pressing the '=' key...

If you know off-hand how to teach my vim to use your preferred indenting,
I'll gladly just brow-beat it into submission.

>         if (starts_with(var, "credential.") ||
>             (starts_with(var, "http.") && ends_with(var, ".extraheader")))
> 
> would make iteasier to see what are the top-level items (there are two)
> and how they are related (just one of them needs to be satisfied).

Fine by me!

> Assuming that we will discover more variables that can be safely
> passed, I'd rather see the above written like this, though:
> 
>         if (starts_with(var, "credential."))
>                 return 1;
>         if (starts_with(var, "http.") && ends_with(var, ".extraheader"))
>                 return 1;
> 
>         return 0;
> 
> Or even something along this line:
> 
>         struct whitelist {
>                 const char *prefix;
>                 const char *suffix;
>         } whitelist[] = {
>                 { "credential.", NULL },
>                 { "http.", ".extraheader" },
>         };
> 
>         for (i = 0; i < ARRAY_SIZE(whitelist); i++) {
>                 struct whitelist *w = &whitelist[i];
>                 if ((!w->prefix || starts_with(var, w->prefix)) &&
>                     (!w->suffix || ends_with(var, w->suffix)))
>                         return 1;
>         }
>         return 0;

Iff. Iff we go with a white-list.

However, I think you did a really good job arguing that the
CONFIG_DATA_ENVIRONMENT filtering is, in fact, overzealous.

Just let me know what to go with, and I'll update the patch accordingly.

Ciao,
Dscho

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Johannes Schindelin]

Hi Junio,

On Fri, 29 Apr 2016, Johannes Schindelin wrote:

> On Thu, 28 Apr 2016, Junio C Hamano wrote:
> 
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> > 
> > > -     if (starts_with(var, "credential."))
> > > +     if (starts_with(var, "credential.") ||
> > > +                     (starts_with(var, "http.") &&
> > > +                      ends_with(var, ".extraheader")))
> > 
> > I know you are fond of indenting with HT without aligning things,
> > but this is going too far in the quest of making the code
> > unreadable.
> 
> Hah. I am actually not fond of anything there, but I go with the default
> in my vim when selecting lines and pressing the '=' key...
> 
> If you know off-hand how to teach my vim to use your preferred indenting,
> I'll gladly just brow-beat it into submission.

For the record, I think the default in vim is to indent two tabs after an
unclosed parenthesis in an `if` line (the "(2s" just below
http://vimdoc.sourceforge.net/htmldoc/indent.html#javascript-indenting in
the default cinoptions). The "(0" setting fixes this, I think, so I forced
my vim to use

	cinoptions=>s,e0,n0,f0,{0,}0,^0,L-1,:s,=s,l0,b0,gs,hs,ps,ts,is,+s,c3,C0,/0,(0,us,U0,w0,W0,m0,j0,J0,)20,*70,#0

from now on.

Ciao,
Dscho

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Jeff King]

On Fri, Apr 29, 2016 at 02:48:44PM +0200, Johannes Schindelin wrote:

> > If you know off-hand how to teach my vim to use your preferred indenting,
> > I'll gladly just brow-beat it into submission.
> 
> For the record, I think the default in vim is to indent two tabs after an
> unclosed parenthesis in an `if` line (the "(2s" just below
> http://vimdoc.sourceforge.net/htmldoc/indent.html#javascript-indenting in
> the default cinoptions). The "(0" setting fixes this, I think, so I forced
> my vim to use
> 
> 	cinoptions=>s,e0,n0,f0,{0,}0,^0,L-1,:s,=s,l0,b0,gs,hs,ps,ts,is,+s,c3,C0,/0,(0,us,U0,w0,W0,m0,j0,J0,)20,*70,#0
> 
> from now on.

Yeah, "(0" is the only non-default cinoption I use for git.  Other than
that, everything fits our guidelines (I guess I explicitly make sure
shiftwidth and tabstops are all at 8, but I think those are the
defaults, too).

-Peff

Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Johannes Schindelin]

Hi Peff,

On Fri, 29 Apr 2016, Jeff King wrote:

> On Fri, Apr 29, 2016 at 02:48:44PM +0200, Johannes Schindelin wrote:
> 
> > > If you know off-hand how to teach my vim to use your preferred indenting,
> > > I'll gladly just brow-beat it into submission.
> > 
> > For the record, I think the default in vim is to indent two tabs after an
> > unclosed parenthesis in an `if` line (the "(2s" just below
> > http://vimdoc.sourceforge.net/htmldoc/indent.html#javascript-indenting in
> > the default cinoptions). The "(0" setting fixes this, I think, so I forced
> > my vim to use
> > 
> > 	cinoptions=>s,e0,n0,f0,{0,}0,^0,L-1,:s,=s,l0,b0,gs,hs,ps,ts,is,+s,c3,C0,/0,(0,us,U0,w0,W0,m0,j0,J0,)20,*70,#0
> > 
> > from now on.
> 
> Yeah, "(0" is the only non-default cinoption I use for git.  Other than
> that, everything fits our guidelines (I guess I explicitly make sure
> shiftwidth and tabstops are all at 8, but I think those are the
> defaults, too).

Thanks for the confirmation!

Ciao,
Dscho

[PATCH v6 0/2] Add support for sending additional HTTP headers

[Johannes Schindelin]

My use case is an army of build agents that need only limited and
selective access to otherwise private repositories.

v6 supports submodules better by allowing

	git -c http.extraheader submodule update

to work as one would expect intuitively.

Johannes Schindelin (2):
  http: support sending custom HTTP headers
  submodule: pass on http.extraheader config settings

 Documentation/config.txt    |  6 ++++++
 builtin/submodule--helper.c |  3 ++-
 http-push.c                 | 10 +++++-----
 http.c                      | 35 ++++++++++++++++++++++++++++++++---
 http.h                      |  1 +
 remote-curl.c               |  4 ++--
 t/lib-httpd/apache.conf     |  8 ++++++++
 t/t5551-http-fetch-smart.sh | 16 ++++++++++++++++
 8 files changed, 72 insertions(+), 11 deletions(-)

Interdiff vs v5:

 diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
 index b338f93..789e081 100644
 --- a/builtin/submodule--helper.c
 +++ b/builtin/submodule--helper.c
 @@ -128,8 +128,7 @@ static int module_name(int argc, const char **argv, const char *prefix)
  static int submodule_config_ok(const char *var)
  {
  	if (starts_with(var, "credential.") ||
 -			(starts_with(var, "http.") &&
 -			 ends_with(var, ".extraheader")))
 +	    (starts_with(var, "http.") && ends_with(var, ".extraheader")))
  		return 1;
  	return 0;
  }

-- 
2.8.1.306.gff998f2

[PATCH v6 1/2] http: support sending custom HTTP headers

[Johannes Schindelin]

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

Note that `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` takes only
a single list, overriding any previous call. This means we have to
collect _all_ of the headers we want to use into a single list, and
feed it to cURL in one shot. Since we already unconditionally set a
"pragma" header when initializing the curl handles, we can add our new
headers to that list.

For callers which override the default header list (like probe_rpc),
we provide `http_copy_default_headers()` so they can do the same
trick.

Big thanks to Jeff King and Junio Hamano for their outstanding help and
patient reviews.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt    |  6 ++++++
 http-push.c                 | 10 +++++-----
 http.c                      | 35 ++++++++++++++++++++++++++++++++---
 http.h                      |  1 +
 remote-curl.c               |  4 ++--
 t/lib-httpd/apache.conf     |  8 ++++++++
 t/t5551-http-fetch-smart.sh |  7 +++++++
 7 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50..c7bbe98 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.  To allow overriding the settings inherited from the system
+	config, an empty value will reset the extra headers to the empty list.
+
 http.cookieFile::
 	File containing previously stored cookie lines which should be used
 	in the Git http session, if they match the server. The file format
diff --git a/http-push.c b/http-push.c
index bd60668..ae2b7f1 100644
--- a/http-push.c
+++ b/http-push.c
@@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
 static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
 {
 	struct strbuf buf = STRBUF_INIT;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 
 	if (options & DAV_HEADER_IF) {
 		strbuf_addf(&buf, "If: (<%s>)", lock->token);
@@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
 static void start_move(struct transfer_request *request)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 
 	slot = get_active_slot();
 	slot->callback_func = process_response;
@@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	char *ep;
 	char timeout_header[25];
 	struct remote_lock *lock = NULL;
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	char *escaped;
 
@@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	struct remote_ls_ctx ls;
 
@@ -1204,7 +1204,7 @@ static int locking_available(void)
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers = NULL;
+	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	int lock_flags = 0;
 	char *escaped;
diff --git a/http.c b/http.c
index 4304b80..985b995 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,19 @@ static int http_options(const char *var, const char *value, void *cb)
 #endif
 	}
 
+	if (!strcmp("http.extraheader", var)) {
+		if (!value) {
+			return config_error_nonbool(var);
+		} else if (!*value) {
+			curl_slist_free_all(extra_http_headers);
+			extra_http_headers = NULL;
+		} else {
+			extra_http_headers =
+				curl_slist_append(extra_http_headers, value);
+		}
+		return 0;
+	}
+
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
 }
@@ -678,8 +692,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_copy_default_headers(),
+		"Pragma: no-cache");
+	no_pragma_header = curl_slist_append(http_copy_default_headers(),
+		"Pragma:");
 
 #ifdef USE_CURL_MULTI
 	{
@@ -765,6 +781,9 @@ void http_cleanup(void)
 #endif
 	curl_global_cleanup();
 
+	curl_slist_free_all(extra_http_headers);
+	extra_http_headers = NULL;
+
 	curl_slist_free_all(pragma_header);
 	pragma_header = NULL;
 
@@ -1163,6 +1182,16 @@ int run_one_slot(struct active_request_slot *slot,
 	return handle_curl_result(results);
 }
 
+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;
+}
+
 static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
 {
 	char *ptr;
@@ -1380,7 +1409,7 @@ static int http_request(const char *url,
 {
 	struct active_request_slot *slot;
 	struct slot_results results;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	struct strbuf buf = STRBUF_INIT;
 	const char *accept_language;
 	int ret;
diff --git a/http.h b/http.h
index 4ef4bbd..36f558b 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(void);
 
 extern long int git_curl_ipresolve;
 extern int active_requests;
diff --git a/remote-curl.c b/remote-curl.c
index 15e48e2..672b382 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot,
 static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	struct strbuf buf = STRBUF_INIT;
 	int err;
 
@@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 static int post_rpc(struct rpc_state *rpc)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = NULL;
+	struct curl_slist *headers = http_copy_default_headers();
 	int use_gzip = rpc->gzip_request;
 	char *gzip_body = NULL;
 	size_t gzip_size = 0;
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 9317ba0..b8ed96f 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -102,6 +102,14 @@ Alias /auth/dumb/ www/auth/dumb/
 	SetEnv GIT_HTTP_EXPORT_ALL
 	Header set Set-Cookie name=value
 </LocationMatch>
+<LocationMatch /smart_headers/>
+	<RequireAll>
+		Require expr %{HTTP:x-magic-one} == 'abra'
+		Require expr %{HTTP:x-magic-two} == 'cadabra'
+	</RequireAll>
+	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+	SetEnv GIT_HTTP_EXPORT_ALL
+</LocationMatch>
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 ScriptAlias /error/ error.sh/
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 58207d8..e44fe72 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -282,5 +282,12 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
 	test_line_count = 100000 tags
 '
 
+test_expect_success 'custom http headers' '
+	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
+	git -c http.extraheader="x-magic-one: abra" \
+	    -c http.extraheader="x-magic-two: cadabra" \
+	    fetch "$HTTPD_URL/smart_headers/repo.git"
+'
+
 stop_httpd
 test_done
-- 
2.8.1.306.gff998f2

Re: [PATCH v6 1/2] http: support sending custom HTTP headers

[Lars Schneider]

On 04 May 2016, at 08:14, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

> 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
> 
> Note that `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` takes only
> a single list, overriding any previous call. This means we have to
> collect _all_ of the headers we want to use into a single list, and
> feed it to cURL in one shot. Since we already unconditionally set a
> "pragma" header when initializing the curl handles, we can add our new
> headers to that list.
> 
> For callers which override the default header list (like probe_rpc),
> we provide `http_copy_default_headers()` so they can do the same
> trick.
> 
> Big thanks to Jeff King and Junio Hamano for their outstanding help and
> patient reviews.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Documentation/config.txt    |  6 ++++++
> http-push.c                 | 10 +++++-----
> http.c                      | 35 ++++++++++++++++++++++++++++++++---
> http.h                      |  1 +
> remote-curl.c               |  4 ++--
> t/lib-httpd/apache.conf     |  8 ++++++++
> t/t5551-http-fetch-smart.sh |  7 +++++++
> 7 files changed, 61 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 42d2b50..c7bbe98 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.  To allow overriding the settings inherited from the system
> +	config, an empty value will reset the extra headers to the empty list.
> +
> http.cookieFile::
> 	File containing previously stored cookie lines which should be used
> 	in the Git http session, if they match the server. The file format
> diff --git a/http-push.c b/http-push.c
> index bd60668..ae2b7f1 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
> static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
> {
> 	struct strbuf buf = STRBUF_INIT;
> -	struct curl_slist *dav_headers = NULL;
> +	struct curl_slist *dav_headers = http_copy_default_headers();
> 
> 	if (options & DAV_HEADER_IF) {
> 		strbuf_addf(&buf, "If: (<%s>)", lock->token);
> @@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
> static void start_move(struct transfer_request *request)
> {
> 	struct active_request_slot *slot;
> -	struct curl_slist *dav_headers = NULL;
> +	struct curl_slist *dav_headers = http_copy_default_headers();
> 
> 	slot = get_active_slot();
> 	slot->callback_func = process_response;
> @@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
> 	char *ep;
> 	char timeout_header[25];
> 	struct remote_lock *lock = NULL;
> -	struct curl_slist *dav_headers = NULL;
> +	struct curl_slist *dav_headers = http_copy_default_headers();
> 	struct xml_ctx ctx;
> 	char *escaped;
> 
> @@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
> 	struct slot_results results;
> 	struct strbuf in_buffer = STRBUF_INIT;
> 	struct buffer out_buffer = { STRBUF_INIT, 0 };
> -	struct curl_slist *dav_headers = NULL;
> +	struct curl_slist *dav_headers = http_copy_default_headers();
> 	struct xml_ctx ctx;
> 	struct remote_ls_ctx ls;
> 
> @@ -1204,7 +1204,7 @@ static int locking_available(void)
> 	struct slot_results results;
> 	struct strbuf in_buffer = STRBUF_INIT;
> 	struct buffer out_buffer = { STRBUF_INIT, 0 };
> -	struct curl_slist *dav_headers = NULL;
> +	struct curl_slist *dav_headers = http_copy_default_headers();
> 	struct xml_ctx ctx;
> 	int lock_flags = 0;
> 	char *escaped;
> diff --git a/http.c b/http.c
> index 4304b80..985b995 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,19 @@ static int http_options(const char *var, const char *value, void *cb)
> #endif
> 	}
> 
> +	if (!strcmp("http.extraheader", var)) {
> +		if (!value) {
> +			return config_error_nonbool(var);
> +		} else if (!*value) {
> +			curl_slist_free_all(extra_http_headers);
> +			extra_http_headers = NULL;
> +		} else {
> +			extra_http_headers =
> +				curl_slist_append(extra_http_headers, value);
> +		}
> +		return 0;
> +	}
> +
> 	/* Fall back on the default ones */
> 	return git_default_config(var, value, cb);
> }
> @@ -678,8 +692,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_copy_default_headers(),
> +		"Pragma: no-cache");
> +	no_pragma_header = curl_slist_append(http_copy_default_headers(),
> +		"Pragma:");
> 
> #ifdef USE_CURL_MULTI
> 	{
> @@ -765,6 +781,9 @@ void http_cleanup(void)
> #endif
> 	curl_global_cleanup();
> 
> +	curl_slist_free_all(extra_http_headers);
> +	extra_http_headers = NULL;
> +
> 	curl_slist_free_all(pragma_header);
> 	pragma_header = NULL;
> 
> @@ -1163,6 +1182,16 @@ int run_one_slot(struct active_request_slot *slot,
> 	return handle_curl_result(results);
> }
> 
> +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;
> +}
> +
> static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
> {
> 	char *ptr;
> @@ -1380,7 +1409,7 @@ static int http_request(const char *url,
> {
> 	struct active_request_slot *slot;
> 	struct slot_results results;
> -	struct curl_slist *headers = NULL;
> +	struct curl_slist *headers = http_copy_default_headers();
> 	struct strbuf buf = STRBUF_INIT;
> 	const char *accept_language;
> 	int ret;
> diff --git a/http.h b/http.h
> index 4ef4bbd..36f558b 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(void);
> 
> extern long int git_curl_ipresolve;
> extern int active_requests;
> diff --git a/remote-curl.c b/remote-curl.c
> index 15e48e2..672b382 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot,
> static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
> {
> 	struct active_request_slot *slot;
> -	struct curl_slist *headers = NULL;
> +	struct curl_slist *headers = http_copy_default_headers();
> 	struct strbuf buf = STRBUF_INIT;
> 	int err;
> 
> @@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
> static int post_rpc(struct rpc_state *rpc)
> {
> 	struct active_request_slot *slot;
> -	struct curl_slist *headers = NULL;
> +	struct curl_slist *headers = http_copy_default_headers();
> 	int use_gzip = rpc->gzip_request;
> 	char *gzip_body = NULL;
> 	size_t gzip_size = 0;
> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index 9317ba0..b8ed96f 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -102,6 +102,14 @@ Alias /auth/dumb/ www/auth/dumb/
> 	SetEnv GIT_HTTP_EXPORT_ALL
> 	Header set Set-Cookie name=value
> </LocationMatch>
> +<LocationMatch /smart_headers/>
> +	<RequireAll>
> +		Require expr %{HTTP:x-magic-one} == 'abra'
> +		Require expr %{HTTP:x-magic-two} == 'cadabra'
> +	</RequireAll>

I think "<RequireAll>" depends on mod_authz_core which is only
available in Apache HTTPD 2.3 or later [1].

Right now the test only checks if Apache version greater 2
is installed. Should we guard this test with a special version
check? Or do you see a way to check the magic values without
"<RequireAll>"?

I only noticed this because I enabled these tests on Travis-CI
and the Travis Linux box comes with Apache 2.2.22 installed...

- Lars

[1] https://httpd.apache.org/docs/trunk/mod/mod_authz_core.html

> +	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
> +	SetEnv GIT_HTTP_EXPORT_ALL
> +</LocationMatch>
> ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
> ScriptAlias /broken_smart/ broken-smart-http.sh/
> ScriptAlias /error/ error.sh/
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 58207d8..e44fe72 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -282,5 +282,12 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
> 	test_line_count = 100000 tags
> '
> 
> +test_expect_success 'custom http headers' '
> +	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
> +	git -c http.extraheader="x-magic-one: abra" \
> +	    -c http.extraheader="x-magic-two: cadabra" \
> +	    fetch "$HTTPD_URL/smart_headers/repo.git"
> +'
> +
> stop_httpd
> test_done
> -- 
> 2.8.1.306.gff998f2
> 
> 
> --
> 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

Re: [PATCH v6 1/2] http: support sending custom HTTP headers

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

[administrivia: next time please snip the 224 lines you are not
responding to when commenting on only five lines].

>> +<LocationMatch /smart_headers/>
>> +	<RequireAll>
>> +		Require expr %{HTTP:x-magic-one} == 'abra'
>> +		Require expr %{HTTP:x-magic-two} == 'cadabra'
>> +	</RequireAll>
>
> I think "<RequireAll>" depends on mod_authz_core which is only
> available in Apache HTTPD 2.3 or later [1].
>
> Right now the test only checks if Apache version greater 2
> is installed. Should we guard this test with a special version
> check? Or do you see a way to check the magic values without
> "<RequireAll>"?
>
> I only noticed this because I enabled these tests on Travis-CI
> and the Travis Linux box comes with Apache 2.2.22 installed...
>
> - Lars
>
> [1] https://httpd.apache.org/docs/trunk/mod/mod_authz_core.html


2.2 is 12 years old, but the last update is last summer, which is
still fairly recent.  It would be really nice if we can accomodate
2.2.x series.

It is only test, so it probably is OK to skip it in the worst case,
but on the other hand, the point of having CI is so that we can
catch bugs that would manifest only in tests that normal people
would not run regularly, so...

Thanks. Real-world value of having Travis canary is seen here.

Re: [PATCH v6 1/2] http: support sending custom HTTP headers

[Jeff King]

On Thu, May 05, 2016 at 09:10:21PM +0200, Lars Schneider wrote:

> > +<LocationMatch /smart_headers/>
> > +	<RequireAll>
> > +		Require expr %{HTTP:x-magic-one} == 'abra'
> > +		Require expr %{HTTP:x-magic-two} == 'cadabra'
> > +	</RequireAll>
> 
> I think "<RequireAll>" depends on mod_authz_core which is only
> available in Apache HTTPD 2.3 or later [1].
> 
> Right now the test only checks if Apache version greater 2
> is installed. Should we guard this test with a special version
> check? Or do you see a way to check the magic values without
> "<RequireAll>"?

I think you can get rid of RequireAll with:

 Require expr %{HTTP:x-magic-one} == 'abra' && %{HTTP:x-magic-two} == 'cadabra'

But I am also not sure that "expr" existed in Apache 2.2.

I think the older way of checking headers was to do some trickery with
RewriteCond; I tried briefly to make that work when I wrote the test,
but never did (but I'm far from an expert in Apache; the only reason I
have touched it in the last 15 years is for Git's test suite).

The nuclear option would be to put a shell script between Apache and
git-http-backend that checks for those headers (I suspect we'd still
need some magic in the Apache config to pass the headers out in the
environment, though).

-Peff

[PATCH v6 2/2] submodule: pass on http.extraheader config settings

[Johannes Schindelin]

To support this developer's use case of allowing build agents token-based
access to private repositories, we introduced the http.extraheader
feature, allowing extra HTTP headers to be sent along with every HTTP
request.

This patch allows us to configure these extra HTTP headers for use with
`git submodule update`, too. It requires somewhat special handling:
submodules do not share the parent project's config. It would be
incorrect to simply reuse that specific part of the parent's config.
Instead, the config option needs to be specified on the command-line or
in ~/.gitconfig or friends.

Example: git -c http.extraheader="Secret: Sauce" submodule update --init

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/submodule--helper.c |  3 ++-
 t/t5551-http-fetch-smart.sh | 11 ++++++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3bd6883..789e081 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -127,7 +127,8 @@ static int module_name(int argc, const char **argv, const char *prefix)
  */
 static int submodule_config_ok(const char *var)
 {
-	if (starts_with(var, "credential."))
+	if (starts_with(var, "credential.") ||
+	    (starts_with(var, "http.") && ends_with(var, ".extraheader")))
 		return 1;
 	return 0;
 }
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index e44fe72..1794168 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -286,7 +286,16 @@ test_expect_success 'custom http headers' '
 	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
 	git -c http.extraheader="x-magic-one: abra" \
 	    -c http.extraheader="x-magic-two: cadabra" \
-	    fetch "$HTTPD_URL/smart_headers/repo.git"
+	    fetch "$HTTPD_URL/smart_headers/repo.git" &&
+	git update-index --add --cacheinfo 160000,$(git rev-parse HEAD),sub &&
+	git config -f .gitmodules submodule.sub.path sub &&
+	git config -f .gitmodules submodule.sub.url \
+		"$HTTPD_URL/smart_headers/repo.git" &&
+	git submodule init sub &&
+	test_must_fail git submodule update sub &&
+	git -c http.extraheader="x-magic-one: abra" \
+	    -c http.extraheader="x-magic-two: cadabra" \
+		submodule update sub
 '
 
 stop_httpd
-- 
2.8.1.306.gff998f2

Re: [PATCH v6 0/2] Add support for sending additional HTTP headers

[Jeff King]

On Wed, May 04, 2016 at 08:14:07AM +0200, Johannes Schindelin wrote:

> My use case is an army of build agents that need only limited and
> selective access to otherwise private repositories.
> 
> v6 supports submodules better by allowing
> 
> 	git -c http.extraheader submodule update
> 
> to work as one would expect intuitively.
> 
> Johannes Schindelin (2):
>   http: support sending custom HTTP headers

I think this one is in "next", with "will merge to master" in the latest
What's Cooking. So fortunately it does not have any changes in this
version of the series. :)

>   submodule: pass on http.extraheader config settings

IMHO this should come on top of jk/submodule-config-sanitize-fix (I was
surprised at first that your test worked at all, but that is because it
is using "clone", which is the one code path that works).

But I think we are waiting on going one of two paths:

  1. drop sanitizing entirely

  2. fix sanitizing and add more variables to it

If we go the route of (2), then we'd want my fix topic and this patch.
And if not, then we don't need any of it (just a patch dropping the
filtering, which AFAIK nobody has written yet).

-Peff

Re: [PATCH v6 0/2] Add support for sending additional HTTP headers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>>   submodule: pass on http.extraheader config settings
>
> IMHO this should come on top of jk/submodule-config-sanitize-fix (I was
> surprised at first that your test worked at all, but that is because it
> is using "clone", which is the one code path that works).

Yes.

> But I think we are waiting on going one of two paths:
>
>   1. drop sanitizing entirely
>
>   2. fix sanitizing and add more variables to it
>
> If we go the route of (2), then we'd want my fix topic and this patch.
> And if not, then we don't need any of it (just a patch dropping the
> filtering, which AFAIK nobody has written yet).

Doubly yes.  That is why I didn't pick up 2/2 in the previous round
and also jk/submodule-config-sanitize-fix is not in 'next' for the
same reason.

I agree with you that we have not yet reached concensus on which one
of the two we would want to take.  I was sort of surprised to see
2/2 sent again, after seeing that Dscho sounded strongly in favor of
not filtering the passed configuration variables, which would make
the patch unnecessary.

Re: [PATCH v6 0/2] Add support for sending additional HTTP headers

[Johannes Schindelin]

Hi Junio,

On Wed, 4 May 2016, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >>   submodule: pass on http.extraheader config settings
> >
> > IMHO this should come on top of jk/submodule-config-sanitize-fix (I was
> > surprised at first that your test worked at all, but that is because it
> > is using "clone", which is the one code path that works).
> 
> Yes.

Okay.

> > But I think we are waiting on going one of two paths:
> >
> >   1. drop sanitizing entirely
> >
> >   2. fix sanitizing and add more variables to it
> >
> > If we go the route of (2), then we'd want my fix topic and this patch.
> > And if not, then we don't need any of it (just a patch dropping the
> > filtering, which AFAIK nobody has written yet).
> 
> Doubly yes.  That is why I didn't pick up 2/2 in the previous round
> and also jk/submodule-config-sanitize-fix is not in 'next' for the
> same reason.

Okay. It was not clear to me that the indentation was not the reason it
was ignored.

> I agree with you that we have not yet reached concensus on which one
> of the two we would want to take.  I was sort of surprised to see
> 2/2 sent again, after seeing that Dscho sounded strongly in favor of
> not filtering the passed configuration variables, which would make
> the patch unnecessary.

Hah, my opinion matters after all.

Yes, I am in favor of not filtering any config settings passed through the
command-line.

But traditionally, I did not always get what I strongly preferred, and in
this case it is really important to me that support for `git -c
http.extraheader=... submodule ...` be part of an official Git version
soon.

I just *really* would hate for, say, 2.9.0 to be released with `git -c
http.extraheader=Hello:\ Junio submodule update --init` not working as
intended.

Ciao,
Dscho

Re: [PATCH v6 0/2] Add support for sending additional HTTP headers

[Junio C Hamano]

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

> Hi Junio,
>
> On Wed, 4 May 2016, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >>   submodule: pass on http.extraheader config settings
>> >
>> > IMHO this should come on top of jk/submodule-config-sanitize-fix (I was
>> > surprised at first that your test worked at all, but that is because it
>> > is using "clone", which is the one code path that works).
>> 
>> Yes.
>
> Okay.
>
>> > But I think we are waiting on going one of two paths:
>> >
>> >   1. drop sanitizing entirely
>> >
>> >   2. fix sanitizing and add more variables to it
>> >
>> > If we go the route of (2), then we'd want my fix topic and this patch.
>> > And if not, then we don't need any of it (just a patch dropping the
>> > filtering, which AFAIK nobody has written yet).
>> 
>> Doubly yes.  That is why I didn't pick up 2/2 in the previous round
>> and also jk/submodule-config-sanitize-fix is not in 'next' for the
>> same reason.
>
> Okay. It was not clear to me that the indentation was not the reason it
> was ignored.

It wasn't even ignored. I looked at it carefully, noticed that it
contradicts with what you said in a different message, and made a
concious decision to wait.

>> I agree with you that we have not yet reached concensus on which one
>> of the two we would want to take.  I was sort of surprised to see
>> 2/2 sent again, after seeing that Dscho sounded strongly in favor of
>> not filtering the passed configuration variables, which would make
>> the patch unnecessary.
>
> Hah, my opinion matters after all.

When your proposal ends up getting rejected, it is not because your
opinion does not matter.  In any case, for this one, the reason I
decided to wait until the "filter or not filter, and if filter, use
whitelist or blacklist" discussion settles does not depend whether I
happen to agreed with your preference (which is "not to filter").

That is, there is a difference between "I will not apply this ever,
because I know the outcome of the the other discussion already and
it will make this patch unnecessary", and "I cannot decide to apply
this yet, because this may be needed if the other discussion goes in
a certain way but this may turn out to be unnecessary if it goes in
another way."  And the topic branch having only the first one is
because this case was (and I think still is) the latter.

Re: [PATCH v6 0/2] Add support for sending additional HTTP headers

[Jeff King]

On Wed, May 04, 2016 at 02:26:18AM -0400, Jeff King wrote:

> >   submodule: pass on http.extraheader config settings
> 
> IMHO this should come on top of jk/submodule-config-sanitize-fix (I was
> surprised at first that your test worked at all, but that is because it
> is using "clone", which is the one code path that works).
> 
> But I think we are waiting on going one of two paths:
> 
>   1. drop sanitizing entirely
> 
>   2. fix sanitizing and add more variables to it
> 
> If we go the route of (2), then we'd want my fix topic and this patch.
> And if not, then we don't need any of it (just a patch dropping the
> filtering, which AFAIK nobody has written yet).

Actually, I think this last bit is not quite true. If we want to go back
to "nothing gets passed to submodules", we can drop all of my patches,
but I don't think anybody wants to do that.

But if we want "everything gets passed to submodules", then we do need
something like my patch series, because every use of local_repo_env
needs to be come "local_repo_env excluding GIT_CONFIG_PARAMETERS". I
don't think we want to simply drop that variable from local_repo_env
(which would also mean that it would be propagated to a local
git-upload-pack, for example, along with any third-party scripts that
use rev-parse --local-env-vars).

So I think we'd actually want my series as a preliminary fix, followed
by dropping the whitelist entirely on top of that, and then probably
simplifying the shell sanitize_submodule_env() on top of that (it would
be correct without the whitelist, but you can also trivially implement
it without having to call submodule--helper at all).

-Peff

[PATCH] submodule: stop sanitizing config options

[Jeff King]

[+cc Stefan and Jacob since this is really resuming that earlier thread]

On Wed, May 04, 2016 at 03:45:59AM -0400, Jeff King wrote:

> On Wed, May 04, 2016 at 02:26:18AM -0400, Jeff King wrote:
> 
> > >   submodule: pass on http.extraheader config settings
> > 
> > IMHO this should come on top of jk/submodule-config-sanitize-fix (I was
> > surprised at first that your test worked at all, but that is because it
> > is using "clone", which is the one code path that works).
> > 
> > But I think we are waiting on going one of two paths:
> > 
> >   1. drop sanitizing entirely
> > 
> >   2. fix sanitizing and add more variables to it
> > 
> > If we go the route of (2), then we'd want my fix topic and this patch.
> > And if not, then we don't need any of it (just a patch dropping the
> > filtering, which AFAIK nobody has written yet).
> 
> Actually, I think this last bit is not quite true. If we want to go back
> to "nothing gets passed to submodules", we can drop all of my patches,
> but I don't think anybody wants to do that.
> 
> But if we want "everything gets passed to submodules", then we do need
> something like my patch series, because every use of local_repo_env
> needs to be come "local_repo_env excluding GIT_CONFIG_PARAMETERS". I
> don't think we want to simply drop that variable from local_repo_env
> (which would also mean that it would be propagated to a local
> git-upload-pack, for example, along with any third-party scripts that
> use rev-parse --local-env-vars).
> 
> So I think we'd actually want my series as a preliminary fix, followed
> by dropping the whitelist entirely on top of that, and then probably
> simplifying the shell sanitize_submodule_env() on top of that (it would
> be correct without the whitelist, but you can also trivially implement
> it without having to call submodule--helper at all).

I think we'd actually do it all in one, and that patch looks something
like the one below (on top of jk/submodule-config-sanitize-fix).

I don't feel that strongly about going either direction with this, but I
figure it doesn't hurt to make the patch so we know what the actual
option looks like.

-- >8 --
Subject: [PATCH] submodule: stop sanitizing config options

The point of having a whitelist of command-line config
options to pass to submodules was two-fold:

  1. It prevented obvious nonsense like using core.worktree
     for multiple repos.

  2. It could prevent surprise when the user did not mean
     for the options to leak to the submodules (e.g.,
     http.sslverify=false).

For case 1, the answer is mostly "if it hurts, don't do
that". For case 2, we can note that any such example has a
matching inverted surprise (e.g., a user who meant
http.sslverify=true to apply everywhere, but it didn't).

So this whitelist is probably not giving us any benefit, and
is already creating a hassle as people propose things to put
on it. Let's just drop it entirely.

Note that we still need to keep a special code path for
"prepare the submodule environment", because we still have
to take care to pass through $GIT_CONFIG_PARAMETERS (and
block the rest of the repo-specific environment variables).

We can do this easily from within the submodule shell
script, which lets us drop the submodule--helper option
entirely (and it's OK to do so because as a "--" program, it
is entirely a private implementation detail).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/submodule--helper.c  | 17 -----------------
 git-submodule.sh             |  4 ++--
 submodule.c                  | 40 +---------------------------------------
 t/t7412-submodule--helper.sh | 26 --------------------------
 4 files changed, 3 insertions(+), 84 deletions(-)
 delete mode 100755 t/t7412-submodule--helper.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index de3ad5b..48cfc48 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -246,22 +246,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-static int module_sanitize_config(int argc, const char **argv, const char *prefix)
-{
-	struct strbuf sanitized_config = STRBUF_INIT;
-
-	if (argc > 1)
-		usage(_("git submodule--helper sanitize-config"));
-
-	git_config_from_parameters(sanitize_submodule_config, &sanitized_config);
-	if (sanitized_config.len)
-		printf("%s\n", sanitized_config.buf);
-
-	strbuf_release(&sanitized_config);
-
-	return 0;
-}
-
 struct submodule_update_clone {
 	/* index into 'list', the list of submodules to look into for cloning */
 	int current;
@@ -522,7 +506,6 @@ static struct cmd_struct commands[] = {
 	{"list", module_list},
 	{"name", module_name},
 	{"clone", module_clone},
-	{"sanitize-config", module_sanitize_config},
 	{"update-clone", update_clone}
 };
 
diff --git a/git-submodule.sh b/git-submodule.sh
index 3a40d4b..c9d53e1 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -197,9 +197,9 @@ isnumber()
 # of the settings from GIT_CONFIG_PARAMETERS.
 sanitize_submodule_env()
 {
-	sanitized_config=$(git submodule--helper sanitize-config)
+	save_config=$GIT_CONFIG_PARAMETERS
 	clear_local_git_env
-	GIT_CONFIG_PARAMETERS=$sanitized_config
+	GIT_CONFIG_PARAMETERS=$save_config
 	export GIT_CONFIG_PARAMETERS
 }
 
diff --git a/submodule.c b/submodule.c
index 4e76b98..072ea82 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1131,50 +1131,12 @@ int parallel_submodules(void)
 	return parallel_jobs;
 }
 
-/*
- * Rules to sanitize configuration variables that are Ok to be passed into
- * submodule operations from the parent project using "-c". Should only
- * include keys which are both (a) safe and (b) necessary for proper
- * operation.
- */
-static int submodule_config_ok(const char *var)
-{
-	if (starts_with(var, "credential."))
-		return 1;
-	return 0;
-}
-
-int sanitize_submodule_config(const char *var, const char *value, void *data)
-{
-	struct strbuf *out = data;
-
-	if (submodule_config_ok(var)) {
-		if (out->len)
-			strbuf_addch(out, ' ');
-
-		if (value)
-			sq_quotef(out, "%s=%s", var, value);
-		else
-			sq_quote_buf(out, var);
-	}
-
-	return 0;
-}
-
 void prepare_submodule_repo_env(struct argv_array *out)
 {
 	const char * const *var;
 
 	for (var = local_repo_env; *var; var++) {
-		if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
-			struct strbuf sanitized_config = STRBUF_INIT;
-			git_config_from_parameters(sanitize_submodule_config,
-						   &sanitized_config);
-			argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
-			strbuf_release(&sanitized_config);
-		} else {
+		if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
 			argv_array_push(out, *var);
-		}
 	}
-
 }
diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
deleted file mode 100755
index 149d428..0000000
--- a/t/t7412-submodule--helper.sh
+++ /dev/null
@@ -1,26 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2016 Jacob Keller
-#
-
-test_description='Basic plumbing support of submodule--helper
-
-This test verifies the submodule--helper plumbing command used to implement
-git-submodule.
-'
-
-. ./test-lib.sh
-
-test_expect_success 'sanitize-config clears configuration' '
-	git -c user.name="Some User" submodule--helper sanitize-config >actual &&
-	test_must_be_empty actual
-'
-
-sq="'"
-test_expect_success 'sanitize-config keeps credential.helper' '
-	git -c credential.helper=helper submodule--helper sanitize-config >actual &&
-	echo "${sq}credential.helper=helper${sq}" >expect &&
-	test_cmp expect actual
-'
-
-test_done
-- 
2.8.2.600.g439cdc9

Re: [PATCH] submodule: stop sanitizing config options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> [+cc Stefan and Jacob since this is really resuming that earlier thread]
> ...
>> 
>> So I think we'd actually want my series as a preliminary fix, followed
>> by dropping the whitelist entirely on top of that, and then probably
>> simplifying the shell sanitize_submodule_env() on top of that (it would
>> be correct without the whitelist, but you can also trivially implement
>> it without having to call submodule--helper at all).
>
> I think we'd actually do it all in one, and that patch looks something
> like the one below (on top of jk/submodule-config-sanitize-fix).
>
> I don't feel that strongly about going either direction with this, but I
> figure it doesn't hurt to make the patch so we know what the actual
> option looks like.

I do not feel strongly either, but I suspect "we do not filter"
would be a lot easier to explain than "we pass these selected
things, each with such and such justification why it has to be
passed down".

Nice code reduction is very attractive, too, but that is secondary.

> -- >8 --
> Subject: [PATCH] submodule: stop sanitizing config options
>
> The point of having a whitelist of command-line config
> options to pass to submodules was two-fold:
>
>   1. It prevented obvious nonsense like using core.worktree
>      for multiple repos.
>
>   2. It could prevent surprise when the user did not mean
>      for the options to leak to the submodules (e.g.,
>      http.sslverify=false).
>
> For case 1, the answer is mostly "if it hurts, don't do
> that". For case 2, we can note that any such example has a
> matching inverted surprise (e.g., a user who meant
> http.sslverify=true to apply everywhere, but it didn't).
>
> So this whitelist is probably not giving us any benefit, and
> is already creating a hassle as people propose things to put
> on it. Let's just drop it entirely.
>
> Note that we still need to keep a special code path for
> "prepare the submodule environment", because we still have
> to take care to pass through $GIT_CONFIG_PARAMETERS (and
> block the rest of the repo-specific environment variables).
>
> We can do this easily from within the submodule shell
> script, which lets us drop the submodule--helper option
> entirely (and it's OK to do so because as a "--" program, it
> is entirely a private implementation detail).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/submodule--helper.c  | 17 -----------------
>  git-submodule.sh             |  4 ++--
>  submodule.c                  | 40 +---------------------------------------
>  t/t7412-submodule--helper.sh | 26 --------------------------
>  4 files changed, 3 insertions(+), 84 deletions(-)
>  delete mode 100755 t/t7412-submodule--helper.sh
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index de3ad5b..48cfc48 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -246,22 +246,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> -static int module_sanitize_config(int argc, const char **argv, const char *prefix)
> -{
> -	struct strbuf sanitized_config = STRBUF_INIT;
> -
> -	if (argc > 1)
> -		usage(_("git submodule--helper sanitize-config"));
> -
> -	git_config_from_parameters(sanitize_submodule_config, &sanitized_config);
> -	if (sanitized_config.len)
> -		printf("%s\n", sanitized_config.buf);
> -
> -	strbuf_release(&sanitized_config);
> -
> -	return 0;
> -}
> -
>  struct submodule_update_clone {
>  	/* index into 'list', the list of submodules to look into for cloning */
>  	int current;
> @@ -522,7 +506,6 @@ static struct cmd_struct commands[] = {
>  	{"list", module_list},
>  	{"name", module_name},
>  	{"clone", module_clone},
> -	{"sanitize-config", module_sanitize_config},
>  	{"update-clone", update_clone}
>  };
>  
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 3a40d4b..c9d53e1 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -197,9 +197,9 @@ isnumber()
>  # of the settings from GIT_CONFIG_PARAMETERS.
>  sanitize_submodule_env()
>  {
> -	sanitized_config=$(git submodule--helper sanitize-config)
> +	save_config=$GIT_CONFIG_PARAMETERS
>  	clear_local_git_env
> -	GIT_CONFIG_PARAMETERS=$sanitized_config
> +	GIT_CONFIG_PARAMETERS=$save_config
>  	export GIT_CONFIG_PARAMETERS
>  }
>  
> diff --git a/submodule.c b/submodule.c
> index 4e76b98..072ea82 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1131,50 +1131,12 @@ int parallel_submodules(void)
>  	return parallel_jobs;
>  }
>  
> -/*
> - * Rules to sanitize configuration variables that are Ok to be passed into
> - * submodule operations from the parent project using "-c". Should only
> - * include keys which are both (a) safe and (b) necessary for proper
> - * operation.
> - */
> -static int submodule_config_ok(const char *var)
> -{
> -	if (starts_with(var, "credential."))
> -		return 1;
> -	return 0;
> -}
> -
> -int sanitize_submodule_config(const char *var, const char *value, void *data)
> -{
> -	struct strbuf *out = data;
> -
> -	if (submodule_config_ok(var)) {
> -		if (out->len)
> -			strbuf_addch(out, ' ');
> -
> -		if (value)
> -			sq_quotef(out, "%s=%s", var, value);
> -		else
> -			sq_quote_buf(out, var);
> -	}
> -
> -	return 0;
> -}
> -
>  void prepare_submodule_repo_env(struct argv_array *out)
>  {
>  	const char * const *var;
>  
>  	for (var = local_repo_env; *var; var++) {
> -		if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
> -			struct strbuf sanitized_config = STRBUF_INIT;
> -			git_config_from_parameters(sanitize_submodule_config,
> -						   &sanitized_config);
> -			argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
> -			strbuf_release(&sanitized_config);
> -		} else {
> +		if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
>  			argv_array_push(out, *var);
> -		}
>  	}
> -
>  }
> diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
> deleted file mode 100755
> index 149d428..0000000
> --- a/t/t7412-submodule--helper.sh
> +++ /dev/null
> @@ -1,26 +0,0 @@
> -#!/bin/sh
> -#
> -# Copyright (c) 2016 Jacob Keller
> -#
> -
> -test_description='Basic plumbing support of submodule--helper
> -
> -This test verifies the submodule--helper plumbing command used to implement
> -git-submodule.
> -'
> -
> -. ./test-lib.sh
> -
> -test_expect_success 'sanitize-config clears configuration' '
> -	git -c user.name="Some User" submodule--helper sanitize-config >actual &&
> -	test_must_be_empty actual
> -'
> -
> -sq="'"
> -test_expect_success 'sanitize-config keeps credential.helper' '
> -	git -c credential.helper=helper submodule--helper sanitize-config >actual &&
> -	echo "${sq}credential.helper=helper${sq}" >expect &&
> -	test_cmp expect actual
> -'
> -
> -test_done

Re: [PATCH] submodule: stop sanitizing config options

[Johannes Schindelin]

Hi,

On Wed, 4 May 2016, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > [+cc Stefan and Jacob since this is really resuming that earlier thread]
> > ...
> >> 
> >> So I think we'd actually want my series as a preliminary fix, followed
> >> by dropping the whitelist entirely on top of that, and then probably
> >> simplifying the shell sanitize_submodule_env() on top of that (it would
> >> be correct without the whitelist, but you can also trivially implement
> >> it without having to call submodule--helper at all).
> >
> > I think we'd actually do it all in one, and that patch looks something
> > like the one below (on top of jk/submodule-config-sanitize-fix).
> >
> > I don't feel that strongly about going either direction with this, but I
> > figure it doesn't hurt to make the patch so we know what the actual
> > option looks like.
> 
> I do not feel strongly either, but I suspect "we do not filter"
> would be a lot easier to explain than "we pass these selected
> things, each with such and such justification why it has to be
> passed down".

I really like the simplicity of the rationale.

> Nice code reduction is very attractive, too, but that is secondary.

Me, too. It just makes things simpler.

Ciao,
Dscho

Re: [PATCH] submodule: stop sanitizing config options

[Stefan Beller]

On Wed, May 4, 2016 at 1:00 AM, Jeff King <peff@peff.net> wrote:
> [+cc Stefan and Jacob since this is really resuming that earlier thread]
>
> On Wed, May 04, 2016 at 03:45:59AM -0400, Jeff King wrote:
>
>> On Wed, May 04, 2016 at 02:26:18AM -0400, Jeff King wrote:
>>
>> > >   submodule: pass on http.extraheader config settings
>> >
>> > IMHO this should come on top of jk/submodule-config-sanitize-fix (I was
>> > surprised at first that your test worked at all, but that is because it
>> > is using "clone", which is the one code path that works).
>> >
>> > But I think we are waiting on going one of two paths:
>> >
>> >   1. drop sanitizing entirely
>> >
>> >   2. fix sanitizing and add more variables to it
>> >
>> > If we go the route of (2), then we'd want my fix topic and this patch.
>> > And if not, then we don't need any of it (just a patch dropping the
>> > filtering, which AFAIK nobody has written yet).
>>
>> Actually, I think this last bit is not quite true. If we want to go back
>> to "nothing gets passed to submodules", we can drop all of my patches,
>> but I don't think anybody wants to do that.
>>
>> But if we want "everything gets passed to submodules", then we do need
>> something like my patch series, because every use of local_repo_env
>> needs to be come "local_repo_env excluding GIT_CONFIG_PARAMETERS". I
>> don't think we want to simply drop that variable from local_repo_env
>> (which would also mean that it would be propagated to a local
>> git-upload-pack, for example, along with any third-party scripts that
>> use rev-parse --local-env-vars).
>>
>> So I think we'd actually want my series as a preliminary fix, followed
>> by dropping the whitelist entirely on top of that, and then probably
>> simplifying the shell sanitize_submodule_env() on top of that (it would
>> be correct without the whitelist, but you can also trivially implement
>> it without having to call submodule--helper at all).
>
> I think we'd actually do it all in one, and that patch looks something
> like the one below (on top of jk/submodule-config-sanitize-fix).
>
> I don't feel that strongly about going either direction with this, but I
> figure it doesn't hurt to make the patch so we know what the actual
> option looks like.
>
> -- >8 --
> Subject: [PATCH] submodule: stop sanitizing config options
>
> The point of having a whitelist of command-line config
> options to pass to submodules was two-fold:
>
>   1. It prevented obvious nonsense like using core.worktree
>      for multiple repos.
>
>   2. It could prevent surprise when the user did not mean
>      for the options to leak to the submodules (e.g.,
>      http.sslverify=false).
>
> For case 1, the answer is mostly "if it hurts, don't do
> that". For case 2, we can note that any such example has a
> matching inverted surprise (e.g., a user who meant
> http.sslverify=true to apply everywhere, but it didn't).
>
> So this whitelist is probably not giving us any benefit, and
> is already creating a hassle as people propose things to put
> on it. Let's just drop it entirely.

Just to recap:
Before jk/submodule-config-sanitize-fix (jk/submodule-c-credential actually)
we passed nothing down to the commands operating on submodules.

Then we decided to pass on some of it based on a curated list.

Curating the list is too hard, so we pass on everything now, because
it is easy to maintain and easy to explain. And when the user is hurt,
they're holding it wrong?

>
> Note that we still need to keep a special code path for
> "prepare the submodule environment", because we still have
> to take care to pass through $GIT_CONFIG_PARAMETERS (and
> block the rest of the repo-specific environment variables).

So when running `git -c foo=bar command --recurse-submodules`
the `-c` parsing calls git_config_push_parameter, which
exports that string `foo=baz` into the environment variable
GIT_CONFIG_PARAMETERS.

When the submodule command is called, sanitize_submodule_env
just wipes all the Git related configurations except those in
GIT_CONFIG_PARAMETERS as they are set again after
clear_local_git_env wiped it.

I wonder about the implementation detail, if we rather want to introduce
a `git rev-parse --repo-only-local-env-vars` which is
`git rev-parse --local-env-vars` without the GIT_CONFIG_PARAMETERS.
such that clear_local_git_env does the right thing and we don't
have to have 2 functions for it (i.e. clear_local_git_env and
sanitize_submodule_env, which is the newer not as strict version of it)

But as Jeff once put it, rev-parse is already a messy kitchensink,
so adding another option there may also not the right way?

>
> We can do this easily from within the submodule shell
> script, which lets us drop the submodule--helper option
> entirely (and it's OK to do so because as a "--" program, it
> is entirely a private implementation detail).

Yeah that -- program may change any time in no backwards
compatible way.

Do we want to add documentation for the new behavior though?
    Before: pass not -c arguments to submodules
    Now: Pass all the -c arguments to submodules

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/submodule--helper.c  | 17 -----------------
>  git-submodule.sh             |  4 ++--
>  submodule.c                  | 40 +---------------------------------------
>  t/t7412-submodule--helper.sh | 26 --------------------------
>  4 files changed, 3 insertions(+), 84 deletions(-)
>  delete mode 100755 t/t7412-submodule--helper.sh
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index de3ad5b..48cfc48 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -246,22 +246,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>         return 0;
>  }
>
> -static int module_sanitize_config(int argc, const char **argv, const char *prefix)
> -{
> -       struct strbuf sanitized_config = STRBUF_INIT;
> -
> -       if (argc > 1)
> -               usage(_("git submodule--helper sanitize-config"));
> -
> -       git_config_from_parameters(sanitize_submodule_config, &sanitized_config);
> -       if (sanitized_config.len)
> -               printf("%s\n", sanitized_config.buf);
> -
> -       strbuf_release(&sanitized_config);
> -
> -       return 0;
> -}
> -
>  struct submodule_update_clone {
>         /* index into 'list', the list of submodules to look into for cloning */
>         int current;
> @@ -522,7 +506,6 @@ static struct cmd_struct commands[] = {
>         {"list", module_list},
>         {"name", module_name},
>         {"clone", module_clone},
> -       {"sanitize-config", module_sanitize_config},
>         {"update-clone", update_clone}
>  };
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 3a40d4b..c9d53e1 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -197,9 +197,9 @@ isnumber()
>  # of the settings from GIT_CONFIG_PARAMETERS.
>  sanitize_submodule_env()
>  {
> -       sanitized_config=$(git submodule--helper sanitize-config)
> +       save_config=$GIT_CONFIG_PARAMETERS
>         clear_local_git_env
> -       GIT_CONFIG_PARAMETERS=$sanitized_config
> +       GIT_CONFIG_PARAMETERS=$save_config
>         export GIT_CONFIG_PARAMETERS
>  }
>
> diff --git a/submodule.c b/submodule.c
> index 4e76b98..072ea82 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1131,50 +1131,12 @@ int parallel_submodules(void)
>         return parallel_jobs;
>  }
>
> -/*
> - * Rules to sanitize configuration variables that are Ok to be passed into
> - * submodule operations from the parent project using "-c". Should only
> - * include keys which are both (a) safe and (b) necessary for proper
> - * operation.
> - */
> -static int submodule_config_ok(const char *var)
> -{
> -       if (starts_with(var, "credential."))
> -               return 1;
> -       return 0;
> -}
> -
> -int sanitize_submodule_config(const char *var, const char *value, void *data)
> -{
> -       struct strbuf *out = data;
> -
> -       if (submodule_config_ok(var)) {
> -               if (out->len)
> -                       strbuf_addch(out, ' ');
> -
> -               if (value)
> -                       sq_quotef(out, "%s=%s", var, value);
> -               else
> -                       sq_quote_buf(out, var);
> -       }
> -
> -       return 0;
> -}
> -
>  void prepare_submodule_repo_env(struct argv_array *out)
>  {
>         const char * const *var;
>
>         for (var = local_repo_env; *var; var++) {
> -               if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
> -                       struct strbuf sanitized_config = STRBUF_INIT;
> -                       git_config_from_parameters(sanitize_submodule_config,
> -                                                  &sanitized_config);
> -                       argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
> -                       strbuf_release(&sanitized_config);
> -               } else {
> +               if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
>                         argv_array_push(out, *var);
> -               }
>         }
> -
>  }
> diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
> deleted file mode 100755
> index 149d428..0000000
> --- a/t/t7412-submodule--helper.sh
> +++ /dev/null
> @@ -1,26 +0,0 @@
> -#!/bin/sh
> -#
> -# Copyright (c) 2016 Jacob Keller
> -#
> -
> -test_description='Basic plumbing support of submodule--helper
> -
> -This test verifies the submodule--helper plumbing command used to implement
> -git-submodule.
> -'
> -
> -. ./test-lib.sh
> -
> -test_expect_success 'sanitize-config clears configuration' '
> -       git -c user.name="Some User" submodule--helper sanitize-config >actual &&
> -       test_must_be_empty actual
> -'
> -
> -sq="'"
> -test_expect_success 'sanitize-config keeps credential.helper' '
> -       git -c credential.helper=helper submodule--helper sanitize-config >actual &&
> -       echo "${sq}credential.helper=helper${sq}" >expect &&
> -       test_cmp expect actual
> -'
> -
> -test_done
> --
> 2.8.2.600.g439cdc9
>

Re: [PATCH] submodule: stop sanitizing config options

[Jeff King]

On Wed, May 04, 2016 at 10:58:26AM -0700, Stefan Beller wrote:

> > So this whitelist is probably not giving us any benefit, and
> > is already creating a hassle as people propose things to put
> > on it. Let's just drop it entirely.
> 
> Just to recap:
> Before jk/submodule-config-sanitize-fix (jk/submodule-c-credential actually)
> we passed nothing down to the commands operating on submodules.
> 
> Then we decided to pass on some of it based on a curated list.
> 
> Curating the list is too hard, so we pass on everything now, because
> it is easy to maintain and easy to explain. And when the user is hurt,
> they're holding it wrong?

Yes, I think that sums it up (the last paragraph is what's under
discussion, so it's up for debate).

> > Note that we still need to keep a special code path for
> > "prepare the submodule environment", because we still have
> > to take care to pass through $GIT_CONFIG_PARAMETERS (and
> > block the rest of the repo-specific environment variables).
> 
> So when running `git -c foo=bar command --recurse-submodules`
> the `-c` parsing calls git_config_push_parameter, which
> exports that string `foo=baz` into the environment variable
> GIT_CONFIG_PARAMETERS.
> 
> When the submodule command is called, sanitize_submodule_env
> just wipes all the Git related configurations except those in
> GIT_CONFIG_PARAMETERS as they are set again after
> clear_local_git_env wiped it.

Right.

> I wonder about the implementation detail, if we rather want to introduce
> a `git rev-parse --repo-only-local-env-vars` which is
> `git rev-parse --local-env-vars` without the GIT_CONFIG_PARAMETERS.
> such that clear_local_git_env does the right thing and we don't
> have to have 2 functions for it (i.e. clear_local_git_env and
> sanitize_submodule_env, which is the newer not as strict version of it)

I don't think that really buys you much. The policy is technically
duplicated in prepare_submodule_repo_env() in submodule.c and
sanitize_submodule_env in submodule.h. But without the whitelist, it's
such a simple policy that I'm not sure it's worth the boilerplate of
having the shell function call into the C code.

If we did want to go that route, the more appropriate interface would
probably be:

  git submodule--helper sanitize-env

or something. That avoids making it a public interface, and makes it
clear that we are invoking the submodule rules.

> Do we want to add documentation for the new behavior though?
>     Before: pass not -c arguments to submodules
>     Now: Pass all the -c arguments to submodules

I don't think there was any documentation for the _old_ behavior, and
certainly jk/submodule-c-credential didn't add any. But it probably is
worth document, maybe as part of "-c"? Care to roll a patch on top?

-Peff

Re: [PATCH] submodule: stop sanitizing config options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 3a40d4b..c9d53e1 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -197,9 +197,9 @@ isnumber()
>  # of the settings from GIT_CONFIG_PARAMETERS.
>  sanitize_submodule_env()
>  {
> -	sanitized_config=$(git submodule--helper sanitize-config)
> +	save_config=$GIT_CONFIG_PARAMETERS
>  	clear_local_git_env
> -	GIT_CONFIG_PARAMETERS=$sanitized_config
> +	GIT_CONFIG_PARAMETERS=$save_config
>  	export GIT_CONFIG_PARAMETERS
>  }

This does "clear the obviously per-repository stuff, but add back in
anything that came from -c".  If it is easy to do "add anything that
came from -c, and then clear the obviously per-repository stuff", we
don't even have to say "exporting core.worktree down may hurt; do
not do it then", which may be the best of both worlds?

Or have we decided that even sharing core.worktree may have a valid
use case and it is better not to filter them?

Re: [PATCH] submodule: stop sanitizing config options

[Jeff King]

On Wed, May 04, 2016 at 11:43:47AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 3a40d4b..c9d53e1 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -197,9 +197,9 @@ isnumber()
> >  # of the settings from GIT_CONFIG_PARAMETERS.
> >  sanitize_submodule_env()
> >  {
> > -	sanitized_config=$(git submodule--helper sanitize-config)
> > +	save_config=$GIT_CONFIG_PARAMETERS
> >  	clear_local_git_env
> > -	GIT_CONFIG_PARAMETERS=$sanitized_config
> > +	GIT_CONFIG_PARAMETERS=$save_config
> >  	export GIT_CONFIG_PARAMETERS
> >  }
> 
> This does "clear the obviously per-repository stuff, but add back in
> anything that came from -c".  If it is easy to do "add anything that
> came from -c, and then clear the obviously per-repository stuff", we
> don't even have to say "exporting core.worktree down may hurt; do
> not do it then", which may be the best of both worlds?

It's not easy at this level. clear_local_git_env() is just clearing
whole variables like $GIT_WORK_TREE. But if you say "-c core.worktree",
that is still live in $GIT_CONFIG_PARAMETERS, and gets fed to
sub-processes on each invocation. Nobody ever removes individual content
from $GIT_CONFIG_PARAMETERS once something is in it.

> Or have we decided that even sharing core.worktree may have a valid
> use case and it is better not to filter them?

I do not personally consider it a valid use case. The sentiment just
seemed to be that it was better to make the rule simple and easy to
explain ("everything gets passed through; if it hurts, don't do it")
than to keep sanitizing.

If you want "pass through everything except core.worktree", then that is
the blacklist approach (which is easy to do if we scrap this patch and
just teak submodule_config_ok() from a whitelist into a blacklist).

-Peff

Re: [PATCH] submodule: stop sanitizing config options

[Stefan Beller]

> I don't think there was any documentation for the _old_ behavior, and
> certainly jk/submodule-c-credential didn't add any. But it probably is
> worth document, maybe as part of "-c"? Care to roll a patch on top?

Sure.

>
> I think we'd actually do it all in one, and that patch looks something
> like the one below (on top of jk/submodule-config-sanitize-fix).

    $ git checkout origin/jk/submodule-config-sanitize-fix
    $ git am p
Applying: submodule: stop sanitizing config options
error: patch failed: builtin/submodule--helper.c:246
error: builtin/submodule--helper.c: patch does not apply
error: patch failed: submodule.c:1131
error: submodule.c: patch does not apply
Patch failed at 0001 submodule: stop sanitizing config options

So if you want some documentation on top of that, where would I base it on?

Re: [PATCH] submodule: stop sanitizing config options

[Jeff King]

On Wed, May 04, 2016 at 03:53:26PM -0700, Stefan Beller wrote:

> > I think we'd actually do it all in one, and that patch looks something
> > like the one below (on top of jk/submodule-config-sanitize-fix).
> 
>     $ git checkout origin/jk/submodule-config-sanitize-fix
>     $ git am p
> Applying: submodule: stop sanitizing config options
> error: patch failed: builtin/submodule--helper.c:246
> error: builtin/submodule--helper.c: patch does not apply
> error: patch failed: submodule.c:1131
> error: submodule.c: patch does not apply
> Patch failed at 0001 submodule: stop sanitizing config options
> 
> So if you want some documentation on top of that, where would I base it on?

I build the patches for jk/submodule-config-sanitize-fix on top of
master as of the other day, and then built this most recent patch on top
of that.

Looks like Junio applied them directly on the tip of
jk/submodule-c-credential, and had to wiggle the code in submodule.c,
which conflicted with the parallel-process stuff that was merged in
between. Since the new patch updates that code, it will likewise run
into conflicts.

I don't think there's a strict right answer here; if the original buggy
submodule-c-credential code had been released, we would definitely want
to build off of it for "maint" releases. But it wasn't, so master is
"just as good" in a sense. But I think Junio makes it a habit to apply
fixes as far back as the introduced bug, even when it's not going to
maint.

So since that's what published, it makes sense to build on that. Here's
a version of my patch that should apply for you (no semantic changes,
just differences in the surrounding context):

-- >8 --
Subject: [PATCH] submodule: stop sanitizing config options

The point of having a whitelist of command-line config
options to pass to submodules was two-fold:

  1. It prevented obvious nonsense like using core.worktree
     for multiple repos.

  2. It could prevent surprise when the user did not mean
     for the options to leak to the submodules (e.g.,
     http.sslverify=false).

For case 1, the answer is mostly "if it hurts, don't do
that". For case 2, we can note that any such example has a
matching inverted surprise (e.g., a user who meant
http.sslverify=true to apply everywhere, but it didn't).

So this whitelist is probably not giving us any benefit, and
is already creating a hassle as people propose things to put
on it. Let's just drop it entirely.

Note that we still need to keep a special code path for
"prepare the submodule environment", because we still have
to take care to pass through $GIT_CONFIG_PARAMETERS (and
block the rest of the repo-specific environment variables).

We can do this easily from within the submodule shell
script, which lets us drop the submodule--helper option
entirely (and it's OK to do so because as a "--" program, it
is entirely a private implementation detail).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/submodule--helper.c  | 17 -----------------
 git-submodule.sh             |  4 ++--
 submodule.c                  | 39 +--------------------------------------
 t/t7412-submodule--helper.sh | 26 --------------------------
 4 files changed, 3 insertions(+), 83 deletions(-)
 delete mode 100755 t/t7412-submodule--helper.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 16d6432..89250f0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -260,22 +260,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-static int module_sanitize_config(int argc, const char **argv, const char *prefix)
-{
-	struct strbuf sanitized_config = STRBUF_INIT;
-
-	if (argc > 1)
-		usage(_("git submodule--helper sanitize-config"));
-
-	git_config_from_parameters(sanitize_submodule_config, &sanitized_config);
-	if (sanitized_config.len)
-		printf("%s\n", sanitized_config.buf);
-
-	strbuf_release(&sanitized_config);
-
-	return 0;
-}
-
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
@@ -285,7 +269,6 @@ static struct cmd_struct commands[] = {
 	{"list", module_list},
 	{"name", module_name},
 	{"clone", module_clone},
-	{"sanitize-config", module_sanitize_config},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 91f5856..b1c056c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -197,9 +197,9 @@ isnumber()
 # of the settings from GIT_CONFIG_PARAMETERS.
 sanitize_submodule_env()
 {
-	sanitized_config=$(git submodule--helper sanitize-config)
+	save_config=$GIT_CONFIG_PARAMETERS
 	clear_local_git_env
-	GIT_CONFIG_PARAMETERS=$sanitized_config
+	GIT_CONFIG_PARAMETERS=$save_config
 	export GIT_CONFIG_PARAMETERS
 }
 
diff --git a/submodule.c b/submodule.c
index c18ab9b..d598881 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1098,50 +1098,13 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
 	strbuf_release(&rel_path);
 	free((void *)real_work_tree);
 }
-/*
- * Rules to sanitize configuration variables that are Ok to be passed into
- * submodule operations from the parent project using "-c". Should only
- * include keys which are both (a) safe and (b) necessary for proper
- * operation.
- */
-static int submodule_config_ok(const char *var)
-{
-	if (starts_with(var, "credential."))
-		return 1;
-	return 0;
-}
-
-int sanitize_submodule_config(const char *var, const char *value, void *data)
-{
-	struct strbuf *out = data;
-
-	if (submodule_config_ok(var)) {
-		if (out->len)
-			strbuf_addch(out, ' ');
-
-		if (value)
-			sq_quotef(out, "%s=%s", var, value);
-		else
-			sq_quote_buf(out, var);
-	}
-
-	return 0;
-}
 
 void prepare_submodule_repo_env(struct argv_array *out)
 {
 	const char * const *var;
 
 	for (var = local_repo_env; *var; var++) {
-		if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
-			struct strbuf sanitized_config = STRBUF_INIT;
-			git_config_from_parameters(sanitize_submodule_config,
-						   &sanitized_config);
-			argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
-			strbuf_release(&sanitized_config);
-		} else {
+		if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
 			argv_array_push(out, *var);
-		}
 	}
-
 }
diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
deleted file mode 100755
index 149d428..0000000
--- a/t/t7412-submodule--helper.sh
+++ /dev/null
@@ -1,26 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2016 Jacob Keller
-#
-
-test_description='Basic plumbing support of submodule--helper
-
-This test verifies the submodule--helper plumbing command used to implement
-git-submodule.
-'
-
-. ./test-lib.sh
-
-test_expect_success 'sanitize-config clears configuration' '
-	git -c user.name="Some User" submodule--helper sanitize-config >actual &&
-	test_must_be_empty actual
-'
-
-sq="'"
-test_expect_success 'sanitize-config keeps credential.helper' '
-	git -c credential.helper=helper submodule--helper sanitize-config >actual &&
-	echo "${sq}credential.helper=helper${sq}" >expect &&
-	test_cmp expect actual
-'
-
-test_done
-- 
2.8.2.600.g439cdc9

Re: [PATCH] submodule: stop sanitizing config options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here's a version of my patch that should apply for you (no
> semantic changes, just differences in the surrounding context):

Sorry for the trouble.

I queued jk/submodule-c-credential to be mergeable to 'maint', as it
could be argued two ways.  We can say that not propagating -c down
is a bug and the series is a bugfix.  Or it is merely a known
limitation and the series is a new feature.  I was leaning towards
the former, but I was also willing to declare that is a known bug
that will left unfixed in the maintenance track.

If I knew the 5 patches on jk/submodule-config-sanitize-fix was what
we would eventually agree to be the right change, I would have
applied them directly on top of jk/submodule-c-credential instead of
forking a separate branch for them.  That way, either the "bugfix"
would all go to 'maint', or the "feature" wouldn't, and we do not
have to worry about making a mistake to merge only half-done state
(i.e. jk/submodule-c-credential that passes only the credential.*
configuration) that nobody would want to 'maint'.

But when I picked up jk/submodule-config-sanitize-fix, I didn't have
enough time to think things through or carefully read the discussion
to convince myself that we already had a list concensus, so I queued
it separately only to make sure I won't lose track of it--I can come
back to it later that way.

It probably is a good time to merge jk/submodule-config-sanitize-fix
into jk/submodule-c-credential (i.e. a mere fast-forward), remove
that "-fix" branch, and apply this patch directly on top of the
resulting jk/submodule-c-credential.  That will make the whole thing
a 13-patch series, consisting of:

 7 patches up to the current jk/submodule-c-credential
  d1f8849 git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS
  14111fc git: submodule honor -c credential.* from command line
  e70986d quote: implement sq_quotef()
  7dad263 submodule: fix segmentation fault in submodule--helper clone
  717416c submodule: fix submodule--helper clone usage
  08e0970 submodule: check argc count for git submodule--helper clone
  d10e3b4 submodule: don't pass empty string arguments to submodule--helper clone
 
 5 patches up to the current jk/submodule-config-sanitize-fix
  c12e865 submodule: use prepare_submodule_repo_env consistently
  4638728 submodule--helper: move config-sanitizing to submodule.c
  860cba6 submodule: export sanitized GIT_CONFIG_PARAMETERS
  455d22c t5550: break submodule config test into multiple sub-tests
  1149ee2 t5550: fix typo in $HTTPD_URL

 1 patch (this one)
  4e6706a submodule: stop sanitizing config options

whose early 7 patches have already been merged to 'master' (and none
has been merged to 'maint').

> -- >8 --
> Subject: [PATCH] submodule: stop sanitizing config options
>
> The point of having a whitelist of command-line config
> options to pass to submodules was two-fold:
>
>   1. It prevented obvious nonsense like using core.worktree
>      for multiple repos.
>
>   2. It could prevent surprise when the user did not mean
>      for the options to leak to the submodules (e.g.,
>      http.sslverify=false).
>
> For case 1, the answer is mostly "if it hurts, don't do
> that". For case 2, we can note that any such example has a
> matching inverted surprise (e.g., a user who meant
> http.sslverify=true to apply everywhere, but it didn't).
>
> So this whitelist is probably not giving us any benefit, and
> is already creating a hassle as people propose things to put
> on it. Let's just drop it entirely.
>
> Note that we still need to keep a special code path for
> "prepare the submodule environment", because we still have
> to take care to pass through $GIT_CONFIG_PARAMETERS (and
> block the rest of the repo-specific environment variables).
>
> We can do this easily from within the submodule shell
> script, which lets us drop the submodule--helper option
> entirely (and it's OK to do so because as a "--" program, it
> is entirely a private implementation detail).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/submodule--helper.c  | 17 -----------------
>  git-submodule.sh             |  4 ++--
>  submodule.c                  | 39 +--------------------------------------
>  t/t7412-submodule--helper.sh | 26 --------------------------
>  4 files changed, 3 insertions(+), 83 deletions(-)
>  delete mode 100755 t/t7412-submodule--helper.sh
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 16d6432..89250f0 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -260,22 +260,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> -static int module_sanitize_config(int argc, const char **argv, const char *prefix)
> -{
> -	struct strbuf sanitized_config = STRBUF_INIT;
> -
> -	if (argc > 1)
> -		usage(_("git submodule--helper sanitize-config"));
> -
> -	git_config_from_parameters(sanitize_submodule_config, &sanitized_config);
> -	if (sanitized_config.len)
> -		printf("%s\n", sanitized_config.buf);
> -
> -	strbuf_release(&sanitized_config);
> -
> -	return 0;
> -}
> -
>  struct cmd_struct {
>  	const char *cmd;
>  	int (*fn)(int, const char **, const char *);
> @@ -285,7 +269,6 @@ static struct cmd_struct commands[] = {
>  	{"list", module_list},
>  	{"name", module_name},
>  	{"clone", module_clone},
> -	{"sanitize-config", module_sanitize_config},
>  };
>  
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 91f5856..b1c056c 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -197,9 +197,9 @@ isnumber()
>  # of the settings from GIT_CONFIG_PARAMETERS.
>  sanitize_submodule_env()
>  {
> -	sanitized_config=$(git submodule--helper sanitize-config)
> +	save_config=$GIT_CONFIG_PARAMETERS
>  	clear_local_git_env
> -	GIT_CONFIG_PARAMETERS=$sanitized_config
> +	GIT_CONFIG_PARAMETERS=$save_config
>  	export GIT_CONFIG_PARAMETERS
>  }
>  
> diff --git a/submodule.c b/submodule.c
> index c18ab9b..d598881 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1098,50 +1098,13 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
>  	strbuf_release(&rel_path);
>  	free((void *)real_work_tree);
>  }
> -/*
> - * Rules to sanitize configuration variables that are Ok to be passed into
> - * submodule operations from the parent project using "-c". Should only
> - * include keys which are both (a) safe and (b) necessary for proper
> - * operation.
> - */
> -static int submodule_config_ok(const char *var)
> -{
> -	if (starts_with(var, "credential."))
> -		return 1;
> -	return 0;
> -}
> -
> -int sanitize_submodule_config(const char *var, const char *value, void *data)
> -{
> -	struct strbuf *out = data;
> -
> -	if (submodule_config_ok(var)) {
> -		if (out->len)
> -			strbuf_addch(out, ' ');
> -
> -		if (value)
> -			sq_quotef(out, "%s=%s", var, value);
> -		else
> -			sq_quote_buf(out, var);
> -	}
> -
> -	return 0;
> -}
>  
>  void prepare_submodule_repo_env(struct argv_array *out)
>  {
>  	const char * const *var;
>  
>  	for (var = local_repo_env; *var; var++) {
> -		if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
> -			struct strbuf sanitized_config = STRBUF_INIT;
> -			git_config_from_parameters(sanitize_submodule_config,
> -						   &sanitized_config);
> -			argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
> -			strbuf_release(&sanitized_config);
> -		} else {
> +		if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
>  			argv_array_push(out, *var);
> -		}
>  	}
> -
>  }
> diff --git a/t/t7412-submodule--helper.sh b/t/t7412-submodule--helper.sh
> deleted file mode 100755
> index 149d428..0000000
> --- a/t/t7412-submodule--helper.sh
> +++ /dev/null
> @@ -1,26 +0,0 @@
> -#!/bin/sh
> -#
> -# Copyright (c) 2016 Jacob Keller
> -#
> -
> -test_description='Basic plumbing support of submodule--helper
> -
> -This test verifies the submodule--helper plumbing command used to implement
> -git-submodule.
> -'
> -
> -. ./test-lib.sh
> -
> -test_expect_success 'sanitize-config clears configuration' '
> -	git -c user.name="Some User" submodule--helper sanitize-config >actual &&
> -	test_must_be_empty actual
> -'
> -
> -sq="'"
> -test_expect_success 'sanitize-config keeps credential.helper' '
> -	git -c credential.helper=helper submodule--helper sanitize-config >actual &&
> -	echo "${sq}credential.helper=helper${sq}" >expect &&
> -	test_cmp expect actual
> -'
> -
> -test_done

Re: [PATCH] submodule: stop sanitizing config options

[Jeff King]

On Thu, May 05, 2016 at 09:59:15AM -0700, Junio C Hamano wrote:

> Sorry for the trouble.
> 
> I queued jk/submodule-c-credential to be mergeable to 'maint', as it
> could be argued two ways.  We can say that not propagating -c down
> is a bug and the series is a bugfix.  Or it is merely a known
> limitation and the series is a new feature.  I was leaning towards
> the former, but I was also willing to declare that is a known bug
> that will left unfixed in the maintenance track.

Ah, OK, that makes perfect sense.

> It probably is a good time to merge jk/submodule-config-sanitize-fix
> into jk/submodule-c-credential (i.e. a mere fast-forward), remove
> that "-fix" branch, and apply this patch directly on top of the
> resulting jk/submodule-c-credential.  That will make the whole thing
> a 13-patch series, consisting of:
> 
>  7 patches up to the current jk/submodule-c-credential
>   d1f8849 git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS
>   14111fc git: submodule honor -c credential.* from command line
>   e70986d quote: implement sq_quotef()
>   7dad263 submodule: fix segmentation fault in submodule--helper clone
>   717416c submodule: fix submodule--helper clone usage
>   08e0970 submodule: check argc count for git submodule--helper clone
>   d10e3b4 submodule: don't pass empty string arguments to submodule--helper clone
>  
>  5 patches up to the current jk/submodule-config-sanitize-fix
>   c12e865 submodule: use prepare_submodule_repo_env consistently
>   4638728 submodule--helper: move config-sanitizing to submodule.c
>   860cba6 submodule: export sanitized GIT_CONFIG_PARAMETERS
>   455d22c t5550: break submodule config test into multiple sub-tests
>   1149ee2 t5550: fix typo in $HTTPD_URL
> 
>  1 patch (this one)
>   4e6706a submodule: stop sanitizing config options

That sounds reasonable. Note that the later patches drop the only caller
of the newly-introduced sq_quotef(). So we could revert e70986d
(omitting it from the series doesn't make sense, as it would leave a
broken state in the middle). I am also fine with leaving it. It seems
like a potentially useful addition.

I had originally thought after the final one that we could further clean
up by turning prepare_submodule_repo_env() into a static function. But
we can't; it gets called in one spot from submodule--helper. However,
while looking at it, I did notice that we probably want to squash this
into the final patch (since sanitize_submodule_config went away
completely):

diff --git a/submodule.h b/submodule.h
index 48690b1..869d259 100644
--- a/submodule.h
+++ b/submodule.h
@@ -43,19 +43,10 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam
 int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
 void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
 
-/*
- * This function is intended as a callback for use with
- * git_config_from_parameters(). It ignores any config options which
- * are not suitable for passing along to a submodule, and accumulates the rest
- * in "data", which must be a pointer to a strbuf. The end result can
- * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process.
- */
-int sanitize_submodule_config(const char *var, const char *value, void *data);
-
 /*
  * Prepare the "env_array" parameter of a "struct child_process" for executing
  * a submodule by clearing any repo-specific envirionment variables, but
- * retaining any config approved by sanitize_submodule_config().
+ * retaining any config in the environment.
  */
 void prepare_submodule_repo_env(struct argv_array *out);
 

-Peff

Re: [PATCH] submodule: stop sanitizing config options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I had originally thought after the final one that we could further clean
> up by turning prepare_submodule_repo_env() into a static function. But
> we can't; it gets called in one spot from submodule--helper. However,
> while looking at it, I did notice that we probably want to squash this
> into the final patch (since sanitize_submodule_config went away
> completely):
>
> diff --git a/submodule.h b/submodule.h
> index 48690b1..869d259 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -43,19 +43,10 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam
>  int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
>  void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
>  
> -/*
> - * This function is intended as a callback for use with
> - * git_config_from_parameters(). It ignores any config options which
> - * are not suitable for passing along to a submodule, and accumulates the rest
> - * in "data", which must be a pointer to a strbuf. The end result can
> - * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process.
> - */
> -int sanitize_submodule_config(const char *var, const char *value, void *data);
> -
>  /*
>   * Prepare the "env_array" parameter of a "struct child_process" for executing
>   * a submodule by clearing any repo-specific envirionment variables, but
> - * retaining any config approved by sanitize_submodule_config().
> + * retaining any config in the environment.
>   */
>  void prepare_submodule_repo_env(struct argv_array *out);
>  
>
> -Peff

Hmph, Stefan, do you want to keep this (if you want to resurrect the
function in some future, for example)?

Re: [PATCH] submodule: stop sanitizing config options

[Stefan Beller]

On Thu, May 5, 2016 at 4:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> I had originally thought after the final one that we could further clean
>> up by turning prepare_submodule_repo_env() into a static function. But
>> we can't; it gets called in one spot from submodule--helper. However,
>> while looking at it, I did notice that we probably want to squash this
>> into the final patch (since sanitize_submodule_config went away
>> completely):
>>
>> diff --git a/submodule.h b/submodule.h
>> index 48690b1..869d259 100644
>> --- a/submodule.h
>> +++ b/submodule.h
>> @@ -43,19 +43,10 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam
>>  int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
>>  void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
>>
>> -/*
>> - * This function is intended as a callback for use with
>> - * git_config_from_parameters(). It ignores any config options which
>> - * are not suitable for passing along to a submodule, and accumulates the rest
>> - * in "data", which must be a pointer to a strbuf. The end result can
>> - * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process.
>> - */
>> -int sanitize_submodule_config(const char *var, const char *value, void *data);
>> -
>>  /*
>>   * Prepare the "env_array" parameter of a "struct child_process" for executing
>>   * a submodule by clearing any repo-specific envirionment variables, but
>> - * retaining any config approved by sanitize_submodule_config().
>> + * retaining any config in the environment.
>>   */
>>  void prepare_submodule_repo_env(struct argv_array *out);
>>
>>
>> -Peff
>
> Hmph, Stefan, do you want to keep this (if you want to resurrect the
> function in some future, for example)?

IMHO it is easier to revert or rewrite than to carry unused code?
Unused code is not tested and untested code is broken code.
And relying on broken code in the future will guarantee bugs.
(Sure new code may also have bugs, but I just think that bugs
in newly written code can be fixed easier)

I would prefer to get rid of it, i.e. taking that patch.

Thanks,
Stefan

Re: [PATCH] submodule: stop sanitizing config options

[Jeff King]

On Thu, May 05, 2016 at 05:23:51PM -0700, Stefan Beller wrote:

> >> -/*
> >> - * This function is intended as a callback for use with
> >> - * git_config_from_parameters(). It ignores any config options which
> >> - * are not suitable for passing along to a submodule, and accumulates the rest
> >> - * in "data", which must be a pointer to a strbuf. The end result can
> >> - * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process.
> >> - */
> >> -int sanitize_submodule_config(const char *var, const char *value, void *data);
> >> -
> >>  /*
> >>   * Prepare the "env_array" parameter of a "struct child_process" for executing
> >>   * a submodule by clearing any repo-specific envirionment variables, but
> >> - * retaining any config approved by sanitize_submodule_config().
> >> + * retaining any config in the environment.
> >>   */
> >>  void prepare_submodule_repo_env(struct argv_array *out);
> >>
> >>
> >> -Peff
> >
> > Hmph, Stefan, do you want to keep this (if you want to resurrect the
> > function in some future, for example)?
> 
> IMHO it is easier to revert or rewrite than to carry unused code?
> Unused code is not tested and untested code is broken code.
> And relying on broken code in the future will guarantee bugs.
> (Sure new code may also have bugs, but I just think that bugs
> in newly written code can be fixed easier)
> 
> I would prefer to get rid of it, i.e. taking that patch.

Keep in mind this squash is just dropping the _declaration_. The code
itself was dropped in the earlier patch. (And I agree there isn't a good
reason to keep it when we can easily "git revert" later).

-Peff

Re: [PATCH] submodule: stop sanitizing config options

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> Hmph, Stefan, do you want to keep this (if you want to resurrect the
>> function in some future, for example)?
>
> IMHO it is easier to revert or rewrite than to carry unused code?
> Unused code is not tested and untested code is broken code.
> And relying on broken code in the future will guarantee bugs.
> (Sure new code may also have bugs, but I just think that bugs
> in newly written code can be fixed easier)
>
> I would prefer to get rid of it, i.e. taking that patch.

OK.

[PATCH v7 0/3] Add support for sending additional HTTP headers (part 2)

[Johannes Schindelin]

My use case is an army of build agents that need only limited and
selective access to otherwise private repositories.

The first part already made it into `master`, this is the remainder.

This iteration still has the specific patch to make `git -c
http.extraHeader=... submodule update` work; I plan to keep only the
test (and adjust the commit message) as soon as Peff's patch is applied
that skips -c ... sanitizing for submodules.


Johannes Schindelin (3):
  tests: Adjust the configuration for Apache 2.2
  t5551: make the test for extra HTTP headers more robust
  submodule: pass on http.extraheader config settings

 builtin/submodule--helper.c |  3 ++-
 t/lib-httpd/apache.conf     | 12 ++++++++----
 t/t5551-http-fetch-smart.sh | 14 ++++++++++++--
 3 files changed, 22 insertions(+), 7 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v7
Interdiff vs v6:

 diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
 index b8ed96f..29b34bb 100644
 --- a/t/lib-httpd/apache.conf
 +++ b/t/lib-httpd/apache.conf
 @@ -103,10 +103,6 @@ Alias /auth/dumb/ www/auth/dumb/
  	Header set Set-Cookie name=value
  </LocationMatch>
  <LocationMatch /smart_headers/>
 -	<RequireAll>
 -		Require expr %{HTTP:x-magic-one} == 'abra'
 -		Require expr %{HTTP:x-magic-two} == 'cadabra'
 -	</RequireAll>
  	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
  	SetEnv GIT_HTTP_EXPORT_ALL
  </LocationMatch>
 @@ -136,6 +132,14 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]
  RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302]
  RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
  
 +# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
 +# And as RewriteCond unfortunately lacks "not equal" matching, we use this
 +# ugly trick to fail *unless* the two headers are present.
 +RewriteCond %{HTTP:x-magic-one} =abra
 +RewriteCond %{HTTP:x-magic-two} =cadabra
 +RewriteRule ^/smart_headers/.* - [L]
 +RewriteRule ^/smart_headers/.* - [F]
 +
  <IfDefine SSL>
  LoadModule ssl_module modules/mod_ssl.so
  
 diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
 index 1794168..2f375eb 100755
 --- a/t/t5551-http-fetch-smart.sh
 +++ b/t/t5551-http-fetch-smart.sh
 @@ -283,7 +283,8 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
  '
  
  test_expect_success 'custom http headers' '
 -	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
 +	test_must_fail git -c http.extraheader="x-magic-two: cadabra" \
 +		fetch "$HTTPD_URL/smart_headers/repo.git" &&
  	git -c http.extraheader="x-magic-one: abra" \
  	    -c http.extraheader="x-magic-two: cadabra" \
  	    fetch "$HTTPD_URL/smart_headers/repo.git" &&

-- 
2.8.2.463.g99156ee

[PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Johannes Schindelin]

Lars Schneider noticed that the configuration introduced to test the extra
HTTP headers cannot be used with Apache 2.2 (which is still actively
maintained, as pointed out by Junio Hamano).

To let the tests pass with Apache 2.2 again, let's substitute the
offending <RequireAll> and `expr` by using old school RewriteCond
statements.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/lib-httpd/apache.conf | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index b8ed96f..29b34bb 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -103,10 +103,6 @@ Alias /auth/dumb/ www/auth/dumb/
 	Header set Set-Cookie name=value
 </LocationMatch>
 <LocationMatch /smart_headers/>
-	<RequireAll>
-		Require expr %{HTTP:x-magic-one} == 'abra'
-		Require expr %{HTTP:x-magic-two} == 'cadabra'
-	</RequireAll>
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 	SetEnv GIT_HTTP_EXPORT_ALL
 </LocationMatch>
@@ -136,6 +132,14 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]
 RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302]
 RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
 
+# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
+# And as RewriteCond unfortunately lacks "not equal" matching, we use this
+# ugly trick to fail *unless* the two headers are present.
+RewriteCond %{HTTP:x-magic-one} =abra
+RewriteCond %{HTTP:x-magic-two} =cadabra
+RewriteRule ^/smart_headers/.* - [L]
+RewriteRule ^/smart_headers/.* - [F]
+
 <IfDefine SSL>
 LoadModule ssl_module modules/mod_ssl.so
 
-- 
2.8.2.463.g99156ee

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Jeff King]

On Mon, May 09, 2016 at 08:18:52AM +0200, Johannes Schindelin wrote:

> +# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
> +# And as RewriteCond unfortunately lacks "not equal" matching, we use this
> +# ugly trick to fail *unless* the two headers are present.
> +RewriteCond %{HTTP:x-magic-one} =abra
> +RewriteCond %{HTTP:x-magic-two} =cadabra
> +RewriteRule ^/smart_headers/.* - [L]
> +RewriteRule ^/smart_headers/.* - [F]
> +

Thanks, this is the magic that eluded me earlier. I had to look up the
flags, so for any observers in the same boat, this works because:

  - the '[L]' flag says "stop doing any more rewrite rules"; it triggers
    only when the RewriteConds above match

  - the '[F]' flag says "return 403 Forbidden"; it triggers always,
    because after a RewriteRule, all RewriteConds are reset

I'm sure that is all apparent to somebody who is familiar with Apache
config, but I think that does not include most people on this project. I
dunno if it is worth a comment here or in the commit message.

-Peff

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Johannes Schindelin]

Hi Peff,

On Mon, 9 May 2016, Jeff King wrote:

> On Mon, May 09, 2016 at 08:18:52AM +0200, Johannes Schindelin wrote:
> 
> > +# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
> > +# And as RewriteCond unfortunately lacks "not equal" matching, we use this
> > +# ugly trick to fail *unless* the two headers are present.
> > +RewriteCond %{HTTP:x-magic-one} =abra
> > +RewriteCond %{HTTP:x-magic-two} =cadabra
> > +RewriteRule ^/smart_headers/.* - [L]
> > +RewriteRule ^/smart_headers/.* - [F]
> > +
> 
> Thanks, this is the magic that eluded me earlier. I had to look up the
> flags, so for any observers in the same boat, this works because:
> 
>   - the '[L]' flag says "stop doing any more rewrite rules"; it triggers
>     only when the RewriteConds above match
> 
>   - the '[F]' flag says "return 403 Forbidden"; it triggers always,
>     because after a RewriteRule, all RewriteConds are reset
> 
> I'm sure that is all apparent to somebody who is familiar with Apache
> config, but I think that does not include most people on this project. I
> dunno if it is worth a comment here or in the commit message.

Oh, you're absolutely correct, I should have described this better. It
took me quite a couple of iterations to get it right, after all.

How about this:

	As RewriteCond does not allow testing for *non*-matches, we simply
	match the desired case first and let it pass by marking the
	RewriteRule as '[L]' ("last rule, do not process any other
	matching RewriteRules after this"), and then have another
	RewriteRule that matches all other cases and lets them fail via
	'[F]' ("fail").

Good enough?

Ciao,
Dscho

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Jeff King]

On Mon, May 09, 2016 at 04:03:48PM +0200, Johannes Schindelin wrote:

> How about this:
> 
> 	As RewriteCond does not allow testing for *non*-matches, we simply
> 	match the desired case first and let it pass by marking the
> 	RewriteRule as '[L]' ("last rule, do not process any other
> 	matching RewriteRules after this"), and then have another
> 	RewriteRule that matches all other cases and lets them fail via
> 	'[F]' ("fail").
> 
> Good enough?

Yep, I think that explains it. Thanks.

-Peff

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Johannes Schindelin]

Hi Peff,

On Mon, 9 May 2016, Jeff King wrote:

> On Mon, May 09, 2016 at 04:03:48PM +0200, Johannes Schindelin wrote:
> 
> > How about this:
> > 
> > 	As RewriteCond does not allow testing for *non*-matches, we simply
> > 	match the desired case first and let it pass by marking the
> > 	RewriteRule as '[L]' ("last rule, do not process any other
> > 	matching RewriteRules after this"), and then have another
> > 	RewriteRule that matches all other cases and lets them fail via
> > 	'[F]' ("fail").
> > 
> > Good enough?
> 
> Yep, I think that explains it. Thanks.

Okay, I already force-pushed my extra-http-header branch and the next
iteration will sport this paragraph.

Hopefully your patch to remove the -c ... sanitizing makes it to `master`
soon, then I can submit my next iteration.

Ciao,
Dscho

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Junio C Hamano]

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

> Okay, I already force-pushed my extra-http-header branch and the next
> iteration will sport this paragraph.

The new explanation is well written and can and should also replace
the comment before the implementation in the configuration file to
help readers.

To be honest, I do not quite understand why you call it "ugly hack"
at all.  It is saying "The client is sending a satisfactory request
when these two headers are there; otherwise the request is not
good", which sounds quite natural way to express what is being
tested.  The rejection being part of "Rewrite" may be clever, but I
do not see it as ugly.

And it matches the spirit of the implementation for 2.3+ that uses
<RequireAll/> quite well--you just do not need to say "Fail"
yourself over there, as that is implied.

> Hopefully your patch to remove the -c ... sanitizing makes it to `master`
> soon, then I can submit my next iteration.

Or we can just merge that "do not sanitize" branch in, and then
queue the "next iteration" which I'd assume would only be the test
addition?

Thanks.

-- >8 --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mon, 9 May 2016 07:59:16 +0200
Subject: [PATCH] tests: adjust the configuration for Apache 2.2

Lars Schneider noticed that the configuration introduced to test the
extra HTTP headers cannot be used with Apache 2.2 (which is still
actively maintained, as pointed out by Junio Hamano).

To let the tests pass with Apache 2.2 again, let's substitute the
offending <RequireAll> and `expr` by using old school RewriteCond
statements.

As RewriteCond does not allow testing for *non*-matches, we simply match
the desired case first and let it pass by marking the RewriteRule as
'[L]' ("last rule, do not process any other matching RewriteRules after
this"), and then have another RewriteRule that matches all other cases
and lets them fail via '[F]' ("fail").

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/lib-httpd/apache.conf | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 838770c..2dcbb00 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -102,10 +102,6 @@ Alias /auth/dumb/ www/auth/dumb/
 	Header set Set-Cookie name=value
 </LocationMatch>
 <LocationMatch /smart_headers/>
-	<RequireAll>
-		Require expr %{HTTP:x-magic-one} == 'abra'
-		Require expr %{HTTP:x-magic-two} == 'cadabra'
-	</RequireAll>
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 	SetEnv GIT_HTTP_EXPORT_ALL
 </LocationMatch>
@@ -135,6 +131,18 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]
 RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302]
 RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
 
+# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
+# And as RewriteCond does not allow testing for non-matches, we match
+# the desired case first (one has abra, two has cadabra), and let it
+# pass by marking the RewriteRule as [L], "last rule, do not process
+# any other matching RewriteRules after this"), and then have another
+# RewriteRule that matches all other cases and lets them fail via '[F]',
+# "fail the request".
+RewriteCond %{HTTP:x-magic-one} =abra
+RewriteCond %{HTTP:x-magic-two} =cadabra
+RewriteRule ^/smart_headers/.* - [L]
+RewriteRule ^/smart_headers/.* - [F]
+
 <IfDefine SSL>
 LoadModule ssl_module modules/mod_ssl.so
 
-- 
2.8.2-557-gee41d5e

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Jeff King]

On Mon, May 09, 2016 at 09:42:32AM -0700, Junio C Hamano wrote:

> > Hopefully your patch to remove the -c ... sanitizing makes it to `master`
> > soon, then I can submit my next iteration.
> 
> Or we can just merge that "do not sanitize" branch in, and then
> queue the "next iteration" which I'd assume would only be the test
> addition?

I think we'd also want the change to the test script to make sure that
it fails with only a single header (Dscho's patch 2).

-Peff

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, May 09, 2016 at 09:42:32AM -0700, Junio C Hamano wrote:
>
>> > Hopefully your patch to remove the -c ... sanitizing makes it to `master`
>> > soon, then I can submit my next iteration.
>> 
>> Or we can just merge that "do not sanitize" branch in, and then
>> queue the "next iteration" which I'd assume would only be the test
>> addition?
>
> I think we'd also want the change to the test script to make sure that
> it fails with only a single header (Dscho's patch 2).

I think so, too.

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Johannes Schindelin]

Hi Junio,

On Mon, 9 May 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Okay, I already force-pushed my extra-http-header branch and the next
> > iteration will sport this paragraph.
> 
> The new explanation is well written and can and should also replace the
> comment before the implementation in the configuration file to help
> readers.

I picked your commit and force-pushed my branch; it will be part of the
next iteration.

> To be honest, I do not quite understand why you call it "ugly hack"
> at all.

Well, it is convoluted. I would have preferred to say "if this condition
is not met or that condition is not met, fail". Instead I had to say "If`
these two conditions are met, proceed as before. Otherwise, fail."

And of course its ugliness increased in my mind because I had to go
through so many iterations until it finally worked. Not really
straight-forward a solution.

> > Hopefully your patch to remove the -c ... sanitizing makes it to
> > `master` soon, then I can submit my next iteration.
> 
> Or we can just merge that "do not sanitize" branch in, and then queue
> the "next iteration" which I'd assume would only be the test addition?

I'll prepare something.

Ciao,
Dscho

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Junio C Hamano]

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

>> To be honest, I do not quite understand why you call it "ugly hack"
>> at all.
>
> Well, it is convoluted. I would have preferred to say "if this condition
> is not met or that condition is not met, fail". Instead I had to say "If`
> these two conditions are met, proceed as before. Otherwise, fail."
>
> And of course its ugliness increased in my mind because I had to go
> through so many iterations until it finally worked. Not really
> straight-forward a solution.

To my eyes without your battle scar, it felt more natural to say "If
somebody says he is user $U and gives a password $P, let him in;
keep everybody else out" when configuring a server than saying "Fail
anybody whose user name is not $U or who says his password is not
$P".  I can certainly understand if somebody who has spent a lot of
effort to make things _fail_ finds the latter more natural.

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Junio C Hamano]

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

> +# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
> +# And as RewriteCond unfortunately lacks "not equal" matching, we use this
> +# ugly trick to fail *unless* the two headers are present.
> +RewriteCond %{HTTP:x-magic-one} =abra
> +RewriteCond %{HTTP:x-magic-two} =cadabra
> +RewriteRule ^/smart_headers/.* - [L]
> +RewriteRule ^/smart_headers/.* - [F]

Clever.  Thanks.  Will queue.

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Lars Schneider]

> On 09 May 2016, at 08:18, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> 
> Lars Schneider noticed that the configuration introduced to test the extra
> HTTP headers cannot be used with Apache 2.2 (which is still actively
> maintained, as pointed out by Junio Hamano).
> 
> To let the tests pass with Apache 2.2 again, let's substitute the
> offending <RequireAll> and `expr` by using old school RewriteCond
> statements.

All Apache 2.2 tests run nicely on Travis CI with Ubuntu and OSX using
this patch series:
https://travis-ci.org/larsxschneider/git/builds/128955548

Thanks,
Lars

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> t/lib-httpd/apache.conf | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index b8ed96f..29b34bb 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -103,10 +103,6 @@ Alias /auth/dumb/ www/auth/dumb/
> 	Header set Set-Cookie name=value
> </LocationMatch>
> <LocationMatch /smart_headers/>
> -	<RequireAll>
> -		Require expr %{HTTP:x-magic-one} == 'abra'
> -		Require expr %{HTTP:x-magic-two} == 'cadabra'
> -	</RequireAll>
> 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
> 	SetEnv GIT_HTTP_EXPORT_ALL
> </LocationMatch>
> @@ -136,6 +132,14 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]
> RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302]
> RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
> 
> +# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
> +# And as RewriteCond unfortunately lacks "not equal" matching, we use this
> +# ugly trick to fail *unless* the two headers are present.
> +RewriteCond %{HTTP:x-magic-one} =abra
> +RewriteCond %{HTTP:x-magic-two} =cadabra
> +RewriteRule ^/smart_headers/.* - [L]
> +RewriteRule ^/smart_headers/.* - [F]
> +
> <IfDefine SSL>
> LoadModule ssl_module modules/mod_ssl.so
> 
> -- 
> 2.8.2.463.g99156ee
> 
> 

Re: [PATCH v7 1/3] tests: Adjust the configuration for Apache 2.2

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

>> On 09 May 2016, at 08:18, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
>> 
>> Lars Schneider noticed that the configuration introduced to test the extra
>> HTTP headers cannot be used with Apache 2.2 (which is still actively
>> maintained, as pointed out by Junio Hamano).
>> 
>> To let the tests pass with Apache 2.2 again, let's substitute the
>> offending <RequireAll> and `expr` by using old school RewriteCond
>> statements.
>
> All Apache 2.2 tests run nicely on Travis CI with Ubuntu and OSX using
> this patch series:
> https://travis-ci.org/larsxschneider/git/builds/128955548
>
> Thanks,
> Lars

Thanks for a report.

[PATCH v7 2/3] t5551: make the test for extra HTTP headers more robust

[Johannes Schindelin]

To test that extra HTTP headers are passed correctly, t5551 verifies that
a fetch succeeds when two required headers are passed, and that the fetch
does not succeed when those headers are not passed.

However, this test would also succeed if the configuration required only
one header. As Apache's configuration is notoriously tricky (this
developer frequently requires StackOverflow's help to understand Apache's
documentation), especially when still supporting the 2.2 line, let's just
really make sure that the test verifies what we want it to verify.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5551-http-fetch-smart.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index e44fe72..43b257e 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -283,7 +283,8 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
 '
 
 test_expect_success 'custom http headers' '
-	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
+	test_must_fail git -c http.extraheader="x-magic-two: cadabra" \
+		fetch "$HTTPD_URL/smart_headers/repo.git" &&
 	git -c http.extraheader="x-magic-one: abra" \
 	    -c http.extraheader="x-magic-two: cadabra" \
 	    fetch "$HTTPD_URL/smart_headers/repo.git"
-- 
2.8.2.463.g99156ee

Re: [PATCH v7 2/3] t5551: make the test for extra HTTP headers more robust

[Lars Schneider]

On 09 May 2016, at 08:19, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

> To test that extra HTTP headers are passed correctly, t5551 verifies that
> a fetch succeeds when two required headers are passed, and that the fetch
> does not succeed when those headers are not passed.
> 
> However, this test would also succeed if the configuration required only
> one header. As Apache's configuration is notoriously tricky (this
> developer frequently requires StackOverflow's help to understand Apache's
> documentation), especially when still supporting the 2.2 line, let's just
> really make sure that the test verifies what we want it to verify.

Haha. Me, too :-) After I wasn't able to find a working solution myself
I posted a question on ServerFault [1] ... I will test your solution
tomorrow!

Cheers,
Lars

[1] https://serverfault.com/questions/775515/requireall-require-expr-equivalent-in-apache-2-2-22-to-check-for-headers

Re: [PATCH v7 2/3] t5551: make the test for extra HTTP headers more robust

[Jeff King]

On Mon, May 09, 2016 at 08:19:00AM +0200, Johannes Schindelin wrote:

> To test that extra HTTP headers are passed correctly, t5551 verifies that
> a fetch succeeds when two required headers are passed, and that the fetch
> does not succeed when those headers are not passed.
> 
> However, this test would also succeed if the configuration required only
> one header. As Apache's configuration is notoriously tricky (this
> developer frequently requires StackOverflow's help to understand Apache's
> documentation), especially when still supporting the 2.2 line, let's just
> really make sure that the test verifies what we want it to verify.

Agreed, this makes sense.

>  test_expect_success 'custom http headers' '
> -	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
> +	test_must_fail git -c http.extraheader="x-magic-two: cadabra" \
> +		fetch "$HTTPD_URL/smart_headers/repo.git" &&
>  	git -c http.extraheader="x-magic-one: abra" \
>  	    -c http.extraheader="x-magic-two: cadabra" \
>  	    fetch "$HTTPD_URL/smart_headers/repo.git"

This loses the 0-header check, but I don't think that is particularly
interesting to us (I had originally wanted to double-check that our
apache config worked at all in the absence of this feature, but I think
it is OK for the 1-header case to cover this; if our code is so buggy we
accidentally send 0 headers in the first command, we'll catch that,
too).

So looks good to me.

-Peff

Re: [PATCH v7 2/3] t5551: make the test for extra HTTP headers more robust

[Johannes Schindelin]

Hi Peff,

do you sleep at all?

On Mon, 9 May 2016, Jeff King wrote:

> On Mon, May 09, 2016 at 08:19:00AM +0200, Johannes Schindelin wrote:
> 
> >  test_expect_success 'custom http headers' '
> > -	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
> > +	test_must_fail git -c http.extraheader="x-magic-two: cadabra" \
> > +		fetch "$HTTPD_URL/smart_headers/repo.git" &&
> >  	git -c http.extraheader="x-magic-one: abra" \
> >  	    -c http.extraheader="x-magic-two: cadabra" \
> >  	    fetch "$HTTPD_URL/smart_headers/repo.git"
> 
> This loses the 0-header check, but I don't think that is particularly
> interesting to us (I had originally wanted to double-check that our
> apache config worked at all in the absence of this feature, but I think
> it is OK for the 1-header case to cover this; if our code is so buggy we
> accidentally send 0 headers in the first command, we'll catch that,
> too).

Yeah, a faulty Apache config will unfortunately *skip* the entire test, as
httpd refuses to start.

And I also considered testing 0-header and 1st header only. But as you
know, Git's test suite takes already 3.5h in a moderately sized Windows
VM, so I am really reluctant to add overly extensive tests.

> So looks good to me.

Thanks,
Dscho

Re: [PATCH v7 2/3] t5551: make the test for extra HTTP headers more robust

[Jeff King]

On Mon, May 09, 2016 at 10:13:51AM +0200, Johannes Schindelin wrote:

> do you sleep at all?

Actually, I just woke up. Nothing like some Git ML to get the blood
pumping in the morning.

> Yeah, a faulty Apache config will unfortunately *skip* the entire test, as
> httpd refuses to start.

If you care about this, you can set GIT_TEST_HTTPD=true. The default is
"auto", which will skip whenever apache setup fails. But with "true", it
will show a hard error (so at least your "make test" will report the
failure).

-Peff

[PATCH v7 3/3] submodule: pass on http.extraheader config settings

[Johannes Schindelin]

To support this developer's use case of allowing build agents token-based
access to private repositories, we introduced the http.extraheader
feature, allowing extra HTTP headers to be sent along with every HTTP
request.

This patch allows us to configure these extra HTTP headers for use with
`git submodule update`, too. It requires somewhat special handling:
submodules do not share the parent project's config. It would be
incorrect to simply reuse that specific part of the parent's config.
Instead, the config option needs to be specified on the command-line or
in ~/.gitconfig or friends.

Example: git -c http.extraheader="Secret: Sauce" submodule update --init

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/submodule--helper.c |  3 ++-
 t/t5551-http-fetch-smart.sh | 11 ++++++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3bd6883..789e081 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -127,7 +127,8 @@ static int module_name(int argc, const char **argv, const char *prefix)
  */
 static int submodule_config_ok(const char *var)
 {
-	if (starts_with(var, "credential."))
+	if (starts_with(var, "credential.") ||
+	    (starts_with(var, "http.") && ends_with(var, ".extraheader")))
 		return 1;
 	return 0;
 }
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 43b257e..2f375eb 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -287,7 +287,16 @@ test_expect_success 'custom http headers' '
 		fetch "$HTTPD_URL/smart_headers/repo.git" &&
 	git -c http.extraheader="x-magic-one: abra" \
 	    -c http.extraheader="x-magic-two: cadabra" \
-	    fetch "$HTTPD_URL/smart_headers/repo.git"
+	    fetch "$HTTPD_URL/smart_headers/repo.git" &&
+	git update-index --add --cacheinfo 160000,$(git rev-parse HEAD),sub &&
+	git config -f .gitmodules submodule.sub.path sub &&
+	git config -f .gitmodules submodule.sub.url \
+		"$HTTPD_URL/smart_headers/repo.git" &&
+	git submodule init sub &&
+	test_must_fail git submodule update sub &&
+	git -c http.extraheader="x-magic-one: abra" \
+	    -c http.extraheader="x-magic-two: cadabra" \
+		submodule update sub
 '
 
 stop_httpd
-- 
2.8.2.463.g99156ee

[PATCH v8 0/3] Add support for sending additional HTTP headers (part 2)

[Johannes Schindelin]

My use case is an army of build agents that need only limited and
selective access to otherwise private repositories.

The first part already made it into `master`, this is the remainder.

This iteration is based on 'jk/submodule-c-credential' and therefore
converted the original config-sanitizing patch into a test-only patch.
This iteration also replaces the "ugly" comment with the explanation
preferred by Junio.


Johannes Schindelin (3):
  tests: adjust the configuration for Apache 2.2
  t5551: make the test for extra HTTP headers more robust
  submodule: ensure that -c http.extraheader is heeded

 t/lib-httpd/apache.conf     | 16 ++++++++++++----
 t/t5551-http-fetch-smart.sh | 14 ++++++++++++--
 2 files changed, 24 insertions(+), 6 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v8
-- 
Interdiff vs v7:

 diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
 [no longer applies; skipped]
 diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
 index 29b34bb..018a83a 100644
 --- a/t/lib-httpd/apache.conf
 +++ b/t/lib-httpd/apache.conf
 @@ -133,8 +133,12 @@ RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302
  RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
  
  # Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
 -# And as RewriteCond unfortunately lacks "not equal" matching, we use this
 -# ugly trick to fail *unless* the two headers are present.
 +# And as RewriteCond does not allow testing for non-matches, we match
 +# the desired case first (one has abra, two has cadabra), and let it
 +# pass by marking the RewriteRule as [L], "last rule, do not process
 +# any other matching RewriteRules after this"), and then have another
 +# RewriteRule that matches all other cases and lets them fail via '[F]',
 +# "fail the request".
  RewriteCond %{HTTP:x-magic-one} =abra
  RewriteCond %{HTTP:x-magic-two} =cadabra
  RewriteRule ^/smart_headers/.* - [L]

2.8.2.463.g99156ee

[PATCH v8 1/3] tests: adjust the configuration for Apache 2.2

[Johannes Schindelin]

Lars Schneider noticed that the configuration introduced to test the
extra HTTP headers cannot be used with Apache 2.2 (which is still
actively maintained, as pointed out by Junio Hamano).

To let the tests pass with Apache 2.2 again, let's substitute the
offending <RequireAll> and `expr` by using old school RewriteCond
statements.

As RewriteCond does not allow testing for *non*-matches, we simply match
the desired case first and let it pass by marking the RewriteRule as
'[L]' ("last rule, do not process any other matching RewriteRules after
this"), and then have another RewriteRule that matches all other cases
and lets them fail via '[F]' ("fail").

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/lib-httpd/apache.conf | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index b8ed96f..018a83a 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -103,10 +103,6 @@ Alias /auth/dumb/ www/auth/dumb/
 	Header set Set-Cookie name=value
 </LocationMatch>
 <LocationMatch /smart_headers/>
-	<RequireAll>
-		Require expr %{HTTP:x-magic-one} == 'abra'
-		Require expr %{HTTP:x-magic-two} == 'cadabra'
-	</RequireAll>
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 	SetEnv GIT_HTTP_EXPORT_ALL
 </LocationMatch>
@@ -136,6 +132,18 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]
 RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302]
 RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
 
+# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
+# And as RewriteCond does not allow testing for non-matches, we match
+# the desired case first (one has abra, two has cadabra), and let it
+# pass by marking the RewriteRule as [L], "last rule, do not process
+# any other matching RewriteRules after this"), and then have another
+# RewriteRule that matches all other cases and lets them fail via '[F]',
+# "fail the request".
+RewriteCond %{HTTP:x-magic-one} =abra
+RewriteCond %{HTTP:x-magic-two} =cadabra
+RewriteRule ^/smart_headers/.* - [L]
+RewriteRule ^/smart_headers/.* - [F]
+
 <IfDefine SSL>
 LoadModule ssl_module modules/mod_ssl.so
 
-- 
2.8.2.463.g99156ee

Re: [PATCH v8 1/3] tests: adjust the configuration for Apache 2.2

[Junio C Hamano]

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

> Lars Schneider noticed that the configuration introduced to test the
> extra HTTP headers cannot be used with Apache 2.2 (which is still
> actively maintained, as pointed out by Junio Hamano).
>
> To let the tests pass with Apache 2.2 again, let's substitute the
> offending <RequireAll> and `expr` by using old school RewriteCond
> statements.
>
> As RewriteCond does not allow testing for *non*-matches, we simply match
> the desired case first and let it pass by marking the RewriteRule as
> '[L]' ("last rule, do not process any other matching RewriteRules after
> this"), and then have another RewriteRule that matches all other cases
> and lets them fail via '[F]' ("fail").
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

I applied this and compared what was queued from the previous round.
It turns out that it is the same, except that I amended it with
"Tested-by:" from Lars', so I'll skip this and nothing is lost ;-)

Thanks for being thorough (the above is not a suggestion to omit
unchanged ones next time--quite the contrary, being able to verify
by comparing is a good thing).

>  t/lib-httpd/apache.conf | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index b8ed96f..018a83a 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -103,10 +103,6 @@ Alias /auth/dumb/ www/auth/dumb/
>  	Header set Set-Cookie name=value
>  </LocationMatch>
>  <LocationMatch /smart_headers/>
> -	<RequireAll>
> -		Require expr %{HTTP:x-magic-one} == 'abra'
> -		Require expr %{HTTP:x-magic-two} == 'cadabra'
> -	</RequireAll>
>  	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
>  	SetEnv GIT_HTTP_EXPORT_ALL
>  </LocationMatch>
> @@ -136,6 +132,18 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]
>  RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302]
>  RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
>  
> +# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
> +# And as RewriteCond does not allow testing for non-matches, we match
> +# the desired case first (one has abra, two has cadabra), and let it
> +# pass by marking the RewriteRule as [L], "last rule, do not process
> +# any other matching RewriteRules after this"), and then have another
> +# RewriteRule that matches all other cases and lets them fail via '[F]',
> +# "fail the request".
> +RewriteCond %{HTTP:x-magic-one} =abra
> +RewriteCond %{HTTP:x-magic-two} =cadabra
> +RewriteRule ^/smart_headers/.* - [L]
> +RewriteRule ^/smart_headers/.* - [F]
> +
>  <IfDefine SSL>
>  LoadModule ssl_module modules/mod_ssl.so

[PATCH v8 2/3] t5551: make the test for extra HTTP headers more robust

[Johannes Schindelin]

To test that extra HTTP headers are passed correctly, t5551 verifies that
a fetch succeeds when two required headers are passed, and that the fetch
does not succeed when those headers are not passed.

However, this test would also succeed if the configuration required only
one header. As Apache's configuration is notoriously tricky (this
developer frequently requires StackOverflow's help to understand Apache's
documentation), especially when still supporting the 2.2 line, let's just
really make sure that the test verifies what we want it to verify.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5551-http-fetch-smart.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index e44fe72..43b257e 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -283,7 +283,8 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
 '
 
 test_expect_success 'custom http headers' '
-	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
+	test_must_fail git -c http.extraheader="x-magic-two: cadabra" \
+		fetch "$HTTPD_URL/smart_headers/repo.git" &&
 	git -c http.extraheader="x-magic-one: abra" \
 	    -c http.extraheader="x-magic-two: cadabra" \
 	    fetch "$HTTPD_URL/smart_headers/repo.git"
-- 
2.8.2.463.g99156ee

Re: [PATCH v8 2/3] t5551: make the test for extra HTTP headers more robust

[Junio C Hamano]

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

> To test that extra HTTP headers are passed correctly, t5551 verifies that
> a fetch succeeds when two required headers are passed, and that the fetch
> does not succeed when those headers are not passed.
>
> However, this test would also succeed if the configuration required only
> one header. As Apache's configuration is notoriously tricky (this
> developer frequently requires StackOverflow's help to understand Apache's
> documentation), especially when still supporting the 2.2 line, let's just
> really make sure that the test verifies what we want it to verify.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Matches the previous one I queued with Reviewed-by: from Peff; good.

>  t/t5551-http-fetch-smart.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index e44fe72..43b257e 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -283,7 +283,8 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
>  '
>  
>  test_expect_success 'custom http headers' '
> -	test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
> +	test_must_fail git -c http.extraheader="x-magic-two: cadabra" \
> +		fetch "$HTTPD_URL/smart_headers/repo.git" &&
>  	git -c http.extraheader="x-magic-one: abra" \
>  	    -c http.extraheader="x-magic-two: cadabra" \
>  	    fetch "$HTTPD_URL/smart_headers/repo.git"

t5551 hangs ?

[Torsten Bögershausen]

On 10.05.16 09:08, Johannes Schindelin wrote:
- I'm not sure, if this is the right thread to report on -

It seems as if t5551 is hanging ?
This is the last line from the log:
ok 25 - large fetch-pack requests can be split across POSTs

I have 7 such processes running:
/trash directory.t5551-http-fetch-smart/httpd -f
/Users/tb/projects/git/git.pu/t/lib-httpd/apache.conf -DDarwin -c Listen
127.0.0.1:5551 -k start

This happens both under Mac OS X and Debian.

Does anybody have the same hanging ?

Re: t5551 hangs ?

[Jeff King]

On Wed, May 11, 2016 at 07:13:56PM +0200, Torsten Bögershausen wrote:

> On 10.05.16 09:08, Johannes Schindelin wrote:
> - I'm not sure, if this is the right thread to report on -
> 
> It seems as if t5551 is hanging ?
> This is the last line from the log:
> ok 25 - large fetch-pack requests can be split across POSTs

Are you running the tests with "--long" or GIT_TEST_LONG in the
environment? The next line should show it skipping test 26 unless one of
those is set.

If you are, can you confirm that it's actually hanging, and not just
slow? On my system, test 26 takes about a minute to run (which is why we
don't do it by default).

> I have 7 such processes running:
> /trash directory.t5551-http-fetch-smart/httpd -f
> /Users/tb/projects/git/git.pu/t/lib-httpd/apache.conf -DDarwin -c Listen
> 127.0.0.1:5551 -k start

That's normal while the test is running; apache pre-forks a bunch of
worker threads.

-Peff

Re: t5551 hangs ?

[Torsten Bögershausen]

On 11.05.16 19:31, Jeff King wrote:
> On Wed, May 11, 2016 at 07:13:56PM +0200, Torsten Bögershausen wrote:
> 
>> On 10.05.16 09:08, Johannes Schindelin wrote:
>> - I'm not sure, if this is the right thread to report on -
>>
>> It seems as if t5551 is hanging ?
>> This is the last line from the log:
>> ok 25 - large fetch-pack requests can be split across POSTs
> 
> Are you running the tests with "--long" or GIT_TEST_LONG in the
> environment? The next line should show it skipping test 26 unless one of
> those is set.
Yes
> 
> If you are, can you confirm that it's actually hanging, and not just
> slow? On my system, test 26 takes about a minute to run (which is why we
> don't do it by default).
Nearly sure. After 10 minutes, the test was still running.

Yesterday another machine was running even longer.

Any tips, how to debug, are welcome.

Re: t5551 hangs ?

[Jeff King]

On Wed, May 11, 2016 at 10:03:45PM +0200, Torsten Bögershausen wrote:

> > If you are, can you confirm that it's actually hanging, and not just
> > slow? On my system, test 26 takes about a minute to run (which is why we
> > don't do it by default).
> Nearly sure. After 10 minutes, the test was still running.
> 
> Yesterday another machine was running even longer.
> 
> Any tips, how to debug, are welcome.

Try running with "-x" to see what the test is doing. It will probably be
in:

   + git -C too-many-refs fetch -q --tags

after a while. Check "ps" to see if you have a fetch-pack sub-process
running. It should be writing "have" lines and reading lots of ACK
lines, which you can check via strace.

If it's blocked on read() or write(), then it's probably some kind of
I/O deadlock.

-Peff

Re: t5551 hangs ?

[Torsten Bögershausen]

On 12.05.16 05:16, Jeff King wrote:
> On Wed, May 11, 2016 at 10:03:45PM +0200, Torsten Bögershausen wrote:
> 
>>> If you are, can you confirm that it's actually hanging, and not just
>>> slow? On my system, test 26 takes about a minute to run (which is why we
>>> don't do it by default).
>> Nearly sure. After 10 minutes, the test was still running.
>>
>> Yesterday another machine was running even longer.
>>
>> Any tips, how to debug, are welcome.
> 
> Try running with "-x" to see what the test is doing. It will probably be
> in:
> 
>    + git -C too-many-refs fetch -q --tags
> 
> after a while. Check "ps" to see if you have a fetch-pack sub-process
> running. It should be writing "have" lines and reading lots of ACK
> lines, which you can check via strace.
> 
> If it's blocked on read() or write(), then it's probably some kind of
> I/O deadlock.
> 
> -Peff

This is the last log that I see:
---------------------------------------------------------------------
pack_report: getpagesize()            =       4096
pack_report: core.packedGitWindowSize = 1073741824
pack_report: core.packedGitLimit      = 8589934592
pack_report: pack_used_ctr            =      96001
pack_report: pack_mmap_calls          =      48002
pack_report: pack_open_windows        =          2 /          2
pack_report: pack_mapped              =    6605494 /    6605494
---------------------------------------------------------------------

+++ perl -e 'print "bla" x 30'
+++ command /usr/bin/perl -e 'print "bla" x 30'
+++ /usr/bin/perl -e 'print "bla" x 30'
++ tag=blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla
++ sed -e 's|^:\([^ ]*\) \(.*\)$|\2 refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-\1|'
++ git -C too-many-refs fetch -q --tags

And this may be the processes :
(Not sure, probaly need to reboot & clean ?)
/bin/sh ./t5551-http-fetch-smart.sh -x
73459 ttys010    0:21.45 /Users/tb/projects/git/git.pu/git -C too-many-refs fetch -q --tags
73460 ttys010    0:00.40 git-remote-http origin http://127.0.0.1:5551/smart/repo.git

 ps | grep fetch
73540 ttys006    0:00.00 grep fetch
73025 ttys010    0:00.14 /bin/sh ./t5551-http-fetch-smart.sh -x
73459 ttys010    3:40.70 /Users/tb/projects/git/git.pu/git -C too-many-refs fetch -q --tags


Beside that, reverting the last 2 commits on 5551 doesn't seem to help:
 Revert "t5551: make the test for extra HTTP headers more robust"
 Revert "submodule: ensure that -c http.extraheader is heeded"

Re: t5551 hangs ?

[Jeff King]

On Thu, May 12, 2016 at 08:21:10AM +0200, Torsten Bögershausen wrote:

> This is the last log that I see:
> [...]
> ++ git -C too-many-refs fetch -q --tags

Not surprising.

> And this may be the processes :
> (Not sure, probaly need to reboot & clean ?)

If you're killing the hung test with "^C", you shouldn't need to; that
tries to clean up any processes and shut down apache.

> /bin/sh ./t5551-http-fetch-smart.sh -x
> 73459 ttys010    0:21.45 /Users/tb/projects/git/git.pu/git -C too-many-refs fetch -q --tags
> 73460 ttys010    0:00.40 git-remote-http origin http://127.0.0.1:5551/smart/repo.git
> 
>  ps | grep fetch
> 73540 ttys006    0:00.00 grep fetch
> 73025 ttys010    0:00.14 /bin/sh ./t5551-http-fetch-smart.sh -x
> 73459 ttys010    3:40.70 /Users/tb/projects/git/git.pu/git -C too-many-refs fetch -q --tags

I'm surprised not to see fetch-pack in that list. And to see so much CPU
going to fetch itself. But perhaps you are simply at a different stage
in the test. 3:40 of CPU time is a lot (the whole thing runs in under a
minute on my machine).

Hmm. Switching to "pu" seems to make things slow on my machine, too, and
the time all goes to fetch. So perhaps there is some recent regression
there. It should be bisectable.

-Peff

Re: t5551 hangs ?

[Jeff King]

On Thu, May 12, 2016 at 02:40:39AM -0400, Jeff King wrote:

> Hmm. Switching to "pu" seems to make things slow on my machine, too, and
> the time all goes to fetch. So perhaps there is some recent regression
> there. It should be bisectable.

It's 66d33af21bd1e398973414435af43d06f2e2099c. I don't think it's
hanging, but it is _really_ slow. I'll reply to that patch separately
with a report.

-Peff

[PATCH v8 3/3] submodule: ensure that -c http.extraheader is heeded

[Johannes Schindelin]

To support this developer's use case of allowing build agents token-based
access to private repositories, we introduced the http.extraheader
feature, allowing extra HTTP headers to be sent along with every HTTP
request.

This patch verifies that we can configure these extra HTTP headers via the
command-line for use with `git submodule update`, too. Example: git -c
http.extraheader="Secret: Sauce" submodule update --init

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5551-http-fetch-smart.sh | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 43b257e..2f375eb 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -287,7 +287,16 @@ test_expect_success 'custom http headers' '
 		fetch "$HTTPD_URL/smart_headers/repo.git" &&
 	git -c http.extraheader="x-magic-one: abra" \
 	    -c http.extraheader="x-magic-two: cadabra" \
-	    fetch "$HTTPD_URL/smart_headers/repo.git"
+	    fetch "$HTTPD_URL/smart_headers/repo.git" &&
+	git update-index --add --cacheinfo 160000,$(git rev-parse HEAD),sub &&
+	git config -f .gitmodules submodule.sub.path sub &&
+	git config -f .gitmodules submodule.sub.url \
+		"$HTTPD_URL/smart_headers/repo.git" &&
+	git submodule init sub &&
+	test_must_fail git submodule update sub &&
+	git -c http.extraheader="x-magic-one: abra" \
+	    -c http.extraheader="x-magic-two: cadabra" \
+		submodule update sub
 '
 
 stop_httpd
-- 
2.8.2.463.g99156ee

Re: [PATCH v8 3/3] submodule: ensure that -c http.extraheader is heeded

[Junio C Hamano]

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

> To support this developer's use case of allowing build agents token-based
> access to private repositories, we introduced the http.extraheader
> feature, allowing extra HTTP headers to be sent along with every HTTP
> request.
>
> This patch verifies that we can configure these extra HTTP headers via the
> command-line for use with `git submodule update`, too. Example: git -c
> http.extraheader="Secret: Sauce" submodule update --init
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Applying this directly on top of the other two fails, and when
merged with jk/submodule-c-credential, the test passes.

Which is exactly what we expect to see.  Nice.

I'll merge jk/submodule-c-credential into js/http-custom-headers
that already has 1 & 2, and then apply this.  An alternative would
be to hold this and wait until both jk/submodule-c-credential and
js/http-custom-headers with 1 & 2 graduates and then apply this,
which is a lot inferior option.

Thanks.

>  t/t5551-http-fetch-smart.sh | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 43b257e..2f375eb 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -287,7 +287,16 @@ test_expect_success 'custom http headers' '
>  		fetch "$HTTPD_URL/smart_headers/repo.git" &&
>  	git -c http.extraheader="x-magic-one: abra" \
>  	    -c http.extraheader="x-magic-two: cadabra" \
> -	    fetch "$HTTPD_URL/smart_headers/repo.git"
> +	    fetch "$HTTPD_URL/smart_headers/repo.git" &&
> +	git update-index --add --cacheinfo 160000,$(git rev-parse HEAD),sub &&
> +	git config -f .gitmodules submodule.sub.path sub &&
> +	git config -f .gitmodules submodule.sub.url \
> +		"$HTTPD_URL/smart_headers/repo.git" &&
> +	git submodule init sub &&
> +	test_must_fail git submodule update sub &&
> +	git -c http.extraheader="x-magic-one: abra" \
> +	    -c http.extraheader="x-magic-two: cadabra" \
> +		submodule update sub
>  '
>  
>  stop_httpd

Re: [PATCH v8 3/3] submodule: ensure that -c http.extraheader is heeded

[Johannes Schindelin]

Hi Junio,

On Tue, 10 May 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > To support this developer's use case of allowing build agents token-based
> > access to private repositories, we introduced the http.extraheader
> > feature, allowing extra HTTP headers to be sent along with every HTTP
> > request.
> >
> > This patch verifies that we can configure these extra HTTP headers via the
> > command-line for use with `git submodule update`, too. Example: git -c
> > http.extraheader="Secret: Sauce" submodule update --init
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> 
> Applying this directly on top of the other two fails, and when
> merged with jk/submodule-c-credential, the test passes.
> 
> Which is exactly what we expect to see.  Nice.

Good.

> I'll merge jk/submodule-c-credential into js/http-custom-headers
> that already has 1 & 2, and then apply this.  An alternative would
> be to hold this and wait until both jk/submodule-c-credential and
> js/http-custom-headers with 1 & 2 graduates and then apply this,
> which is a lot inferior option.

As long as it ends up in 2.9.0, I am happy with whatever path these
patches take ;-)

Thanks,
Dscho

mail-patch-series.sh, was Re: [PATCH v7 0/3] Add support for sending additional HTTP headers (part 2)

[Johannes Schindelin]

Hi Lars,

On Tue, 10 May 2016, Lars Schneider wrote:

> > On 09 May 2016, at 08:18, Johannes Schindelin
> > <johannes.schindelin@gmx.de> wrote:
> > 
> > [...]
> > 
> > Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v7
> > Interdiff vs v6:
> 
> Published-As and Interdiff are really neat. I assume you have some kind
> of script to create all this? If yes, are you willing to share it?

Thanks for prodding me. I had planned to clean up my script and make it
public for a long time already, as I think that submitting code to the Git
project is quite a little bit too tedious, even for old timers like me, in
particular when compared to the ease of opening Pull Requests a la GitHub.

So now I cleaned up my script a bit and published it here:

	https://github.com/dscho/mail-patch-series

Ironically, I welcome Pull Requests for this script ;-)

Ciao,
Dscho