Re: [PATCH 2/3] remote-curl: tighten "version 2" check for smart-http

[Josh Steadmon]

On 2018.11.16 03:48, Jeff King wrote:
> In a v2 smart-http conversation, the server should reply to our initial
> request with a pkt-line saying "version 2" (this is the start of the
> "capabilities advertisement"). We check for the string using
> starts_with(), but that's overly permissive (it would match "version
> 20", for example).
> 
> Let's tighten this check to use strcmp(). Note that we don't need to
> worry about a trailing newline here, because the ptk-line code will have
> chomped it for us already.
> 
> Signed-off-by: Jeff King 
> ---
>  remote-curl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/remote-curl.c b/remote-curl.c
> index dd9bc41aa1..3c9c4a07c3 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -375,7 +375,7 @@ static void check_smart_http(struct discovery *d, const 
> char *service,
>   d->len = src_len;
>   d->proto_git = 1;
>  
> - } else if (starts_with(line, "version 2")) {
> + } else if (!strcmp(line, "version 2")) {
>   /*
>* v2 smart http; do not consume version packet, which will
>* be handled elsewhere.
> -- 
> 2.19.1.1636.gc7a073d580
> 

Looks good to me.

Reviewed-by: Josh Steadmon 

Re: [PATCH 1/3] remote-curl: refactor smart-http discovery

[Josh Steadmon]

On 2018.11.16 03:47, Jeff King wrote:
> After making initial contact with an http server, we have to decide if
> the server supports smart-http, and if so, which version. Our rules are
> a bit inconsistent:
> 
>   1. For v0, we require that the content-type indicates a smart-http
>  response. We also require the response to look vaguely like a
>  pkt-line starting with "#". If one of those does not match, we fall
>  back to dumb-http.
> 
>  But according to our http protocol spec[1]:
> 
>Dumb servers MUST NOT return a return type starting with
>`application/x-git-`.
> 
>  If we see the expected content-type, we should consider it
>  smart-http. At that point we can parse the pkt-line for real, and
>  complain if it is not syntactically valid.
> 
>   2. For v2, we do not actually check the content-type. Our v2 protocol
>  spec says[2]:
> 
>When using the http:// or https:// transport a client makes a
>"smart" info/refs request as described in `http-protocol.txt`[...]
> 
>  and the http spec is clear that for a smart-http[3]:
> 
>The Content-Type MUST be `application/x-$servicename-advertisement`.
> 
>  So it is required according to the spec.
> 
> These inconsistencies were easy to miss because of the way the original
> code was written as an inline conditional. Let's pull it out into its
> own function for readability, and improve a few things:
> 
>  - we now predicate the smart/dumb decision entirely on the presence of
>the correct content-type
> 
>  - we do a real pkt-line parse before deciding how to proceed (and die
>if it isn't valid)
> 
>  - use skip_prefix() for comparing service strings, instead of
>constructing expected output in a strbuf; this avoids dealing with
>memory cleanup
> 
> Note that this _is_ tightening what the client will allow. It's all
> according to the spec, but it's possible that other implementations
> might violate these. However, violating these particular rules seems
> like an odd choice for a server to make.
> 
> [1] Documentation/technical/http-protocol.txt, l. 166-167
> [2] Documentation/technical/protocol-v2.txt, l. 63-64
> [3] Documentation/technical/http-protocol.txt, l. 247
> 
> Helped-by: Josh Steadmon 
> Signed-off-by: Jeff King 
> ---
>  remote-curl.c | 93 ---
>  1 file changed, 59 insertions(+), 34 deletions(-)
> 
> diff --git a/remote-curl.c b/remote-curl.c
> index 762a55a75f..dd9bc41aa1 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -330,9 +330,65 @@ static int get_protocol_http_header(enum 
> protocol_version version,
>   return 0;
>  }
>  
> +static void check_smart_http(struct discovery *d, const char *service,
> +  struct strbuf *type)
> +{
> + char *src_buf;
> + size_t src_len;
> + char *line;
> + const char *p;
> +
> + /*
> +  * If we don't see x-$service-advertisement, then it's not smart-http.
> +  * But once we do, we commit to it and assume any other protocol
> +  * violations are hard errors.
> +  */
> + if (!skip_prefix(type->buf, "application/x-", ) ||
> + !skip_prefix(p, service, ) ||
> + strcmp(p, "-advertisement"))
> + return;
> +
> + /*
> +  * "Peek" at the first packet by using a separate buf/len pair; some
> +  * cases below require us leaving the originals intact.
> +  */
> + src_buf = d->buf;
> + src_len = d->len;
> + line = packet_read_line_buf(_buf, _len, NULL);
> + if (!line)
> + die("invalid server response; expected service, got flush 
> packet");
> +
> + if (skip_prefix(line, "# service=", ) && !strcmp(p, service)) {
> + /*
> +  * The header can include additional metadata lines, up
> +  * until a packet flush marker.  Ignore these now, but
> +  * in the future we might start to scan them.
> +  */
> + while (packet_read_line_buf(_buf, _len, NULL))
> + ;
> +
> + /*
> +  * v0 smart http; callers expect us to soak up the
> +  * service and header packets
> +  */
> + d->buf = src_buf;
> + d->len = src_len;
> + d->proto_git = 1;
> +
> + } else if (starts_with(line, "version 2")) {
> + /*
> +  * v2 smart http; do not consume version packet, which will
> +  * be handled elsewhere.
> +  */
> + d->proto_git = 1;
> +
> + } else {
> + die("invalid server response; got '%s'", line);
> + }
> +}
> +
>  static struct discovery *discover_refs(const char *service, int for_push)
>  {
> - struct strbuf exp = STRBUF_INIT;
>   struct strbuf type = STRBUF_INIT;
>   struct strbuf charset = STRBUF_INIT;
>   struct strbuf buffer = STRBUF_INIT;
> @@ -405,38 +461,8 @@ static struct discovery 

Re: [PATCH 0/3] remote-curl smart-http discovery cleanup

[Josh Steadmon]

On 2018.11.16 03:44, Jeff King wrote:
[...]
> Amusingly, this does break the test you just added, because it tries to
> issue an ERR after claiming "text/html" (and after my patch, we
> correctly fall back to dumb-http).

Heh yeah, I copied the script from a dumb-http test without reading the
spec.

[PATCH 3/3] remote-curl: die on server-side errors

[Jeff King]

From: Josh Steadmon 

When a smart HTTP server sends an error message via pkt-line,
remote-curl will fail to detect the error (which usually results in
incorrectly falling back to dumb-HTTP mode).

This patch adds a check in check_smart_http() for server-side error
messages, as well as a test case for this issue.

Signed-off-by: Josh Steadmon 
Signed-off-by: Jeff King 
---
 remote-curl.c   | 3 +++
 t/lib-httpd.sh  | 1 +
 t/lib-httpd/apache.conf | 4 
 t/lib-httpd/error-smart-http.sh | 3 +++
 t/t5551-http-fetch-smart.sh | 5 +
 5 files changed, 16 insertions(+)
 create mode 100644 t/lib-httpd/error-smart-http.sh

diff --git a/remote-curl.c b/remote-curl.c
index 3c9c4a07c3..b1309f2bdc 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -382,6 +382,9 @@ static void check_smart_http(struct discovery *d, const 
char *service,
 */
d->proto_git = 1;
 
+   } else if (skip_prefix(line, "ERR ", )) {
+   die(_("remote error: %s"), p);
+
} else {
die("invalid server response; got '%s'", line);
}
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index a8729f8232..4e946623bb 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -131,6 +131,7 @@ prepare_httpd() {
mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
install_script broken-smart-http.sh
+   install_script error-smart-http.sh
install_script error.sh
install_script apply-one-time-sed.sh
 
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8f..6de2bc775c 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -117,6 +117,7 @@ Alias /auth/dumb/ www/auth/dumb/
 
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
+ScriptAlias /error_smart/ error-smart-http.sh/
 ScriptAlias /error/ error.sh/
 ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1
 
@@ -125,6 +126,9 @@ ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1
 
Options ExecCGI
 
+
+   Options ExecCGI
+
 
   Options ExecCGI
 
diff --git a/t/lib-httpd/error-smart-http.sh b/t/lib-httpd/error-smart-http.sh
new file mode 100644
index 00..e65d447fc4
--- /dev/null
+++ b/t/lib-httpd/error-smart-http.sh
@@ -0,0 +1,3 @@
+echo "Content-Type: application/x-git-upload-pack-advertisement"
+echo
+printf "%s" "0019ERR server-side error"
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 8630b0cc39..ba83e567e5 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -429,5 +429,10 @@ test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data 
from being traced' '
! grep "=> Send data" err
 '
 
+test_expect_success 'server-side error detected' '
+   test_must_fail git clone $HTTPD_URL/error_smart/repo.git 2>actual &&
+   grep "server-side error" actual
+'
+
 stop_httpd
 test_done
-- 
2.19.1.1636.gc7a073d580

[PATCH 2/3] remote-curl: tighten "version 2" check for smart-http

[Jeff King]

In a v2 smart-http conversation, the server should reply to our initial
request with a pkt-line saying "version 2" (this is the start of the
"capabilities advertisement"). We check for the string using
starts_with(), but that's overly permissive (it would match "version
20", for example).

Let's tighten this check to use strcmp(). Note that we don't need to
worry about a trailing newline here, because the ptk-line code will have
chomped it for us already.

Signed-off-by: Jeff King 
---
 remote-curl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index dd9bc41aa1..3c9c4a07c3 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -375,7 +375,7 @@ static void check_smart_http(struct discovery *d, const 
char *service,
d->len = src_len;
d->proto_git = 1;
 
-   } else if (starts_with(line, "version 2")) {
+   } else if (!strcmp(line, "version 2")) {
/*
 * v2 smart http; do not consume version packet, which will
 * be handled elsewhere.
-- 
2.19.1.1636.gc7a073d580

[PATCH 1/3] remote-curl: refactor smart-http discovery

[Jeff King]

After making initial contact with an http server, we have to decide if
the server supports smart-http, and if so, which version. Our rules are
a bit inconsistent:

  1. For v0, we require that the content-type indicates a smart-http
 response. We also require the response to look vaguely like a
 pkt-line starting with "#". If one of those does not match, we fall
 back to dumb-http.

 But according to our http protocol spec[1]:

   Dumb servers MUST NOT return a return type starting with
   `application/x-git-`.

 If we see the expected content-type, we should consider it
 smart-http. At that point we can parse the pkt-line for real, and
 complain if it is not syntactically valid.

  2. For v2, we do not actually check the content-type. Our v2 protocol
 spec says[2]:

   When using the http:// or https:// transport a client makes a
   "smart" info/refs request as described in `http-protocol.txt`[...]

 and the http spec is clear that for a smart-http[3]:

   The Content-Type MUST be `application/x-$servicename-advertisement`.

 So it is required according to the spec.

These inconsistencies were easy to miss because of the way the original
code was written as an inline conditional. Let's pull it out into its
own function for readability, and improve a few things:

 - we now predicate the smart/dumb decision entirely on the presence of
   the correct content-type

 - we do a real pkt-line parse before deciding how to proceed (and die
   if it isn't valid)

 - use skip_prefix() for comparing service strings, instead of
   constructing expected output in a strbuf; this avoids dealing with
   memory cleanup

Note that this _is_ tightening what the client will allow. It's all
according to the spec, but it's possible that other implementations
might violate these. However, violating these particular rules seems
like an odd choice for a server to make.

[1] Documentation/technical/http-protocol.txt, l. 166-167
[2] Documentation/technical/protocol-v2.txt, l. 63-64
[3] Documentation/technical/http-protocol.txt, l. 247

Helped-by: Josh Steadmon 
Signed-off-by: Jeff King 
---
 remote-curl.c | 93 ---
 1 file changed, 59 insertions(+), 34 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 762a55a75f..dd9bc41aa1 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -330,9 +330,65 @@ static int get_protocol_http_header(enum protocol_version 
version,
return 0;
 }
 
+static void check_smart_http(struct discovery *d, const char *service,
+struct strbuf *type)
+{
+   char *src_buf;
+   size_t src_len;
+   char *line;
+   const char *p;
+
+   /*
+* If we don't see x-$service-advertisement, then it's not smart-http.
+* But once we do, we commit to it and assume any other protocol
+* violations are hard errors.
+*/
+   if (!skip_prefix(type->buf, "application/x-", ) ||
+   !skip_prefix(p, service, ) ||
+   strcmp(p, "-advertisement"))
+   return;
+
+   /*
+* "Peek" at the first packet by using a separate buf/len pair; some
+* cases below require us leaving the originals intact.
+*/
+   src_buf = d->buf;
+   src_len = d->len;
+   line = packet_read_line_buf(_buf, _len, NULL);
+   if (!line)
+   die("invalid server response; expected service, got flush 
packet");
+
+   if (skip_prefix(line, "# service=", ) && !strcmp(p, service)) {
+   /*
+* The header can include additional metadata lines, up
+* until a packet flush marker.  Ignore these now, but
+* in the future we might start to scan them.
+*/
+   while (packet_read_line_buf(_buf, _len, NULL))
+   ;
+
+   /*
+* v0 smart http; callers expect us to soak up the
+* service and header packets
+*/
+   d->buf = src_buf;
+   d->len = src_len;
+   d->proto_git = 1;
+
+   } else if (starts_with(line, "version 2")) {
+   /*
+* v2 smart http; do not consume version packet, which will
+* be handled elsewhere.
+*/
+   d->proto_git = 1;
+
+   } else {
+   die("invalid server response; got '%s'", line);
+   }
+}
+
 static struct discovery *discover_refs(const char *service, int for_push)
 {
-   struct strbuf exp = STRBUF_INIT;
struct strbuf type = STRBUF_INIT;
struct strbuf charset = STRBUF_INIT;
struct strbuf buffer = STRBUF_INIT;
@@ -405,38 +461,8 @@ static struct discovery *discover_refs(const char 
*service, int for_push)
last->buf_alloc = strbuf_detach(, >len);
last->buf = last->buf_alloc;
 
-   strbuf_addf(, "application/x-%s-advertisement", service);
-  

[PATCH 0/3] remote-curl smart-http discovery cleanup

[Jeff King]

On Thu, Nov 15, 2018 at 01:51:52PM -0800, Josh Steadmon wrote:

> > This patch tightens both of those (I also made a few stylistic tweaks,
> > and added the ERR condition to show where it would go). I dunno. Part of
> > me sees this as a nice cleanup, but maybe it is better to just leave it
> > alone. A lot of these behaviors are just how it happens to work now, and
> > not part of the spec, but we don't know what might be relying on them.
> 
> At least according to the protocol-v2 and http-protocol docs, the
> stricter behavior seems correct:
> 
> For the first point above, dumb servers should never use an
> "application/x-git-*" content type (http-protocol.txt line 163-167).
> 
> For the second point, the docs require v2 servers to use
> "application/x-git-*" content types. protocol-v2.txt lines 63-65 state
> that v2 clients should make a smart http request, while
> http-protocol.txt lines 247-252 state that a smart server's response
> type must be "application/x-git-*".

Thanks for digging into the spec. I agree that it's pretty clear that
the appropriate content-type is expected.

> Of course we don't know if other implementations follow the spec, but
> ISTM that this patch at least doesn't contradict how we've promised the
> protocols should work.

These seem like pretty unlikely ways for a buggy server to behave, so I
think it's a reasonable risk. I also checked GitHub's implementation
(which recently learned to speak v2) and made sure that it works. :)
I didn't check JGit, but given the provenance, I assume it's fine.

Amusingly, this does break the test you just added, because it tries to
issue an ERR after claiming "text/html" (and after my patch, we
correctly fall back to dumb-http).

> If no one has any objections, I'll include the diff below in v2. Thanks
> for the help Jeff!

I think it makes sense to do the refactoring first as a separate step.
And of course it needs a commit message. So how about this series (your
original is rebased on top)?

  [1/3]: remote-curl: refactor smart-http discovery
  [2/3]: remote-curl: tighten "version 2" check for smart-http
  [3/3]: remote-curl: die on server-side errors

 remote-curl.c   | 96 +
 t/lib-httpd.sh  |  1 +
 t/lib-httpd/apache.conf |  4 ++
 t/lib-httpd/error-smart-http.sh |  3 ++
 t/t5551-http-fetch-smart.sh |  5 ++
 5 files changed, 75 insertions(+), 34 deletions(-)
 create mode 100644 t/lib-httpd/error-smart-http.sh

-Peff

Re: [PATCH] remote-curl: die on server-side errors

[Josh Steadmon]

On 2018.11.14 02:00, Jeff King wrote:
> On Tue, Nov 13, 2018 at 07:49:15PM -0500, Jeff King wrote:
> 
> > Yes, the packet_read_line_buf() interface will both advance the buf
> > pointer and decrement the length.  So if we want to "peek", we have to
> > do so with a copy (there's a peek function if you use the packet_reader
> > interface, but that might be overkill here).
> > 
> > You can rewrite it like this, which is a pretty faithful conversion and
> > passes the tests (but see below).
> > [...]
> 
> Here's a version which is less faithful, but I think does sensible
> things in all cases, and is much easier to follow. I get a little
> nervous just because it tightens some cases, and one never knows if
> other implementations might be relying on the looseness. E.g.:
> 
>   - in the current code we'd still drop back to dumb http if the server
> tells us "application/x-git-upload-pack" but the initial pktline
> doesn't start with "#" (even though if it _does_ have "#" at
> position 5 but isn't a valid pktline, we'd complain!)
> 
>   - right now the v2 protocol does not require the server to say
> "application/x-git-upload-pack" for the content-type
> 
> This patch tightens both of those (I also made a few stylistic tweaks,
> and added the ERR condition to show where it would go). I dunno. Part of
> me sees this as a nice cleanup, but maybe it is better to just leave it
> alone. A lot of these behaviors are just how it happens to work now, and
> not part of the spec, but we don't know what might be relying on them.

At least according to the protocol-v2 and http-protocol docs, the
stricter behavior seems correct:

For the first point above, dumb servers should never use an
"application/x-git-*" content type (http-protocol.txt line 163-167).

For the second point, the docs require v2 servers to use
"application/x-git-*" content types. protocol-v2.txt lines 63-65 state
that v2 clients should make a smart http request, while
http-protocol.txt lines 247-252 state that a smart server's response
type must be "application/x-git-*".

Of course we don't know if other implementations follow the spec, but
ISTM that this patch at least doesn't contradict how we've promised the
protocols should work.

If no one has any objections, I'll include the diff below in v2. Thanks
for the help Jeff!

> diff --git a/remote-curl.c b/remote-curl.c
> index 762a55a75f..1adb96311b 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -330,9 +330,61 @@ static int get_protocol_http_header(enum 
> protocol_version version,
>   return 0;
>  }
>  
> +static void check_smart_http(struct discovery *d, const char *service,
> +  struct strbuf *type)
> +{
> + char *src_buf;
> + size_t src_len;
> + char *line;
> + const char *p;
> +
> + if (!skip_prefix(type->buf, "application/x-", ) ||
> + !skip_prefix(p, service, ) ||
> + strcmp(p, "-advertisement"))
> + return;
> +
> + /*
> +  * We speculatively try to read a packet, which means we must preserve
> +  * the original buf/len pair in some cases.
> +  */
> + src_buf = d->buf;
> + src_len = d->len;
> + line = packet_read_line_buf(_buf, _len, NULL);
> + if (!line)
> + die("invalid server response; expected service, got flush 
> packet");
> +
> + if (skip_prefix(line, "# service=", ) && !strcmp(p, service)) {
> + /*
> +  * The header can include additional metadata lines, up
> +  * until a packet flush marker.  Ignore these now, but
> +  * in the future we might start to scan them.
> +  */
> + while (packet_read_line_buf(_buf, _len, NULL))
> + ;
> +
> + /*
> +  * v0 smart http; callers expect us to soak up the
> +  * service and header packets
> +  */
> + d->buf = src_buf;
> + d->len = src_len;
> + d->proto_git = 1;
> +
> + } else if (starts_with(line, "version 2")) { /* should be strcmp()? */
> + /*
> +  * v2 smart http; do not consume version packet, which will
> +  * be handled elsewhere.
> +  */
> + d->proto_git = 1;
> + } else if (skip_prefix(line, "ERR ", )) {
> + die(_("remote error: %s"), p);
> + } else {
> + die("invalid server response; got '%s'", line);
> + }
> +}
> +
>  static struct discovery *discover_refs(const char *service, int for_push)
>  {
> - struct strbuf exp = STRBUF_INIT;
>   struct strbuf type = STRBUF_INIT;
>   struct strbuf charset = STRBUF_INIT;
>   struct strbuf buffer = STRBUF_INIT;
> @@ -405,38 +457,8 @@ static struct discovery *discover_refs(const char 
> *service, int for_push)
>   last->buf_alloc = strbuf_detach(, >len);
>   last->buf = last->buf_alloc;
>  
> - strbuf_addf(, "application/x-%s-advertisement", service);
> - if 

Re: [PATCH] remote-curl: die on server-side errors

[Jeff King]

On Tue, Nov 13, 2018 at 07:49:15PM -0500, Jeff King wrote:

> Yes, the packet_read_line_buf() interface will both advance the buf
> pointer and decrement the length.  So if we want to "peek", we have to
> do so with a copy (there's a peek function if you use the packet_reader
> interface, but that might be overkill here).
> 
> You can rewrite it like this, which is a pretty faithful conversion and
> passes the tests (but see below).
> [...]

Here's a version which is less faithful, but I think does sensible
things in all cases, and is much easier to follow. I get a little
nervous just because it tightens some cases, and one never knows if
other implementations might be relying on the looseness. E.g.:

  - in the current code we'd still drop back to dumb http if the server
tells us "application/x-git-upload-pack" but the initial pktline
doesn't start with "#" (even though if it _does_ have "#" at
position 5 but isn't a valid pktline, we'd complain!)

  - right now the v2 protocol does not require the server to say
"application/x-git-upload-pack" for the content-type

This patch tightens both of those (I also made a few stylistic tweaks,
and added the ERR condition to show where it would go). I dunno. Part of
me sees this as a nice cleanup, but maybe it is better to just leave it
alone. A lot of these behaviors are just how it happens to work now, and
not part of the spec, but we don't know what might be relying on them.

diff --git a/remote-curl.c b/remote-curl.c
index 762a55a75f..1adb96311b 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -330,9 +330,61 @@ static int get_protocol_http_header(enum protocol_version 
version,
return 0;
 }
 
+static void check_smart_http(struct discovery *d, const char *service,
+struct strbuf *type)
+{
+   char *src_buf;
+   size_t src_len;
+   char *line;
+   const char *p;
+
+   if (!skip_prefix(type->buf, "application/x-", ) ||
+   !skip_prefix(p, service, ) ||
+   strcmp(p, "-advertisement"))
+   return;
+
+   /*
+* We speculatively try to read a packet, which means we must preserve
+* the original buf/len pair in some cases.
+*/
+   src_buf = d->buf;
+   src_len = d->len;
+   line = packet_read_line_buf(_buf, _len, NULL);
+   if (!line)
+   die("invalid server response; expected service, got flush 
packet");
+
+   if (skip_prefix(line, "# service=", ) && !strcmp(p, service)) {
+   /*
+* The header can include additional metadata lines, up
+* until a packet flush marker.  Ignore these now, but
+* in the future we might start to scan them.
+*/
+   while (packet_read_line_buf(_buf, _len, NULL))
+   ;
+
+   /*
+* v0 smart http; callers expect us to soak up the
+* service and header packets
+*/
+   d->buf = src_buf;
+   d->len = src_len;
+   d->proto_git = 1;
+
+   } else if (starts_with(line, "version 2")) { /* should be strcmp()? */
+   /*
+* v2 smart http; do not consume version packet, which will
+* be handled elsewhere.
+*/
+   d->proto_git = 1;
+   } else if (skip_prefix(line, "ERR ", )) {
+   die(_("remote error: %s"), p);
+   } else {
+   die("invalid server response; got '%s'", line);
+   }
+}
+
 static struct discovery *discover_refs(const char *service, int for_push)
 {
-   struct strbuf exp = STRBUF_INIT;
struct strbuf type = STRBUF_INIT;
struct strbuf charset = STRBUF_INIT;
struct strbuf buffer = STRBUF_INIT;
@@ -405,38 +457,8 @@ static struct discovery *discover_refs(const char 
*service, int for_push)
last->buf_alloc = strbuf_detach(, >len);
last->buf = last->buf_alloc;
 
-   strbuf_addf(, "application/x-%s-advertisement", service);
-   if (maybe_smart &&
-   (5 <= last->len && last->buf[4] == '#') &&
-   !strbuf_cmp(, )) {
-   char *line;
-
-   /*
-* smart HTTP response; validate that the service
-* pkt-line matches our request.
-*/
-   line = packet_read_line_buf(>buf, >len, NULL);
-   if (!line)
-   die("invalid server response; expected service, got 
flush packet");
-
-   strbuf_reset();
-   strbuf_addf(, "# service=%s", service);
-   if (strcmp(line, exp.buf))
-   die("invalid server response; got '%s'", line);
-   strbuf_release();
-
-   /* The header can include additional metadata lines, up
-* until a packet flush marker.  Ignore these now, but
-* in the future we might start to scan them.
-

Re: [PATCH] remote-curl: die on server-side errors

[Jeff King]

On Tue, Nov 13, 2018 at 02:25:40PM -0800, Josh Steadmon wrote:

> > The magic "4" here and in the existing "version 2" check is because we
> > are expecting pkt-lines. The original conditional always calls
> > packed_read_line_buf() and will complain if we didn't actually get a
> > pkt-line.
> > 
> > Should we confirm that we got a real packet-line? Or at least that those
> > first four are even plausible hex chars?
> > 
> > I admit that it's pretty unlikely that the server is going to fool us
> > here. It would need something like "foo ERRORS ARE FUN!". And even then
> > we'd report an error (whereas the correct behavior is falling back to
> > dumb http, but we know that won't work anyway because that's not a valid
> > ref advertisement). So I doubt this is really a bug per se, but it might
> > make it easier to understand what's going on if we actually parsed the
> > packet.
> 
> Unfortunately we can't just directly parse the data in last->buf,
> because other parts of the code are expecting to see the raw pkt-line
> data there. I tried adding a duplicate pointer and length variable for
> this data and parsing that through packet_read_line_buf(), but even
> without using the results this apparently has side-effects that break
> all of t5550 (and probably other tests as well). It also fails if I
> completely duplicate last->buf into a new char* and call
> packet_readline_buf() on that, so there's clearly some interaction in
> the pkt-line guts that I'm not properly accounting for.

Yes, the packet_read_line_buf() interface will both advance the buf
pointer and decrement the length.  So if we want to "peek", we have to
do so with a copy (there's a peek function if you use the packet_reader
interface, but that might be overkill here).

You can rewrite it like this, which is a pretty faithful conversion and
passes the tests (but see below).

diff --git a/remote-curl.c b/remote-curl.c
index 762a55a75f..66c57c9d62 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -330,9 +330,78 @@ static int get_protocol_http_header(enum protocol_version 
version,
return 0;
 }
 
+static void check_smart_http(struct discovery *d, const char *service,
+struct strbuf *type)
+{
+   char *src_buf;
+   size_t src_len;
+   char pkt[LARGE_PACKET_MAX];
+   int pkt_len;
+   enum packet_read_status r;
+
+   /*
+* We speculatively try to read a packet, which means we must preserve
+* the original buf/len pair in some cases.
+*/
+   src_buf = d->buf;
+   src_len = d->len;
+   r = packet_read_with_status(-1, _buf, _len,
+   pkt, sizeof(pkt), _len,
+   PACKET_READ_GENTLE |
+   PACKET_READ_CHOMP_NEWLINE);
+
+   /*
+* This could probably just be handled as "not smart" like all the
+* other pkt-line error cases, but traditionally we've complained
+* about it (technically we used to do so only when we got the right
+* content-type, but it probably doesn't matter).
+*/
+   if (r == PACKET_READ_FLUSH)
+   die("invalid server response; expected service, got flush 
packet");
+   if (r != PACKET_READ_NORMAL)
+   return; /* not smart */
+
+   if (pkt[0] == '#') {
+   /* v0 smart http */
+   struct strbuf exp = STRBUF_INIT;
+
+   strbuf_addf(, "application/x-%s-advertisement", service);
+   if (strbuf_cmp(, type)) {
+   strbuf_release();
+   return;
+   }
+
+   strbuf_reset();
+   strbuf_addf(, "# service=%s", service);
+   if (strcmp(pkt, exp.buf))
+   die("invalid server response; got '%s'", pkt);
+
+   strbuf_release();
+
+   /*
+* The header can include additional metadata lines, up
+* until a packet flush marker.  Ignore these now, but
+* in the future we might start to scan them.
+*/
+   while (packet_read_line_buf(_buf, _len, NULL))
+   ;
+
+   d->buf = src_buf;
+   d->len = src_len;
+   d->proto_git = 1;
+
+   } else if (starts_with(pkt, "version 2")) {
+   /*
+* v2 smart http; do not consume version packet, which will
+* be handled elsewhere. Should we be checking the content-type
+* in this code-path, too?
+*/
+   d->proto_git = 1;
+   }
+}
+
 static struct discovery *discover_refs(const char *service, int for_push)
 {
-   struct strbuf exp = STRBUF_INIT;
struct strbuf type = STRBUF_INIT;
struct strbuf charset = STRBUF_INIT;
struct strbuf buffer = STRBUF_INIT;
@@ -405,38 +474,8 @@ static struct discovery *discover_refs(const char 
*service, int 

Re: [PATCH] remote-curl: die on server-side errors

[Josh Steadmon]

On 2018.11.13 23:30, Junio C Hamano wrote:
> stead...@google.com writes:
> 
> > When a smart HTTP server sends an error message via pkt-line,
> > remote-curl will fail to detect the error (which usually results in
> > incorrectly falling back to dumb-HTTP mode).
> 
> OK, that is a valid thing to worry about.
> 
> >
> > This patch adds a check in discover_refs() for server-side error
> > messages, as well as a test case for this issue.
> 
> This makes me wonder if discoer_refs() is the only place where we
> ought to be checking for ERR packets but we are not.  Are there
> other places that we also need a similar fix?

Quite possibly; I'll review the other client code to see if there are
similar issues before sending v2.

Re: [PATCH] remote-curl: die on server-side errors

[Josh Steadmon]

On 2018.11.13 09:26, Jeff King wrote:
> On Mon, Nov 12, 2018 at 02:44:56PM -0800, stead...@google.com wrote:
> 
> > When a smart HTTP server sends an error message via pkt-line,
> > remote-curl will fail to detect the error (which usually results in
> > incorrectly falling back to dumb-HTTP mode).
> > 
> > This patch adds a check in discover_refs() for server-side error
> > messages, as well as a test case for this issue.
> 
> Aside from the reformatting of the conditional that Junio mentioned,
> this seems pretty good to me. But while looking at that, I found a few
> things, some old and some new. :)
> 
> > diff --git a/remote-curl.c b/remote-curl.c
> > index 762a55a75f..bb3a86505e 100644
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> > @@ -436,7 +436,9 @@ static struct discovery *discover_refs(const char 
> > *service, int for_push)
> > } else if (maybe_smart &&
> >last->len > 5 && starts_with(last->buf + 4, "version 2")) {
> > last->proto_git = 1;
> > -   }
> > +   } else if (maybe_smart && last->len > 5 &&
> > +  starts_with(last->buf + 4, "ERR "))
> > +   die(_("remote error: %s"), last->buf + 8);
> 
> The magic "4" here and in the existing "version 2" check is because we
> are expecting pkt-lines. The original conditional always calls
> packed_read_line_buf() and will complain if we didn't actually get a
> pkt-line.
> 
> Should we confirm that we got a real packet-line? Or at least that those
> first four are even plausible hex chars?
> 
> I admit that it's pretty unlikely that the server is going to fool us
> here. It would need something like "foo ERRORS ARE FUN!". And even then
> we'd report an error (whereas the correct behavior is falling back to
> dumb http, but we know that won't work anyway because that's not a valid
> ref advertisement). So I doubt this is really a bug per se, but it might
> make it easier to understand what's going on if we actually parsed the
> packet.

Unfortunately we can't just directly parse the data in last->buf,
because other parts of the code are expecting to see the raw pkt-line
data there. I tried adding a duplicate pointer and length variable for
this data and parsing that through packet_read_line_buf(), but even
without using the results this apparently has side-effects that break
all of t5550 (and probably other tests as well). It also fails if I
completely duplicate last->buf into a new char* and call
packet_readline_buf() on that, so there's clearly some interaction in
the pkt-line guts that I'm not properly accounting for.

> Similarly, we seem eager to accept "version 2" even if we are only
> expecting v0. I know you have another series working in that direction,
> but I don't think it touches this "proto_git". I guess accepting
> "version 2" as "we're speaking git protocol" and then barfing later with
> "wait, I didn't mean to speak v2" is probably OK.
> 
> -Peff

Re: [PATCH] remote-curl: die on server-side errors

[Josh Steadmon]

On 2018.11.13 12:02, Junio C Hamano wrote:
> stead...@google.com writes:
> 
> > When a smart HTTP server sends an error message via pkt-line,
> > remote-curl will fail to detect the error (which usually results in
> > incorrectly falling back to dumb-HTTP mode).
> >
> > This patch adds a check in discover_refs() for server-side error
> > messages, as well as a test case for this issue.
> >
> > Signed-off-by: Josh Steadmon 
> > ---
> 
> Forgot to mention one procedural comment.
> 
> As you can see in the To: line of this reply, your MUA is placing
> only the e-mail address without name on your From: line.
> 
> Preferrably I'd like to see the same string as your sign-off on the
> "From:" line in your messages for a bit of human touch ;-) Can you
> tweak your MUA to make that happen?
> 
> The second preference is to add an in-body header (i.e. as the first
> line of the body of the message) so that the body of the message
> starts like this:
> 
> From: Josh Steadmon 
> 
> When a smart HTTP server sends an error message via pkt-line,
> remote-curl will fail to detect the error (which usually results in
> incorrectly falling back to dumb-HTTP mode).
> 
> This patch adds a check in discover_refs() for server-side error
> messages, as well as a test case for this issue.
> 
> Signed-off-by: Josh Steadmon 
> ---
>  remote-curl.c   | 4 +++-
>  t/lib-httpd.sh  | 1 +
>  t/lib-httpd/apache.conf | 4 
> 
> Either way would make sure that the resulting patch's author line
> will be attributed correctly to the same identity as who is signing
> it off first as the author.
> 
> Thanks.

This should be fixed for future patch submissions. Thanks for the
heads-up.

Re: [PATCH] remote-curl: die on server-side errors

[Junio C Hamano]

stead...@google.com writes:

> When a smart HTTP server sends an error message via pkt-line,
> remote-curl will fail to detect the error (which usually results in
> incorrectly falling back to dumb-HTTP mode).

OK, that is a valid thing to worry about.

>
> This patch adds a check in discover_refs() for server-side error
> messages, as well as a test case for this issue.

This makes me wonder if discoer_refs() is the only place where we
ought to be checking for ERR packets but we are not.  Are there
other places that we also need a similar fix?

Re: [PATCH] remote-curl: die on server-side errors

[Jeff King]

On Mon, Nov 12, 2018 at 02:44:56PM -0800, stead...@google.com wrote:

> When a smart HTTP server sends an error message via pkt-line,
> remote-curl will fail to detect the error (which usually results in
> incorrectly falling back to dumb-HTTP mode).
> 
> This patch adds a check in discover_refs() for server-side error
> messages, as well as a test case for this issue.

Aside from the reformatting of the conditional that Junio mentioned,
this seems pretty good to me. But while looking at that, I found a few
things, some old and some new. :)

> diff --git a/remote-curl.c b/remote-curl.c
> index 762a55a75f..bb3a86505e 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -436,7 +436,9 @@ static struct discovery *discover_refs(const char 
> *service, int for_push)
>   } else if (maybe_smart &&
>  last->len > 5 && starts_with(last->buf + 4, "version 2")) {
>   last->proto_git = 1;
> - }
> + } else if (maybe_smart && last->len > 5 &&
> +starts_with(last->buf + 4, "ERR "))
> + die(_("remote error: %s"), last->buf + 8);

The magic "4" here and in the existing "version 2" check is because we
are expecting pkt-lines. The original conditional always calls
packed_read_line_buf() and will complain if we didn't actually get a
pkt-line.

Should we confirm that we got a real packet-line? Or at least that those
first four are even plausible hex chars?

I admit that it's pretty unlikely that the server is going to fool us
here. It would need something like "foo ERRORS ARE FUN!". And even then
we'd report an error (whereas the correct behavior is falling back to
dumb http, but we know that won't work anyway because that's not a valid
ref advertisement). So I doubt this is really a bug per se, but it might
make it easier to understand what's going on if we actually parsed the
packet.

Similarly, we seem eager to accept "version 2" even if we are only
expecting v0. I know you have another series working in that direction,
but I don't think it touches this "proto_git". I guess accepting
"version 2" as "we're speaking git protocol" and then barfing later with
"wait, I didn't mean to speak v2" is probably OK.

-Peff

Re: [PATCH] remote-curl: die on server-side errors

[Junio C Hamano]

stead...@google.com writes:

> When a smart HTTP server sends an error message via pkt-line,
> remote-curl will fail to detect the error (which usually results in
> incorrectly falling back to dumb-HTTP mode).
>
> This patch adds a check in discover_refs() for server-side error
> messages, as well as a test case for this issue.
>
> Signed-off-by: Josh Steadmon 
> ---

Forgot to mention one procedural comment.

As you can see in the To: line of this reply, your MUA is placing
only the e-mail address without name on your From: line.

Preferrably I'd like to see the same string as your sign-off on the
"From:" line in your messages for a bit of human touch ;-) Can you
tweak your MUA to make that happen?

The second preference is to add an in-body header (i.e. as the first
line of the body of the message) so that the body of the message
starts like this:

From: Josh Steadmon 

When a smart HTTP server sends an error message via pkt-line,
remote-curl will fail to detect the error (which usually results in
incorrectly falling back to dumb-HTTP mode).

This patch adds a check in discover_refs() for server-side error
messages, as well as a test case for this issue.

Signed-off-by: Josh Steadmon 
---
 remote-curl.c   | 4 +++-
 t/lib-httpd.sh  | 1 +
 t/lib-httpd/apache.conf | 4 

Either way would make sure that the resulting patch's author line
will be attributed correctly to the same identity as who is signing
it off first as the author.

Thanks.

Re: [PATCH] remote-curl: die on server-side errors

[Junio C Hamano]

stead...@google.com writes:

> When a smart HTTP server sends an error message via pkt-line,
> remote-curl will fail to detect the error (which usually results in
> incorrectly falling back to dumb-HTTP mode).
>
> This patch adds a check in discover_refs() for server-side error
> messages, as well as a test case for this issue.
>
> Signed-off-by: Josh Steadmon 
> ---
>  remote-curl.c   | 4 +++-
>  t/lib-httpd.sh  | 1 +
>  t/lib-httpd/apache.conf | 4 
>  t/lib-httpd/error-smart-http.sh | 3 +++
>  t/t5551-http-fetch-smart.sh | 5 +
>  5 files changed, 16 insertions(+), 1 deletion(-)
>  create mode 100644 t/lib-httpd/error-smart-http.sh
>
> diff --git a/remote-curl.c b/remote-curl.c
> index 762a55a75f..bb3a86505e 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -436,7 +436,9 @@ static struct discovery *discover_refs(const char 
> *service, int for_push)
>   } else if (maybe_smart &&
>  last->len > 5 && starts_with(last->buf + 4, "version 2")) {
>   last->proto_git = 1;
> - }
> + } else if (maybe_smart && last->len > 5 &&
> +starts_with(last->buf + 4, "ERR "))
> + die(_("remote error: %s"), last->buf + 8);

Two observations and a half.

 - All of these if/else if/ blocks (currently 2, now you added the
   third one) are to special case the "maybe_smart" case.  Perhaps
   the whole thing should be restrucutred to

if (maybe_smart) {
if ...
else if ...
else if ...
}

 - All conditions skip the first four bytes in last->buf and does
   starts_with (the first branch is "starts_with("#") in disguise).
   Can we do something to the hardcoded magic number 4, which looks
   somewhat irritating?

 - This is less of the problem with the suggested change in the
   first bullet item above, but the current code structure makes
   readers wonder if maybe_start that starts as 1 is turned off
   somewhere in the if/else if/ cascade when we find out that we are
   not dealing with smart HTTP after all.  That however is not what
   is happening.  The "maybe" variable is primarily there to avoid
   repeating the logic that determined its initial value in the
   early part of the function.  The last->proto_git field is used to
   record if we are using smart HTTP or not.

Otherwise the change looks sensible.


> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
> index a8729f8232..4e946623bb 100644
> --- a/t/lib-httpd.sh
> +++ b/t/lib-httpd.sh
> @@ -131,6 +131,7 @@ prepare_httpd() {
>   mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
>   cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
>   install_script broken-smart-http.sh
> + install_script error-smart-http.sh
>   install_script error.sh
>   install_script apply-one-time-sed.sh
>  
> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index 581c010d8f..6de2bc775c 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -117,6 +117,7 @@ Alias /auth/dumb/ www/auth/dumb/
>  
>  ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
>  ScriptAlias /broken_smart/ broken-smart-http.sh/
> +ScriptAlias /error_smart/ error-smart-http.sh/
>  ScriptAlias /error/ error.sh/
>  ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1
>  
> @@ -125,6 +126,9 @@ ScriptAliasMatch /one_time_sed/(.*) 
> apply-one-time-sed.sh/$1
>  
>   Options ExecCGI
>  
> +
> + Options ExecCGI
> +
>  
>Options ExecCGI
>  
> diff --git a/t/lib-httpd/error-smart-http.sh b/t/lib-httpd/error-smart-http.sh
> new file mode 100644
> index 00..0a1af318f7
> --- /dev/null
> +++ b/t/lib-httpd/error-smart-http.sh
> @@ -0,0 +1,3 @@
> +printf "Content-Type: text/%s\n" "html"
> +echo
> +printf "%s" "0019ERR server-side error"
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 8630b0cc39..ba83e567e5 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -429,5 +429,10 @@ test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents 
> data from being traced' '
>   ! grep "=> Send data" err
>  '
>  
> +test_expect_success 'server-side error detected' '
> + test_must_fail git clone $HTTPD_URL/error_smart/repo.git 2>actual &&
> + grep "server-side error" actual
> +'
> +
>  stop_httpd
>  test_done

Re: [PATCH] remote-curl: die on server-side errors

[Stefan Beller]

On Mon, Nov 12, 2018 at 2:45 PM  wrote:
>
> When a smart HTTP server sends an error message via pkt-line,
> remote-curl will fail to detect the error (which usually results in
> incorrectly falling back to dumb-HTTP mode).
>
> This patch adds a check in discover_refs() for server-side error
> messages, as well as a test case for this issue.
>
> Signed-off-by: Josh Steadmon 

Reviewed-by: Stefan Beller 

[PATCH] remote-curl: die on server-side errors

[steadmon]

When a smart HTTP server sends an error message via pkt-line,
remote-curl will fail to detect the error (which usually results in
incorrectly falling back to dumb-HTTP mode).

This patch adds a check in discover_refs() for server-side error
messages, as well as a test case for this issue.

Signed-off-by: Josh Steadmon 
---
 remote-curl.c   | 4 +++-
 t/lib-httpd.sh  | 1 +
 t/lib-httpd/apache.conf | 4 
 t/lib-httpd/error-smart-http.sh | 3 +++
 t/t5551-http-fetch-smart.sh | 5 +
 5 files changed, 16 insertions(+), 1 deletion(-)
 create mode 100644 t/lib-httpd/error-smart-http.sh

diff --git a/remote-curl.c b/remote-curl.c
index 762a55a75f..bb3a86505e 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -436,7 +436,9 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
} else if (maybe_smart &&
   last->len > 5 && starts_with(last->buf + 4, "version 2")) {
last->proto_git = 1;
-   }
+   } else if (maybe_smart && last->len > 5 &&
+  starts_with(last->buf + 4, "ERR "))
+   die(_("remote error: %s"), last->buf + 8);
 
if (last->proto_git)
last->refs = parse_git_refs(last, for_push);
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index a8729f8232..4e946623bb 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -131,6 +131,7 @@ prepare_httpd() {
mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
install_script broken-smart-http.sh
+   install_script error-smart-http.sh
install_script error.sh
install_script apply-one-time-sed.sh
 
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8f..6de2bc775c 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -117,6 +117,7 @@ Alias /auth/dumb/ www/auth/dumb/
 
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
+ScriptAlias /error_smart/ error-smart-http.sh/
 ScriptAlias /error/ error.sh/
 ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1
 
@@ -125,6 +126,9 @@ ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1
 
Options ExecCGI
 
+
+   Options ExecCGI
+
 
   Options ExecCGI
 
diff --git a/t/lib-httpd/error-smart-http.sh b/t/lib-httpd/error-smart-http.sh
new file mode 100644
index 00..0a1af318f7
--- /dev/null
+++ b/t/lib-httpd/error-smart-http.sh
@@ -0,0 +1,3 @@
+printf "Content-Type: text/%s\n" "html"
+echo
+printf "%s" "0019ERR server-side error"
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 8630b0cc39..ba83e567e5 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -429,5 +429,10 @@ test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data 
from being traced' '
! grep "=> Send data" err
 '
 
+test_expect_success 'server-side error detected' '
+   test_must_fail git clone $HTTPD_URL/error_smart/repo.git 2>actual &&
+   grep "server-side error" actual
+'
+
 stop_httpd
 test_done
-- 
2.19.1.930.g4563a0d9d0-goog

[PATCH v4 3/4] fix curl version to support CURL_HTTP_VERSION_2TLS

[Force Charlie via GitGitGadget]

From: Force Charlie 

Signed-off-by: Force Charlie 
---
 http.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index b2ec31aef5..86e454cff5 100644
--- a/http.c
+++ b/http.c
@@ -811,10 +811,10 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 2);
}
 
-#if LIBCURL_VERSION_NUM >= 0x074700
+#if LIBCURL_VERSION_NUM >= 0x072f00 // 7.47.0
 // curl_http_version 0 is default.
 if (curl_http_version == 20) {
-   /* Enable HTTP2 when request TLS*/
+   /* Enable HTTP2*/
curl_easy_setopt(result, 
CURLOPT_HTTP_VERSION,CURL_HTTP_VERSION_2TLS);
 } else if (curl_http_version == 11) {
curl_easy_setopt(result, 
CURLOPT_HTTP_VERSION,CURL_HTTP_VERSION_1_1);
-- 
gitgitgadget

[PATCH v3 3/4] fix curl version to support CURL_HTTP_VERSION_2TLS

[Force Charlie via GitGitGadget]

From: Force Charlie 

Signed-off-by: Force Charlie 
---
 http.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index b2ec31aef5..86e454cff5 100644
--- a/http.c
+++ b/http.c
@@ -811,10 +811,10 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 2);
}
 
-#if LIBCURL_VERSION_NUM >= 0x074700
+#if LIBCURL_VERSION_NUM >= 0x072f00 // 7.47.0
 // curl_http_version 0 is default.
 if (curl_http_version == 20) {
-   /* Enable HTTP2 when request TLS*/
+   /* Enable HTTP2*/
curl_easy_setopt(result, 
CURLOPT_HTTP_VERSION,CURL_HTTP_VERSION_2TLS);
 } else if (curl_http_version == 11) {
curl_easy_setopt(result, 
CURLOPT_HTTP_VERSION,CURL_HTTP_VERSION_1_1);
-- 
gitgitgadget

[PATCH v2 3/3] fix curl version to support CURL_HTTP_VERSION_2TLS

[Force Charlie via GitGitGadget]

From: Force Charlie 

Signed-off-by: Force Charlie 
---
 http.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index b2ec31aef5..86e454cff5 100644
--- a/http.c
+++ b/http.c
@@ -811,10 +811,10 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 2);
}
 
-#if LIBCURL_VERSION_NUM >= 0x074700
+#if LIBCURL_VERSION_NUM >= 0x072f00 // 7.47.0
 // curl_http_version 0 is default.
 if (curl_http_version == 20) {
-   /* Enable HTTP2 when request TLS*/
+   /* Enable HTTP2*/
curl_easy_setopt(result, 
CURLOPT_HTTP_VERSION,CURL_HTTP_VERSION_2TLS);
 } else if (curl_http_version == 11) {
curl_easy_setopt(result, 
CURLOPT_HTTP_VERSION,CURL_HTTP_VERSION_1_1);
-- 
gitgitgadget

Re: [PATCH v2] build: link with curl-defined linker flags

[Junio C Hamano]

James Knight  writes:

> Changes v1 -> v2:
>  - Improved support for detecting curl linker flags when not using a
> configure-based build (mentioned by Junio C Hamano).
>  - Adding a description on how to explicitly use the CURL_LDFLAGS
> define when not using configure (suggested by Junio C Hamano).
>
> The original patch made a (bad) assumption that builds would always
> invoke ./configure before attempting to build Git. To support a
> make-invoked build, CURL_LDFLAGS can also be populated using the defined
> curl-config utility. This change also comes with the assumption that
> since both ./configure and Makefile are using curl-config to determine
> aspects of the build, it should be also fine to use the same utility to
> provide the linker flags to compile against (please indicate so if this
> is another bad assumption). With this change, the explicit configuration
> of CURL_LDFLAGS inside config.mak.uname for Minix and NONSTOP_KERNEL
> have been dropped.

Will replace; thanks.


>  Makefile | 30 +++---
>  config.mak.uname |  3 ---
>  configure.ac | 17 +++--
>  3 files changed, 22 insertions(+), 28 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 
> b08d5ea258c69a78745dfa73fe698c11d021858a..36da17dc1f9b1a70c9142604afe989f1eb8ee87f
>  100644
> --- a/Makefile
> +++ b/Makefile
> @@ -59,6 +59,13 @@ all::
>  # Define CURL_CONFIG to curl's configuration program that prints information
>  # about the library (e.g., its version number).  The default is 
> 'curl-config'.
>  #
> +# Define CURL_LDFLAGS to specify flags that you need to link when using 
> libcurl,
> +# if you do not want to rely on the libraries provided by CURL_CONFIG.  The
> +# default value is a result of `curl-config --libs`.  An example value for
> +# CURL_LDFLAGS is as follows:
> +#
> +# CURL_LDFLAGS=-lcurl
> +#
>  # Define NO_EXPAT if you do not have expat installed.  git-http-push is
>  # not built, and you cannot push using http:// and https:// transports 
> (dumb).
>  #
> @@ -183,10 +190,6 @@ all::
>  #
>  # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto 
> (Darwin).
>  #
> -# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).
> -#
> -# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).
> -#
>  # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin).
>  #
>  # Define NEEDS_LIBINTL_BEFORE_LIBICONV if you need libintl before libiconv.
> @@ -1305,20 +1308,17 @@ else
>   ifdef CURLDIR
>   # Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
>   BASIC_CFLAGS += -I$(CURLDIR)/include
> - CURL_LIBCURL = -L$(CURLDIR)/$(lib) 
> $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl
> + CURL_LIBCURL = -L$(CURLDIR)/$(lib) 
> $(CC_LD_DYNPATH)$(CURLDIR)/$(lib)
>   else
> - CURL_LIBCURL = -lcurl
> - endif
> - ifdef NEEDS_SSL_WITH_CURL
> - CURL_LIBCURL += -lssl
> - ifdef NEEDS_CRYPTO_WITH_SSL
> - CURL_LIBCURL += -lcrypto
> - endif
> - endif
> - ifdef NEEDS_IDN_WITH_CURL
> - CURL_LIBCURL += -lidn
> + CURL_LIBCURL =
>   endif
>  
> +ifdef CURL_LDFLAGS
> + CURL_LIBCURL += $(CURL_LDFLAGS)
> +else
> + CURL_LIBCURL += $(shell $(CURL_CONFIG) --libs)
> +endif
> +
>   REMOTE_CURL_PRIMARY = git-remote-http$X
>   REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X 
> git-remote-ftps$X
>   REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
> diff --git a/config.mak.uname b/config.mak.uname
> index 
> 8acdeb71fdab3b3e8e3c536110eb6de13f14e8ff..19e6633040b1db4a148d7b33c4e9d374fe7f87ba
>  100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -431,8 +431,6 @@ ifeq ($(uname_S),Minix)
>   NO_NSEC = YesPlease
>   NEEDS_LIBGEN =
>   NEEDS_CRYPTO_WITH_SSL = YesPlease
> - NEEDS_IDN_WITH_CURL = YesPlease
> - NEEDS_SSL_WITH_CURL = YesPlease
>   NEEDS_RESOLV =
>   NO_HSTRERROR = YesPlease
>   NO_MMAP = YesPlease
> @@ -458,7 +456,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
>   # Missdetected, hence commented out, see below.
>   #NO_CURL = YesPlease
>   # Added manually, see above.
> - NEEDS_SSL_WITH_CURL = YesPlease
>   HAVE_LIBCHARSET_H = YesPlease
>   HAVE_STRINGS_H = YesPlease
>   NEEDS_LIBICONV = YesPlease
> diff --git a/configure.ac b/configure.ac
> index 
> e11b7976ab1c93d8ccec2e499d0093db42551059..44e8c036b6ec417e95ca4e5c2861785900d8634c
>  100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -600,17 +600,1

[PATCH v2] build: link with curl-defined linker flags

[James Knight]

Adjusting the build process to rely more on curl-config to populate
linker flags instead of manually populating flags based off detected
features.

Originally, a configure-invoked build would check for SSL-support in the
target curl library. If enabled, NEEDS_SSL_WITH_CURL would be set and
used in the Makefile to append additional libraries to link against. As
for systems building solely with make, the defines NEEDS_IDN_WITH_CURL
and NEEDS_SSL_WITH_CURL could be set to indirectly enable respective
linker flags. Since both configure.ac and Makefile already rely on
curl-config utility to provide curl-related build information, adjusting
the respective assets to populate required linker flags using the
utility (unless explicitly configured).

Signed-off-by: James Knight 
---
Changes v1 -> v2:
 - Improved support for detecting curl linker flags when not using a
configure-based build (mentioned by Junio C Hamano).
 - Adding a description on how to explicitly use the CURL_LDFLAGS
define when not using configure (suggested by Junio C Hamano).

The original patch made a (bad) assumption that builds would always
invoke ./configure before attempting to build Git. To support a
make-invoked build, CURL_LDFLAGS can also be populated using the defined
curl-config utility. This change also comes with the assumption that
since both ./configure and Makefile are using curl-config to determine
aspects of the build, it should be also fine to use the same utility to
provide the linker flags to compile against (please indicate so if this
is another bad assumption). With this change, the explicit configuration
of CURL_LDFLAGS inside config.mak.uname for Minix and NONSTOP_KERNEL
have been dropped.
---
 Makefile | 30 +++---
 config.mak.uname |  3 ---
 configure.ac | 17 +++--
 3 files changed, 22 insertions(+), 28 deletions(-)

diff --git a/Makefile b/Makefile
index 
b08d5ea258c69a78745dfa73fe698c11d021858a..36da17dc1f9b1a70c9142604afe989f1eb8ee87f
 100644
--- a/Makefile
+++ b/Makefile
@@ -59,6 +59,13 @@ all::
 # Define CURL_CONFIG to curl's configuration program that prints information
 # about the library (e.g., its version number).  The default is 'curl-config'.
 #
+# Define CURL_LDFLAGS to specify flags that you need to link when using 
libcurl,
+# if you do not want to rely on the libraries provided by CURL_CONFIG.  The
+# default value is a result of `curl-config --libs`.  An example value for
+# CURL_LDFLAGS is as follows:
+#
+# CURL_LDFLAGS=-lcurl
+#
 # Define NO_EXPAT if you do not have expat installed.  git-http-push is
 # not built, and you cannot push using http:// and https:// transports (dumb).
 #
@@ -183,10 +190,6 @@ all::
 #
 # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).
 #
-# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).
-#
-# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).
-#
 # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin).
 #
 # Define NEEDS_LIBINTL_BEFORE_LIBICONV if you need libintl before libiconv.
@@ -1305,20 +1308,17 @@ else
ifdef CURLDIR
# Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
BASIC_CFLAGS += -I$(CURLDIR)/include
-   CURL_LIBCURL = -L$(CURLDIR)/$(lib) 
$(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl
+   CURL_LIBCURL = -L$(CURLDIR)/$(lib) 
$(CC_LD_DYNPATH)$(CURLDIR)/$(lib)
else
-   CURL_LIBCURL = -lcurl
-   endif
-   ifdef NEEDS_SSL_WITH_CURL
-   CURL_LIBCURL += -lssl
-   ifdef NEEDS_CRYPTO_WITH_SSL
-   CURL_LIBCURL += -lcrypto
-   endif
-   endif
-   ifdef NEEDS_IDN_WITH_CURL
-   CURL_LIBCURL += -lidn
+   CURL_LIBCURL =
endif
 
+ifdef CURL_LDFLAGS
+   CURL_LIBCURL += $(CURL_LDFLAGS)
+else
+   CURL_LIBCURL += $(shell $(CURL_CONFIG) --libs)
+endif
+
REMOTE_CURL_PRIMARY = git-remote-http$X
REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X 
git-remote-ftps$X
REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
diff --git a/config.mak.uname b/config.mak.uname
index 
8acdeb71fdab3b3e8e3c536110eb6de13f14e8ff..19e6633040b1db4a148d7b33c4e9d374fe7f87ba
 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -431,8 +431,6 @@ ifeq ($(uname_S),Minix)
NO_NSEC = YesPlease
NEEDS_LIBGEN =
NEEDS_CRYPTO_WITH_SSL = YesPlease
-   NEEDS_IDN_WITH_CURL = YesPlease
-   NEEDS_SSL_WITH_CURL = YesPlease
NEEDS_RESOLV =
NO_HSTRERROR = YesPlease
NO_MMAP = YesPlease
@@ -458,7 +456,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
# Missdetected, hence commented out, see below.
#NO_CURL = YesPlease
# Added manually, see above.
-   NEEDS_SSL_WITH_CURL = YesPlease
HAVE_LIBCHARSET_H = YesPlease
HAVE_STRINGS_H = YesPlease
NEEDS_LIBICON

Re: [PATCH] build: link with curl-defined linker flags

[Junio C Hamano]

James Knight  writes:

>  Makefile | 21 +++--
>  config.mak.uname |  5 ++---
>  configure.ac | 17 +++--
>  3 files changed, 16 insertions(+), 27 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index b08d5ea25..c3be87b0e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -183,10 +183,6 @@ all::
>  #
>  # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto 
> (Darwin).
>  #
> -# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).
> -#
> -# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).
> -#

Hmm, because the use of autoconf -> ./configure in our build
procedure is optional, wouldn't this change give regression to those
of us who use these Makefile variables to configure their build,
instead of relying on autoconf?

> diff --git a/config.mak.uname b/config.mak.uname
> index 8acdeb71f..923b8fa09 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -431,8 +431,7 @@ ifeq ($(uname_S),Minix)
>   NO_NSEC = YesPlease
>   NEEDS_LIBGEN =
>   NEEDS_CRYPTO_WITH_SSL = YesPlease
> - NEEDS_IDN_WITH_CURL = YesPlease
> - NEEDS_SSL_WITH_CURL = YesPlease
> + CURL_LDFLAGS = -lssl -lcrypto -lidn

OK, as long as we describe how to update their config.mak to adjust
to the new world order, the "regression" I noticed earlier is not
too bad, and the way the new CURL_LDFLAGS variable drives the build
is much more direct and transparent than via the old way to specify
NEEDS_*_WITH_CURL to affect the build indirectly.

I think such "describe how to configure in the new world order"
should go to where NEEDS_*_WITH_CURL used to be, e.g. "Define
CURL_LDFLAGS to specify flags that you need to link using -lcurl;
see config.mak.uname and look for the variable for examples"
or something like that, perhaps.

Thanks.

[PATCH] build: link with curl-defined linker flags

[James Knight]

Adjust the autotools configuration to populate libcurl-related linker
flags from curl-config instead of manually populating flags based off
detected features.

Originally, the configuration would check for SSL-support in the target
curl library. If enabled, NEEDS_SSL_WITH_CURL would be set and used in
the Makefile to append additional libraries to link against. Since the
process is already depending on a curl-config utility to provide
curl-related build information, adjusting the build to track the linker
flags in CURL_LIBCURL and pass the configuration option into the
Makefile.

Signed-off-by: James Knight 
---
 Makefile | 21 +++--
 config.mak.uname |  5 ++---
 configure.ac | 17 +++--
 3 files changed, 16 insertions(+), 27 deletions(-)

diff --git a/Makefile b/Makefile
index b08d5ea25..c3be87b0e 100644
--- a/Makefile
+++ b/Makefile
@@ -183,10 +183,6 @@ all::
 #
 # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).
 #
-# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).
-#
-# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).
-#
 # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin).
 #
 # Define NEEDS_LIBINTL_BEFORE_LIBICONV if you need libintl before libiconv.
@@ -1305,18 +1301,15 @@ else
ifdef CURLDIR
# Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
BASIC_CFLAGS += -I$(CURLDIR)/include
-   CURL_LIBCURL = -L$(CURLDIR)/$(lib) 
$(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl
+   CURL_LIBCURL = -L$(CURLDIR)/$(lib) 
$(CC_LD_DYNPATH)$(CURLDIR)/$(lib)
else
-   CURL_LIBCURL = -lcurl
-   endif
-   ifdef NEEDS_SSL_WITH_CURL
-   CURL_LIBCURL += -lssl
-   ifdef NEEDS_CRYPTO_WITH_SSL
-   CURL_LIBCURL += -lcrypto
-   endif
+   CURL_LIBCURL =
endif
-   ifdef NEEDS_IDN_WITH_CURL
-   CURL_LIBCURL += -lidn
+
+   ifdef CURL_LDFLAGS
+   CURL_LIBCURL += $(CURL_LDFLAGS)
+   else
+   CURL_LIBCURL += -lcurl
endif
 
REMOTE_CURL_PRIMARY = git-remote-http$X
diff --git a/config.mak.uname b/config.mak.uname
index 8acdeb71f..923b8fa09 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -431,8 +431,7 @@ ifeq ($(uname_S),Minix)
NO_NSEC = YesPlease
NEEDS_LIBGEN =
NEEDS_CRYPTO_WITH_SSL = YesPlease
-   NEEDS_IDN_WITH_CURL = YesPlease
-   NEEDS_SSL_WITH_CURL = YesPlease
+   CURL_LDFLAGS = -lssl -lcrypto -lidn
NEEDS_RESOLV =
NO_HSTRERROR = YesPlease
NO_MMAP = YesPlease
@@ -458,7 +457,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
# Missdetected, hence commented out, see below.
#NO_CURL = YesPlease
# Added manually, see above.
-   NEEDS_SSL_WITH_CURL = YesPlease
+   CURL_LDFLAGS = -lssl -lcrypto
HAVE_LIBCHARSET_H = YesPlease
HAVE_STRINGS_H = YesPlease
NEEDS_LIBICONV = YesPlease
diff --git a/configure.ac b/configure.ac
index e11b7976a..44e8c036b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -600,17 +600,14 @@ AC_CHECK_PROG([CURL_CONFIG], [curl-config],
 
 if test $CURL_CONFIG != no; then
 GIT_CONF_SUBST([CURL_CONFIG])
-if test -z "${NO_OPENSSL}"; then
-  AC_MSG_CHECKING([if Curl supports SSL])
-  if test $(curl-config --features|grep SSL) = SSL; then
- NEEDS_SSL_WITH_CURL=YesPlease
- AC_MSG_RESULT([yes])
-  else
- NEEDS_SSL_WITH_CURL=
- AC_MSG_RESULT([no])
-  fi
-  GIT_CONF_SUBST([NEEDS_SSL_WITH_CURL])
+
+if test -z "$CURL_CONFIG_OPTS"; then
+CURL_CONFIG_OPTS="--libs"
 fi
+
+CURL_LDFLAGS=$($CURL_CONFIG $CURL_CONFIG_OPTS)
+AC_MSG_NOTICE([Setting CURL_LDFLAGS to '$CURL_LDFLAGS'])
+GIT_CONF_SUBST([CURL_LDFLAGS], [$CURL_LDFLAGS])
 fi
 
 fi
-- 
2.19.1

Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

[Junio C Hamano]

Johannes Schindelin  writes:

>> I did not and I do not think it would.  I was wondering if the
>> ability to be able to specify these per destination is something
>> very useful and deserves to be called out in the doc, together with
>> ...
>
> I do not think that it needs to be called out specifically in the docs. It
> is just yet another http.* setting that can be overridden per-URL. It
> would be different if it had not worked.

OK, thanks for sanity checking.

Re: [PATCH] http: give curl version warnings consistently

[Junio C Hamano]

Johannes Schindelin  writes:

> On Thu, 25 Oct 2018, Junio C Hamano wrote:
>
>> When a requested feature cannot be activated because the version of
>> cURL library used to build Git with is too old, most of the codepaths
>> give a warning like "$Feature is not supported with cURL < $Version",
>> marked for l10n.  A few of them, however, did not follow that pattern
>> and said things like "$Feature is not activated, your curl version is
>> too old (>= $Version)", and without marking them for l10n.
>> 
>> Update these to match the style of the majority of warnings and mark
>> them for l10n.
>> 
>> Signed-off-by: Junio C Hamano 
>> ---
>
> I like this patch better than the one I had prepared for v2, so I dropped
> it again, and "hit the Submit button".

I took your v3 and queue this on top, instead of the previous one
on which this was prepared.

By the way, I wondered if we want to unify them by introducing

static void curl_version_warning(const char *feature, const char 
*verstring)
{
warning(_("%s is not supported with cURL < %s"),
feature, verstring);
}

so that translators need to deal with a single instance of the
message, but the "feature" part may have to get localized, in which
case we'd end up with sentence lego, which is not a good idea, so I
dropped it.

Thanks.

Re: [PATCH] http: give curl version warnings consistently

[Johannes Schindelin]

Hi Junio,

On Thu, 25 Oct 2018, Junio C Hamano wrote:

> When a requested feature cannot be activated because the version of
> cURL library used to build Git with is too old, most of the codepaths
> give a warning like "$Feature is not supported with cURL < $Version",
> marked for l10n.  A few of them, however, did not follow that pattern
> and said things like "$Feature is not activated, your curl version is
> too old (>= $Version)", and without marking them for l10n.
> 
> Update these to match the style of the majority of warnings and mark
> them for l10n.
> 
> Signed-off-by: Junio C Hamano 
> ---

I like this patch better than the one I had prepared for v2, so I dropped
it again, and "hit the Submit button".

Ciao,
Dscho

> 
>  > I have a clean-up suggestion related to this but is orthogonal to
>  > this three-patch series (after the fix-up is applied, anyway), which
>  > I'll be sending out separately.
> 
>  So, here it is.
> 
>  http.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/http.c b/http.c
> index 43e75ac583..2214100e3b 100644
> --- a/http.c
> +++ b/http.c
> @@ -834,8 +834,7 @@ static CURL *get_curl_handle(void)
>  #if LIBCURL_VERSION_NUM >= 0x072c00
>   curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
> CURLSSLOPT_NO_REVOKE);
>  #else
> - warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options 
> because\n"
> - "your curl version is too old (< 7.44.0)");
> + warning(_("CURLSSLOPT_NO_REVOKE not suported with cURL < 
> 7.44.0"));
>  #endif
>   }
>  
> @@ -908,8 +907,7 @@ static CURL *get_curl_handle(void)
>   curl_easy_setopt(result, CURLOPT_PROTOCOLS,
>get_curl_allowed_protocols(-1));
>  #else
> - warning("protocol restrictions not applied to curl redirects because\n"
> - "your curl version is too old (>= 7.19.4)");
> + warning(_("Protocol restrictions not supported with cURL < 7.19.4"));
>  #endif
>   if (getenv("GIT_CURL_VERBOSE"))
>   curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
> -- 
> 2.19.1-542-gc4df23f792
> 
> 

[PATCH v2 2/3] http: add support for disabling SSL revocation checks in cURL

[Brendan Forster via GitGitGadget]

From: Brendan Forster 

This adds support for a new http.schannelCheckRevoke config value.

This config value is only used if http.sslBackend is set to "schannel",
which forces cURL to use the Windows Certificate Store when validating
server certificates associated with a remote server.

This config value should only be set to "false" if you are in an
environment where revocation checks are blocked by the network, with
no alternative options.

This is only supported in cURL 7.44 or later.

Note: originally, we wanted to call the config setting
`http.schannel.checkRevoke`. This, however, does not work: the `http.*`
config settings can be limited to specific URLs via `http..*`
(and this feature would mistake `schannel` for a URL).

Helped by Agustín Martín Barbero.

Signed-off-by: Brendan Forster 
Signed-off-by: Johannes Schindelin 
---
 Documentation/config.txt |  8 
 http.c   | 17 +
 2 files changed, 25 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7d38f0bf1..d569ebd49 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1989,6 +1989,14 @@ http.sslBackend::
This option is ignored if cURL lacks support for choosing the SSL
backend at runtime.
 
+http.schannelCheckRevoke::
+   Used to enforce or disable certificate revocation checks in cURL
+   when http.sslBackend is set to "schannel". Defaults to `true` if
+   unset. Only necessary to disable this if Git consistently errors
+   and the message is about checking the revocation status of a
+   certificate. This option is ignored if cURL lacks support for
+   setting the relevant SSL option at runtime.
+
 http.pinnedpubkey::
Public key of the https service. It may either be the filename of
a PEM or DER encoded public key file or a string starting with
diff --git a/http.c b/http.c
index 7fb37a061..65daa9bfa 100644
--- a/http.c
+++ b/http.c
@@ -157,6 +157,8 @@ static char *cached_accept_language;
 
 static char *http_ssl_backend;
 
+static int http_schannel_check_revoke = 1;
+
 size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
size_t size = eltsize * nmemb;
@@ -310,6 +312,11 @@ static int http_options(const char *var, const char 
*value, void *cb)
return 0;
}
 
+   if (!strcmp("http.schannelcheckrevoke", var)) {
+   http_schannel_check_revoke = git_config_bool(var, value);
+   return 0;
+   }
+
if (!strcmp("http.minsessions", var)) {
min_curl_sessions = git_config_int(var, value);
 #ifndef USE_CURL_MULTI
@@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
}
 #endif
 
+   if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
+   !http_schannel_check_revoke) {
+#if LIBCURL_VERSION_NUM >= 0x072c00
+   curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
CURLSSLOPT_NO_REVOKE);
+#else
+   warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options 
because\n"
+   "your curl version is too old (< 7.44.0)");
+#endif
+   }
+
if (http_proactive_auth)
init_curl_http_auth(result);
 
-- 
gitgitgadget

[PATCH v2 0/3] Allow choosing the SSL backend cURL uses (plus related patches)

[Johannes Schindelin via GitGitGadget]

This topic branch brings support for choosing cURL's SSL backend (a feature
developed in Git for Windows' context) at runtime via http.sslBackend, and
two more patches that are related (and only of interest for Windows users).

Changes since v1:

 * Reworded the commit message of v1's patch 2/3, to talk about the original
   design instead of "an earlier iteration" that was never contributed to
   the Git mailing list.
 * Changed the confusing >= 7.44.0 to < 7.44.0.

Note: I had prepared 
https://github.com/dscho/git/commit/81e8c9a4006c919747a0b6a287f28f25799fcaf4
, intended to be included in v2, but Junio came up with 
https://public-inbox.org/git/xmqqsh0uln5c.fsf...@gitster-ct.c.googlers.com/ 
in the meantime, which I like better.

Brendan Forster (1):
  http: add support for disabling SSL revocation checks in cURL

Johannes Schindelin (2):
  http: add support for selecting SSL backends at runtime
  http: when using Secure Channel, ignore sslCAInfo by default

 Documentation/config.txt | 21 
 http.c   | 71 +++-
 2 files changed, 91 insertions(+), 1 deletion(-)


base-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-46%2Fdscho%2Fhttp-ssl-backend-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-46/dscho/http-ssl-backend-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/46

Range-diff vs v1:

 1:  8c5ecdb6c = 1:  85bd0fb27 http: add support for selecting SSL backends at 
runtime
 2:  764791d13 ! 2:  951383695 http: add support for disabling SSL revocation 
checks in cURL
 @@ -14,10 +14,10 @@
  
  This is only supported in cURL 7.44 or later.
  
 -Note: an earlier iteration tried to use the config setting
 -http.schannel.checkRevoke, but the http.* config settings can be 
limited
 -to specific URLs via http..* (which would mistake `schannel` for 
a
 -URL).
 +Note: originally, we wanted to call the config setting
 +`http.schannel.checkRevoke`. This, however, does not work: the 
`http.*`
 +config settings can be limited to specific URLs via `http..*`
 +(and this feature would mistake `schannel` for a URL).
  
  Helped by Agustín Martín Barbero.
  
 @@ -77,7 +77,7 @@
  + curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
CURLSSLOPT_NO_REVOKE);
  +#else
  + warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options 
because\n"
 -+ "your curl version is too old (>= 7.44.0)");
 ++ "your curl version is too old (< 7.44.0)");
  +#endif
  + }
  +
 3:  9927e4ce6 = 3:  a5f937a36 http: when using Secure Channel, ignore 
sslCAInfo by default

-- 
gitgitgadget

Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

[Johannes Schindelin]

Hi Junio,

On Thu, 18 Oct 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >> In any case, you can use "http..$variable" to say "I want the
> >> http.$variable to be in effect but only when I am talking to ".
> >> Does it make sense for this new variable, too?  That is, does it
> >> benefit the users to be able to do something like this?
> >> 
> >> [http] schannelCheckRevoke = no
> >> [http "https://microsoft.com/;] schannelCheckRevoke = yes
> >> 
> >> I am guessing that the answer is yes.
> >
> > Frankly, I do not know.  Does it hurt, though?
> 
> I did not and I do not think it would.  I was wondering if the
> ability to be able to specify these per destination is something
> very useful and deserves to be called out in the doc, together with
> ...

I do not think that it needs to be called out specifically in the docs. It
is just yet another http.* setting that can be overridden per-URL. It
would be different if it had not worked.

> >> I guess the same comment applies to the previous step, but I suspect
> >> that the code structure may not allow us to switch the SSL backend
> >> so late in the game (e.g. "when talking to microsoft, use schannel,
> >> but when talking to github, use openssl").
> 
> ... this bit.
> 
> > Crucially, this fails. The short version is: this is good! Because it
> > means that Git used the OpenSSL backend, as clearly intended.
> >
> > 
> > Why does it fail?
> > ...
> > 
> 
> So there may still be some polishing needed, but as long as people
> are not using the "per destination" thing, the code is already good?
> That is something we may want to document.

Actually, just because there is a peculiar bug in this particular code
flow in Git for Windows should not necessarily be interesting to Git's
commit history.

On Linux, for example, it would work.

So I don't think we need to mention anything about that unrelated bug.

Thanks,
Dscho

> Thanks.
> 

Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

[Johannes Schindelin]

Hi Junio,

On Thu, 25 Oct 2018, Junio C Hamano wrote:

> Eric Sunshine  writes:
> 
> > On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
> >  wrote:
> >> This config value is only used if http.sslBackend is set to "schannel",
> >> which forces cURL to use the Windows Certificate Store when validating
> >> server certificates associated with a remote server.
> >>
> >> This is only supported in cURL 7.44 or later.
> >> [...]
> >> Signed-off-by: Brendan Forster 
> >> ---
> >> diff --git a/http.c b/http.c
> >> @@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
> >> +   if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
> >> +   !http_schannel_check_revoke) {
> >> +#if LIBCURL_VERSION_NUM >= 0x072c00
> >> +   curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
> >> CURLSSLOPT_NO_REVOKE);
> >> +#else
> >> +   warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL 
> >> options because\n"
> >> +   "your curl version is too old (>= 7.44.0)");
> >
> > This message is confusing. If your curl is too old, shouldn't the ">=" be a 
> > "<"?
> 
> I do not think I saw any update to correct this, and worse yet I do
> not offhand recall if there was any other issue raised on the
> series.

Sorry, my bad. I dropped the ball. As you can see here:

https://github.com/gitgitgadget/git/pull/46

I have some updates that are already pushed, but I still wanted to really
think through your response here:

https://public-inbox.org/git/xmqq1s8oxbpc@gitster-ct.c.googlers.com/

and what I should do about it, before sending off v2. You can see that I
already updated the description in preparation for sending another
iteration.

I hope to get back to this tonight, for now I must scramble off to
non-work-related activities.

Ciao,
Dscho

> So assuming that this is the only remaining one, I'll squash the
> following to step 2/3 of this three-patch series and plan to merge
> it down to 'next' in the coming few days.
> 
> I have a clean-up suggestion related to this but is orthogonal to
> this three-patch series (after the fix-up is applied, anyway), which
> I'll be sending out separately.
> 
> Thanks.
> 
>  http.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/http.c b/http.c
> index 0ebf8f77a6..43e75ac583 100644
> --- a/http.c
> +++ b/http.c
> @@ -835,7 +835,7 @@ static CURL *get_curl_handle(void)
>   curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
> CURLSSLOPT_NO_REVOKE);
>  #else
>   warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options 
> because\n"
> - "your curl version is too old (>= 7.44.0)");
> + "your curl version is too old (< 7.44.0)");
>  #endif
>   }
>  
> -- 
> 2.19.1-542-gc4df23f792
> 
> 

Re: [PATCH] http: give curl version warnings consistently

[Jeff King]

On Thu, Oct 25, 2018 at 12:29:19PM +0900, Junio C Hamano wrote:

> When a requested feature cannot be activated because the version of
> cURL library used to build Git with is too old, most of the codepaths
> give a warning like "$Feature is not supported with cURL < $Version",
> marked for l10n.  A few of them, however, did not follow that pattern
> and said things like "$Feature is not activated, your curl version is
> too old (>= $Version)", and without marking them for l10n.
> 
> Update these to match the style of the majority of warnings and mark
> them for l10n.

This is definitely an improvement.

> @@ -908,8 +907,7 @@ static CURL *get_curl_handle(void)
>   curl_easy_setopt(result, CURLOPT_PROTOCOLS,
>get_curl_allowed_protocols(-1));
>  #else
> - warning("protocol restrictions not applied to curl redirects because\n"
> - "your curl version is too old (>= 7.19.4)");
> + warning(_("Protocol restrictions not supported with cURL < 7.19.4"));

This loses the mention of redirects, but I think that is actually a
bonus. The #ifdef'd code covers both original and redirected requests.

-Peff

[PATCH] http: give curl version warnings consistently

[Junio C Hamano]

When a requested feature cannot be activated because the version of
cURL library used to build Git with is too old, most of the codepaths
give a warning like "$Feature is not supported with cURL < $Version",
marked for l10n.  A few of them, however, did not follow that pattern
and said things like "$Feature is not activated, your curl version is
too old (>= $Version)", and without marking them for l10n.

Update these to match the style of the majority of warnings and mark
them for l10n.

Signed-off-by: Junio C Hamano 
---

 > I have a clean-up suggestion related to this but is orthogonal to
 > this three-patch series (after the fix-up is applied, anyway), which
 > I'll be sending out separately.

 So, here it is.

 http.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index 43e75ac583..2214100e3b 100644
--- a/http.c
+++ b/http.c
@@ -834,8 +834,7 @@ static CURL *get_curl_handle(void)
 #if LIBCURL_VERSION_NUM >= 0x072c00
curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
CURLSSLOPT_NO_REVOKE);
 #else
-   warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options 
because\n"
-   "your curl version is too old (< 7.44.0)");
+   warning(_("CURLSSLOPT_NO_REVOKE not suported with cURL < 
7.44.0"));
 #endif
}
 
@@ -908,8 +907,7 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_PROTOCOLS,
 get_curl_allowed_protocols(-1));
 #else
-   warning("protocol restrictions not applied to curl redirects because\n"
-   "your curl version is too old (>= 7.19.4)");
+   warning(_("Protocol restrictions not supported with cURL < 7.19.4"));
 #endif
if (getenv("GIT_CURL_VERBOSE"))
curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
-- 
2.19.1-542-gc4df23f792

Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

[Junio C Hamano]

Eric Sunshine  writes:

> On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
>  wrote:
>> This config value is only used if http.sslBackend is set to "schannel",
>> which forces cURL to use the Windows Certificate Store when validating
>> server certificates associated with a remote server.
>>
>> This is only supported in cURL 7.44 or later.
>> [...]
>> Signed-off-by: Brendan Forster 
>> ---
>> diff --git a/http.c b/http.c
>> @@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
>> +   if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
>> +   !http_schannel_check_revoke) {
>> +#if LIBCURL_VERSION_NUM >= 0x072c00
>> +   curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
>> CURLSSLOPT_NO_REVOKE);
>> +#else
>> +   warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL 
>> options because\n"
>> +   "your curl version is too old (>= 7.44.0)");
>
> This message is confusing. If your curl is too old, shouldn't the ">=" be a 
> "<"?

I do not think I saw any update to correct this, and worse yet I do
not offhand recall if there was any other issue raised on the
series.

So assuming that this is the only remaining one, I'll squash the
following to step 2/3 of this three-patch series and plan to merge
it down to 'next' in the coming few days.

I have a clean-up suggestion related to this but is orthogonal to
this three-patch series (after the fix-up is applied, anyway), which
I'll be sending out separately.

Thanks.

 http.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/http.c b/http.c
index 0ebf8f77a6..43e75ac583 100644
--- a/http.c
+++ b/http.c
@@ -835,7 +835,7 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
CURLSSLOPT_NO_REVOKE);
 #else
warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options 
because\n"
-   "your curl version is too old (>= 7.44.0)");
+   "your curl version is too old (< 7.44.0)");
 #endif
}
 
-- 
2.19.1-542-gc4df23f792

Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

[Junio C Hamano]

Johannes Schindelin  writes:

>> In any case, you can use "http..$variable" to say "I want the
>> http.$variable to be in effect but only when I am talking to ".
>> Does it make sense for this new variable, too?  That is, does it
>> benefit the users to be able to do something like this?
>> 
>> [http] schannelCheckRevoke = no
>> [http "https://microsoft.com/;] schannelCheckRevoke = yes
>> 
>> I am guessing that the answer is yes.
>
> Frankly, I do not know.  Does it hurt, though?

I did not and I do not think it would.  I was wondering if the
ability to be able to specify these per destination is something
very useful and deserves to be called out in the doc, together with
...

>> I guess the same comment applies to the previous step, but I suspect
>> that the code structure may not allow us to switch the SSL backend
>> so late in the game (e.g. "when talking to microsoft, use schannel,
>> but when talking to github, use openssl").

... this bit.

> Crucially, this fails. The short version is: this is good! Because it
> means that Git used the OpenSSL backend, as clearly intended.
>
> 
> Why does it fail?
> ...
> 

So there may still be some polishing needed, but as long as people
are not using the "per destination" thing, the code is already good?
That is something we may want to document.

Thanks.

Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

[Jeff King]

On Tue, Oct 16, 2018 at 02:25:57PM +0200, Johannes Schindelin wrote:

> > > That ">=" is hard to grok.  I think you meant it to be pronounced
> > > "requries at least", but that is not a common reading.  People more
> > > commonly pronounce it "is greater than or equal to".
> > 
> > This seemed oddly familiar:
> > 
> >   
> > https://public-inbox.org/git/8da9d436-88b9-7959-dd9c-65bdd376b...@mail.mipt.ru/
> > 
> > Since this one is clearly copied from there, it may be worth fixing the
> > original.
> 
> Good memory. I just integrated the patch here. It was not signed off, but
> it is too obvious to be protected under copyright, so I re-did it, adding
> a nice commit message.

Yay, thank you (I considered doing the same, but was scratching my head
over how to deal with authorship and signoff).

-Peff

Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

[Johannes Schindelin]

Hi Peff,

On Tue, 16 Oct 2018, Jeff King wrote:

> On Tue, Oct 16, 2018 at 01:23:25PM +0900, Junio C Hamano wrote:
> 
> > > +#if LIBCURL_VERSION_NUM >= 0x072c00
> > > + curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
> > > CURLSSLOPT_NO_REVOKE);
> > > +#else
> > > + warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options 
> > > because\n"
> > > + "your curl version is too old (>= 7.44.0)");
> > > +#endif
> > 
> > That ">=" is hard to grok.  I think you meant it to be pronounced
> > "requries at least", but that is not a common reading.  People more
> > commonly pronounce it "is greater than or equal to".
> 
> This seemed oddly familiar:
> 
>   
> https://public-inbox.org/git/8da9d436-88b9-7959-dd9c-65bdd376b...@mail.mipt.ru/
> 
> Since this one is clearly copied from there, it may be worth fixing the
> original.

Good memory. I just integrated the patch here. It was not signed off, but
it is too obvious to be protected under copyright, so I re-did it, adding
a nice commit message.

Ciao,
Dscho

Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

[Johannes Schindelin]

Hi Junio,

On Tue, 16 Oct 2018, Junio C Hamano wrote:

> "Brendan Forster via GitGitGadget"  writes:
> 
> > Note: an earlier iteration tried to use the config setting
> > http.schannel.checkRevoke, but the http.* config settings can be limited
> > to specific URLs via http..* (which would mistake `schannel` for a
> > URL).
> 
> Yeah, "http.schannel.anything" would not work, but is this note
> relevant here?  As far as the git development community as a whole
> is concerned, this is the first iteration of the patch we see and
> review.

I did review that commit message before typing /submit, and I figured that
it would still make sense to leave a comment there why we did *not* use
http.schannel.checkRevoke. I know that *my* review comments would include
a question about this, had I not been part of the development of this
patch.

> In any case, you can use "http..$variable" to say "I want the
> http.$variable to be in effect but only when I am talking to ".
> Does it make sense for this new variable, too?  That is, does it
> benefit the users to be able to do something like this?
> 
> [http] schannelCheckRevoke = no
> [http "https://microsoft.com/;] schannelCheckRevoke = yes
> 
> I am guessing that the answer is yes.

Frankly, I do not know.  Does it hurt, though?

This patch neither introduces the `http..` feature nor prevents
it. Its original design (which I found more logical than the current one),
however, clashes with that feature.

> I guess the same comment applies to the previous step, but I suspect
> that the code structure may not allow us to switch the SSL backend
> so late in the game (e.g. "when talking to microsoft, use schannel,
> but when talking to github, use openssl").

That's a really good question, and I suspect that it is actually not too
late. Let me try.

*clicketyclick*

-- snip --
$ git config --show-origin http.sslbackend
file:C:/Program Files/Git/mingw64/etc/gitconfig schannel

$ GIT_TRACE_CURL=1 git -c 
'http.https://github.com/dscho/images.sslbackend=openssl' ls-remote 
https://github.com/dscho/images
14:03:52.319986 http.c:724  == Info: Couldn't find host github.com 
in the _netrc file; using defaults
14:03:52.366858 http.c:724  == Info:   Trying 192.30.253.113...
14:03:52.366858 http.c:724  == Info: TCP_NODELAY set
14:03:52.482825 http.c:724  == Info: Connected to github.com 
(192.30.253.113) port 443 (#0)
14:03:52.721173 http.c:724  == Info: ALPN, offering http/1.1
14:03:52.721173 http.c:724  == Info: Cipher selection: 
ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
14:03:52.721173 http.c:724  == Info: error setting certificate 
verify locations:
  CAfile: C:/Program Files/Git/mingw64/libexec/ssl/certs/ca-bundle.crt
  CApath: none
fatal: unable to access 'https://github.com/dscho/images/': error setting 
certificate verify locations:
  CAfile: C:/Program Files/Git/mingw64/libexec/ssl/certs/ca-bundle.crt
  CApath: none
-- snap --

Please allow me to help understand this log. First, I verified that the
currently-configured backend is Secure Channel. Then, I ask Git to list
the remote refs of a small repository, special-casing it to the OpenSSL
backend.

Crucially, this fails. The short version is: this is good! Because it
means that Git used the OpenSSL backend, as clearly intended.


Why does it fail?

Two reasons:

1) Git for Windows has to disable the certificate bundle. The gist of it
is: Git LFS uses Git for Windows' certificate bundle, if configured, and
that would override the Windows Certificate Store. When users configure
Secure Channel, however, they *want* to use the Windows Certificate Store,
so to accommodate Git LFS, we "unconfigure" http.sslCAInfo in that case.

2) The libcurl used by Git for Windows has some smarts built in to
understand relative paths to its "Unix pseudo root". However, this fails
when libcurl is loaded from libexec/git-core/ (which is the case, to avoid
any libcurl-4.dll in C:\Windows\system32 from being picked up by mistake).

If this explanation sounds obscure, the reason is that it *is* obscure. If
you are truly interested in the details, I can point you to the relevant
tickets on Git for Windows' bug tracker.


> > +#if LIBCURL_VERSION_NUM >= 0x072c00
> > +   curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
> > CURLSSLOPT_NO_REVOKE);
> > +#else
> > +   warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options 
> > because\n"
> > +   "your curl version is too old (>= 7.44.0)");
> > +#endif
> 
> That ">=" is hard to grok.  I think you meant it to be pronounced
> "requries at least", but that is not a common reading.  People more
>

Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

[Johannes Schindelin]

Hi Eric,


On Mon, 15 Oct 2018, Eric Sunshine wrote:

> On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
>  wrote:
> > This config value is only used if http.sslBackend is set to "schannel",
> > which forces cURL to use the Windows Certificate Store when validating
> > server certificates associated with a remote server.
> >
> > This is only supported in cURL 7.44 or later.
> > [...]
> > Signed-off-by: Brendan Forster 
> > ---
> > diff --git a/http.c b/http.c
> > @@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
> > +   if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
> > +   !http_schannel_check_revoke) {
> > +#if LIBCURL_VERSION_NUM >= 0x072c00
> > +   curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
> > CURLSSLOPT_NO_REVOKE);
> > +#else
> > +   warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL 
> > options because\n"
> > +   "your curl version is too old (>= 7.44.0)");
> 
> This message is confusing. If your curl is too old, shouldn't the ">=" be a 
> "<"?

Absolutely correct. Will fix,
Dscho

> 
> > +#endif
> > +   }
> 

Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

[Jeff King]

On Tue, Oct 16, 2018 at 01:23:25PM +0900, Junio C Hamano wrote:

> > +#if LIBCURL_VERSION_NUM >= 0x072c00
> > +   curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
> > CURLSSLOPT_NO_REVOKE);
> > +#else
> > +   warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options 
> > because\n"
> > +   "your curl version is too old (>= 7.44.0)");
> > +#endif
> 
> That ">=" is hard to grok.  I think you meant it to be pronounced
> "requries at least", but that is not a common reading.  People more
> commonly pronounce it "is greater than or equal to".

This seemed oddly familiar:

  
https://public-inbox.org/git/8da9d436-88b9-7959-dd9c-65bdd376b...@mail.mipt.ru/

Since this one is clearly copied from there, it may be worth fixing the
original.

-Peff

Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

[Junio C Hamano]

"Brendan Forster via GitGitGadget"  writes:

> Note: an earlier iteration tried to use the config setting
> http.schannel.checkRevoke, but the http.* config settings can be limited
> to specific URLs via http..* (which would mistake `schannel` for a
> URL).

Yeah, "http.schannel.anything" would not work, but is this note
relevant here?  As far as the git development community as a whole
is concerned, this is the first iteration of the patch we see and
review.

In any case, you can use "http..$variable" to say "I want the
http.$variable to be in effect but only when I am talking to ".
Does it make sense for this new variable, too?  That is, does it
benefit the users to be able to do something like this?

[http] schannelCheckRevoke = no
[http "https://microsoft.com/;] schannelCheckRevoke = yes

I am guessing that the answer is yes.

I guess the same comment applies to the previous step, but I suspect
that the code structure may not allow us to switch the SSL backend
so late in the game (e.g. "when talking to microsoft, use schannel,
but when talking to github, use openssl").

> +#if LIBCURL_VERSION_NUM >= 0x072c00
> + curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
> CURLSSLOPT_NO_REVOKE);
> +#else
> + warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options 
> because\n"
> + "your curl version is too old (>= 7.44.0)");
> +#endif

That ">=" is hard to grok.  I think you meant it to be pronounced
"requries at least", but that is not a common reading.  People more
commonly pronounce it "is greater than or equal to".

> + }
> +
>   if (http_proactive_auth)
>   init_curl_http_auth(result);

Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

[Eric Sunshine]

On Mon, Oct 15, 2018 at 6:14 AM Brendan Forster via GitGitGadget
 wrote:
> This config value is only used if http.sslBackend is set to "schannel",
> which forces cURL to use the Windows Certificate Store when validating
> server certificates associated with a remote server.
>
> This is only supported in cURL 7.44 or later.
> [...]
> Signed-off-by: Brendan Forster 
> ---
> diff --git a/http.c b/http.c
> @@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
> +   if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
> +   !http_schannel_check_revoke) {
> +#if LIBCURL_VERSION_NUM >= 0x072c00
> +   curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
> CURLSSLOPT_NO_REVOKE);
> +#else
> +   warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options 
> because\n"
> +   "your curl version is too old (>= 7.44.0)");

This message is confusing. If your curl is too old, shouldn't the ">=" be a "<"?

> +#endif
> +   }

[PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

[Brendan Forster via GitGitGadget]

From: Brendan Forster 

This adds support for a new http.schannelCheckRevoke config value.

This config value is only used if http.sslBackend is set to "schannel",
which forces cURL to use the Windows Certificate Store when validating
server certificates associated with a remote server.

This config value should only be set to "false" if you are in an
environment where revocation checks are blocked by the network, with
no alternative options.

This is only supported in cURL 7.44 or later.

Note: an earlier iteration tried to use the config setting
http.schannel.checkRevoke, but the http.* config settings can be limited
to specific URLs via http..* (which would mistake `schannel` for a
URL).

Helped by Agustín Martín Barbero.

Signed-off-by: Brendan Forster 
Signed-off-by: Johannes Schindelin 
---
 Documentation/config.txt |  8 
 http.c   | 17 +
 2 files changed, 25 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7d38f0bf1a..d569ebd49e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1989,6 +1989,14 @@ http.sslBackend::
This option is ignored if cURL lacks support for choosing the SSL
backend at runtime.
 
+http.schannelCheckRevoke::
+   Used to enforce or disable certificate revocation checks in cURL
+   when http.sslBackend is set to "schannel". Defaults to `true` if
+   unset. Only necessary to disable this if Git consistently errors
+   and the message is about checking the revocation status of a
+   certificate. This option is ignored if cURL lacks support for
+   setting the relevant SSL option at runtime.
+
 http.pinnedpubkey::
Public key of the https service. It may either be the filename of
a PEM or DER encoded public key file or a string starting with
diff --git a/http.c b/http.c
index 7fb37a061b..8998056b60 100644
--- a/http.c
+++ b/http.c
@@ -157,6 +157,8 @@ static char *cached_accept_language;
 
 static char *http_ssl_backend;
 
+static int http_schannel_check_revoke = 1;
+
 size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
size_t size = eltsize * nmemb;
@@ -310,6 +312,11 @@ static int http_options(const char *var, const char 
*value, void *cb)
return 0;
}
 
+   if (!strcmp("http.schannelcheckrevoke", var)) {
+   http_schannel_check_revoke = git_config_bool(var, value);
+   return 0;
+   }
+
if (!strcmp("http.minsessions", var)) {
min_curl_sessions = git_config_int(var, value);
 #ifndef USE_CURL_MULTI
@@ -811,6 +818,16 @@ static CURL *get_curl_handle(void)
}
 #endif
 
+   if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) &&
+   !http_schannel_check_revoke) {
+#if LIBCURL_VERSION_NUM >= 0x072c00
+   curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, 
CURLSSLOPT_NO_REVOKE);
+#else
+   warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options 
because\n"
+   "your curl version is too old (>= 7.44.0)");
+#endif
+   }
+
if (http_proactive_auth)
init_curl_http_auth(result);
 
-- 
gitgitgadget

[PATCH 0/3] Allow choosing the SSL backend cURL uses (plus related patches)

[Johannes Schindelin via GitGitGadget]

This topic branch brings support for choosing cURL's SSL backend (a feature
developed in Git for Windows' context) at runtime via http.sslBackend, and
two more patches that are related (and only of interest for Windows users).

Brendan Forster (1):
  http: add support for disabling SSL revocation checks in cURL

Johannes Schindelin (2):
  http: add support for selecting SSL backends at runtime
  http: when using Secure Channel, ignore sslCAInfo by default

 Documentation/config.txt | 21 
 http.c   | 71 +++-
 2 files changed, 91 insertions(+), 1 deletion(-)


base-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-46%2Fdscho%2Fhttp-ssl-backend-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-46/dscho/http-ssl-backend-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/46
-- 
gitgitgadget

Re: [PATCH 2/2] remote-curl: remove spurious period

[Elijah Newren]

On Wed, Aug 8, 2018 at 4:52 AM Johannes Schindelin via GitGitGadget
 wrote:
>
> We should not interrupt. sentences in the middle.

I love this. commit message.  :-)

...
> /* The client backend isn't giving us compressed data so
> -* we can try to deflate it ourselves, this may save on.
> +* we can try to deflate it ourselves, this may save on
>  * the transfer time.

[PATCH 2/2] remote-curl: remove spurious period

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin 

We should not interrupt. sentences in the middle.

Signed-off-by: Johannes Schindelin 
---
 remote-curl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 99b0bedc6..fb28309e8 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -714,7 +714,7 @@ retry:
 
} else if (use_gzip && 1024 < rpc->len) {
/* The client backend isn't giving us compressed data so
-* we can try to deflate it ourselves, this may save on.
+* we can try to deflate it ourselves, this may save on
 * the transfer time.
 */
git_zstream stream;
-- 
gitgitgadget

Re: [curl PATCH 2/2] ignore SIGPIPE during curl_multi_cleanup

[dxt29]

I have curl 7.35.0 installed on my ubuntu14.04, version infos is as below


I have recompiled git against openssl. the git version is 1.9.1. I
encountered this error "error: git-remote-http died of signal 13" when I
issue `git clone http://github.com/tensorflow/tensorflow.git`. Should I
upgrade curl to a higher version? Or is there other easy solutions?

Thanks.



--
Sent from: http://git.661346.n2.nabble.com/

[PATCH v1 20/25] structured-logging: add structured logging to remote-curl

[git]

From: Jeff Hostetler 

remote-curl is not a builtin command and therefore, does not inherit
the common cmd_main() startup in git.c

Wrap cmd_main() with slog_cmd_main() in remote-curl to initialize
logging.

Add slog timers around push, fetch, and list verbs.

Signed-off-by: Jeff Hostetler 
---
 remote-curl.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 99b0bed..ed910f8 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1322,8 +1322,9 @@ static int stateless_connect(const char *service_name)
return 0;
 }
 
-int cmd_main(int argc, const char **argv)
+static int real_cmd_main(int argc, const char **argv)
 {
+   int slog_tid;
struct strbuf buf = STRBUF_INIT;
int nongit;
 
@@ -1333,6 +1334,8 @@ int cmd_main(int argc, const char **argv)
return 1;
}
 
+   slog_set_command_name("remote-curl");
+
options.verbosity = 1;
options.progress = !!isatty(2);
options.thin = 1;
@@ -1362,14 +1365,20 @@ int cmd_main(int argc, const char **argv)
if (starts_with(buf.buf, "fetch ")) {
if (nongit)
die("remote-curl: fetch attempted without a 
local repo");
+   slog_tid = slog_start_timer("curl", "fetch");
parse_fetch();
+   slog_stop_timer(slog_tid);
 
} else if (!strcmp(buf.buf, "list") || starts_with(buf.buf, 
"list ")) {
int for_push = !!strstr(buf.buf + 4, "for-push");
+   slog_tid = slog_start_timer("curl", "list");
output_refs(get_refs(for_push));
+   slog_stop_timer(slog_tid);
 
    } else if (starts_with(buf.buf, "push ")) {
+   slog_tid = slog_start_timer("curl", "push");
parse_push();
+   slog_stop_timer(slog_tid);
 
} else if (skip_prefix(buf.buf, "option ", )) {
char *value = strchr(arg, ' ');
@@ -1411,3 +1420,8 @@ int cmd_main(int argc, const char **argv)
 
return 0;
 }
+
+int cmd_main(int argc, const char **argv)
+{
+   return slog_wrap_main(real_cmd_main, argc, argv);
+}
-- 
2.9.3

RE: [PATCH 1/2] remote-curl: accept all encoding supported by curl

[anton.golubev]

Hi Jonathan,

I'd like to confirm, that your patch fixes my problem: I can sync with
google repository now using git and curl version without gzip support. 
Do you know how this patch is going to be released? Just HEAD now and GA in
the next planned release?

Communication looks like follow now:

root@bsb:~# GIT_TRACE_PACKET=1 GIT_CURL_VERBOSE=1 git ls-remote
https://source.developers.google.co m/p/wired-balm-187912/r/dotfiles 2>&1 |
sed -e 's/\(git-[^=]*\)=.*/\1=REDACTED/' -e 's/Authorizatio n:
.*/Authorization: REDACTED/'
> GET /p/wired-balm-187912/r/dotfiles/info/refs?service=git-upload-pack
HTTP/1.1
Host: source.developers.google.com
User-Agent: git/2.17.0
Accept: */*
Accept-Encoding: identity
Cookie: o=git-anton.golubev.gmail.com=REDACTED
Pragma: no-cache

< HTTP/1.1 200 OK
< Cache-Control: no-cache, max-age=0, must-revalidate
< Content-Length: 374
< Content-Type: application/x-git-upload-pack-advertisement
< Expires: Fri, 01 Jan 1980 00:00:00 GMT
< Pragma: no-cache
< X-Content-Type-Options: nosniff
< X-Frame-Options: SAMEORIGIN
< X-Xss-Protection: 1; mode=block
< Date: Sat, 26 May 2018 11:04:41 GMT
< Alt-Svc: hq=":443"; ma=2592000; quic=51303433; quic=51303432;
quic=51303431; quic=51303339; quic=51303335,quic=":443"; ma=2592000;
v="43,42,41,39,35"
<
13:04:41.274561 pkt-line.c:80   packet:  git< #
service=git-upload-pack
13:04:41.274634 pkt-line.c:80   packet:  git< 
13:04:41.274693 pkt-line.c:80   packet:  git<
45e2c99dd1790529cc4b7e029b1e9dfcc817d18e HEAD\0 include-tag
multi_ack_detailed multi_ack ofs-delta side-band side-band-64k thin-pack
no-progress shallow no-done allow-tip-sha1-in-want
allow-reachable-sha1-in-want agent=JGit/4-google filter
symref=HEAD:refs/heads/master
13:04:41.274739 pkt-line.c:80   packet:  git<
45e2c99dd1790529cc4b7e029b1e9dfcc817d18e refs/heads/master
13:04:41.274777 pkt-line.c:80   packet:  git< 
45e2c99dd1790529cc4b7e029b1e9dfcc817d18eHEAD
45e2c99dd1790529cc4b7e029b1e9dfcc817d18erefs/heads/master

Kind regards,
Anton Golubev



-Original Message-
From: Jonathan Nieder [mailto:jrnie...@gmail.com] 
Sent: Dienstag, 22. Mai 2018 02:01
To: Brandon Williams <bmw...@google.com>
Cc: git@vger.kernel.org; Anton Golubev <anton.golu...@gmail.com>
Subject: Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl

Hi,

Brandon Williams wrote:

> Subject: remote-curl: accept all encoding supported by curl

nit: s/encoding/encodings

> Configure curl to accept all encoding which curl supports instead of 
> only accepting gzip responses.

Likewise.

> This is necessary to fix a bug when using an installation of curl 
> which doesn't support gzip.  Since curl doesn't do any checking to 
> verify that it supports the encoding set when calling 
> 'curl_easy_setopt()', curl can end up sending an "Accept-Encoding" 
> header indicating that it supports a particular encoding when in fact 
> it doesn't.  Instead when the empty string "" is used when setting 
> `CURLOPT_ENCODING`, curl will send an "Accept-Encoding" header 
> containing only the encoding methods curl supports.
>
> Signed-off-by: Brandon Williams <bmw...@google.com>

Thanks for the analysis and fix.

Reported-by: Anton Golubev <anton.golu...@gmail.com>

Also ccing the reporter so we can hopefully get a tested-by.  Anton, can you
test this patch and let us know how it goes?  You can apply it as follows:

  curl \
 
https://public-inbox.org/git/20180521234004.142548-1-bmw...@google.com/raw \
>patch.txt
  git am -3 patch.txt

Brandon, can the commit message also say a little more about the motivating
context and symptoms?

  $ curl --version
  curl 7.52.1 (arm-openwrt-linux-gnu) libcurl/7.52.1 mbedTLS/2.6.0
  Protocols: file ftp ftps http https
  Features: IPv6 Largefile SSL

The issue is that when curl is built without the "zlib" feature, since
v1.8.0-rc0~14^2 (Enable info/refs gzip decompression in HTTP client,
2012-09-19) we end up requesting "gzip" encoding anyway despite libcurl not
being able to decode it.  Worse, instead of getting a clear error message
indicating so, we end up falling back to "dumb"
http, producing a confusing and difficult to debug result.

> ---
>  http.c  | 2 +-
>  remote-curl.c   | 2 +-
>  t/t5551-http-fetch-smart.sh | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/http.c b/http.c
> index fed13b216..709150fc7 100644
> --- a/http.c
> +++ b/http.c
> @@ -1788,7 +1788,7 @@ static int http_request(const char *url,
>  
>   curl_easy_setopt(slot->curl, CURLOPT_URL, url);
>   curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
> - curl_easy_

Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl

[Daniel Stenberg]

On Wed, 23 May 2018, Junio C Hamano wrote:


-> Accept-Encoding: gzip
+> Accept-Encoding: ENCODINGS


Is the ordering of these headers determined by the user of cURL library 
(i.e. Git), or whatever the version of cURL we happened to link with happens 
to produce?


The point is whether the order is expected to be stable, or we are better 
off sorting the actual log before comparing.


The order is not guaranteed by libcurl to be fixed, but it is likely to remain 
stable since we too have test cases and compare outputs with expected outputs! 
=)


Going forward, brotli (br) is going to become more commonly present in that 
header.


--

 / daniel.haxx.se

Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl

[brian m. carlson]

On Wed, May 23, 2018 at 10:23:26AM +0900, Junio C Hamano wrote:
> Similarly, how much control do we have to ensure that the test HTTPD
> server (1) supports gzip and (2) does not support encoding algos
> with confusing names e.g. "funnygzipalgo" that may accidentally
> match that pattern?

I feel it's quite likely indeed that pretty much any Apache instance is
going to have the gzip encoding.  Every distributor I know supports it.

As for whether there are confusing alternate algorithms, I think it's
best to just look at the IANA registration[0] to see what people are
using.  Potential matches include gzip, x-gzip (a deprecated alias that
versions of Apache we can use are not likely to support), and
pack200-gzip (a format for Java archives, which we hope the remote side
will not be sending).

Overall, I think this is not likely to be a problem, but if necessary in
the future, we can add a prerequisite that looks in the module directory
for the appropriate module.  We haven't seen an issue with it yet,
though, TTBOMK.

[0] 
https://www.iana.org/assignments/http-parameters/http-parameters.xml#content-coding
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl

[Junio C Hamano]

Brandon Williams <bmw...@google.com> writes:

> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index f5721b4a5..913089b14 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -26,14 +26,14 @@ setup_askpass_helper
>  cat >exp <  > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
>  > Accept: */*
> -> Accept-Encoding: gzip
> +> Accept-Encoding: ENCODINGS
>  > Pragma: no-cache

Is the ordering of these headers determined by the user of cURL
library (i.e. Git), or whatever the version of cURL we happened to
link with happens to produce?

The point is whether the order is expected to be stable, or we are
better off sorting the actual log before comparing.

>  < HTTP/1.1 200 OK
>  < Pragma: no-cache
>  < Cache-Control: no-cache, max-age=0, must-revalidate
>  < Content-Type: application/x-git-upload-pack-advertisement

A similar question for this response.

>  > POST /smart/repo.git/git-upload-pack HTTP/1.1
> -> Accept-Encoding: gzip
> +> Accept-Encoding: ENCODINGS
>  > Content-Type: application/x-git-upload-pack-request
>  > Accept: application/x-git-upload-pack-result
>  > Content-Length: xxx

Ditto for this request.

> @@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '
>   /^< Date: /d
>   /^< Content-Length: /d
>   /^< Transfer-Encoding: /d
> - " >act &&
> - test_cmp exp act
> + " >actual &&
> + sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \
> + actual >actual.smudged &&
> + test_cmp exp actual.smudged &&
> +
> + grep "Accept-Encoding:.*gzip" actual >actual.gzip &&
> + test_line_count = 2 actual.gzip
>  '

Similarly, how much control do we have to ensure that the test HTTPD
server (1) supports gzip and (2) does not support encoding algos
with confusing names e.g. "funnygzipalgo" that may accidentally
match that pattern?

Thanks.  Not a new issue with this patch, but just being curious if
you or anybody thought about it as a possible issue.

Re: [PATCH v2 2/2] remote-curl: accept compressed responses with protocol v2

[Jonathan Nieder]

Brandon Williams wrote:

> Configure curl to accept compressed responses when using protocol v2 by
> setting `CURLOPT_ENCODING` to "", which indicates that curl should send
> an "Accept-Encoding" header with all supported compression encodings.
>
> Signed-off-by: Brandon Williams <bmw...@google.com>
> ---
>  remote-curl.c | 1 +
>  1 file changed, 1 insertion(+)

Yay!

> diff --git a/remote-curl.c b/remote-curl.c
> index 565bba104..99b0bedc6 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -1259,6 +1259,7 @@ static int proxy_request(struct proxy_state *p)
>  
>   slot = get_active_slot();
>  
> + curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>   curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
>   curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
>   curl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);

Can this get a test?

I'm particularly interested in it since it's easy to accidentally
apply this patch to the wrong duplicated place (luckily 'p' is a
different variable name than 'rpc' but it's an easy mistake to make if
applying the patch manually).

With or without such a test,
Reviewed-by: Jonathan Nieder <jrnie...@gmail.com>

Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl

[Jonathan Nieder]

Brandon Williams wrote:

> Configure curl to accept all encodings which curl supports instead of
> only accepting gzip responses.
>
> This fixes an issue when using an installation of curl which is built
> without the "zlib" feature. Since aa90b9697 (Enable info/refs gzip
> decompression in HTTP client, 2012-09-19) we end up requesting "gzip"
> encoding anyway despite libcurl not being able to decode it.  Worse,
> instead of getting a clear error message indicating so, we end up
> falling back to "dumb" http, producing a confusing and difficult to
> debug result.
>
> Since curl doesn't do any checking to verify that it supports the a
> requested encoding, instead set the curl option `CURLOPT_ENCODING` with
> an empty string indicating that curl should send an "Accept-Encoding"
> header containing only the encodings supported by curl.

Even better, this means we get the benefit of future of even better
compression algorithms once libcurl learns them.

> Reported-by: Anton Golubev <anton.golu...@gmail.com>
> Signed-off-by: Brandon Williams <bmw...@google.com>
> ---
> Version 2 of this series just tweaks the commit message and the test per
> Jonathan's suggestion.
>
>  http.c  |  2 +-
>  remote-curl.c   |  2 +-
>  t/t5551-http-fetch-smart.sh | 13 +
>  3 files changed, 11 insertions(+), 6 deletions(-)

Reviewed-by: Jonathan Nieder <jrnie...@gmail.com>

Thanks for fixing it.

Patch left unsnipped for reference.

> --- a/http.c
> +++ b/http.c
> @@ -1788,7 +1788,7 @@ static int http_request(const char *url,
>  
>   curl_easy_setopt(slot->curl, CURLOPT_URL, url);
>   curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
> - curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
> + curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>  
>   ret = run_one_slot(slot, );
>  
> diff --git a/remote-curl.c b/remote-curl.c
> index ceb05347b..565bba104 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)
>   curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
>   curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
>   curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);
> - curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
> + curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>  
>   if (large_request) {
>   /* The request body is large and the size cannot be predicted.
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index f5721b4a5..913089b14 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -26,14 +26,14 @@ setup_askpass_helper
>  cat >exp <  > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
>  > Accept: */*
> -> Accept-Encoding: gzip
> +> Accept-Encoding: ENCODINGS
>  > Pragma: no-cache
>  < HTTP/1.1 200 OK
>  < Pragma: no-cache
>  < Cache-Control: no-cache, max-age=0, must-revalidate
>  < Content-Type: application/x-git-upload-pack-advertisement
>  > POST /smart/repo.git/git-upload-pack HTTP/1.1
> -> Accept-Encoding: gzip
> +> Accept-Encoding: ENCODINGS
>  > Content-Type: application/x-git-upload-pack-request
>  > Accept: application/x-git-upload-pack-result
>  > Content-Length: xxx
> @@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '
>   /^< Date: /d
>   /^< Content-Length: /d
>   /^< Transfer-Encoding: /d
> - " >act &&
> - test_cmp exp act
> + " >actual &&
> + sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \
> + actual >actual.smudged &&
> + test_cmp exp actual.smudged &&
> +
> + grep "Accept-Encoding:.*gzip" actual >actual.gzip &&
> + test_line_count = 2 actual.gzip
>  '
>  
>  test_expect_success 'fetch changes via http' '

[PATCH v2 1/2] remote-curl: accept all encodings supported by curl

[Brandon Williams]

Configure curl to accept all encodings which curl supports instead of
only accepting gzip responses.

This fixes an issue when using an installation of curl which is built
without the "zlib" feature. Since aa90b9697 (Enable info/refs gzip
decompression in HTTP client, 2012-09-19) we end up requesting "gzip"
encoding anyway despite libcurl not being able to decode it.  Worse,
instead of getting a clear error message indicating so, we end up
falling back to "dumb" http, producing a confusing and difficult to
debug result.

Since curl doesn't do any checking to verify that it supports the a
requested encoding, instead set the curl option `CURLOPT_ENCODING` with
an empty string indicating that curl should send an "Accept-Encoding"
header containing only the encodings supported by curl.

Reported-by: Anton Golubev <anton.golu...@gmail.com>
Signed-off-by: Brandon Williams <bmw...@google.com>
---

Version 2 of this series just tweaks the commit message and the test per
Jonathan's suggestion.

 http.c  |  2 +-
 remote-curl.c   |  2 +-
 t/t5551-http-fetch-smart.sh | 13 +
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/http.c b/http.c
index fed13b216..709150fc7 100644
--- a/http.c
+++ b/http.c
@@ -1788,7 +1788,7 @@ static int http_request(const char *url,
 
curl_easy_setopt(slot->curl, CURLOPT_URL, url);
curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
-   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
+   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
 
ret = run_one_slot(slot, );
 
diff --git a/remote-curl.c b/remote-curl.c
index ceb05347b..565bba104 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
    curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);
-   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
+   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
 
if (large_request) {
/* The request body is large and the size cannot be predicted.
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index f5721b4a5..913089b14 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -26,14 +26,14 @@ setup_askpass_helper
 cat >exp < GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
 > Accept: */*
-> Accept-Encoding: gzip
+> Accept-Encoding: ENCODINGS
 > Pragma: no-cache
 < HTTP/1.1 200 OK
 < Pragma: no-cache
 < Cache-Control: no-cache, max-age=0, must-revalidate
 < Content-Type: application/x-git-upload-pack-advertisement
 > POST /smart/repo.git/git-upload-pack HTTP/1.1
-> Accept-Encoding: gzip
+> Accept-Encoding: ENCODINGS
 > Content-Type: application/x-git-upload-pack-request
 > Accept: application/x-git-upload-pack-result
 > Content-Length: xxx
@@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '
/^< Date: /d
/^< Content-Length: /d
/^< Transfer-Encoding: /d
-   " >act &&
-   test_cmp exp act
+   " >actual &&
+   sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \
+   actual >actual.smudged &&
+   test_cmp exp actual.smudged &&
+
+   grep "Accept-Encoding:.*gzip" actual >actual.gzip &&
+   test_line_count = 2 actual.gzip
 '
 
 test_expect_success 'fetch changes via http' '
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 2/2] remote-curl: accept compressed responses with protocol v2

[Brandon Williams]

Configure curl to accept compressed responses when using protocol v2 by
setting `CURLOPT_ENCODING` to "", which indicates that curl should send
an "Accept-Encoding" header with all supported compression encodings.

Signed-off-by: Brandon Williams <bmw...@google.com>
---
 remote-curl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/remote-curl.c b/remote-curl.c
index 565bba104..99b0bedc6 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1259,6 +1259,7 @@ static int proxy_request(struct proxy_state *p)
 
slot = get_active_slot();
 
+   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
curl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl

[Brandon Williams]

On 05/22, Daniel Stenberg wrote:
> On Mon, 21 May 2018, Jonathan Nieder wrote:
> 
> > > Looking at the code here, this succeeds if enough memory is available.
> > > There is no check if the given parameter is part of
> > > Curl_all_content_encodings();
> > 
> > By "this" are you referring to the preimage or the postimage?  Are you
> > suggesting a change in git or in libcurl?
> > 
> > Curl_all_content_encodings() is an internal function in libcurl, so I'm
> > assuming the latter.
> 
> Ack, that certainly isn't the most wonderful API for selecting a compression
> method. In reality, almost everyone sticks to passing on a "" to that option
> to let libcurl pick and ask for the compression algos it knows since both
> gzip and brotli are present only conditionally depending on build options.

Thanks for the clarification.  Sounds like the best option is to
continue with this patch and let curl decide using "".

> 
> I would agree that the libcurl setopt call should probably be made to fail
> if asked to use a compression method not built-in/supported. Then an
> application could in fact try different algos in order until one works or
> ask to disable compression completely.
> 
> In the generic HTTP case, it usually makes sense to ask for more than one
> algorthim though, since this is asking the server for a compressed version
> and typically a HTTP client doesn't know which compression methods the
> server offers. Not sure this is actually true to the same extent for git.
> 
> -- 
> 
>  / daniel.haxx.se

-- 
Brandon Williams

Re: [curl PATCH 2/2] ignore SIGPIPE during curl_multi_cleanup

[Daniel Stenberg]

On Tue, 22 May 2018, Suganthi wrote:

We may not be able to upgrade to 7.60.0 any soon, Is the fix present in 7.45 
, in this sequence of code. Please let me know.


I don't know.

I can't recall any SIGPIPE problems in recent days in libcurl, which is why I 
believe this problem doesn't exist anymore. libcurl 7.45.0 is 2.5 years and 
1500+ bug fixes old after all. My casual searches for a curl problem like this 
- fixed in 7.45.0 or later - also failed.


--

 / daniel.haxx.se

Re: [curl PATCH 2/2] ignore SIGPIPE during curl_multi_cleanup

[curlUser]

We may not be able to upgrade to 7.60.0 any soon, 
Is the fix present in 7.45 , in this sequence of code. 
Please let me know. 



--
Sent from: http://git.661346.n2.nabble.com/

Re: [curl PATCH 2/2] ignore SIGPIPE during curl_multi_cleanup

[Daniel Stenberg]

On Tue, 22 May 2018, curlUser wrote:


Again SIGPIPE is seen with curl version 7.45.0 with multi interface.
Backtrace shows :


...

Looks like SIGPIPE_IGNORE to be added in prune_dead connections or in 
disconnect_if_dead? Can anyone comment on this.


I'm pretty sure this issue isn't present in any recent libcurl versions, but 
if you can reproduce it with 7.60.0, I'll be very interested.


--

 / daniel.haxx.se

Re: [curl PATCH 2/2] ignore SIGPIPE during curl_multi_cleanup

[curlUser]

Hi,

Again SIGPIPE is seen with curl version 7.45.0 with multi interface.
Backtrace shows :

#7  0x7f141bea40cd in Curl_ossl_close (conn=0x7f14193f9848, sockindex=0)
at vtls/openssl.c:881
#8  0x7f141bea8f54 in Curl_ssl_close (conn=0x7f14193f9848, sockindex=0)
at vtls/vtls.c:527
#9  0x7f141be63969 in Curl_disconnect (conn=0x7f14193f9848,
dead_connection=true) at url.c:2791
#10 0x7f141be63f4b in disconnect_if_dead (conn=0x7f14193f9848,
data=0xb6a598) at url.c:3050
#11 0x7f141be63f84 in call_disconnect_if_dead (conn=0x7f14193f9848,
param=0xb6a598) at url.c:3066
#12 0x7f141bea01c2 in Curl_conncache_foreach (connc=0xae0f48,
param=0xb6a598, func=0x7f141be63f59 )
at conncache.c:295
#13 0x7f141be6400f in prune_dead_connections (data=0xb6a598) at
url.c:3081

Looks like SIGPIPE_IGNORE to be added in prune_dead connections or in
disconnect_if_dead?
Can anyone comment on this. 



--
Sent from: http://git.661346.n2.nabble.com/

Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl

[Daniel Stenberg]

On Mon, 21 May 2018, Jonathan Nieder wrote:


Looking at the code here, this succeeds if enough memory is available.
There is no check if the given parameter is part of
Curl_all_content_encodings();


By "this" are you referring to the preimage or the postimage?  Are you 
suggesting a change in git or in libcurl?


Curl_all_content_encodings() is an internal function in libcurl, so I'm 
assuming the latter.


Ack, that certainly isn't the most wonderful API for selecting a compression 
method. In reality, almost everyone sticks to passing on a "" to that option 
to let libcurl pick and ask for the compression algos it knows since both gzip 
and brotli are present only conditionally depending on build options.


I would agree that the libcurl setopt call should probably be made to fail if 
asked to use a compression method not built-in/supported. Then an application 
could in fact try different algos in order until one works or ask to disable 
compression completely.


In the generic HTTP case, it usually makes sense to ask for more than one 
algorthim though, since this is asking the server for a compressed version and 
typically a HTTP client doesn't know which compression methods the server 
offers. Not sure this is actually true to the same extent for git.


--

 / daniel.haxx.se

Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl

[Jonathan Nieder]

Hi,

Stefan Beller wrote:
> On Mon, May 21, 2018 at 4:40 PM, Brandon Williams <bmw...@google.com> wrote:

>> Configure curl to accept all encoding which curl supports instead of
>> only accepting gzip responses.
>
> This partially reverts aa90b9697f9 (Enable info/refs gzip decompression
> in HTTP client, 2012-09-19), as that specifically called out deflate not being
> a good option. Is that worth mentioning in the commit message?

More specifically, it mentions the wasted 9 extra bytes from including
"deflate, " on the Accept-Encoding line.  I think the extra bandwidth
usage will be okay. :)

[...]
>> -   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
>> +   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>
> Looking at the code here, this succeeds if enough memory is available.
> There is no check if the given parameter is part of
> Curl_all_content_encodings();
> https://github.com/curl/curl/blob/e66cca046cef20d00fba89260dfa6b4a3997233d/lib/setopt.c#L429
> https://github.com/curl/curl/blob/c675c40295045d4988eeb6291c54eb48f138822f/lib/content_encoding.c#L686
>
> which may be worth checking first?

By "this" are you referring to the preimage or the postimage?  Are you
suggesting a change in git or in libcurl?

Curl_all_content_encodings() is an internal function in libcurl, so
I'm assuming the latter.

Thanks,
Jonathan

Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl

[Stefan Beller]

On Mon, May 21, 2018 at 4:40 PM, Brandon Williams <bmw...@google.com> wrote:
> Configure curl to accept all encoding which curl supports instead of
> only accepting gzip responses.

This partially reverts aa90b9697f9 (Enable info/refs gzip decompression
in HTTP client, 2012-09-19), as that specifically called out deflate not being
a good option. Is that worth mentioning in the commit message?

> This is necessary to fix a bug when using an installation of curl which
> doesn't support gzip.  Since curl doesn't do any checking to verify that
> it supports the encoding set when calling 'curl_easy_setopt()', curl can
> end up sending an "Accept-Encoding" header indicating that it supports
> a particular encoding when in fact it doesn't.  Instead when the empty
> string "" is used when setting `CURLOPT_ENCODING`, curl will send an
> "Accept-Encoding" header containing only the encoding methods curl
> supports.
>
> Signed-off-by: Brandon Williams <bmw...@google.com>
> ---
>  http.c  | 2 +-
>  remote-curl.c   | 2 +-
>  t/t5551-http-fetch-smart.sh | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/http.c b/http.c
> index fed13b216..709150fc7 100644
> --- a/http.c
> +++ b/http.c
> @@ -1788,7 +1788,7 @@ static int http_request(const char *url,
>
> curl_easy_setopt(slot->curl, CURLOPT_URL, url);
> curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
> -   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
> +   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>
> ret = run_one_slot(slot, );
>
> diff --git a/remote-curl.c b/remote-curl.c
> index ceb05347b..565bba104 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)
> curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
> curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
> curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);
> -   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
> +   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");

Looking at the code here, this succeeds if enough memory is available.
There is no check if the given parameter is part of
Curl_all_content_encodings();
https://github.com/curl/curl/blob/e66cca046cef20d00fba89260dfa6b4a3997233d/lib/setopt.c#L429
https://github.com/curl/curl/blob/c675c40295045d4988eeb6291c54eb48f138822f/lib/content_encoding.c#L686

which may be worth checking first?

>
> if (large_request) {
> /* The request body is large and the size cannot be predicted.
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index f5721b4a5..39c65482c 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -26,14 +26,14 @@ setup_askpass_helper
>  cat >exp <  > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
>  > Accept: */*
> -> Accept-Encoding: gzip
> +> Accept-Encoding: deflate, gzip
>  > Pragma: no-cache
>  < HTTP/1.1 200 OK
>  < Pragma: no-cache
>  < Cache-Control: no-cache, max-age=0, must-revalidate
>  < Content-Type: application/x-git-upload-pack-advertisement
>  > POST /smart/repo.git/git-upload-pack HTTP/1.1
> -> Accept-Encoding: gzip
> +> Accept-Encoding: deflate, gzip
>  > Content-Type: application/x-git-upload-pack-request
>  > Accept: application/x-git-upload-pack-result
>  > Content-Length: xxx
> --
> 2.17.0.441.gb46fe60e1d-goog
>

Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl

[Jonathan Nieder]

Hi,

Brandon Williams wrote:

> Subject: remote-curl: accept all encoding supported by curl

nit: s/encoding/encodings

> Configure curl to accept all encoding which curl supports instead of
> only accepting gzip responses.

Likewise.

> This is necessary to fix a bug when using an installation of curl which
> doesn't support gzip.  Since curl doesn't do any checking to verify that
> it supports the encoding set when calling 'curl_easy_setopt()', curl can
> end up sending an "Accept-Encoding" header indicating that it supports
> a particular encoding when in fact it doesn't.  Instead when the empty
> string "" is used when setting `CURLOPT_ENCODING`, curl will send an
> "Accept-Encoding" header containing only the encoding methods curl
> supports.
>
> Signed-off-by: Brandon Williams <bmw...@google.com>

Thanks for the analysis and fix.

Reported-by: Anton Golubev <anton.golu...@gmail.com>

Also ccing the reporter so we can hopefully get a tested-by.  Anton,
can you test this patch and let us know how it goes?  You can apply it
as follows:

  curl \
https://public-inbox.org/git/20180521234004.142548-1-bmw...@google.com/raw \
>patch.txt
  git am -3 patch.txt

Brandon, can the commit message also say a little more about the
motivating context and symptoms?

  $ curl --version
  curl 7.52.1 (arm-openwrt-linux-gnu) libcurl/7.52.1 mbedTLS/2.6.0
  Protocols: file ftp ftps http https
  Features: IPv6 Largefile SSL

The issue is that when curl is built without the "zlib" feature, since
v1.8.0-rc0~14^2 (Enable info/refs gzip decompression in HTTP client,
2012-09-19) we end up requesting "gzip" encoding anyway despite
libcurl not being able to decode it.  Worse, instead of getting a
clear error message indicating so, we end up falling back to "dumb"
http, producing a confusing and difficult to debug result.

> ---
>  http.c  | 2 +-
>  remote-curl.c   | 2 +-
>  t/t5551-http-fetch-smart.sh | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/http.c b/http.c
> index fed13b216..709150fc7 100644
> --- a/http.c
> +++ b/http.c
> @@ -1788,7 +1788,7 @@ static int http_request(const char *url,
>  
>   curl_easy_setopt(slot->curl, CURLOPT_URL, url);
>   curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
> - curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
> + curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>  
>   ret = run_one_slot(slot, );
>  
> diff --git a/remote-curl.c b/remote-curl.c
> index ceb05347b..565bba104 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)
>   curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
>   curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
>   curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);
> - curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
> + curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
>  
>   if (large_request) {
>   /* The request body is large and the size cannot be predicted.

Makes sense.

> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index f5721b4a5..39c65482c 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -26,14 +26,14 @@ setup_askpass_helper
>  cat >exp <  > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
>  > Accept: */*
> -> Accept-Encoding: gzip
> +> Accept-Encoding: deflate, gzip
>  > Pragma: no-cache
>  < HTTP/1.1 200 OK
>  < Pragma: no-cache
>  < Cache-Control: no-cache, max-age=0, must-revalidate
>  < Content-Type: application/x-git-upload-pack-advertisement
>  > POST /smart/repo.git/git-upload-pack HTTP/1.1
> -> Accept-Encoding: gzip
> +> Accept-Encoding: deflate, gzip
>  > Content-Type: application/x-git-upload-pack-request
>  > Accept: application/x-git-upload-pack-result
>  > Content-Length: xxx

If libcurl gains support for another encoding in the future, this test
would start failing.  Can we make the matching less strict?  For
example, how about something like the following for squashing in?

Thanks,
Jonathan

diff --git i/t/t5551-http-fetch-smart.sh w/t/t5551-http-fetch-smart.sh
index 39c65482ce..913089b144 100755
--- i/t/t5551-http-fetch-smart.sh
+++ w/t/t5551-http-fetch-smart.sh
@@ -26,14 +26,14 @@ setup_askpass_helper
 cat >exp < GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
 > Accept: */*
-> Accept-Encoding: deflate, gzip
+> Accept-Encoding: ENCODINGS
 > Pragma: no-cache
 < HTTP/1.1 200 OK
 < Pragma: no-cache
 

[PATCH 2/2] remote-curl: accept compressed responses with protocol v2

[Brandon Williams]

Configure curl to accept compressed responses when using protocol v2 by
setting `CURLOPT_ENCODING` to "", which indicates that curl should send
an "Accept-Encoding" header with all supported compression encodings.

Signed-off-by: Brandon Williams <bmw...@google.com>
---
 remote-curl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/remote-curl.c b/remote-curl.c
index 565bba104..99b0bedc6 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1259,6 +1259,7 @@ static int proxy_request(struct proxy_state *p)
 
slot = get_active_slot();
 
+   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
curl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 1/2] remote-curl: accept all encoding supported by curl

[Brandon Williams]

Configure curl to accept all encoding which curl supports instead of
only accepting gzip responses.

This is necessary to fix a bug when using an installation of curl which
doesn't support gzip.  Since curl doesn't do any checking to verify that
it supports the encoding set when calling 'curl_easy_setopt()', curl can
end up sending an "Accept-Encoding" header indicating that it supports
a particular encoding when in fact it doesn't.  Instead when the empty
string "" is used when setting `CURLOPT_ENCODING`, curl will send an
"Accept-Encoding" header containing only the encoding methods curl
supports.

Signed-off-by: Brandon Williams <bmw...@google.com>
---
 http.c  | 2 +-
 remote-curl.c   | 2 +-
 t/t5551-http-fetch-smart.sh | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index fed13b216..709150fc7 100644
--- a/http.c
+++ b/http.c
@@ -1788,7 +1788,7 @@ static int http_request(const char *url,
 
curl_easy_setopt(slot->curl, CURLOPT_URL, url);
curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
-   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
+   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
 
ret = run_one_slot(slot, );
 
diff --git a/remote-curl.c b/remote-curl.c
index ceb05347b..565bba104 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);
-   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
+   curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
 
if (large_request) {
/* The request body is large and the size cannot be predicted.
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index f5721b4a5..39c65482c 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -26,14 +26,14 @@ setup_askpass_helper
 cat >exp < GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
 > Accept: */*
-> Accept-Encoding: gzip
+> Accept-Encoding: deflate, gzip
 > Pragma: no-cache
 < HTTP/1.1 200 OK
 < Pragma: no-cache
 < Cache-Control: no-cache, max-age=0, must-revalidate
 < Content-Type: application/x-git-upload-pack-advertisement
 > POST /smart/repo.git/git-upload-pack HTTP/1.1
-> Accept-Encoding: gzip
+> Accept-Encoding: deflate, gzip
 > Content-Type: application/x-git-upload-pack-request
 > Accept: application/x-git-upload-pack-result
 > Content-Length: xxx
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 2/2] t5561: skip tests if curl is not available

[Jeff King]

It's possible to have libcurl installed but not the curl
command-line utility. The latter is not generally needed for
Git's http support, but we use it in t5561 for basic tests
of http-backend's functionality. Let's detect when it's
missing and skip this test.

Note that we can't mark the individual tests with the CURL
prerequisite. They're in a shared t556x_common that uses the
GET and POST functions as a level of indirection, and it's
only our implementations of those functions in t5561 that
requires curl. It's not a problem, though, as literally
every test in the script would depend on the prerequisite
anyway.

Reported-by: Jens Krüger <jens.krue...@frm2.tum.de>
Signed-off-by: Jeff King <p...@peff.net>
---
 t/t5561-http-backend.sh | 6 ++
 t/test-lib.sh   | 4 
 2 files changed, 10 insertions(+)

diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
index 6167563986..84a955770a 100755
--- a/t/t5561-http-backend.sh
+++ b/t/t5561-http-backend.sh
@@ -3,6 +3,12 @@
 test_description='test git-http-backend'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-httpd.sh
+
+if ! test_have_prereq CURL; then
+   skip_all='skipping raw http-backend tests, curl not available'
+   test_done
+fi
+
 start_httpd
 
 GET() {
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7740d511d2..1aa39fe889 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1208,3 +1208,7 @@ test_lazy_prereq LONG_IS_64BIT '
 
 test_lazy_prereq TIME_IS_64BIT 'test-date is64bit'
 test_lazy_prereq TIME_T_IS_64BIT 'test-date time_t-is64bit'
+
+test_lazy_prereq CURL '
+   curl --version
+'
-- 
2.17.0.686.g08b0810b04

[PATCH 1/2] t5561: drop curl stderr redirects

[Jeff King]

For a normal test run, stderr is already redirected to
/dev/null by the test suite. When used with "-v",
suppressing stderr is actively harmful, as it may hide the
reason for curl failing.

Reported-by: Jens Krüger <jens.krue...@frm2.tum.de>
Signed-off-by: Jeff King <p...@peff.net>
---
 t/t5561-http-backend.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
index 90e0d6f0fe..6167563986 100755
--- a/t/t5561-http-backend.sh
+++ b/t/t5561-http-backend.sh
@@ -6,7 +6,7 @@ test_description='test git-http-backend'
 start_httpd
 
 GET() {
-   curl --include "$HTTPD_URL/$SMART/repo.git/$1" >out 2>/dev/null &&
+   curl --include "$HTTPD_URL/$SMART/repo.git/$1" >out &&
tr '\015' Q out 2>/dev/null &&
+   "$HTTPD_URL/smart/repo.git/$1" >out &&
tr '\015' Q 

[PATCH 0/2] t5561 fails without curl installed

[Jeff King]

On Tue, Apr 03, 2018 at 09:14:47AM -0400, Jeff King wrote:

> As far as code changes in Git, perhaps (assuming my guess is right):
> 
>   - drop the redirect of stderr here; the test suite already handles
> hiding stderr from the user (without "-v"), and in "-v" mode you
> probably would have gotten a useful error like "curl: not found"
> 
>   - it's rare but possible to have libcurl installed (which is needed
> for the server side, and what we key on for running the httpd tests)
> but not the curl binary. This test probably should check for the
> existence of the curl binary as a prerequisite.

So let's do this before we forget.

  [1/2]: t5561: drop curl stderr redirects
  [2/2]: t5561: skip tests if curl is not available

 t/t5561-http-backend.sh | 10 --
 t/test-lib.sh   |  4 
 2 files changed, 12 insertions(+), 2 deletions(-)

-Peff

[PATCH v6 34/35] remote-curl: implement stateless-connect command

[Brandon Williams]

Teach remote-curl the 'stateless-connect' command which is used to
establish a stateless connection with servers which support protocol
version 2.  This allows remote-curl to act as a proxy, allowing the git
client to communicate natively with a remote end, simply using
remote-curl as a pass through to convert requests to http.

Signed-off-by: Brandon Williams <bmw...@google.com>
---
 remote-curl.c  | 207 -
 t/t5702-protocol-v2.sh |  45 +
 2 files changed, 251 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 66a53f74bb..87f5b77b29 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -188,7 +188,12 @@ static struct ref *parse_git_refs(struct discovery *heads, 
int for_push)
heads->version = discover_version();
switch (heads->version) {
case protocol_v2:
-   die("support for protocol v2 not implemented yet");
+   /*
+* Do nothing.  This isn't a list of refs but rather a
+* capability advertisement.  Client would have run
+* 'stateless-connect' so we'll dump this capability listing
+* and let them request the refs themselves.
+*/
break;
case protocol_v1:
case protocol_v0:
@@ -1085,6 +1090,202 @@ static void parse_push(struct strbuf *buf)
free(specs);
 }
 
+/*
+ * Used to represent the state of a connection to an HTTP server when
+ * communicating using git's wire-protocol version 2.
+ */
+struct proxy_state {
+   char *service_name;
+   char *service_url;
+   struct curl_slist *headers;
+   struct strbuf request_buffer;
+   int in;
+   int out;
+   struct packet_reader reader;
+   size_t pos;
+   int seen_flush;
+};
+
+static void proxy_state_init(struct proxy_state *p, const char *service_name,
+enum protocol_version version)
+{
+   struct strbuf buf = STRBUF_INIT;
+
+   memset(p, 0, sizeof(*p));
+   p->service_name = xstrdup(service_name);
+
+   p->in = 0;
+   p->out = 1;
+   strbuf_init(>request_buffer, 0);
+
+   strbuf_addf(, "%s%s", url.buf, p->service_name);
+   p->service_url = strbuf_detach(, NULL);
+
+   p->headers = http_copy_default_headers();
+
+   strbuf_addf(, "Content-Type: application/x-%s-request", 
p->service_name);
+   p->headers = curl_slist_append(p->headers, buf.buf);
+   strbuf_reset();
+
+   strbuf_addf(, "Accept: application/x-%s-result", p->service_name);
+   p->headers = curl_slist_append(p->headers, buf.buf);
+   strbuf_reset();
+
+   p->headers = curl_slist_append(p->headers, "Transfer-Encoding: 
chunked");
+
+   /* Add the Git-Protocol header */
+   if (get_protocol_http_header(version, ))
+   p->headers = curl_slist_append(p->headers, buf.buf);
+
+   packet_reader_init(>reader, p->in, NULL, 0,
+  PACKET_READ_GENTLE_ON_EOF);
+
+   strbuf_release();
+}
+
+static void proxy_state_clear(struct proxy_state *p)
+{
+   free(p->service_name);
+   free(p->service_url);
+   curl_slist_free_all(p->headers);
+   strbuf_release(>request_buffer);
+}
+
+/*
+ * CURLOPT_READFUNCTION callback function.
+ * Attempts to copy over a single packet-line at a time into the
+ * curl provided buffer.
+ */
+static size_t proxy_in(char *buffer, size_t eltsize,
+  size_t nmemb, void *userdata)
+{
+   size_t max;
+   struct proxy_state *p = userdata;
+   size_t avail = p->request_buffer.len - p->pos;
+
+
+   if (eltsize != 1)
+   BUG("curl read callback called with size = %"PRIuMAX" != 1",
+   (uintmax_t)eltsize);
+   max = nmemb;
+
+   if (!avail) {
+   if (p->seen_flush) {
+   p->seen_flush = 0;
+   return 0;
+   }
+
+   strbuf_reset(>request_buffer);
+   switch (packet_reader_read(>reader)) {
+   case PACKET_READ_EOF:
+   die("unexpected EOF when reading from parent process");
+   case PACKET_READ_NORMAL:
+   packet_buf_write_len(>request_buffer, p->reader.line,
+p->reader.pktlen);
+   break;
+   case PACKET_READ_DELIM:
+   packet_buf_delim(>request_buffer);
+   break;
+   case PACKET_READ_FLUSH:
+   packet_buf_flush(>request_buffer);
+   p->seen_flush = 1;
+   break;
+   }
+   p->pos = 0;
+   avail = p->request_buffer.len;
+ 

[PATCH v6 35/35] remote-curl: don't request v2 when pushing

[Brandon Williams]

In order to be able to ship protocol v2 with only supporting fetch, we
need clients to not issue a request to use protocol v2 when pushing
(since the client currently doesn't know how to push using protocol v2).
This allows a client to have protocol v2 configured in
`protocol.version` and take advantage of using v2 for fetch and falling
back to using v0 when pushing while v2 for push is being designed.

We could run into issues if we didn't fall back to protocol v2 when
pushing right now.  This is because currently a server will ignore a request to
use v2 when contacting the 'receive-pack' endpoint and fall back to
using v0, but when push v2 is rolled out to servers, the 'receive-pack'
endpoint will start responding using v2.  So we don't want to get into a
state where a client is requesting to push with v2 before they actually
know how to push using v2.

Signed-off-by: Brandon Williams 
---
 remote-curl.c  | 11 ++-
 t/t5702-protocol-v2.sh | 24 
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 87f5b77b29..595447b16e 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -322,6 +322,7 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
struct discovery *last = last_discovery;
int http_ret, maybe_smart = 0;
struct http_get_options http_options;
+   enum protocol_version version = get_protocol_version_config();
 
if (last && !strcmp(service, last->service))
return last;
@@ -338,8 +339,16 @@ static struct discovery *discover_refs(const char 
*service, int for_push)
strbuf_addf(_url, "service=%s", service);
}
 
+   /*
+* NEEDSWORK: If we are trying to use protocol v2 and we are planning
+* to perform a push, then fallback to v0 since the client doesn't know
+* how to push yet using v2.
+*/
+   if (version == protocol_v2 && !strcmp("git-receive-pack", service))
+   version = protocol_v0;
+
/* Add the extra Git-Protocol header */
-   if (get_protocol_http_header(get_protocol_version_config(), 
_header))
+   if (get_protocol_http_header(version, _header))
string_list_append(_headers, protocol_header.buf);
 
memset(_options, 0, sizeof(http_options));
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 124063c2c4..56f7c3c326 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -244,6 +244,30 @@ test_expect_success 'fetch with http:// using protocol v2' 
'
grep "git< version 2" log
 '
 
+test_expect_success 'push with http:// and a config of v2 does not request v2' 
'
+   test_when_finished "rm -f log" &&
+   # Till v2 for push is designed, make sure that if a client has
+   # protocol.version configured to use v2, that the client instead falls
+   # back and uses v0.
+
+   test_commit -C http_child three &&
+
+   # Push to another branch, as the target repository has the
+   # master branch checked out and we cannot push into it.
+   GIT_TRACE_PACKET="$(pwd)/log" git -C http_child -c protocol.version=2 \
+   push origin HEAD:client_branch &&
+
+   git -C http_child log -1 --format=%s >actual &&
+   git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" log -1 --format=%s 
client_branch >expect &&
+   test_cmp expect actual &&
+
+   # Client didnt request to use protocol v2
+   ! grep "Git-Protocol: version=2" log &&
+   # Server didnt respond using protocol v2
+   ! grep "git< version 2" log
+'
+
+
 stop_httpd
 
 test_done
-- 
2.16.2.804.g6dcf76e118-goog

[PATCH v6 29/35] remote-curl: create copy of the service name

[Brandon Williams]

Make a copy of the service name being requested instead of relying on
the buffer pointed to by the passed in 'const char *' to remain
unchanged.

Currently, all service names are string constants, but a subsequent
patch will introduce service names from external sources.

Signed-off-by: Brandon Williams 
---
 remote-curl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index dae8a4a48d..4086aa733b 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -165,7 +165,7 @@ static int set_option(const char *name, const char *value)
 }
 
 struct discovery {
-   const char *service;
+   char *service;
char *buf_alloc;
char *buf;
size_t len;
@@ -257,6 +257,7 @@ static void free_discovery(struct discovery *d)
free(d->shallow.oid);
free(d->buf_alloc);
free_refs(d->refs);
+   free(d->service);
free(d);
}
 }
@@ -343,7 +344,7 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
warning(_("redirecting to %s"), url.buf);
 
last= xcalloc(1, sizeof(*last_discovery));
-   last->service = service;
+   last->service = xstrdup(service);
last->buf_alloc = strbuf_detach(, >len);
last->buf = last->buf_alloc;
 
-- 
2.16.2.804.g6dcf76e118-goog

[PATCH v6 30/35] remote-curl: store the protocol version the server responded with

[Brandon Williams]

Store the protocol version the server responded with when performing
discovery.  This will be used in a future patch to either change the
'Git-Protocol' header sent in subsequent requests or to determine if a
client needs to fallback to using a different protocol version.

Signed-off-by: Brandon Williams 
---
 remote-curl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 4086aa733b..c540358438 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -171,6 +171,7 @@ struct discovery {
size_t len;
struct ref *refs;
struct oid_array shallow;
+   enum protocol_version version;
unsigned proto_git : 1;
 };
 static struct discovery *last_discovery;
@@ -184,7 +185,8 @@ static struct ref *parse_git_refs(struct discovery *heads, 
int for_push)
   PACKET_READ_CHOMP_NEWLINE |
   PACKET_READ_GENTLE_ON_EOF);
 
-   switch (discover_version()) {
+   heads->version = discover_version();
+   switch (heads->version) {
case protocol_v2:
die("support for protocol v2 not implemented yet");
break;
-- 
2.16.2.804.g6dcf76e118-goog

[PATCH v5 30/35] remote-curl: store the protocol version the server responded with

[Brandon Williams]

Store the protocol version the server responded with when performing
discovery.  This will be used in a future patch to either change the
'Git-Protocol' header sent in subsequent requests or to determine if a
client needs to fallback to using a different protocol version.

Signed-off-by: Brandon Williams 
---
 remote-curl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 4086aa733b..c540358438 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -171,6 +171,7 @@ struct discovery {
size_t len;
struct ref *refs;
struct oid_array shallow;
+   enum protocol_version version;
unsigned proto_git : 1;
 };
 static struct discovery *last_discovery;
@@ -184,7 +185,8 @@ static struct ref *parse_git_refs(struct discovery *heads, 
int for_push)
   PACKET_READ_CHOMP_NEWLINE |
   PACKET_READ_GENTLE_ON_EOF);
 
-   switch (discover_version()) {
+   heads->version = discover_version();
+   switch (heads->version) {
case protocol_v2:
die("support for protocol v2 not implemented yet");
break;
-- 
2.16.2.804.g6dcf76e118-goog

[PATCH v5 34/35] remote-curl: implement stateless-connect command

[Brandon Williams]

Teach remote-curl the 'stateless-connect' command which is used to
establish a stateless connection with servers which support protocol
version 2.  This allows remote-curl to act as a proxy, allowing the git
client to communicate natively with a remote end, simply using
remote-curl as a pass through to convert requests to http.

Signed-off-by: Brandon Williams <bmw...@google.com>
---
 remote-curl.c  | 207 -
 t/t5702-protocol-v2.sh |  45 +
 2 files changed, 251 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 66a53f74bb..87f5b77b29 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -188,7 +188,12 @@ static struct ref *parse_git_refs(struct discovery *heads, 
int for_push)
heads->version = discover_version();
switch (heads->version) {
case protocol_v2:
-   die("support for protocol v2 not implemented yet");
+   /*
+* Do nothing.  This isn't a list of refs but rather a
+* capability advertisement.  Client would have run
+* 'stateless-connect' so we'll dump this capability listing
+* and let them request the refs themselves.
+*/
break;
case protocol_v1:
case protocol_v0:
@@ -1085,6 +1090,202 @@ static void parse_push(struct strbuf *buf)
free(specs);
 }
 
+/*
+ * Used to represent the state of a connection to an HTTP server when
+ * communicating using git's wire-protocol version 2.
+ */
+struct proxy_state {
+   char *service_name;
+   char *service_url;
+   struct curl_slist *headers;
+   struct strbuf request_buffer;
+   int in;
+   int out;
+   struct packet_reader reader;
+   size_t pos;
+   int seen_flush;
+};
+
+static void proxy_state_init(struct proxy_state *p, const char *service_name,
+enum protocol_version version)
+{
+   struct strbuf buf = STRBUF_INIT;
+
+   memset(p, 0, sizeof(*p));
+   p->service_name = xstrdup(service_name);
+
+   p->in = 0;
+   p->out = 1;
+   strbuf_init(>request_buffer, 0);
+
+   strbuf_addf(, "%s%s", url.buf, p->service_name);
+   p->service_url = strbuf_detach(, NULL);
+
+   p->headers = http_copy_default_headers();
+
+   strbuf_addf(, "Content-Type: application/x-%s-request", 
p->service_name);
+   p->headers = curl_slist_append(p->headers, buf.buf);
+   strbuf_reset();
+
+   strbuf_addf(, "Accept: application/x-%s-result", p->service_name);
+   p->headers = curl_slist_append(p->headers, buf.buf);
+   strbuf_reset();
+
+   p->headers = curl_slist_append(p->headers, "Transfer-Encoding: 
chunked");
+
+   /* Add the Git-Protocol header */
+   if (get_protocol_http_header(version, ))
+   p->headers = curl_slist_append(p->headers, buf.buf);
+
+   packet_reader_init(>reader, p->in, NULL, 0,
+  PACKET_READ_GENTLE_ON_EOF);
+
+   strbuf_release();
+}
+
+static void proxy_state_clear(struct proxy_state *p)
+{
+   free(p->service_name);
+   free(p->service_url);
+   curl_slist_free_all(p->headers);
+   strbuf_release(>request_buffer);
+}
+
+/*
+ * CURLOPT_READFUNCTION callback function.
+ * Attempts to copy over a single packet-line at a time into the
+ * curl provided buffer.
+ */
+static size_t proxy_in(char *buffer, size_t eltsize,
+  size_t nmemb, void *userdata)
+{
+   size_t max;
+   struct proxy_state *p = userdata;
+   size_t avail = p->request_buffer.len - p->pos;
+
+
+   if (eltsize != 1)
+   BUG("curl read callback called with size = %"PRIuMAX" != 1",
+   (uintmax_t)eltsize);
+   max = nmemb;
+
+   if (!avail) {
+   if (p->seen_flush) {
+   p->seen_flush = 0;
+   return 0;
+   }
+
+   strbuf_reset(>request_buffer);
+   switch (packet_reader_read(>reader)) {
+   case PACKET_READ_EOF:
+   die("unexpected EOF when reading from parent process");
+   case PACKET_READ_NORMAL:
+   packet_buf_write_len(>request_buffer, p->reader.line,
+p->reader.pktlen);
+   break;
+   case PACKET_READ_DELIM:
+   packet_buf_delim(>request_buffer);
+   break;
+   case PACKET_READ_FLUSH:
+   packet_buf_flush(>request_buffer);
+   p->seen_flush = 1;
+   break;
+   }
+   p->pos = 0;
+   avail = p->request_buffer.len;
+ 

[PATCH v5 35/35] remote-curl: don't request v2 when pushing

[Brandon Williams]

In order to be able to ship protocol v2 with only supporting fetch, we
need clients to not issue a request to use protocol v2 when pushing
(since the client currently doesn't know how to push using protocol v2).
This allows a client to have protocol v2 configured in
`protocol.version` and take advantage of using v2 for fetch and falling
back to using v0 when pushing while v2 for push is being designed.

We could run into issues if we didn't fall back to protocol v2 when
pushing right now.  This is because currently a server will ignore a request to
use v2 when contacting the 'receive-pack' endpoint and fall back to
using v0, but when push v2 is rolled out to servers, the 'receive-pack'
endpoint will start responding using v2.  So we don't want to get into a
state where a client is requesting to push with v2 before they actually
know how to push using v2.

Signed-off-by: Brandon Williams 
---
 remote-curl.c  | 11 ++-
 t/t5702-protocol-v2.sh | 24 
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 87f5b77b29..595447b16e 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -322,6 +322,7 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
struct discovery *last = last_discovery;
int http_ret, maybe_smart = 0;
struct http_get_options http_options;
+   enum protocol_version version = get_protocol_version_config();
 
if (last && !strcmp(service, last->service))
return last;
@@ -338,8 +339,16 @@ static struct discovery *discover_refs(const char 
*service, int for_push)
strbuf_addf(_url, "service=%s", service);
}
 
+   /*
+* NEEDSWORK: If we are trying to use protocol v2 and we are planning
+* to perform a push, then fallback to v0 since the client doesn't know
+* how to push yet using v2.
+*/
+   if (version == protocol_v2 && !strcmp("git-receive-pack", service))
+   version = protocol_v0;
+
/* Add the extra Git-Protocol header */
-   if (get_protocol_http_header(get_protocol_version_config(), 
_header))
+   if (get_protocol_http_header(version, _header))
string_list_append(_headers, protocol_header.buf);
 
memset(_options, 0, sizeof(http_options));
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 124063c2c4..56f7c3c326 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -244,6 +244,30 @@ test_expect_success 'fetch with http:// using protocol v2' 
'
grep "git< version 2" log
 '
 
+test_expect_success 'push with http:// and a config of v2 does not request v2' 
'
+   test_when_finished "rm -f log" &&
+   # Till v2 for push is designed, make sure that if a client has
+   # protocol.version configured to use v2, that the client instead falls
+   # back and uses v0.
+
+   test_commit -C http_child three &&
+
+   # Push to another branch, as the target repository has the
+   # master branch checked out and we cannot push into it.
+   GIT_TRACE_PACKET="$(pwd)/log" git -C http_child -c protocol.version=2 \
+   push origin HEAD:client_branch &&
+
+   git -C http_child log -1 --format=%s >actual &&
+   git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" log -1 --format=%s 
client_branch >expect &&
+   test_cmp expect actual &&
+
+   # Client didnt request to use protocol v2
+   ! grep "Git-Protocol: version=2" log &&
+   # Server didnt respond using protocol v2
+   ! grep "git< version 2" log
+'
+
+
 stop_httpd
 
 test_done
-- 
2.16.2.804.g6dcf76e118-goog

[PATCH v5 29/35] remote-curl: create copy of the service name

[Brandon Williams]

Make a copy of the service name being requested instead of relying on
the buffer pointed to by the passed in 'const char *' to remain
unchanged.

Currently, all service names are string constants, but a subsequent
patch will introduce service names from external sources.

Signed-off-by: Brandon Williams 
---
 remote-curl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index dae8a4a48d..4086aa733b 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -165,7 +165,7 @@ static int set_option(const char *name, const char *value)
 }
 
 struct discovery {
-   const char *service;
+   char *service;
char *buf_alloc;
char *buf;
size_t len;
@@ -257,6 +257,7 @@ static void free_discovery(struct discovery *d)
free(d->shallow.oid);
free(d->buf_alloc);
free_refs(d->refs);
+   free(d->service);
free(d);
}
 }
@@ -343,7 +344,7 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
warning(_("redirecting to %s"), url.buf);
 
last= xcalloc(1, sizeof(*last_discovery));
-   last->service = service;
+   last->service = xstrdup(service);
last->buf_alloc = strbuf_detach(, >len);
last->buf = last->buf_alloc;
 
-- 
2.16.2.804.g6dcf76e118-goog

Re: [PATCH v4 35/35] remote-curl: don't request v2 when pushing

[Jonathan Tan]

On Wed, 28 Feb 2018 15:22:52 -0800
Brandon Williams  wrote:

> In order to be able to ship protocol v2 with only supporting fetch, we
> need clients to not issue a request to use protocol v2 when pushing
> (since the client currently doesn't know how to push using protocol v2).
> This allows a client to have protocol v2 configured in
> `protocol.version` and take advantage of using v2 for fetch and falling
> back to using v0 when pushing while v2 for push is being designed.
> 
> We could run into issues if we didn't fall back to protocol v2 when
> pushing right now.  This is because currently a server will ignore a request 
> to
> use v2 when contacting the 'receive-pack' endpoint and fall back to
> using v0, but when push v2 is rolled out to servers, the 'receive-pack'
> endpoint will start responding using v2.  So we don't want to get into a
> state where a client is requesting to push with v2 before they actually
> know how to push using v2.
> 
> Signed-off-by: Brandon Williams 

I noticed that my review comments have been addressed, so:

Reviewed-by: Jonathan Tan 

Re: [PATCH v4 29/35] remote-curl: create copy of the service name

[Jonathan Tan]

On Wed, 28 Feb 2018 15:22:46 -0800
Brandon Williams  wrote:

> Make a copy of the service name being requested instead of relying on
> the buffer pointed to by the passed in 'const char *' to remain
> unchanged.
> 
> Currently, all service names are string constants, but a subsequent
> patch will introduce service names from external sources.
> 
> Signed-off-by: Brandon Williams 

Once again,

Reviewed-by: Jonathan Tan 

Re: [PATCH v4 34/35] remote-curl: implement stateless-connect command

[Brandon Williams]

On 03/02, Johannes Schindelin wrote:
> Hi Brandon,
> 
> On Wed, 28 Feb 2018, Brandon Williams wrote:
> 
> > diff --git a/remote-curl.c b/remote-curl.c
> > index 66a53f74b..3f882d766 100644
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> > @@ -188,7 +188,12 @@ static struct ref *parse_git_refs(struct discovery 
> > *heads, int for_push)
> > [...]
> > +static size_t proxy_in(char *buffer, size_t eltsize,
> > +  size_t nmemb, void *userdata)
> > +{
> > +   size_t max;
> > +   struct proxy_state *p = userdata;
> > +   size_t avail = p->request_buffer.len - p->pos;
> > +
> > +
> > +   if (eltsize != 1)
> > +   BUG("curl read callback called with size = %zu != 1", eltsize);
> 
> The format specified %z is not actually portable. Please use PRIuMAX and
> cast to (uintmax_t) instead.
> 
> This breaks the Windows build of `pu` (before that, there was still a test
> failure that I did not have the time to chase down).

Oh sorry, Looks like Junio put a patch ontop in pu to fix this.  I'll
squash that fix into this patch.

Thanks for catching this.

> 
> Ciao,
> Dscho

-- 
Brandon Williams

Re: [PATCH v4 34/35] remote-curl: implement stateless-connect command

[Johannes Schindelin]

Hi Brandon,

On Wed, 28 Feb 2018, Brandon Williams wrote:

> diff --git a/remote-curl.c b/remote-curl.c
> index 66a53f74b..3f882d766 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -188,7 +188,12 @@ static struct ref *parse_git_refs(struct discovery 
> *heads, int for_push)
> [...]
> +static size_t proxy_in(char *buffer, size_t eltsize,
> +size_t nmemb, void *userdata)
> +{
> + size_t max;
> + struct proxy_state *p = userdata;
> + size_t avail = p->request_buffer.len - p->pos;
> +
> +
> + if (eltsize != 1)
> + BUG("curl read callback called with size = %zu != 1", eltsize);

The format specified %z is not actually portable. Please use PRIuMAX and
cast to (uintmax_t) instead.

This breaks the Windows build of `pu` (before that, there was still a test
failure that I did not have the time to chase down).

Ciao,
Dscho

[PATCH v4 30/35] remote-curl: store the protocol version the server responded with

[Brandon Williams]

Store the protocol version the server responded with when performing
discovery.  This will be used in a future patch to either change the
'Git-Protocol' header sent in subsequent requests or to determine if a
client needs to fallback to using a different protocol version.

Signed-off-by: Brandon Williams 
---
 remote-curl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 4086aa733..c54035843 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -171,6 +171,7 @@ struct discovery {
size_t len;
struct ref *refs;
struct oid_array shallow;
+   enum protocol_version version;
unsigned proto_git : 1;
 };
 static struct discovery *last_discovery;
@@ -184,7 +185,8 @@ static struct ref *parse_git_refs(struct discovery *heads, 
int for_push)
   PACKET_READ_CHOMP_NEWLINE |
   PACKET_READ_GENTLE_ON_EOF);
 
-   switch (discover_version()) {
+   heads->version = discover_version();
+   switch (heads->version) {
case protocol_v2:
die("support for protocol v2 not implemented yet");
break;
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH v4 29/35] remote-curl: create copy of the service name

[Brandon Williams]

Make a copy of the service name being requested instead of relying on
the buffer pointed to by the passed in 'const char *' to remain
unchanged.

Currently, all service names are string constants, but a subsequent
patch will introduce service names from external sources.

Signed-off-by: Brandon Williams 
---
 remote-curl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index dae8a4a48..4086aa733 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -165,7 +165,7 @@ static int set_option(const char *name, const char *value)
 }
 
 struct discovery {
-   const char *service;
+   char *service;
char *buf_alloc;
char *buf;
size_t len;
@@ -257,6 +257,7 @@ static void free_discovery(struct discovery *d)
free(d->shallow.oid);
free(d->buf_alloc);
free_refs(d->refs);
+   free(d->service);
free(d);
}
 }
@@ -343,7 +344,7 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
warning(_("redirecting to %s"), url.buf);
 
last= xcalloc(1, sizeof(*last_discovery));
-   last->service = service;
+   last->service = xstrdup(service);
last->buf_alloc = strbuf_detach(, >len);
last->buf = last->buf_alloc;
 
-- 
2.16.2.395.g2e18187dfd-goog

[PATCH v4 34/35] remote-curl: implement stateless-connect command

[Brandon Williams]

Teach remote-curl the 'stateless-connect' command which is used to
establish a stateless connection with servers which support protocol
version 2.  This allows remote-curl to act as a proxy, allowing the git
client to communicate natively with a remote end, simply using
remote-curl as a pass through to convert requests to http.

Signed-off-by: Brandon Williams <bmw...@google.com>
---
 remote-curl.c  | 205 -
 t/t5702-protocol-v2.sh |  45 +
 2 files changed, 249 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 66a53f74b..3f882d766 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -188,7 +188,12 @@ static struct ref *parse_git_refs(struct discovery *heads, 
int for_push)
heads->version = discover_version();
switch (heads->version) {
case protocol_v2:
-   die("support for protocol v2 not implemented yet");
+   /*
+* Do nothing.  This isn't a list of refs but rather a
+* capability advertisement.  Client would have run
+* 'stateless-connect' so we'll dump this capability listing
+* and let them request the refs themselves.
+*/
break;
case protocol_v1:
case protocol_v0:
@@ -1085,6 +1090,200 @@ static void parse_push(struct strbuf *buf)
free(specs);
 }
 
+/*
+ * Used to represent the state of a connection to an HTTP server when
+ * communicating using git's wire-protocol version 2.
+ */
+struct proxy_state {
+   char *service_name;
+   char *service_url;
+   struct curl_slist *headers;
+   struct strbuf request_buffer;
+   int in;
+   int out;
+   struct packet_reader reader;
+   size_t pos;
+   int seen_flush;
+};
+
+static void proxy_state_init(struct proxy_state *p, const char *service_name,
+enum protocol_version version)
+{
+   struct strbuf buf = STRBUF_INIT;
+
+   memset(p, 0, sizeof(*p));
+   p->service_name = xstrdup(service_name);
+
+   p->in = 0;
+   p->out = 1;
+   strbuf_init(>request_buffer, 0);
+
+   strbuf_addf(, "%s%s", url.buf, p->service_name);
+   p->service_url = strbuf_detach(, NULL);
+
+   p->headers = http_copy_default_headers();
+
+   strbuf_addf(, "Content-Type: application/x-%s-request", 
p->service_name);
+   p->headers = curl_slist_append(p->headers, buf.buf);
+   strbuf_reset();
+
+   strbuf_addf(, "Accept: application/x-%s-result", p->service_name);
+   p->headers = curl_slist_append(p->headers, buf.buf);
+   strbuf_reset();
+
+   p->headers = curl_slist_append(p->headers, "Transfer-Encoding: 
chunked");
+
+   /* Add the Git-Protocol header */
+   if (get_protocol_http_header(version, ))
+   p->headers = curl_slist_append(p->headers, buf.buf);
+
+   packet_reader_init(>reader, p->in, NULL, 0,
+  PACKET_READ_GENTLE_ON_EOF);
+
+   strbuf_release();
+}
+
+static void proxy_state_clear(struct proxy_state *p)
+{
+   free(p->service_name);
+   free(p->service_url);
+   curl_slist_free_all(p->headers);
+   strbuf_release(>request_buffer);
+}
+
+/*
+ * CURLOPT_READFUNCTION callback function.
+ * Attempts to copy over a single packet-line at a time into the
+ * curl provided buffer.
+ */
+static size_t proxy_in(char *buffer, size_t eltsize,
+  size_t nmemb, void *userdata)
+{
+   size_t max;
+   struct proxy_state *p = userdata;
+   size_t avail = p->request_buffer.len - p->pos;
+
+
+   if (eltsize != 1)
+   BUG("curl read callback called with size = %zu != 1", eltsize);
+   max = nmemb;
+
+   if (!avail) {
+   if (p->seen_flush) {
+   p->seen_flush = 0;
+   return 0;
+   }
+
+   strbuf_reset(>request_buffer);
+   switch (packet_reader_read(>reader)) {
+   case PACKET_READ_EOF:
+   die("unexpected EOF when reading from parent process");
+   case PACKET_READ_NORMAL:
+   packet_buf_write_len(>request_buffer, p->reader.line,
+p->reader.pktlen);
+   break;
+   case PACKET_READ_DELIM:
+   packet_buf_delim(>request_buffer);
+   break;
+   case PACKET_READ_FLUSH:
+   packet_buf_flush(>request_buffer);
+   p->seen_flush = 1;
+   break;
+   }
+   p->pos = 0;
+   avail = p->request_buffer.len;
+   }
+
+   if (max < avail)
+

[PATCH v4 35/35] remote-curl: don't request v2 when pushing

[Brandon Williams]

In order to be able to ship protocol v2 with only supporting fetch, we
need clients to not issue a request to use protocol v2 when pushing
(since the client currently doesn't know how to push using protocol v2).
This allows a client to have protocol v2 configured in
`protocol.version` and take advantage of using v2 for fetch and falling
back to using v0 when pushing while v2 for push is being designed.

We could run into issues if we didn't fall back to protocol v2 when
pushing right now.  This is because currently a server will ignore a request to
use v2 when contacting the 'receive-pack' endpoint and fall back to
using v0, but when push v2 is rolled out to servers, the 'receive-pack'
endpoint will start responding using v2.  So we don't want to get into a
state where a client is requesting to push with v2 before they actually
know how to push using v2.

Signed-off-by: Brandon Williams 
---
 remote-curl.c  | 11 ++-
 t/t5702-protocol-v2.sh | 24 
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 3f882d766..379ab9b21 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -322,6 +322,7 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
struct discovery *last = last_discovery;
int http_ret, maybe_smart = 0;
struct http_get_options http_options;
+   enum protocol_version version = get_protocol_version_config();
 
if (last && !strcmp(service, last->service))
return last;
@@ -338,8 +339,16 @@ static struct discovery *discover_refs(const char 
*service, int for_push)
strbuf_addf(_url, "service=%s", service);
}
 
+   /*
+* NEEDSWORK: If we are trying to use protocol v2 and we are planning
+* to perform a push, then fallback to v0 since the client doesn't know
+* how to push yet using v2.
+*/
+   if (version == protocol_v2 && !strcmp("git-receive-pack", service))
+   version = protocol_v0;
+
/* Add the extra Git-Protocol header */
-   if (get_protocol_http_header(get_protocol_version_config(), 
_header))
+   if (get_protocol_http_header(version, _header))
string_list_append(_headers, protocol_header.buf);
 
memset(_options, 0, sizeof(http_options));
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 124063c2c..56f7c3c32 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -244,6 +244,30 @@ test_expect_success 'fetch with http:// using protocol v2' 
'
grep "git< version 2" log
 '
 
+test_expect_success 'push with http:// and a config of v2 does not request v2' 
'
+   test_when_finished "rm -f log" &&
+   # Till v2 for push is designed, make sure that if a client has
+   # protocol.version configured to use v2, that the client instead falls
+   # back and uses v0.
+
+   test_commit -C http_child three &&
+
+   # Push to another branch, as the target repository has the
+   # master branch checked out and we cannot push into it.
+   GIT_TRACE_PACKET="$(pwd)/log" git -C http_child -c protocol.version=2 \
+   push origin HEAD:client_branch &&
+
+   git -C http_child log -1 --format=%s >actual &&
+   git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" log -1 --format=%s 
client_branch >expect &&
+   test_cmp expect actual &&
+
+   # Client didnt request to use protocol v2
+   ! grep "Git-Protocol: version=2" log &&
+   # Server didnt respond using protocol v2
+   ! grep "git< version 2" log
+'
+
+
 stop_httpd
 
 test_done
-- 
2.16.2.395.g2e18187dfd-goog

Re: [PATCH v3 34/35] remote-curl: implement stateless-connect command

[Brandon Williams]

On 02/27, Jonathan Nieder wrote:
> Hi,
> 
> Brandon Williams wrote:
> 
> > Teach remote-curl the 'stateless-connect' command which is used to
> > establish a stateless connection with servers which support protocol
> > version 2.  This allows remote-curl to act as a proxy, allowing the git
> > client to communicate natively with a remote end, simply using
> > remote-curl as a pass through to convert requests to http.
> 
> Cool!  I better look at the spec for that first.
> 
> *looks at the previous patch*
> 
> Oh, there is no documented spec. :/  I'll muddle through this instead,
> then.

I'll make sure to add one :)

> 
> [...]
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> > @@ -188,7 +188,10 @@ static struct ref *parse_git_refs(struct discovery 
> > *heads, int for_push)
> > heads->version = discover_version();
> > switch (heads->version) {
> > case protocol_v2:
> > -   die("support for protocol v2 not implemented yet");
> > +   /*
> > +* Do nothing.  Client should run 'stateless-connect' and
> > +* request the refs themselves.
> > +*/
> >     break;
> 
> This is the 'list' command, right?  Since we expect the client to run
> 'stateless-connect' instead, can we make it error out?

Yes and no.  Remote-curl will run this when trying to establish a
stateless-connection.  If the response is v2 then this is a capability
list and not refs.  So the capabilities will be dumped to the client and
they will be able to request the refs themselves at a later point.  The
comment here is just misleading, so i'll make sure to fix it.

> 
> [...]
> > @@ -1082,6 +1085,184 @@ static void parse_push(struct strbuf *buf)
> > free(specs);
> >  }
> >  
> > +struct proxy_state {
> > +   char *service_name;
> > +   char *service_url;
> > +   struct curl_slist *headers;
> > +   struct strbuf request_buffer;
> > +   int in;
> > +   int out;
> > +   struct packet_reader reader;
> > +   size_t pos;
> > +   int seen_flush;
> > +};
> 
> Can this have a comment describing what it is/does?  It's not obvious
> to me at first glance.
> 
> It doesn't have to go in a lot of detail since this is an internal
> implementation detail, but something saying e.g. that this represents
> a connection to an HTTP server (that's an example; I'm not saying
> that's what it represents :)) would help.

Always making new code have higher standards than the existing code ;)
Haha, I'll add a simple comment explaining it.

> 
> > +
> > +static void proxy_state_init(struct proxy_state *p, const char 
> > *service_name,
> > +enum protocol_version version)
> [...]
> > +static void proxy_state_clear(struct proxy_state *p)
> 
> Looks sensible.
> 
> [...]
> > +static size_t proxy_in(char *buffer, size_t eltsize,
> > +  size_t nmemb, void *userdata)
> 
> Can this have a comment describing the interface?  (E.g. does it read
> a single pkt_line?  How is the caller expected to use it?  Does it
> satisfy the interface of some callback?)

I'll add a comment that its used as a READFUNCTION callback for curl and
that it tries to copy over a packet-line at a time.

> 
> libcurl's example https://curl.haxx.se/libcurl/c/ftpupload.html just
> calls this read_callback.  Such a name plus a pointer to
> CURLOPT_READFUNCTION should do the trick; bonus points if the comment 
> says what our implementation of the callback does.
> 
> Is this about having peek ability?

No its just that Curl only requests a set about of data at a time so you
need to be able to buffer the data that can't be read yet.

> 
> > +   struct proxy_state *p = userdata;
> > +   size_t avail = p->request_buffer.len - p->pos;
> > +
> > +   if (!avail) {
> > +   if (p->seen_flush) {
> > +   p->seen_flush = 0;
> > +   return 0;
> > +   }
> > +
> > +   strbuf_reset(>request_buffer);
> > +   switch (packet_reader_read(>reader)) {
> > +   case PACKET_READ_EOF:
> > +   die("unexpected EOF when reading from parent process");
> > +   case PACKET_READ_NORMAL:
> > +   packet_buf_write_len(>request_buffer, p->reader.line,
> > +p->reader.pktlen);
> > +   break;
> > +   case PACKET_READ_DELIM:
> > +   packet_buf_delim(>request_buffer);
> > +   br

Re: [PATCH v3 34/35] remote-curl: implement stateless-connect command

[Jonathan Nieder]

Hi,

Brandon Williams wrote:

> Teach remote-curl the 'stateless-connect' command which is used to
> establish a stateless connection with servers which support protocol
> version 2.  This allows remote-curl to act as a proxy, allowing the git
> client to communicate natively with a remote end, simply using
> remote-curl as a pass through to convert requests to http.

Cool!  I better look at the spec for that first.

*looks at the previous patch*

Oh, there is no documented spec. :/  I'll muddle through this instead,
then.

[...]
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -188,7 +188,10 @@ static struct ref *parse_git_refs(struct discovery 
> *heads, int for_push)
>   heads->version = discover_version();
>   switch (heads->version) {
>   case protocol_v2:
> - die("support for protocol v2 not implemented yet");
> + /*
> +  * Do nothing.  Client should run 'stateless-connect' and
> +  * request the refs themselves.
> +  */
>   break;

This is the 'list' command, right?  Since we expect the client to run
'stateless-connect' instead, can we make it error out?

[...]
> @@ -1082,6 +1085,184 @@ static void parse_push(struct strbuf *buf)
>   free(specs);
>  }
>  
> +struct proxy_state {
> + char *service_name;
> + char *service_url;
> + struct curl_slist *headers;
> + struct strbuf request_buffer;
> + int in;
> + int out;
> + struct packet_reader reader;
> + size_t pos;
> + int seen_flush;
> +};

Can this have a comment describing what it is/does?  It's not obvious
to me at first glance.

It doesn't have to go in a lot of detail since this is an internal
implementation detail, but something saying e.g. that this represents
a connection to an HTTP server (that's an example; I'm not saying
that's what it represents :)) would help.

> +
> +static void proxy_state_init(struct proxy_state *p, const char *service_name,
> +  enum protocol_version version)
[...]
> +static void proxy_state_clear(struct proxy_state *p)

Looks sensible.

[...]
> +static size_t proxy_in(char *buffer, size_t eltsize,
> +size_t nmemb, void *userdata)

Can this have a comment describing the interface?  (E.g. does it read
a single pkt_line?  How is the caller expected to use it?  Does it
satisfy the interface of some callback?)

libcurl's example https://curl.haxx.se/libcurl/c/ftpupload.html just
calls this read_callback.  Such a name plus a pointer to
CURLOPT_READFUNCTION should do the trick; bonus points if the comment 
says what our implementation of the callback does.

Is this about having peek ability?

> +{
> + size_t max = eltsize * nmemb;

Can this overflow?  st_mult can avoid having to worry about that.

> + struct proxy_state *p = userdata;
> + size_t avail = p->request_buffer.len - p->pos;
> +
> + if (!avail) {
> + if (p->seen_flush) {
> + p->seen_flush = 0;
> + return 0;
> + }
> +
> + strbuf_reset(>request_buffer);
> + switch (packet_reader_read(>reader)) {
> + case PACKET_READ_EOF:
> + die("unexpected EOF when reading from parent process");
> + case PACKET_READ_NORMAL:
> + packet_buf_write_len(>request_buffer, p->reader.line,
> +  p->reader.pktlen);
> + break;
> + case PACKET_READ_DELIM:
> + packet_buf_delim(>request_buffer);
> + break;
> + case PACKET_READ_FLUSH:
> + packet_buf_flush(>request_buffer);
> + p->seen_flush = 1;
> + break;
> + }
> + p->pos = 0;
> + avail = p->request_buffer.len;
> + }
> +
> + if (max < avail)
> + avail = max;
> + memcpy(buffer, p->request_buffer.buf + p->pos, avail);
> + p->pos += avail;
> + return avail;

This is a number of bytes, but CURLOPT_READFUNCTION expects a number
of items, fread-style.  That is:

if (avail < eltsize)
... handle somehow, maybe by reading in more? ...

avail_memb = avail / eltsize;
memcpy(buffer,
   p->request_buffer.buf + p->pos,
   st_mult(avail_memb, eltsize));
p->pos += st_mult(avail_memb, eltsize);
return avail_memb;

But https://curl.haxx.se/libcurl/c/CURLOPT_READFUNCTION.html says

Your function must then return the actual number of bytes that
it stored in that memory ar

Re: [PATCH v3 31/35] remote-curl: store the protocol version the server responded with

[Jonathan Nieder]

Brandon Williams wrote:

> Store the protocol version the server responded with when performing
> discovery.  This will be used in a future patch to either change the
> 'Git-Protocol' header sent in subsequent requests or to determine if a
> client needs to fallback to using a different protocol version.

nit: s/fallback/fall back/ (fallback is the noun/adjective, fall back
the verb)

> Signed-off-by: Brandon Williams 

With or without that tweak,
Reviewed-by: Jonathan Nieder 

Thanks.

Re: [PATCH v3 35/35] remote-curl: don't request v2 when pushing

[Brandon Williams]

On 02/22, Brandon Williams wrote:
> On 02/21, Jonathan Tan wrote:
> > On Tue,  6 Feb 2018 17:13:12 -0800
> > Brandon Williams  wrote:
> > 
> > > +test_expect_success 'push with http:// and a config of v2 does not 
> > > request v2' '
> > > + # Till v2 for push is designed, make sure that if a client has
> > > + # protocol.version configured to use v2, that the client instead falls
> > > + # back and uses v0.
> > > +
> > > + test_commit -C http_child three &&
> > > +
> > > + # Push to another branch, as the target repository has the
> > > + # master branch checked out and we cannot push into it.
> > > + GIT_TRACE_PACKET=1 git -C http_child -c protocol.version=1 \
> > > + push origin HEAD:client_branch && 2>log &&
> > 
> > Should it be protocol.version=2? Also, two double ampersands?
> > 
> > Also, optionally, it might be better to do
> > GIT_TRACE_PACKET="$(pwd)/log", so that it does not get mixed with other
> > stderr output.
> 
> Wow thanks for catching that, let me fix that.

I like setting the log via "$(pwd)/log" but it turns out that this
appends to the file if it already exists, which means the previous tests
need to do some cleanup.  This is actually probably preferable anyway.

> 
> -- 
> Brandon Williams

-- 
Brandon Williams

Re: [PATCH v3 35/35] remote-curl: don't request v2 when pushing

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:13:12 -0800
> Brandon Williams  wrote:
> 
> > +test_expect_success 'push with http:// and a config of v2 does not request 
> > v2' '
> > +   # Till v2 for push is designed, make sure that if a client has
> > +   # protocol.version configured to use v2, that the client instead falls
> > +   # back and uses v0.
> > +
> > +   test_commit -C http_child three &&
> > +
> > +   # Push to another branch, as the target repository has the
> > +   # master branch checked out and we cannot push into it.
> > +   GIT_TRACE_PACKET=1 git -C http_child -c protocol.version=1 \
> > +   push origin HEAD:client_branch && 2>log &&
> 
> Should it be protocol.version=2? Also, two double ampersands?
> 
> Also, optionally, it might be better to do
> GIT_TRACE_PACKET="$(pwd)/log", so that it does not get mixed with other
> stderr output.

Wow thanks for catching that, let me fix that.

-- 
Brandon Williams

Re: [PATCH v3 30/35] remote-curl: create copy of the service name

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:13:07 -0800
> Brandon Williams  wrote:
> 
> > Make a copy of the service name being requested instead of relying on
> > the buffer pointed to by the passed in 'const char *' to remain
> > unchanged.
> > 
> > Signed-off-by: Brandon Williams 
> 
> Probably worth mentioning in the commit message:
> 
>   Currently, all service names are string constants, but a subsequent
>   patch will introduce service names from external sources.
> 
> Other than that,
> 
> Reviewed-by: Jonathan Tan 

I'll add that.

-- 
Brandon Williams

Re: [PATCH v3 35/35] remote-curl: don't request v2 when pushing

[Jonathan Tan]

On Tue,  6 Feb 2018 17:13:12 -0800
Brandon Williams  wrote:

> +test_expect_success 'push with http:// and a config of v2 does not request 
> v2' '
> + # Till v2 for push is designed, make sure that if a client has
> + # protocol.version configured to use v2, that the client instead falls
> + # back and uses v0.
> +
> + test_commit -C http_child three &&
> +
> + # Push to another branch, as the target repository has the
> + # master branch checked out and we cannot push into it.
> + GIT_TRACE_PACKET=1 git -C http_child -c protocol.version=1 \
> + push origin HEAD:client_branch && 2>log &&

Should it be protocol.version=2? Also, two double ampersands?

Also, optionally, it might be better to do
GIT_TRACE_PACKET="$(pwd)/log", so that it does not get mixed with other
stderr output.

Re: [PATCH v3 30/35] remote-curl: create copy of the service name

[Jonathan Tan]

On Tue,  6 Feb 2018 17:13:07 -0800
Brandon Williams  wrote:

> Make a copy of the service name being requested instead of relying on
> the buffer pointed to by the passed in 'const char *' to remain
> unchanged.
> 
> Signed-off-by: Brandon Williams 

Probably worth mentioning in the commit message:

  Currently, all service names are string constants, but a subsequent
  patch will introduce service names from external sources.

Other than that,

Reviewed-by: Jonathan Tan 

Re: [PATCH 2/2] remote-curl: unquote incoming push-options

[Brandon Williams]

On 02/19, Jeff King wrote:
> The transport-helper protocol c-style quotes the value of
> any options passed to the helper via the "option  "
> directive. However, remote-curl doesn't actually unquote the
> push-option values, meaning that we will send the quoted
> version to the other side (whereas git-over-ssh would send
> the raw value).
> 
> The pack-protocol.txt documentation defines the push-options
> as a series of VCHARs, which excludes most characters that
> would need quoting. But:
> 
>   1. You can still see the bug with a valid push-option that
>  starts with a double-quote (since that triggers
>  quoting).
> 
>   2. We do currently handle any non-NUL characters correctly
>  in git-over-ssh. So even though the spec does not say
>  that we need to handle most quoted characters, it's
>  nice if our behavior is consistent between protocols.
> 
> There are two new tests: the "direct" one shows that this
> already works in the non-http case, and the http one covers
> this bugfix.

This seems like a fairly obvious fix.  If the value is quoted, unquote
it and send the unquoted value as a push-option, otherwise just send the
already unquoted value as a push-option.

Thanks for finding and fixing this :)

> 
> Reported-by: Jon Simons <j...@jonsimons.org>
> Signed-off-by: Jeff King <p...@peff.net>
> ---
>  remote-curl.c   | 11 ++-
>  t/t5545-push-options.sh | 18 ++
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/remote-curl.c b/remote-curl.c
> index 6ec5352435..f5b3d22e26 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -13,6 +13,7 @@
>  #include "credential.h"
>  #include "sha1-array.h"
>  #include "send-pack.h"
> +#include "quote.h"
>  
>  static struct remote *remote;
>  /* always ends with a trailing slash */
> @@ -145,7 +146,15 @@ static int set_option(const char *name, const char 
> *value)
>   return -1;
>   return 0;
>   } else if (!strcmp(name, "push-option")) {
> - string_list_append(_options, value);
> + if (*value != '"')
> + string_list_append(_options, value);
> + else {
> + struct strbuf unquoted = STRBUF_INIT;
> + if (unquote_c_style(, value, NULL) < 0)
> + die("invalid quoting in push-option value");
> + string_list_append_nodup(_options,
> +  strbuf_detach(, 
> NULL));
> + }
>   return 0;
>  
>  #if LIBCURL_VERSION_NUM >= 0x070a08
> diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
> index c64dee2127..b47a95871c 100755
> --- a/t/t5545-push-options.sh
> +++ b/t/t5545-push-options.sh
> @@ -217,6 +217,15 @@ test_expect_success 'invalid push option in config' '
>   test_refs master HEAD@{1}
>  '
>  
> +test_expect_success 'push options keep quoted characters intact (direct)' '
> + mk_repo_pair &&
> + git -C upstream config receive.advertisePushOptions true &&
> + test_commit -C workbench one &&
> + git -C workbench push --push-option="\"embedded quotes\"" up master &&
> + echo "\"embedded quotes\"" >expect &&
> + test_cmp expect upstream/.git/hooks/pre-receive.push_options
> +'
> +
>  . "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd
>  
> @@ -260,6 +269,15 @@ test_expect_success 'push options work properly across 
> http' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'push options keep quoted characters intact (http)' '
> + mk_http_pair true &&
> +
> + test_commit -C test_http_clone one &&
> + git -C test_http_clone push --push-option="\"embedded quotes\"" origin 
> master &&
> + echo "\"embedded quotes\"" >expect &&
> + test_cmp expect 
> "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/pre-receive.push_options
> +'
> +
>  stop_httpd
>  
>  test_done
> -- 
> 2.16.2.552.gea2a3cf654

-- 
Brandon Williams

[PATCH 2/2] remote-curl: unquote incoming push-options

[Jeff King]

The transport-helper protocol c-style quotes the value of
any options passed to the helper via the "option  "
directive. However, remote-curl doesn't actually unquote the
push-option values, meaning that we will send the quoted
version to the other side (whereas git-over-ssh would send
the raw value).

The pack-protocol.txt documentation defines the push-options
as a series of VCHARs, which excludes most characters that
would need quoting. But:

  1. You can still see the bug with a valid push-option that
 starts with a double-quote (since that triggers
 quoting).

  2. We do currently handle any non-NUL characters correctly
 in git-over-ssh. So even though the spec does not say
 that we need to handle most quoted characters, it's
 nice if our behavior is consistent between protocols.

There are two new tests: the "direct" one shows that this
already works in the non-http case, and the http one covers
this bugfix.

Reported-by: Jon Simons <j...@jonsimons.org>
Signed-off-by: Jeff King <p...@peff.net>
---
 remote-curl.c   | 11 ++-
 t/t5545-push-options.sh | 18 ++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 6ec5352435..f5b3d22e26 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -13,6 +13,7 @@
 #include "credential.h"
 #include "sha1-array.h"
 #include "send-pack.h"
+#include "quote.h"
 
 static struct remote *remote;
 /* always ends with a trailing slash */
@@ -145,7 +146,15 @@ static int set_option(const char *name, const char *value)
return -1;
return 0;
} else if (!strcmp(name, "push-option")) {
-   string_list_append(_options, value);
+   if (*value != '"')
+   string_list_append(_options, value);
+   else {
+   struct strbuf unquoted = STRBUF_INIT;
+   if (unquote_c_style(, value, NULL) < 0)
+   die("invalid quoting in push-option value");
+   string_list_append_nodup(_options,
+strbuf_detach(, 
NULL));
+   }
return 0;
 
 #if LIBCURL_VERSION_NUM >= 0x070a08
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index c64dee2127..b47a95871c 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -217,6 +217,15 @@ test_expect_success 'invalid push option in config' '
test_refs master HEAD@{1}
 '
 
+test_expect_success 'push options keep quoted characters intact (direct)' '
+   mk_repo_pair &&
+   git -C upstream config receive.advertisePushOptions true &&
+   test_commit -C workbench one &&
+   git -C workbench push --push-option="\"embedded quotes\"" up master &&
+   echo "\"embedded quotes\"" >expect &&
+   test_cmp expect upstream/.git/hooks/pre-receive.push_options
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
@@ -260,6 +269,15 @@ test_expect_success 'push options work properly across 
http' '
test_cmp expect actual
 '
 
+test_expect_success 'push options keep quoted characters intact (http)' '
+   mk_http_pair true &&
+
+   test_commit -C test_http_clone one &&
+   git -C test_http_clone push --push-option="\"embedded quotes\"" origin 
master &&
+   echo "\"embedded quotes\"" >expect &&
+   test_cmp expect 
"$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/pre-receive.push_options
+'
+
 stop_httpd
 
 test_done
-- 
2.16.2.552.gea2a3cf654

[PATCH v3 30/35] remote-curl: create copy of the service name

[Brandon Williams]

Make a copy of the service name being requested instead of relying on
the buffer pointed to by the passed in 'const char *' to remain
unchanged.

Signed-off-by: Brandon Williams 
---
 remote-curl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index dae8a4a48..4086aa733 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -165,7 +165,7 @@ static int set_option(const char *name, const char *value)
 }
 
 struct discovery {
-   const char *service;
+   char *service;
char *buf_alloc;
char *buf;
size_t len;
@@ -257,6 +257,7 @@ static void free_discovery(struct discovery *d)
free(d->shallow.oid);
free(d->buf_alloc);
free_refs(d->refs);
+   free(d->service);
free(d);
}
 }
@@ -343,7 +344,7 @@ static struct discovery *discover_refs(const char *service, 
int for_push)
warning(_("redirecting to %s"), url.buf);
 
last= xcalloc(1, sizeof(*last_discovery));
-   last->service = service;
+   last->service = xstrdup(service);
last->buf_alloc = strbuf_detach(, >len);
last->buf = last->buf_alloc;
 
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH v3 31/35] remote-curl: store the protocol version the server responded with

[Brandon Williams]

Store the protocol version the server responded with when performing
discovery.  This will be used in a future patch to either change the
'Git-Protocol' header sent in subsequent requests or to determine if a
client needs to fallback to using a different protocol version.

Signed-off-by: Brandon Williams 
---
 remote-curl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 4086aa733..c54035843 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -171,6 +171,7 @@ struct discovery {
size_t len;
struct ref *refs;
struct oid_array shallow;
+   enum protocol_version version;
unsigned proto_git : 1;
 };
 static struct discovery *last_discovery;
@@ -184,7 +185,8 @@ static struct ref *parse_git_refs(struct discovery *heads, 
int for_push)
   PACKET_READ_CHOMP_NEWLINE |
   PACKET_READ_GENTLE_ON_EOF);
 
-   switch (discover_version()) {
+   heads->version = discover_version();
+   switch (heads->version) {
case protocol_v2:
die("support for protocol v2 not implemented yet");
break;
-- 
2.16.0.rc1.238.g530d649a79-goog