Re: [PATCH 1/1] fetch: Cache the want OIDs for faster lookup
On Sun, Sep 15, 2019 at 7:35 PM Derrick Stolee wrote:
>
> On 9/15/2019 5:18 PM, Masaya Suzuki wrote:
> > During git-fetch, the client checks if the advertised tags' OIDs are
> > already in the fetch request's want OID set. This check is done in a
> > linear scan. For a repository that has a lot of refs, repeating this
> > scan takes 15+ minutes. In order to speed this up, create a oid_set for
> > other refs' OIDs.
>
> Good catch! Quadratic performance is never good.
>
> The patch below looks like it works, but could you also share your
> performance timings for the 15+ minute case after your patch is
> applied?
With the following code change, I measured the time for
find_non_local_tags. It shows 215 msec with the example commands. (I
didn't measure entire fetch time as good portion of the time is spent
on the server side.)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 51a276dfaa..d3b06c733d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -25,6 +25,7 @@
#include "list-objects-filter-options.h"
#include "commit-reach.h"
#include "branch.h"
+#include
#define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
@@ -322,8 +323,11 @@ static void find_non_local_tags(const struct ref *refs,
const struct ref *ref;
struct refname_hash_entry *item = NULL;
+ struct timespec start, end;
+
refname_hash_init(&existing_refs);
refname_hash_init(&remote_refs);
+ clock_gettime(CLOCK_MONOTONIC, &start);
create_fetch_oidset(head, &fetch_oids);
for_each_ref(add_one_refname, &existing_refs);
@@ -405,6 +409,12 @@ static void find_non_local_tags(const struct ref *refs,
}
hashmap_free(&remote_refs, 1);
string_list_clear(&remote_refs_list, 0);
+ clock_gettime(CLOCK_MONOTONIC, &end);
+ {
+ uint64_t millisec = (end.tv_sec - start.tv_sec) * 1000
+ (end.tv_nsec - start.tv_nsec) / 100;
+ fprintf(stderr, "find_non_local_tags: %ld msec\n", millisec);
+ }
+
oidset_clear(&fetch_oids);
}
Re: [PATCH] credential: add nocache option to the credentials API
On Mon, Aug 26, 2019 at 9:28 AM Junio C Hamano wrote: > > Jeff King writes: > > > I was thinking that Git itself could treat "ttl=0" specially, the same > > as your nocache, and avoid passing it along to any helpers during the > > approve stage. That would make it exactly equivalent to your patch > > (modulo the name change). > > ... > > And as you noted above, if we don't suppress the helper calls inside > > Git, then every matching storage helper needs to learn about "nocache" > > (or "ttl") before it will do any good. > > I was waiting for this discussion to settle and then the discussion > seems to have petered out. Any interest to following the "ttl with > special casing value 0 as 'nocache'" idea thru from either two of > you, or should I take the patch as is in the meantime? Sorry for the late reply. I think about this again. I imagine that, if I would like to have credentials with an expiration and I want to have them managed by other helpers, it's probably better to use an absolute timestamp instead of duration. The second call to the helpers is done after the remote call. For the helpers that store TTL-ed credentials, they cannot tell the start time of the TTL in the second call. This makes it hard to cache the short-lived credentials safely because some time has spent during the remote call and the actual TTL is shorter than ttl=N option. From this, instead of adding ttl=DURATION, it might be better to have expires_at=TIMESTAMP. Maybe my observation is not an issue. I don't know. For now, adding nocache seems a safer action for me, so I vote for taking nocache patch as-is in the meantime. If there's somebody who wants to receive TTL or expiration timestamp, they can decide what's actually needed later.
[PATCH 1/1] fetch: Cache the want OIDs for faster lookup
During git-fetch, the client checks if the advertised tags' OIDs are
already in the fetch request's want OID set. This check is done in a
linear scan. For a repository that has a lot of refs, repeating this
scan takes 15+ minutes. In order to speed this up, create a oid_set for
other refs' OIDs.
Signed-off-by: Masaya Suzuki
---
builtin/fetch.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 54d6b01892..51a276dfaa 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -7,6 +7,7 @@
#include "refs.h"
#include "refspec.h"
#include "object-store.h"
+#include "oidset.h"
#include "commit.h"
#include "builtin.h"
#include "string-list.h"
@@ -243,15 +244,13 @@ static void add_merge_config(struct ref **head,
}
}
-static int will_fetch(struct ref **head, const unsigned char *sha1)
+static void create_fetch_oidset(struct ref **head, struct oidset *out)
{
struct ref *rm = *head;
while (rm) {
- if (hasheq(rm->old_oid.hash, sha1))
- return 1;
+ oidset_insert(out, &rm->old_oid);
rm = rm->next;
}
- return 0;
}
struct refname_hash_entry {
@@ -317,6 +316,7 @@ static void find_non_local_tags(const struct ref *refs,
{
struct hashmap existing_refs;
struct hashmap remote_refs;
+ struct oidset fetch_oids = OIDSET_INIT;
struct string_list remote_refs_list = STRING_LIST_INIT_NODUP;
struct string_list_item *remote_ref_item;
const struct ref *ref;
@@ -324,6 +324,7 @@ static void find_non_local_tags(const struct ref *refs,
refname_hash_init(&existing_refs);
refname_hash_init(&remote_refs);
+ create_fetch_oidset(head, &fetch_oids);
for_each_ref(add_one_refname, &existing_refs);
for (ref = refs; ref; ref = ref->next) {
@@ -340,9 +341,9 @@ static void find_non_local_tags(const struct ref *refs,
if (item &&
!has_object_file_with_flags(&ref->old_oid,
OBJECT_INFO_QUICK) &&
- !will_fetch(head, ref->old_oid.hash) &&
+ !oidset_contains(&fetch_oids, &ref->old_oid) &&
!has_object_file_with_flags(&item->oid,
OBJECT_INFO_QUICK) &&
- !will_fetch(head, item->oid.hash))
+ !oidset_contains(&fetch_oids, &item->oid))
clear_item(item);
item = NULL;
continue;
@@ -356,7 +357,7 @@ static void find_non_local_tags(const struct ref *refs,
*/
if (item &&
!has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK)
&&
- !will_fetch(head, item->oid.hash))
+ !oidset_contains(&fetch_oids, &item->oid))
clear_item(item);
item = NULL;
@@ -377,7 +378,7 @@ static void find_non_local_tags(const struct ref *refs,
*/
if (item &&
!has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&
- !will_fetch(head, item->oid.hash))
+ !oidset_contains(&fetch_oids, &item->oid))
clear_item(item);
/*
@@ -404,6 +405,7 @@ static void find_non_local_tags(const struct ref *refs,
}
hashmap_free(&remote_refs, 1);
string_list_clear(&remote_refs_list, 0);
+ oidset_clear(&fetch_oids);
}
static struct ref *get_ref_map(struct remote *remote,
--
2.23.0.237.gc6a4ce50a0-goog
[PATCH 0/1] fetch: Cache the want OIDs for faster lookup
When mirroring a repository with a lot of refs, there is a case where the client takes a long time to calculate the want OIDs. This is observable with Chromium's repository for example. $ mkdir testing $ cd testing $ git init . --bare $ git remote add origin master https://chromium.googlesource.com/chromium/src --mirror=fetch $ git fetch origin With the commands above, it takes a long time before sending a fetch request. I stopped the command after 15 minutes. Debugging this, it seems most of the time is spent on iterating the want refs to see OIDs are included there. This patch speeds up this process by using oid_set. Now the client can send a fetch request almost immediately. Masaya Suzuki (1): fetch: Cache the want OIDs for faster lookup builtin/fetch.c | 18 ++ 1 file changed, 10 insertions(+), 8 deletions(-) -- 2.23.0.237.gc6a4ce50a0-goog
Re: [PATCH] credential: add nocache option to the credentials API
On Tue, Jul 9, 2019 at 5:56 AM Jeff King wrote:
>
> On Sat, Jul 06, 2019 at 10:51:32PM -0700, Masaya Suzuki wrote:
>
> > The credentials API calls credentials helpers in order. If a
> > username/password pair is returned the helpers and if it's used for
> > authentication successfully, it's announced to the helpers and they can
> > store it for later use.
> >
> > Some credentials are valid only for the limited time and should not be
> > cached. In this case, because the credential is announced to all helpers
> > and they can independently decide whether they will cache it or not,
> > those short-lived credentials can be cached.
> >
> > This change adds an option that a credential helper can specify that the
> > credential returned by the helper should not be cached. If this is
> > specified, even after the credential is used successfully, it won't be
> > announced to other helpers for store.
>
> I think this makes sense to do, though note that there's an old
> discussion which covers some alternatives:
>
> https://public-inbox.org/git/20120407033417.ga13...@sigill.intra.peff.net/
>
> In that patch, I essentially proposed making all gathered credentials as
> nocache. That's a more secure default (though in some cases less
> convenient).
>
> It did break a case Shawn had of caching the result of another helper. I
> showed some options there for providing a mechanism to chain helpers
> together explicitly.
I think that it's better to make it nocache by default. Having one
helper produce a credential and having another cache it looks storage.
But since this is the current behavior, I'm OK with just keeping
nocache an option. It's backward compatible.
> We also discussed helpers passing out an explicit ttl. That's a more
> general case of your nocache flag (i.e., ttl=0 covers that case, but we
> could additionally pass "ttl" to the cache helper to let it be smarter).
TTL sounds like it's a generalized version. It might be a bit awkward
because the existing credential helpers that don't support TTL would
anyway cache the credentials. I think in practice the password saving
feature is mainly used by those password management software (like
git-credential-osxkeychain), and they wouldn't support a short-lived
credential. Just having nocache seems fine to me. As you said, if
needed, "ttl" can be added and "nocache" can be just a shorthand of
"ttl=0".
> Given the age of that discussion and the fact that nobody has really
> complained much in the interim, I'm OK to go with your much simpler
> approach. But I think it's worth at least thinking for a few minutes on
> whether there's anything to pull from that discussion. :)
>
> (As a side note, I've had all those patches on my "to revisit and send
> upstream" queue for 7 years; if we take yours, maybe I can finally let
> them go. ;) ).
>
> > Documentation/technical/api-credentials.txt | 4 +++-
> > credential.c| 4 +++-
> > credential.h| 3 ++-
> > t/t0300-credentials.sh | 9 +
> > 4 files changed, 17 insertions(+), 3 deletions(-)
>
> The patch itself looks good; two minor comments:
>
> > @@ -296,7 +298,7 @@ void credential_approve(struct credential *c)
> > {
> > int i;
> >
> > - if (c->approved)
> > + if (c->approved || c->no_cache)
> > return;
> > if (!c->username || !c->password)
> > return;
>
> Here we're disallowing a "nocache" credential from being passed to _any_
> helper, whether it's caching or not. It could be storing permanently,
> though perhaps that's semantic nitpicking (if it's not to be cached, it
> probably shouldn't be stored permanently either). Other helpers could in
> theory be doing something else with the data, though in practice I doubt
> here are any uses beyond debugging.
I cannot think of a usage either. If there's a good usage, I would
change this, but if it's for debugging, it's better to be done with
those debugging features (like GIT_TRACE_CURL). Note that this is
called only when the credential is successfully used. We probably want
to use such debugging feature for the credentials that are not
successfully used.
[PATCH] credential: add nocache option to the credentials API
The credentials API calls credentials helpers in order. If a
username/password pair is returned the helpers and if it's used for
authentication successfully, it's announced to the helpers and they can
store it for later use.
Some credentials are valid only for the limited time and should not be
cached. In this case, because the credential is announced to all helpers
and they can independently decide whether they will cache it or not,
those short-lived credentials can be cached.
This change adds an option that a credential helper can specify that the
credential returned by the helper should not be cached. If this is
specified, even after the credential is used successfully, it won't be
announced to other helpers for store.
Signed-off-by: Masaya Suzuki
---
Documentation/technical/api-credentials.txt | 4 +++-
credential.c| 4 +++-
credential.h| 3 ++-
t/t0300-credentials.sh | 9 +
4 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/Documentation/technical/api-credentials.txt
b/Documentation/technical/api-credentials.txt
index 75368f26ca..3db5841b40 100644
--- a/Documentation/technical/api-credentials.txt
+++ b/Documentation/technical/api-credentials.txt
@@ -251,7 +251,9 @@ even no values at all if it has nothing useful to provide.
Any provided
attributes will overwrite those already known about by Git. If a helper
outputs a `quit` attribute with a value of `true` or `1`, no further
helpers will be consulted, nor will the user be prompted (if no
-credential has been provided, the operation will then fail).
+credential has been provided, the operation will then fail). If a helper
outputs
+a `nocache` attribute with a value of `true` or `1`, `credential_approve` will
+not be called even after the credential is used for authentication sucessfully.
For a `store` or `erase` operation, the helper's output is ignored.
If it fails to perform the requested operation, it may complain to
diff --git a/credential.c b/credential.c
index 62be651b03..db7b351447 100644
--- a/credential.c
+++ b/credential.c
@@ -179,6 +179,8 @@ int credential_read(struct credential *c, FILE *fp)
credential_from_url(c, value);
} else if (!strcmp(key, "quit")) {
c->quit = !!git_config_bool("quit", value);
+ } else if (!strcmp(key, "nocache")) {
+ c->no_cache= !!git_config_bool("nocache", value);
}
/*
* Ignore other lines; we don't know what they mean, but
@@ -296,7 +298,7 @@ void credential_approve(struct credential *c)
{
int i;
- if (c->approved)
+ if (c->approved || c->no_cache)
return;
if (!c->username || !c->password)
return;
diff --git a/credential.h b/credential.h
index 6b0cd16be2..be0f35d841 100644
--- a/credential.h
+++ b/credential.h
@@ -8,7 +8,8 @@ struct credential {
unsigned approved:1,
configured:1,
quit:1,
-use_http_path:1;
+use_http_path:1,
+no_cache:1;
char *username;
char *password;
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 82eaaea0f4..ad06f6fe11 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -118,6 +118,15 @@ test_expect_success 'do not bother storing password-less
credential' '
EOF
'
+test_expect_success 'credential_approve does not call helpers for nocache' '
+ check approve useless <<-\EOF
+ username=foo
+ password=bar
+ nocache=1
+ --
+ --
+ EOF
+'
test_expect_success 'credential_reject calls all helpers' '
check reject useless "verbatim one two" <<-\EOF
--
2.22.0.410.gd8fdbe21b5-goog
[PATCH v4 5/5] test: test GIT_CURL_VERBOSE=1 shows an error
This tests GIT_CURL_VERBOSE shows an error when an URL returns 500. This
exercises the code in remote_curl.
Signed-off-by: Masaya Suzuki
---
t/lib-httpd/apache.conf | 1 +
t/t5581-http-curl-verbose.sh | 28
2 files changed, 29 insertions(+)
create mode 100755 t/t5581-http-curl-verbose.sh
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8f..cc4b87507e 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -115,6 +115,7 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
+ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
ScriptAlias /error/ error.sh/
diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh
new file mode 100755
index 00..cd9283eeec
--- /dev/null
+++ b/t/t5581-http-curl-verbose.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+test_description='test GIT_CURL_VERBOSE'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'setup repository' '
+ mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" --bare init &&
+ git config push.default matching &&
+ echo content >file &&
+ git add file &&
+ git commit -m one &&
+ git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ git push public master:master
+'
+
+test_expect_success 'failure in git-upload-pack is shown' '
+ test_might_fail env GIT_CURL_VERBOSE=1 \
+ git clone "$HTTPD_URL/error_git_upload_pack/smart/repo.git" \
+ 2>curl_log &&
+ grep "< HTTP/1.1 500 Intentional Breakage" curl_log
+'
+
+stop_httpd
+
+test_done
--
2.20.1.97.g81188d93c3-goog
[PATCH v4 3/5] remote-curl: define struct for CURLOPT_WRITEFUNCTION
In order to pass more values for rpc_in, define a struct and pass it as
an additional value.
Signed-off-by: Masaya Suzuki
---
remote-curl.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index d8eda2380a..d4673b6e8c 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -545,14 +545,22 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void
*clientp)
}
#endif
+struct rpc_in_data {
+ struct rpc_state *rpc;
+};
+
+/*
+ * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed
+ * from ptr.
+ */
static size_t rpc_in(char *ptr, size_t eltsize,
size_t nmemb, void *buffer_)
{
size_t size = eltsize * nmemb;
- struct rpc_state *rpc = buffer_;
+ struct rpc_in_data *data = buffer_;
if (size)
- rpc->any_written = 1;
- write_or_die(rpc->in, ptr, size);
+ data->rpc->any_written = 1;
+ write_or_die(data->rpc->in, ptr, size);
return size;
}
@@ -632,6 +640,7 @@ static int post_rpc(struct rpc_state *rpc)
size_t gzip_size = 0;
int err, large_request = 0;
int needs_100_continue = 0;
+ struct rpc_in_data rpc_in_data;
/* Try to load the entire request, if we can fit it into the
* allocated buffer space we can use HTTP/1.0 and avoid the
@@ -764,7 +773,8 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
- curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
+ rpc_in_data.rpc = rpc;
+ curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
rpc->any_written = 0;
--
2.20.1.97.g81188d93c3-goog
[PATCH v4 4/5] remote-curl: unset CURLOPT_FAILONERROR
By not setting CURLOPT_FAILONERROR, curl parses the HTTP response
headers even if the response is an error. This makes GIT_CURL_VERBOSE to
show the HTTP headers, which is useful for debugging.
Signed-off-by: Masaya Suzuki
---
remote-curl.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/remote-curl.c b/remote-curl.c
index d4673b6e8c..91b39ca098 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -547,6 +547,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void
*clientp)
struct rpc_in_data {
struct rpc_state *rpc;
+ struct active_request_slot *slot;
};
/*
@@ -558,6 +559,13 @@ static size_t rpc_in(char *ptr, size_t eltsize,
{
size_t size = eltsize * nmemb;
struct rpc_in_data *data = buffer_;
+ long response_code;
+
+ if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
+ &response_code) != CURLE_OK)
+ return size;
+ if (response_code >= 300)
+ return size;
if (size)
data->rpc->any_written = 1;
write_or_die(data->rpc->in, ptr, size);
@@ -774,7 +782,9 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
rpc_in_data.rpc = rpc;
+ rpc_in_data.slot = slot;
curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
+ curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
rpc->any_written = 0;
--
2.20.1.97.g81188d93c3-goog
[PATCH v4 1/5] http: support file handles for HTTP_KEEP_ERROR
HTTP_KEEP_ERROR makes it easy to debug HTTP transport errors. In order
to make HTTP_KEEP_ERROR enabled for all requests, file handles need to
be supported.
Signed-off-by: Masaya Suzuki
---
http.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/http.c b/http.c
index 0b6807cef9..4eccf4c5d8 100644
--- a/http.c
+++ b/http.c
@@ -1991,16 +1991,26 @@ static int http_request_reauth(const char *url,
/*
* If we are using KEEP_ERROR, the previous request may have
* put cruft into our output stream; we should clear it out before
-* making our next request. We only know how to do this for
-* the strbuf case, but that is enough to satisfy current callers.
+* making our next request.
*/
if (options && options->keep_error) {
switch (target) {
case HTTP_REQUEST_STRBUF:
strbuf_reset(result);
break;
+ case HTTP_REQUEST_FILE:
+ if (fflush(result)) {
+ error_errno("unable to flush a file");
+ return HTTP_START_FAILED;
+ }
+ rewind(result);
+ if (ftruncate(fileno(result), 0) < 0) {
+ error_errno("unable to truncate a file");
+ return HTTP_START_FAILED;
+ }
+ break;
default:
- BUG("HTTP_KEEP_ERROR is only supported with strbufs");
+ BUG("Unknown http_request target");
}
}
--
2.20.1.97.g81188d93c3-goog
[PATCH v4 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE
Diff from v3[1]: * Handle ftruncate and fflush return values * Call rewind to set the position back [1]: https://public-inbox.org/git/20190108024741.62176-1-masayasuz...@google.com/ Masaya Suzuki (5): http: support file handles for HTTP_KEEP_ERROR http: enable keep_error for HTTP requests remote-curl: define struct for CURLOPT_WRITEFUNCTION remote-curl: unset CURLOPT_FAILONERROR test: test GIT_CURL_VERBOSE=1 shows an error http.c | 32 +++- http.h | 1 - remote-curl.c| 29 - t/lib-httpd/apache.conf | 1 + t/t5581-http-curl-verbose.sh | 28 5 files changed, 72 insertions(+), 19 deletions(-) create mode 100755 t/t5581-http-curl-verbose.sh -- 2.20.1.97.g81188d93c3-goog
[PATCH v4 2/5] http: enable keep_error for HTTP requests
curl stops parsing a response when it sees a bad HTTP status code and it
has CURLOPT_FAILONERROR set. This prevents GIT_CURL_VERBOSE to show HTTP
headers on error.
keep_error is an option to receive the HTTP response body for those
error responses. By enabling this option, curl will process the HTTP
response headers, and they're shown if GIT_CURL_VERBOSE is set.
Signed-off-by: Masaya Suzuki
---
http.c| 42 +++---
http.h| 1 -
remote-curl.c | 1 -
3 files changed, 19 insertions(+), 25 deletions(-)
diff --git a/http.c b/http.c
index 4eccf4c5d8..954bebf684 100644
--- a/http.c
+++ b/http.c
@@ -1876,8 +1876,6 @@ static int http_request(const char *url,
strbuf_addstr(&buf, "Pragma:");
if (options && options->no_cache)
strbuf_addstr(&buf, " no-cache");
- if (options && options->keep_error)
- curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
if (options && options->initial_request &&
http_follow_config == HTTP_FOLLOW_INITIAL)
curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
@@ -1895,6 +1893,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, "");
+ curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
ret = run_one_slot(slot, &results);
@@ -1989,29 +1988,26 @@ static int http_request_reauth(const char *url,
return ret;
/*
-* If we are using KEEP_ERROR, the previous request may have
-* put cruft into our output stream; we should clear it out before
-* making our next request.
+* The previous request may have put cruft into our output stream; we
+* should clear it out before making our next request.
*/
- if (options && options->keep_error) {
- switch (target) {
- case HTTP_REQUEST_STRBUF:
- strbuf_reset(result);
- break;
- case HTTP_REQUEST_FILE:
- if (fflush(result)) {
- error_errno("unable to flush a file");
- return HTTP_START_FAILED;
- }
- rewind(result);
- if (ftruncate(fileno(result), 0) < 0) {
- error_errno("unable to truncate a file");
- return HTTP_START_FAILED;
- }
- break;
- default:
- BUG("Unknown http_request target");
+ switch (target) {
+ case HTTP_REQUEST_STRBUF:
+ strbuf_reset(result);
+ break;
+ case HTTP_REQUEST_FILE:
+ if (fflush(result)) {
+ error_errno("unable to flush a file");
+ return HTTP_START_FAILED;
}
+ rewind(result);
+ if (ftruncate(fileno(result), 0) < 0) {
+ error_errno("unable to truncate a file");
+ return HTTP_START_FAILED;
+ }
+ break;
+ default:
+ BUG("Unknown http_request target");
}
credential_fill(&http_auth);
diff --git a/http.h b/http.h
index d305ca1dc7..eebf40688c 100644
--- a/http.h
+++ b/http.h
@@ -146,7 +146,6 @@ extern char *get_remote_object_url(const char *url, const
char *hex,
/* Options for http_get_*() */
struct http_get_options {
unsigned no_cache:1,
-keep_error:1,
initial_request:1;
/* If non-NULL, returns the content-type of the response. */
diff --git a/remote-curl.c b/remote-curl.c
index 1220dffcdc..d8eda2380a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -380,7 +380,6 @@ static struct discovery *discover_refs(const char *service,
int for_push)
http_options.extra_headers = &extra_headers;
http_options.initial_request = 1;
http_options.no_cache = 1;
- http_options.keep_error = 1;
http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);
switch (http_ret) {
--
2.20.1.97.g81188d93c3-goog
Re: [PATCH v2 1/2] Use packet_reader instead of packet_read_line
On Mon, Jan 7, 2019 at 2:33 PM Josh Steadmon wrote:
>
> On 2018.12.29 13:19, Masaya Suzuki wrote:
> > By using and sharing a packet_reader while handling a Git pack protocol
> > request, the same reader option is used throughout the code. This makes
> > it easy to set a reader option to the request parsing code.
> >
> > Signed-off-by: Masaya Suzuki
> > ---
> > builtin/archive.c | 19 ++---
> > builtin/receive-pack.c | 60 +
> > fetch-pack.c | 61 +++---
> > remote-curl.c | 22 ++-
> > send-pack.c| 37 -
> > upload-pack.c | 38 +-
> > 6 files changed, 129 insertions(+), 108 deletions(-)
> >
> > diff --git a/builtin/archive.c b/builtin/archive.c
> > index d2455237c..2fe1f05ca 100644
> > --- a/builtin/archive.c
> > +++ b/builtin/archive.c
> > @@ -27,10 +27,10 @@ static int run_remote_archiver(int argc, const char
> > **argv,
> > const char *remote, const char *exec,
> > const char *name_hint)
> > {
> > - char *buf;
> > int fd[2], i, rv;
> > struct transport *transport;
> > struct remote *_remote;
> > + struct packet_reader reader;
> >
> > _remote = remote_get(remote);
> > if (!_remote->url[0])
> > @@ -53,18 +53,19 @@ static int run_remote_archiver(int argc, const char
> > **argv,
> > packet_write_fmt(fd[1], "argument %s\n", argv[i]);
> > packet_flush(fd[1]);
> >
> > - buf = packet_read_line(fd[0], NULL);
> > - if (!buf)
> > + packet_reader_init(&reader, fd[0], NULL, 0,
> > PACKET_READ_CHOMP_NEWLINE);
> > +
> > + if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
>
> packet_read_line() can also return NULL if the packet is zero-length, so
> you may want to add a "|| reader.pktlen <= 0" to the condition here (and
> in other places where we were checking that packet_read_line() != NULL)
> to make sure the behavior doesn't change. See discussion on my previous
> attempt[1] to refactor this in builtin/archive.c.
>
> [1]: https://public-inbox.org/git/20180912053519.31085-1-stead...@google.com/
That is interesting. In Documentation/technical/protocol-common.txt,
it says "Implementations SHOULD NOT send an empty pkt-line ("0004").".
The existing code won't distinguish "" and "0004", while "0004" is
actually not a valid pkt-line. I'll make this patch with no behavior
change, but I think we can make that behavior change to stop accepting
0004 as , and remove the pktlen checks.
[PATCH v3 5/5] test: test GIT_CURL_VERBOSE=1 shows an error
This tests GIT_CURL_VERBOSE shows an error when an URL returns 500. This
exercises the code in remote_curl.
Signed-off-by: Masaya Suzuki
---
t/lib-httpd/apache.conf | 1 +
t/t5581-http-curl-verbose.sh | 28
2 files changed, 29 insertions(+)
create mode 100755 t/t5581-http-curl-verbose.sh
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8f..cc4b87507e 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -115,6 +115,7 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
+ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
ScriptAlias /error/ error.sh/
diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh
new file mode 100755
index 00..cd9283eeec
--- /dev/null
+++ b/t/t5581-http-curl-verbose.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+test_description='test GIT_CURL_VERBOSE'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'setup repository' '
+ mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" --bare init &&
+ git config push.default matching &&
+ echo content >file &&
+ git add file &&
+ git commit -m one &&
+ git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ git push public master:master
+'
+
+test_expect_success 'failure in git-upload-pack is shown' '
+ test_might_fail env GIT_CURL_VERBOSE=1 \
+ git clone "$HTTPD_URL/error_git_upload_pack/smart/repo.git" \
+ 2>curl_log &&
+ grep "< HTTP/1.1 500 Intentional Breakage" curl_log
+'
+
+stop_httpd
+
+test_done
--
2.20.1.97.g81188d93c3-goog
[PATCH v3 2/5] http: enable keep_error for HTTP requests
curl stops parsing a response when it sees a bad HTTP status code and it
has CURLOPT_FAILONERROR set. This prevents GIT_CURL_VERBOSE to show HTTP
headers on error.
keep_error is an option to receive the HTTP response body for those
error responses. By enabling this option, curl will process the HTTP
response headers, and they're shown if GIT_CURL_VERBOSE is set.
Signed-off-by: Masaya Suzuki
---
http.c| 30 +-
http.h| 1 -
remote-curl.c | 1 -
3 files changed, 13 insertions(+), 19 deletions(-)
diff --git a/http.c b/http.c
index 06450da96e..a72db87723 100644
--- a/http.c
+++ b/http.c
@@ -1876,8 +1876,6 @@ static int http_request(const char *url,
strbuf_addstr(&buf, "Pragma:");
if (options && options->no_cache)
strbuf_addstr(&buf, " no-cache");
- if (options && options->keep_error)
- curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
if (options && options->initial_request &&
http_follow_config == HTTP_FOLLOW_INITIAL)
curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
@@ -1895,6 +1893,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, "");
+ curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
ret = run_one_slot(slot, &results);
@@ -1989,22 +1988,19 @@ static int http_request_reauth(const char *url,
return ret;
/*
-* If we are using KEEP_ERROR, the previous request may have
-* put cruft into our output stream; we should clear it out before
-* making our next request.
+* The previous request may have put cruft into our output stream; we
+* should clear it out before making our next request.
*/
- if (options && options->keep_error) {
- switch (target) {
- case HTTP_REQUEST_STRBUF:
- strbuf_reset(result);
- break;
- case HTTP_REQUEST_FILE:
- fflush(result);
- ftruncate(fileno(result), 0);
- break;
- default:
- BUG("Unknown http_request target");
- }
+ switch (target) {
+ case HTTP_REQUEST_STRBUF:
+ strbuf_reset(result);
+ break;
+ case HTTP_REQUEST_FILE:
+ fflush(result);
+ ftruncate(fileno(result), 0);
+ break;
+ default:
+ BUG("Unknown http_request target");
}
credential_fill(&http_auth);
diff --git a/http.h b/http.h
index d305ca1dc7..eebf40688c 100644
--- a/http.h
+++ b/http.h
@@ -146,7 +146,6 @@ extern char *get_remote_object_url(const char *url, const
char *hex,
/* Options for http_get_*() */
struct http_get_options {
unsigned no_cache:1,
-keep_error:1,
initial_request:1;
/* If non-NULL, returns the content-type of the response. */
diff --git a/remote-curl.c b/remote-curl.c
index 1220dffcdc..d8eda2380a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -380,7 +380,6 @@ static struct discovery *discover_refs(const char *service,
int for_push)
http_options.extra_headers = &extra_headers;
http_options.initial_request = 1;
http_options.no_cache = 1;
- http_options.keep_error = 1;
http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);
switch (http_ret) {
--
2.20.1.97.g81188d93c3-goog
[PATCH v3 4/5] remote-curl: unset CURLOPT_FAILONERROR
By not setting CURLOPT_FAILONERROR, curl parses the HTTP response
headers even if the response is an error. This makes GIT_CURL_VERBOSE to
show the HTTP headers, which is useful for debugging.
Signed-off-by: Masaya Suzuki
---
remote-curl.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/remote-curl.c b/remote-curl.c
index d4673b6e8c..91b39ca098 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -547,6 +547,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void
*clientp)
struct rpc_in_data {
struct rpc_state *rpc;
+ struct active_request_slot *slot;
};
/*
@@ -558,6 +559,13 @@ static size_t rpc_in(char *ptr, size_t eltsize,
{
size_t size = eltsize * nmemb;
struct rpc_in_data *data = buffer_;
+ long response_code;
+
+ if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
+ &response_code) != CURLE_OK)
+ return size;
+ if (response_code >= 300)
+ return size;
if (size)
data->rpc->any_written = 1;
write_or_die(data->rpc->in, ptr, size);
@@ -774,7 +782,9 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
rpc_in_data.rpc = rpc;
+ rpc_in_data.slot = slot;
curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
+ curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
rpc->any_written = 0;
--
2.20.1.97.g81188d93c3-goog
[PATCH v3 1/5] http: support file handles for HTTP_KEEP_ERROR
HTTP_KEEP_ERROR makes it easy to debug HTTP transport errors. In order
to make HTTP_KEEP_ERROR enabled for all requests, file handles need to
be supported.
Signed-off-by: Masaya Suzuki
---
http.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/http.c b/http.c
index 0b6807cef9..06450da96e 100644
--- a/http.c
+++ b/http.c
@@ -1991,16 +1991,19 @@ static int http_request_reauth(const char *url,
/*
* If we are using KEEP_ERROR, the previous request may have
* put cruft into our output stream; we should clear it out before
-* making our next request. We only know how to do this for
-* the strbuf case, but that is enough to satisfy current callers.
+* making our next request.
*/
if (options && options->keep_error) {
switch (target) {
case HTTP_REQUEST_STRBUF:
strbuf_reset(result);
break;
+ case HTTP_REQUEST_FILE:
+ fflush(result);
+ ftruncate(fileno(result), 0);
+ break;
default:
- BUG("HTTP_KEEP_ERROR is only supported with strbufs");
+ BUG("Unknown http_request target");
}
}
--
2.20.1.97.g81188d93c3-goog
[PATCH v3 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE
Diff from v2[1]: * Remove http_resonse_dest This was introduced to have a filename for freopen. Jeff King proposed [2] using fflush and ftruncate and this makes this struct not needed. * Unset CURLOPT_FAILONERROR only when it's necessary. Previously, CURLOPT_FAILONERROR was unset for everything. This patch series does so only when it's necessary. This is from the observation in [3] that pointed out there are other possible code paths that hit http.c. * Split the patches for easier review [1]: https://public-inbox.org/git/20181229194447.157763-1-masayasuz...@google.com/ [2]: https://public-inbox.org/git/20190104101149.ga26...@sigill.intra.peff.net/ [3]: https://public-inbox.org/git/20190104104907.gc26...@sigill.intra.peff.net/ Masaya Suzuki (5): http: support file handles for HTTP_KEEP_ERROR http: enable keep_error for HTTP requests remote-curl: define struct for CURLOPT_WRITEFUNCTION remote-curl: unset CURLOPT_FAILONERROR test: test GIT_CURL_VERBOSE=1 shows an error http.c | 27 +-- http.h | 1 - remote-curl.c| 29 - t/lib-httpd/apache.conf | 1 + t/t5581-http-curl-verbose.sh | 28 5 files changed, 66 insertions(+), 20 deletions(-) create mode 100755 t/t5581-http-curl-verbose.sh -- 2.20.1.97.g81188d93c3-goog
[PATCH v3 3/5] remote-curl: define struct for CURLOPT_WRITEFUNCTION
In order to pass more values for rpc_in, define a struct and pass it as
an additional value.
Signed-off-by: Masaya Suzuki
---
remote-curl.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index d8eda2380a..d4673b6e8c 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -545,14 +545,22 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void
*clientp)
}
#endif
+struct rpc_in_data {
+ struct rpc_state *rpc;
+};
+
+/*
+ * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed
+ * from ptr.
+ */
static size_t rpc_in(char *ptr, size_t eltsize,
size_t nmemb, void *buffer_)
{
size_t size = eltsize * nmemb;
- struct rpc_state *rpc = buffer_;
+ struct rpc_in_data *data = buffer_;
if (size)
- rpc->any_written = 1;
- write_or_die(rpc->in, ptr, size);
+ data->rpc->any_written = 1;
+ write_or_die(data->rpc->in, ptr, size);
return size;
}
@@ -632,6 +640,7 @@ static int post_rpc(struct rpc_state *rpc)
size_t gzip_size = 0;
int err, large_request = 0;
int needs_100_continue = 0;
+ struct rpc_in_data rpc_in_data;
/* Try to load the entire request, if we can fit it into the
* allocated buffer space we can use HTTP/1.0 and avoid the
@@ -764,7 +773,8 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
- curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
+ rpc_in_data.rpc = rpc;
+ curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
rpc->any_written = 0;
--
2.20.1.97.g81188d93c3-goog
Re: [PATCH v2 2/2] Unset CURLOPT_FAILONERROR
On Fri, Jan 4, 2019 at 2:49 AM Jeff King wrote: > > On Sat, Dec 29, 2018 at 11:44:47AM -0800, Masaya Suzuki wrote: > > > When GIT_CURL_VERBOSE is set, libcurl produces request/response headers > > to stderr. However, if the response is an error response and > > CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it > > won't dump the headers. Showing HTTP response headers is useful for > > debugging, especially for non-OK responses. > > Out of curiosity, does GIT_TRACE_CURL do any better? Or is it simply > that curl closes the handle when it sees the bad response code, and > nobody ever gets to see the rest of the data? curl disregards the rest of the contents when it sees a bad response code when CURLOPT_FAILONERROR is set (https://github.com/curl/curl/blob/dea3f94298ac0859464768959488938c4e104545/lib/http.c#L3691). So nobody gets the rest of the data. > > > This is substantially same as setting http_options.keep_error to all > > requests. Hence, remove this option. > > The assumption here is that every code path using FAILONERROR is > prepared to handle the failing http response codes itself (since we no > longer set it at all in get_active_slot()). Is that so? I was thinking that I covered the all code paths using FAILONERROR, but it seems it's not the case. http-walker.c and http-push.c also calls get_active_slot(). I'll narrow down the scope on removing FAILONERROR. > > Anything that uses handle_curl_result() is OK. That means run_one_slot() > is fine, which in turn covers run_slot() for RPCs, and http_request() > for normal one-at-a-time requests. But what about the parallel multiple > requests issued by the dumb-http walker code? Right. I'll keep FAILONERROR in get_active_slot and remove it only for the code paths that can handle HTTP errors. > > There I think we end up in step_active_slots(), which calls into > finish_active_slot() for completed requests. I think that > unconditionally fetches the http code without bothering to look at > whether curl reported success or not. > > So I _think_ that's probably all of the users of the curl handles > provided by get_active_slot(). Though given the tangled mess of our HTTP > code, I won't be surprised if there's a corner case I missed in that > analysis. > > -Peff
Re: [PATCH v2 0/2] Accept error packets in any context
On Thu, Jan 3, 2019 at 3:05 PM Junio C Hamano wrote: > > Masaya Suzuki writes: > > > This makes it possible for servers to send an error message back to clients > > in > > an arbitrary situation. > > > > The first patch was originally sent in [1]. This version includes some fix. > > > > The second patch was originally sent in [2]. Later, this was cherry-picked > > in > > [3]. In the discussion in [3], we agreed that this error packet handling > > should > > be done only against the Git pack protocol handling code. With this > > agreement, > > the patch series sent in [3] is abandoned (according to [4]). This is a > > patch > > series based on that agreement on limiting the error packet handling. > > In short, are you shooting js/smart-http-detect-remote-error topic > down and replacing it with this one? > > As that topic is not yet in 'next', I am perfectly fine doing that. > I just want to make sure that is what you meant, as my reading of > [4] was a bit fuzzy. Sorry, I think I referenced a wrong email. [4] was meant to be https://public-inbox.org/git/20181219233005.gi37...@google.com/. I think he wants to have https://public-inbox.org/git/20181116084725.ga31...@sigill.intra.peff.net/, https://public-inbox.org/git/20181116084838.gb31...@sigill.intra.peff.net/, and https://public-inbox.org/git/20181116084951.gc31...@sigill.intra.peff.net/ for js/smart-http-detect-remote-error.
[PATCH v2 0/2] Accept error packets in any context
This makes it possible for servers to send an error message back to clients in an arbitrary situation. The first patch was originally sent in [1]. This version includes some fix. The second patch was originally sent in [2]. Later, this was cherry-picked in [3]. In the discussion in [3], we agreed that this error packet handling should be done only against the Git pack protocol handling code. With this agreement, the patch series sent in [3] is abandoned (according to [4]). This is a patch series based on that agreement on limiting the error packet handling. [1]: https://public-inbox.org/git/20181227065210.60817-1-masayasuz...@google.com/ [2]: https://public-inbox.org/git/20181127045301.103807-1-masayasuz...@google.com/ [3]: https://public-inbox.org/git/df7d3659ae5f11d163f1e992f3b9403be709ddb7.1544572142.git.stead...@google.com/ [4]: https://public-inbox.org/git/20181213221826.ge37...@google.com/ Masaya Suzuki (2): Use packet_reader instead of packet_read_line pack-protocol.txt: accept error packets in any context Documentation/technical/pack-protocol.txt | 20 +++ builtin/archive.c | 19 +++ builtin/fetch-pack.c | 3 +- builtin/receive-pack.c| 62 +++-- builtin/send-pack.c | 3 +- connect.c | 3 -- fetch-pack.c | 65 +-- pkt-line.c| 4 ++ pkt-line.h| 8 ++- remote-curl.c | 29 ++ send-pack.c | 39 +++--- serve.c | 5 +- t/t5703-upload-pack-ref-in-want.sh| 4 +- transport.c | 3 +- upload-pack.c | 40 +++--- 15 files changed, 174 insertions(+), 133 deletions(-) -- 2.20.1.415.g653613c723-goog
[PATCH v2 2/2] pack-protocol.txt: accept error packets in any context
In the Git pack protocol definition, an error packet may appear only in
a certain context. However, servers can face a runtime error (e.g. I/O
error) at an arbitrary timing. This patch changes the protocol to allow
an error packet to be sent instead of any packet.
Without this protocol spec change, when a server cannot process a
request, there's no way to tell that to a client. Since the server
cannot produce a valid response, it would be forced to cut a connection
without telling why. With this protocol spec change, the server can be
more gentle in this situation. An old client may see these error packets
as an unexpected packet, but this is not worse than having an unexpected
EOF.
Following this protocol spec change, the error packet handling code is
moved to pkt-line.c. Implementation wise, this implementation uses
pkt-line to communicate with a subprocess. Since this is not a part of
Git protocol, it's possible that a packet that is not supposed to be an
error packet is mistakenly parsed as an error packet. This error packet
handling is enabled only for the Git pack protocol parsing code
considering this.
Signed-off-by: Masaya Suzuki
---
Documentation/technical/pack-protocol.txt | 20 +++-
builtin/archive.c | 6 +++---
builtin/fetch-pack.c | 3 ++-
builtin/receive-pack.c| 4 +++-
builtin/send-pack.c | 3 ++-
connect.c | 3 ---
fetch-pack.c | 8
pkt-line.c| 4
pkt-line.h| 8 ++--
remote-curl.c | 9 ++---
send-pack.c | 4 +++-
serve.c | 5 +++--
t/t5703-upload-pack-ref-in-want.sh| 4 ++--
transport.c | 3 ++-
upload-pack.c | 4 +++-
15 files changed, 54 insertions(+), 34 deletions(-)
diff --git a/Documentation/technical/pack-protocol.txt
b/Documentation/technical/pack-protocol.txt
index 6ac774d5f..7a2375a55 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -22,6 +22,16 @@ protocol-common.txt. When the grammar indicate
`PKT-LINE(...)`, unless
otherwise noted the usual pkt-line LF rules apply: the sender SHOULD
include a LF, but the receiver MUST NOT complain if it is not present.
+An error packet is a special pkt-line that contains an error string.
+
+
+ error-line = PKT-LINE("ERR" SP explanation-text)
+
+
+Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY
+be sent. Once this packet is sent by a client or a server, the data transfer
+process defined in this protocol is terminated.
+
Transports
--
There are three transports over which the packfile protocol is
@@ -89,13 +99,6 @@ process on the server side over the Git protocol is this:
"0039git-upload-pack /schacon/gitbook.git\0host=example.com\0" |
nc -v example.com 9418
-If the server refuses the request for some reasons, it could abort
-gracefully with an error message.
-
-
- error-line = PKT-LINE("ERR" SP explanation-text)
-
-
SSH Transport
-
@@ -398,12 +401,11 @@ from the client).
Then the server will start sending its packfile data.
- server-response = *ack_multi ack / nak / error-line
+ server-response = *ack_multi ack / nak
ack_multi = PKT-LINE("ACK" SP obj-id ack_status)
ack_status = "continue" / "common" / "ready"
ack = PKT-LINE("ACK" SP obj-id)
nak = PKT-LINE("NAK")
- error-line = PKT-LINE("ERR" SP explanation-text)
A simple clone may look like this (with no 'have' lines):
diff --git a/builtin/archive.c b/builtin/archive.c
index 2fe1f05ca..45d11669a 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -53,15 +53,15 @@ static int run_remote_archiver(int argc, const char **argv,
packet_write_fmt(fd[1], "argument %s\n", argv[i]);
packet_flush(fd[1]);
- packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);
+ packet_reader_init(&reader, fd[0], NULL, 0,
+ PACKET_READ_CHOMP_NEWLINE |
+ PACKET_READ_DIE_ON_ERR_PACKET);
if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
die(_("git archive: expected ACK/NAK, got a flush packet"));
if (strcmp(reader.line, "ACK")) {
if (starts_with(reader.line, "NACK "))
die(_("git archive: NACK %s"), reader.line + 5);
- if (starts_with(reader.line, "ERR "))
- die
[PATCH v2 1/2] Use packet_reader instead of packet_read_line
By using and sharing a packet_reader while handling a Git pack protocol
request, the same reader option is used throughout the code. This makes
it easy to set a reader option to the request parsing code.
Signed-off-by: Masaya Suzuki
---
builtin/archive.c | 19 ++---
builtin/receive-pack.c | 60 +
fetch-pack.c | 61 +++---
remote-curl.c | 22 ++-
send-pack.c| 37 -
upload-pack.c | 38 +-
6 files changed, 129 insertions(+), 108 deletions(-)
diff --git a/builtin/archive.c b/builtin/archive.c
index d2455237c..2fe1f05ca 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -27,10 +27,10 @@ static int run_remote_archiver(int argc, const char **argv,
const char *remote, const char *exec,
const char *name_hint)
{
- char *buf;
int fd[2], i, rv;
struct transport *transport;
struct remote *_remote;
+ struct packet_reader reader;
_remote = remote_get(remote);
if (!_remote->url[0])
@@ -53,18 +53,19 @@ static int run_remote_archiver(int argc, const char **argv,
packet_write_fmt(fd[1], "argument %s\n", argv[i]);
packet_flush(fd[1]);
- buf = packet_read_line(fd[0], NULL);
- if (!buf)
+ packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);
+
+ if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
die(_("git archive: expected ACK/NAK, got a flush packet"));
- if (strcmp(buf, "ACK")) {
- if (starts_with(buf, "NACK "))
- die(_("git archive: NACK %s"), buf + 5);
- if (starts_with(buf, "ERR "))
- die(_("remote error: %s"), buf + 4);
+ if (strcmp(reader.line, "ACK")) {
+ if (starts_with(reader.line, "NACK "))
+ die(_("git archive: NACK %s"), reader.line + 5);
+ if (starts_with(reader.line, "ERR "))
+ die(_("remote error: %s"), reader.line + 4);
die(_("git archive: protocol error"));
}
- if (packet_read_line(fd[0], NULL))
+ if (packet_reader_read(&reader) != PACKET_READ_FLUSH)
die(_("git archive: expected a flush"));
/* Now, start reading from fd[0] and spit it out to stdout */
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 33187bd8e..81cc07370 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1569,30 +1569,29 @@ static void queue_commands_from_cert(struct command
**tail,
}
}
-static struct command *read_head_info(struct oid_array *shallow)
+static struct command *read_head_info(struct packet_reader *reader,
+ struct oid_array *shallow)
{
struct command *commands = NULL;
struct command **p = &commands;
for (;;) {
- char *line;
- int len, linelen;
+ int linelen;
- line = packet_read_line(0, &len);
- if (!line)
+ if (packet_reader_read(reader) != PACKET_READ_NORMAL)
break;
- if (len > 8 && starts_with(line, "shallow ")) {
+ if (reader->pktlen > 8 && starts_with(reader->line, "shallow
")) {
struct object_id oid;
- if (get_oid_hex(line + 8, &oid))
+ if (get_oid_hex(reader->line + 8, &oid))
die("protocol error: expected shallow sha, got
'%s'",
- line + 8);
+ reader->line + 8);
oid_array_append(shallow, &oid);
continue;
}
- linelen = strlen(line);
- if (linelen < len) {
- const char *feature_list = line + linelen + 1;
+ linelen = strlen(reader->line);
+ if (linelen < reader->pktlen) {
+ const char *feature_list = reader->line + linelen + 1;
if (parse_feature_request(feature_list,
"report-status"))
report_status = 1;
if (parse_feature_request(feature_list,
"side-band-64k"))
@@ -1607,28 +1606,32 @@ static struct command *read_head_info(struct oid_array
*shallow)
use_push_options = 1;
}
- if (!strcmp(line, "pu
Re: [PATCH] Use packet_reader instead of packet_read_line
Sorry. This is really broken. I'll fix the tests and send another version.
[PATCH v2 2/2] Unset CURLOPT_FAILONERROR
When GIT_CURL_VERBOSE is set, libcurl produces request/response headers
to stderr. However, if the response is an error response and
CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it
won't dump the headers. Showing HTTP response headers is useful for
debugging, especially for non-OK responses.
This is substantially same as setting http_options.keep_error to all
requests. Hence, remove this option.
Signed-off-by: Masaya Suzuki
---
http.c | 4
http.h | 1 -
remote-curl.c| 1 -
t/lib-httpd/apache.conf | 1 +
t/t5581-http-curl-verbose.sh | 28
5 files changed, 29 insertions(+), 6 deletions(-)
create mode 100755 t/t5581-http-curl-verbose.sh
diff --git a/http.c b/http.c
index d23417670..8f8101da3 100644
--- a/http.c
+++ b/http.c
@@ -1269,7 +1269,6 @@ struct active_request_slot *get_active_slot(void)
curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);
curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
- curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);
/*
@@ -1848,8 +1847,6 @@ static int http_request(const char *url,
strbuf_addstr(&buf, "Pragma:");
if (options && options->no_cache)
strbuf_addstr(&buf, " no-cache");
- if (options && options->keep_error)
- curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
if (options && options->initial_request &&
http_follow_config == HTTP_FOLLOW_INITIAL)
curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
@@ -2415,7 +2412,6 @@ struct http_object_request *new_http_object_request(const
char *base_url,
freq->slot = get_active_slot();
curl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq);
- curl_easy_setopt(freq->slot->curl, CURLOPT_FAILONERROR, 0);
curl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION,
fwrite_sha1_file);
curl_easy_setopt(freq->slot->curl, CURLOPT_ERRORBUFFER, freq->errorstr);
curl_easy_setopt(freq->slot->curl, CURLOPT_URL, freq->url);
diff --git a/http.h b/http.h
index d305ca1dc..eebf40688 100644
--- a/http.h
+++ b/http.h
@@ -146,7 +146,6 @@ extern char *get_remote_object_url(const char *url, const
char *hex,
/* Options for http_get_*() */
struct http_get_options {
unsigned no_cache:1,
-keep_error:1,
initial_request:1;
/* If non-NULL, returns the content-type of the response. */
diff --git a/remote-curl.c b/remote-curl.c
index 48656bf18..43e7a1d80 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -380,7 +380,6 @@ static struct discovery *discover_refs(const char *service,
int for_push)
http_options.extra_headers = &extra_headers;
http_options.initial_request = 1;
http_options.no_cache = 1;
- http_options.keep_error = 1;
http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);
switch (http_ret) {
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8..cc4b87507 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -115,6 +115,7 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
+ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
ScriptAlias /error/ error.sh/
diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh
new file mode 100755
index 0..73148eeba
--- /dev/null
+++ b/t/t5581-http-curl-verbose.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+test_description='test GIT_CURL_VERBOSE'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'setup repository' '
+ mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" --bare init &&
+ git config push.default matching &&
+ echo content >file &&
+ git add file &&
+ git commit -m one &&
+ git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ git push public master:master
+'
+
+test_expect_success 'failure in git-upload-pack is shown' '
+ test_might_fail env GIT_CURL_VERBOSE=1 \
+ git clone "$HTTPD_URL/error_git_upload_pack/smart/repo.git" \
+ 2>curl_log &&
+ cat curl_log | grep "< HTTP/1.1 500 Intentional Breakage"
+'
+
+stop_httpd
+
+test_done
--
2.20.1.415.g653613c723-goog
[PATCH v2 1/2] Change how HTTP response body is returned
This changes the way HTTP response body is returned in
http_request_reauth and post_rpc.
1. http_request_reauth makes up to two requests; one without a
credential and one with a credential. The first request can fail if
it needs a credential. When the keep_error option is specified, the
response to the first request can be written to the HTTP response
body destination. If the response body destination is a string
buffer, it erases the buffer before making the second request. By
introducing http_response_dest, it can handle the case that the
destination is a file handle.
2. post_rpc makes an HTTP request and the response body is directly
written to a file descriptor. This makes it check the HTTP status
code before writing it, and do not write the response body if it's an
error response. It's ok without this check now because post_rpc makes
a request with CURLOPT_FAILONERROR, and libcurl won't call the
callback if the response has an error status code.
Signed-off-by: Masaya Suzuki
---
http.c| 99 +--
remote-curl.c | 29 ---
2 files changed, 81 insertions(+), 47 deletions(-)
diff --git a/http.c b/http.c
index eacc2a75e..d23417670 100644
--- a/http.c
+++ b/http.c
@@ -165,6 +165,19 @@ static int http_schannel_check_revoke = 1;
*/
static int http_schannel_use_ssl_cainfo;
+/*
+ * Where to store the result of http_request.
+ *
+ * At most one of buffer or file can be non-NULL. The buffer and file are not
+ * allocated by http_request, and the caller is responsible for releasing them.
+ */
+struct http_response_dest {
+ struct strbuf *buffer;
+
+ FILE *file;
+ const char *filename;
+};
+
size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
{
size_t size = eltsize * nmemb;
@@ -1794,12 +1807,8 @@ static void http_opt_request_remainder(CURL *curl, off_t
pos)
curl_easy_setopt(curl, CURLOPT_RANGE, buf);
}
-/* http_request() targets */
-#define HTTP_REQUEST_STRBUF0
-#define HTTP_REQUEST_FILE 1
-
static int http_request(const char *url,
- void *result, int target,
+ struct http_response_dest *dest,
const struct http_get_options *options)
{
struct active_request_slot *slot;
@@ -1812,21 +1821,23 @@ static int http_request(const char *url,
slot = get_active_slot();
curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
- if (result == NULL) {
- curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
- } else {
+ if (dest->file) {
+ off_t posn = ftello(dest->file);
curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
- curl_easy_setopt(slot->curl, CURLOPT_FILE, result);
-
- if (target == HTTP_REQUEST_FILE) {
- off_t posn = ftello(result);
- curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
-fwrite);
- if (posn > 0)
- http_opt_request_remainder(slot->curl, posn);
- } else
- curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
-fwrite_buffer);
+ curl_easy_setopt(slot->curl, CURLOPT_FILE,
+dest->file);
+ curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
+fwrite);
+ if (posn > 0)
+ http_opt_request_remainder(slot->curl, posn);
+ } else if (dest->buffer) {
+ curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
+ curl_easy_setopt(slot->curl, CURLOPT_FILE,
+dest->buffer);
+ curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
+fwrite_buffer);
+ } else {
+ curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
}
accept_language = get_accept_language();
@@ -1930,10 +1941,10 @@ static int update_url_from_redirect(struct strbuf *base,
}
static int http_request_reauth(const char *url,
- void *result, int target,
+ struct http_response_dest *dest,
struct http_get_options *options)
{
- int ret = http_request(url, result, target, options);
+ int ret = http_request(url, dest, options);
if (ret != HTTP_OK && ret != HTTP_REAUTH)
return ret;
@@ -1949,32 +1960,34 @@ static int http_request_reauth(const char *url,
if (ret != HTTP_REAUTH)
return ret;
- /*
-* If we are using KEEP_ERROR, the previous request may have
-* put cruft into our output stream; we should c
[PATCH v2 0/2] Show HTTP headers of failed requests with GIT_CURL_VERBOSE
When GIT_CURL_VERBOSE is set, libcurl produces request/response headers to stderr. However, if the response is an error response and CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it won't dump the headers. Showing HTTP response headers is useful for debugging, especially for non-OK responses. To this end, the caller of libcurl needs to handle HTTP request failures by themselves. The first patch makes git prepared to handle those failures. The second patch actually unsets CURLOPT_FAILONERROR. Masaya Suzuki (2): Change how HTTP response body is returned Unset CURLOPT_FAILONERROR http.c | 103 +++ http.h | 1 - remote-curl.c| 30 -- t/lib-httpd/apache.conf | 1 + t/t5581-http-curl-verbose.sh | 28 ++ 5 files changed, 110 insertions(+), 53 deletions(-) create mode 100755 t/t5581-http-curl-verbose.sh -- 2.20.1.415.g653613c723-goog
Re: [PATCH 2/2] Unset CURLOPT_FAILONERROR
On Fri, Dec 28, 2018 at 11:58 AM Eric Sunshine wrote: > > On Fri, Dec 28, 2018 at 2:51 PM Masaya Suzuki wrote: > > On Fri, Dec 28, 2018 at 11:37 AM Eric Sunshine > > wrote: > > > On Thu, Dec 27, 2018 at 8:47 PM Masaya Suzuki > > > wrote: > > > > +test_expect_success 'failure in git-upload-pack is shown' ' > > > > + (GIT_CURL_VERBOSE=1 git clone --bare > > > > "$HTTPD_URL/error_git_upload_pack/smart/repo.git" 2>curl_log || > > > > +true) && > > > > > > Using test_might_fail() would allow you to drop the subshell and the "|| > > > true": > > > > > > test_might_fail env GIT_CURL_VERBOSE=1 git clone ... && > > > > > > > + cat curl_log | grep "< HTTP/1.1 500 Intentional Breakage" > > > > +' > > > > The test should success. This is a test that a log is produced after a > > git command fails. The point of this test is "cat curl_log | grep ..." > > part that asserts the log. > > Unfortunately, the name "test_might_fail" is confusing. It is not > saying that the entire test might or might not fail. Rather, it is > saying that the one command might or might not fail (and that you > don't care if it does fail). The idiom: > > (some-git-command || true) && > > can be replaced with: > >test_might_fail some-git-command && > > without changing its meaning, and without affecting the > success/failure status of the test overall. > > So, this new test could be written like this: > > --- 8< --- > test_expect_success 'failure in git-upload-pack is shown' ' >test_might_fail env GIT_CURL_VERBOSE=1 git clone --bare > "$HTTPD_URL/error_git_upload_pack/smart/repo.git" 2>curl_log && >cat curl_log | grep "< HTTP/1.1 500 Intentional Breakage" > ' > --- 8< --- > > and have the same meaning. Ah. I see. It's used inside the test. Thanks.
Re: [PATCH 2/2] Unset CURLOPT_FAILONERROR
On Fri, Dec 28, 2018 at 11:37 AM Eric Sunshine wrote: > > On Thu, Dec 27, 2018 at 8:47 PM Masaya Suzuki wrote: > > When GIT_CURL_VERBOSE is set, libcurl produces request/response headers > > to stderr. However, if the response is an error response and > > CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it > > won't ump the headers. Showing HTTP response headers is useful for > > s/ump/dump/ > > > debugging, especially for non-OK responses. > > > > This is substantially same as setting http_options.keep_error to all > > requests. Hence, removing this option. > > s/removing/remove/ > > > Signed-off-by: Masaya Suzuki > > --- > > diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh > > @@ -0,0 +1,32 @@ > > +test_expect_success 'setup repository' ' > > + ... > > +' > > + > > +test_expect_success 'create http-accessible bare repository' ' > > Not a big deal, but this seems like more setup, so it could be folded > into the "setup" test above it. > > > + mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && > > + (cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && > > +git --bare init > > + ) && > > Since this is a new test script, it makes sense to format the subshell > in the modern style: > > ( > cd ... && > git ... > ) && > > Alternately, use -C and drop the subshell altogether: > > git -C $BLAH/repo.git --bare init && > > > + git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && > > + git push public master:master > > +' > > + > > +test_expect_success 'failure in git-upload-pack is shown' ' > > + (GIT_CURL_VERBOSE=1 git clone --bare > > "$HTTPD_URL/error_git_upload_pack/smart/repo.git" 2>curl_log || > > +true) && > > Using test_might_fail() would allow you to drop the subshell and the "|| > true": > > test_might_fail env GIT_CURL_VERBOSE=1 git clone ... && > > > + cat curl_log | grep "< HTTP/1.1 500 Intentional Breakage" > > +' The test should success. This is a test that a log is produced after a git command fails. The point of this test is "cat curl_log | grep ..." part that asserts the log.
[PATCH 1/2] Change how HTTP response body is returned
This changes the way HTTP response body is returned in
http_request_reauth and post_rpc.
1. http_request_reauth makes up to two requests; one without a
credential and one with a credential. The first request can fail if
it needs a credential. When the keep_error option is specified, the
response to the first request can be written to the HTTP response
body destination. If the response body destination is a string
buffer, it erases the buffer before making the second request. By
introducing http_response_dest, it can handle the case that the
destination is a file handle.
2. post_rpc makes an HTTP request and the response body is directly
written to a file descriptor. This makes it check the HTTP status
code before writing it, and do not write the response body if it's an
error response. It's ok without this check now because post_rpc makes
a request with CURLOPT_FAILONERROR, and libcurl won't call the
callback if the response has an error status code.
Signed-off-by: Masaya Suzuki
---
http.c| 99 +--
remote-curl.c | 29 ---
2 files changed, 81 insertions(+), 47 deletions(-)
diff --git a/http.c b/http.c
index eacc2a75e..d23417670 100644
--- a/http.c
+++ b/http.c
@@ -165,6 +165,19 @@ static int http_schannel_check_revoke = 1;
*/
static int http_schannel_use_ssl_cainfo;
+/*
+ * Where to store the result of http_request.
+ *
+ * At most one of buffer or file can be non-NULL. The buffer and file are not
+ * allocated by http_request, and the caller is responsible for releasing them.
+ */
+struct http_response_dest {
+ struct strbuf *buffer;
+
+ FILE *file;
+ const char *filename;
+};
+
size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
{
size_t size = eltsize * nmemb;
@@ -1794,12 +1807,8 @@ static void http_opt_request_remainder(CURL *curl, off_t
pos)
curl_easy_setopt(curl, CURLOPT_RANGE, buf);
}
-/* http_request() targets */
-#define HTTP_REQUEST_STRBUF0
-#define HTTP_REQUEST_FILE 1
-
static int http_request(const char *url,
- void *result, int target,
+ struct http_response_dest *dest,
const struct http_get_options *options)
{
struct active_request_slot *slot;
@@ -1812,21 +1821,23 @@ static int http_request(const char *url,
slot = get_active_slot();
curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
- if (result == NULL) {
- curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
- } else {
+ if (dest->file) {
+ off_t posn = ftello(dest->file);
curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
- curl_easy_setopt(slot->curl, CURLOPT_FILE, result);
-
- if (target == HTTP_REQUEST_FILE) {
- off_t posn = ftello(result);
- curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
-fwrite);
- if (posn > 0)
- http_opt_request_remainder(slot->curl, posn);
- } else
- curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
-fwrite_buffer);
+ curl_easy_setopt(slot->curl, CURLOPT_FILE,
+dest->file);
+ curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
+fwrite);
+ if (posn > 0)
+ http_opt_request_remainder(slot->curl, posn);
+ } else if (dest->buffer) {
+ curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
+ curl_easy_setopt(slot->curl, CURLOPT_FILE,
+dest->buffer);
+ curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
+fwrite_buffer);
+ } else {
+ curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
}
accept_language = get_accept_language();
@@ -1930,10 +1941,10 @@ static int update_url_from_redirect(struct strbuf *base,
}
static int http_request_reauth(const char *url,
- void *result, int target,
+ struct http_response_dest *dest,
struct http_get_options *options)
{
- int ret = http_request(url, result, target, options);
+ int ret = http_request(url, dest, options);
if (ret != HTTP_OK && ret != HTTP_REAUTH)
return ret;
@@ -1949,32 +1960,34 @@ static int http_request_reauth(const char *url,
if (ret != HTTP_REAUTH)
return ret;
- /*
-* If we are using KEEP_ERROR, the previous request may have
-* put cruft into our output stream; we should c
[PATCH 2/2] Unset CURLOPT_FAILONERROR
When GIT_CURL_VERBOSE is set, libcurl produces request/response headers
to stderr. However, if the response is an error response and
CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it
won't ump the headers. Showing HTTP response headers is useful for
debugging, especially for non-OK responses.
This is substantially same as setting http_options.keep_error to all
requests. Hence, removing this option.
Signed-off-by: Masaya Suzuki
---
http.c | 4
http.h | 1 -
remote-curl.c| 1 -
t/lib-httpd/apache.conf | 1 +
t/t5581-http-curl-verbose.sh | 32
5 files changed, 33 insertions(+), 6 deletions(-)
create mode 100755 t/t5581-http-curl-verbose.sh
diff --git a/http.c b/http.c
index d23417670..8f8101da3 100644
--- a/http.c
+++ b/http.c
@@ -1269,7 +1269,6 @@ struct active_request_slot *get_active_slot(void)
curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);
curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
- curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);
/*
@@ -1848,8 +1847,6 @@ static int http_request(const char *url,
strbuf_addstr(&buf, "Pragma:");
if (options && options->no_cache)
strbuf_addstr(&buf, " no-cache");
- if (options && options->keep_error)
- curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
if (options && options->initial_request &&
http_follow_config == HTTP_FOLLOW_INITIAL)
curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
@@ -2415,7 +2412,6 @@ struct http_object_request *new_http_object_request(const
char *base_url,
freq->slot = get_active_slot();
curl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq);
- curl_easy_setopt(freq->slot->curl, CURLOPT_FAILONERROR, 0);
curl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION,
fwrite_sha1_file);
curl_easy_setopt(freq->slot->curl, CURLOPT_ERRORBUFFER, freq->errorstr);
curl_easy_setopt(freq->slot->curl, CURLOPT_URL, freq->url);
diff --git a/http.h b/http.h
index d305ca1dc..eebf40688 100644
--- a/http.h
+++ b/http.h
@@ -146,7 +146,6 @@ extern char *get_remote_object_url(const char *url, const
char *hex,
/* Options for http_get_*() */
struct http_get_options {
unsigned no_cache:1,
-keep_error:1,
initial_request:1;
/* If non-NULL, returns the content-type of the response. */
diff --git a/remote-curl.c b/remote-curl.c
index 48656bf18..43e7a1d80 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -380,7 +380,6 @@ static struct discovery *discover_refs(const char *service,
int for_push)
http_options.extra_headers = &extra_headers;
http_options.initial_request = 1;
http_options.no_cache = 1;
- http_options.keep_error = 1;
http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);
switch (http_ret) {
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8..cc4b87507 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -115,6 +115,7 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
+ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
ScriptAlias /error/ error.sh/
diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh
new file mode 100755
index 0..c89e06e12
--- /dev/null
+++ b/t/t5581-http-curl-verbose.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='test GIT_CURL_VERBOSE'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'setup repository' '
+ git config push.default matching &&
+ echo content >file &&
+ git add file &&
+ git commit -m one
+'
+
+test_expect_success 'create http-accessible bare repository' '
+ mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ (cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+git --bare init
+ ) &&
+ git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ git push public master:master
+'
+
+test_expect_success 'failure in git-upload-pack is shown' '
+ (GIT_CURL_VERBOSE=1 git clone --bare
"$HTTPD_URL/error_git_upload_pack/smart/repo.git" 2>curl_log ||
+true) &&
+ cat curl_log | grep "< HTTP/1.1 500 Intentional Breakage"
+'
+
+stop_httpd
+
+test_done
--
2.20.1.415.g653613c723-goog
Re: [PATCH] Specify -Wformat along with -Wformat-security
On Thu, Dec 27, 2018 at 10:36 AM Duy Nguyen wrote: > > On Thu, Dec 27, 2018 at 7:18 PM Masaya Suzuki wrote: > > > > Without -Wformat, -Wformat-security won't work. > > > > > cc1: error: -Wformat-security ignored without -Wformat > > > [-Werror=format-security] > > Compiler name and version? I'm not familar with the tools, so please be patient. According to Makefile, `CC = cc`. With `cc --version`, it says: cc (Debian 7.3.0-5) 7.3.0 Copyright (C) 2017 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. Jonathan said there was the same patch. Looks like https://public-inbox.org/git/20181012184037.15076-1-t.gumme...@gmail.com/. I'll check my config again.
[PATCH] Specify -Wformat along with -Wformat-security
Without -Wformat, -Wformat-security won't work. > cc1: error: -Wformat-security ignored without -Wformat > [-Werror=format-security] Signed-off-by: Masaya Suzuki --- config.mak.dev | 1 + 1 file changed, 1 insertion(+) diff --git a/config.mak.dev b/config.mak.dev index bbeeff44f..aae9db67d 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -7,6 +7,7 @@ CFLAGS += -pedantic CFLAGS += -DUSE_PARENS_AROUND_GETTEXT_N=0 endif CFLAGS += -Wdeclaration-after-statement +CFLAGS += -Wformat CFLAGS += -Wformat-security CFLAGS += -Wno-format-zero-length CFLAGS += -Wold-style-definition -- 2.20.1.415.g653613c723-goog
[PATCH] Use packet_reader instead of packet_read_line
By using and sharing a packet_reader while handling a Git pack protocol
request, the same reader option is used throughout the code. This makes
it easy to set a reader option to the request parsing code.
Signed-off-by: Masaya Suzuki
---
builtin/archive.c | 19 ++---
builtin/receive-pack.c | 60 +
fetch-pack.c | 61 +++---
remote-curl.c | 20 --
send-pack.c| 37 -
upload-pack.c | 38 +-
6 files changed, 126 insertions(+), 109 deletions(-)
diff --git a/builtin/archive.c b/builtin/archive.c
index d2455237c..2fe1f05ca 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -27,10 +27,10 @@ static int run_remote_archiver(int argc, const char **argv,
const char *remote, const char *exec,
const char *name_hint)
{
- char *buf;
int fd[2], i, rv;
struct transport *transport;
struct remote *_remote;
+ struct packet_reader reader;
_remote = remote_get(remote);
if (!_remote->url[0])
@@ -53,18 +53,19 @@ static int run_remote_archiver(int argc, const char **argv,
packet_write_fmt(fd[1], "argument %s\n", argv[i]);
packet_flush(fd[1]);
- buf = packet_read_line(fd[0], NULL);
- if (!buf)
+ packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);
+
+ if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
die(_("git archive: expected ACK/NAK, got a flush packet"));
- if (strcmp(buf, "ACK")) {
- if (starts_with(buf, "NACK "))
- die(_("git archive: NACK %s"), buf + 5);
- if (starts_with(buf, "ERR "))
- die(_("remote error: %s"), buf + 4);
+ if (strcmp(reader.line, "ACK")) {
+ if (starts_with(reader.line, "NACK "))
+ die(_("git archive: NACK %s"), reader.line + 5);
+ if (starts_with(reader.line, "ERR "))
+ die(_("remote error: %s"), reader.line + 4);
die(_("git archive: protocol error"));
}
- if (packet_read_line(fd[0], NULL))
+ if (packet_reader_read(&reader) != PACKET_READ_FLUSH)
die(_("git archive: expected a flush"));
/* Now, start reading from fd[0] and spit it out to stdout */
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 33187bd8e..81cc07370 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1569,30 +1569,29 @@ static void queue_commands_from_cert(struct command
**tail,
}
}
-static struct command *read_head_info(struct oid_array *shallow)
+static struct command *read_head_info(struct packet_reader *reader,
+ struct oid_array *shallow)
{
struct command *commands = NULL;
struct command **p = &commands;
for (;;) {
- char *line;
- int len, linelen;
+ int linelen;
- line = packet_read_line(0, &len);
- if (!line)
+ if (packet_reader_read(reader) != PACKET_READ_NORMAL)
break;
- if (len > 8 && starts_with(line, "shallow ")) {
+ if (reader->pktlen > 8 && starts_with(reader->line, "shallow
")) {
struct object_id oid;
- if (get_oid_hex(line + 8, &oid))
+ if (get_oid_hex(reader->line + 8, &oid))
die("protocol error: expected shallow sha, got
'%s'",
- line + 8);
+ reader->line + 8);
oid_array_append(shallow, &oid);
continue;
}
- linelen = strlen(line);
- if (linelen < len) {
- const char *feature_list = line + linelen + 1;
+ linelen = strlen(reader->line);
+ if (linelen < reader->pktlen) {
+ const char *feature_list = reader->line + linelen + 1;
if (parse_feature_request(feature_list,
"report-status"))
report_status = 1;
if (parse_feature_request(feature_list,
"side-band-64k"))
@@ -1607,28 +1606,32 @@ static struct command *read_head_info(struct oid_array
*shallow)
use_push_options = 1;
}
- if (!strcmp(line, "pu
Re: [PATCH v3 1/4] pack-protocol.txt: accept error packets in any context
On Wed, Dec 12, 2018 at 3:02 AM Jeff King wrote:
>
> On Tue, Dec 11, 2018 at 04:25:15PM -0800, Josh Steadmon wrote:
>
> > From: Masaya Suzuki
> >
> > In the Git pack protocol definition, an error packet may appear only in
> > a certain context. However, servers can face a runtime error (e.g. I/O
> > error) at an arbitrary timing. This patch changes the protocol to allow
> > an error packet to be sent instead of any packet.
> >
> > Following this protocol spec change, the error packet handling code is
> > moved to pkt-line.c.
>
> This is a change in the spec with an accompanying change in the code,
> which raises the question: what do other implementations do with this
> change (both older Git, and implementations like JGit, libgit2, etc)?
JGit is similar to Git. It parses "ERR " in limited places. When it sees an ERR
packet in an unexpected place, it'll fail somewhere in the parsing code.
https://github.com/eclipse/jgit/blob/30c6c7542190c149e2aee792f992a312a5fc5793/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java#L145-L147
https://github.com/eclipse/jgit/blob/f40b39345cd9b54473ee871bff401fe3d394ffe3/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java#L208
I'm not familiar with libgit2 code, but it seems it handles this at a
lower level. An error type packet is parsed out at a low level, and
the error handling is done by the callers of the packet parser.
https://github.com/libgit2/libgit2/blob/bea65980c7a42e34edfafbdc40b199ba7b2a564e/src/transports/smart_pkt.c#L482-L483
I cannot find an ERR packet handling in go-git. It seems to me that if
an ERR packet appears it treats it as a parsing error.
https://github.com/src-d/go-git/blob/master/plumbing/protocol/packp/common.go#L60-L62
>
> I think the answer for older Git is "hang up unceremoniously", which is
> probably OK given the semantics of the change. And I'd suspect most
> other implementations would do the same. I just wonder if anybody tested
> it with other implementations.
I'm thinking aloud here. There would be two aspects of the protocol
compatibility: (1) new clients speak to old servers (2) old clients
speak to a new server that speaks the updated protocol.
For (1), I assume that in the Git pack protocol, a packet starting
from "ERR " does not appear naturally except for a very special case
that the server doesn't support sideband, but using the updated
protocol. As you mentioned, at first it looks like this can mistakenly
parse the pack file of git-receive-pack as an ERR packet, assuming
that git-receive-pack's pack file is packetized. Actually
git-receive-pack's pack file is not packetized in the Git pack
protocol (https://github.com/git/git/blob/master/builtin/receive-pack.c#L1695).
I recently wrote a Git protocol parser
(https://github.com/google/gitprotocolio), and I confirmed that this
is the case at least for the HTTP transport. git-upload-pack's pack
file is indeed packetized, but packetized with sideband. Except for
the case where sideband is not used, the packfiles wouldn't be
considered as an ERR packet accidentally.
For (2), if the old clients see an unexpected ERR packet, they cannot
parse it. They would handle this unparsable data as if the server is
not speaking Git protocol correctly. Even if the old clients just
ignore the packet, due to the nature of the ERR packet, the server
won't send further data. The client won't be able to proceed. Overall,
the clients anyway face an error, and the only difference would be
whether the clients can show an error nicely or not. The new clients
will show the errors nicely to users. Old clients will not.
>
> > +An error packet is a special pkt-line that contains an error string.
> > +
> > +
> > + error-line = PKT-LINE("ERR" SP explanation-text)
> > +
> > +
> > +Throughout the protocol, where `PKT-LINE(...)` is expected, an error
> > packet MAY
> > +be sent. Once this packet is sent by a client or a server, the data
> > transfer
> > +process defined in this protocol is terminated.
>
> The packfile data is typically packetized, too, and contains arbitrary
> data (that could have "ERR" in it). It looks like we don't specifically
> say PKT-LINE() in that part of the protocol spec, though, so I think
> this is OK.
As I described above, as far as I can see, the packfile in
git-upload-pack is not packetized. The packfile in git-receive-pack is
packetized but typically with sideband. At least at the Git pack
protocol level, this should be OK.
>
> Likewise, in the implementation:
>
> > diff --git a/pkt-line.c b/pkt-line.c
> > index 04d10bbd03..ce9e42d10e 100644
> > --- a/pkt-line.c
> > +++ b/pkt-line.c
> > @@ -346
Re: [PATCH] pack-protocol.txt: accept error packets in any context
On Thu, Nov 29, 2018 at 3:42 AM Junio C Hamano wrote: > > Masaya Suzuki writes: > > > In the Git pack protocol definition, an error packet may appear only in > > a certain context. However, servers can face a runtime error (e.g. I/O > > error) at an arbitrary timing. This patch changes the protocol to allow > > an error packet to be sent instead of any packet. > > > > Following this protocol spec change, the error packet handling code is > > moved to pkt-line.c. > > > > Signed-off-by: Masaya Suzuki > > --- > > Have you run tests with this applied? I noticed 5703.9 now has > stale expectations, but there may be others. Yes, I did. And it also didn't end up in a build error. Do I have a different build option...?
[PATCH] pack-protocol.txt: accept error packets in any context
In the Git pack protocol definition, an error packet may appear only in
a certain context. However, servers can face a runtime error (e.g. I/O
error) at an arbitrary timing. This patch changes the protocol to allow
an error packet to be sent instead of any packet.
Following this protocol spec change, the error packet handling code is
moved to pkt-line.c.
Signed-off-by: Masaya Suzuki
---
Documentation/technical/pack-protocol.txt | 20 +++-
builtin/archive.c | 2 --
connect.c | 2 --
fetch-pack.c | 2 --
pkt-line.c| 4
5 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/Documentation/technical/pack-protocol.txt
b/Documentation/technical/pack-protocol.txt
index 6ac774d5f..7a2375a55 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -22,6 +22,16 @@ protocol-common.txt. When the grammar indicate
`PKT-LINE(...)`, unless
otherwise noted the usual pkt-line LF rules apply: the sender SHOULD
include a LF, but the receiver MUST NOT complain if it is not present.
+An error packet is a special pkt-line that contains an error string.
+
+
+ error-line = PKT-LINE("ERR" SP explanation-text)
+
+
+Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY
+be sent. Once this packet is sent by a client or a server, the data transfer
+process defined in this protocol is terminated.
+
Transports
--
There are three transports over which the packfile protocol is
@@ -89,13 +99,6 @@ process on the server side over the Git protocol is this:
"0039git-upload-pack /schacon/gitbook.git\0host=example.com\0" |
nc -v example.com 9418
-If the server refuses the request for some reasons, it could abort
-gracefully with an error message.
-
-
- error-line = PKT-LINE("ERR" SP explanation-text)
-
-
SSH Transport
-
@@ -398,12 +401,11 @@ from the client).
Then the server will start sending its packfile data.
- server-response = *ack_multi ack / nak / error-line
+ server-response = *ack_multi ack / nak
ack_multi = PKT-LINE("ACK" SP obj-id ack_status)
ack_status = "continue" / "common" / "ready"
ack = PKT-LINE("ACK" SP obj-id)
nak = PKT-LINE("NAK")
- error-line = PKT-LINE("ERR" SP explanation-text)
A simple clone may look like this (with no 'have' lines):
diff --git a/builtin/archive.c b/builtin/archive.c
index d2455237c..5d179bbd1 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -59,8 +59,6 @@ static int run_remote_archiver(int argc, const char **argv,
if (strcmp(buf, "ACK")) {
if (starts_with(buf, "NACK "))
die(_("git archive: NACK %s"), buf + 5);
- if (starts_with(buf, "ERR "))
- die(_("remote error: %s"), buf + 4);
die(_("git archive: protocol error"));
}
diff --git a/connect.c b/connect.c
index 24281b608..458906e60 100644
--- a/connect.c
+++ b/connect.c
@@ -306,8 +306,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,
die_initial_contact(1);
case PACKET_READ_NORMAL:
len = reader->pktlen;
- if (len > 4 && skip_prefix(reader->line, "ERR ", &arg))
- die(_("remote error: %s"), arg);
break;
case PACKET_READ_FLUSH:
state = EXPECTING_DONE;
diff --git a/fetch-pack.c b/fetch-pack.c
index 9691046e6..e66cd7b71 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -178,8 +178,6 @@ static enum ack_type get_ack(int fd, struct object_id
*result_oid)
return ACK;
}
}
- if (skip_prefix(line, "ERR ", &arg))
- die(_("remote error: %s"), arg);
die(_("git fetch-pack: expected ACK/NAK, got '%s'"), line);
}
diff --git a/pkt-line.c b/pkt-line.c
index 04d10bbd0..ce9e42d10 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd,
char **src_buffer,
return PACKET_READ_EOF;
}
+ if (starts_with(buffer, "ERR ")) {
+ die(_("remote error: %s"), buffer + 4);
+ }
+
if ((options & PACKET_READ_CHOMP_NEWLINE) &&
len && buffer[len-1] == '\n')
len--;
--
2.20.0.rc0.387.gc7a69e6b6c-goog
[PATCH] doc: fix want-capability separator
Unlike ref advertisement, client capabilities and the first want are
separated by SP, not NUL, in the implementation. Fix the documentation
to align with the implementation. pack-protocol.txt is already fixed.
Signed-off-by: Masaya Suzuki
---
Documentation/technical/http-protocol.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/technical/http-protocol.txt
b/Documentation/technical/http-protocol.txt
index 64f49d0bb..9c5b6f0fa 100644
--- a/Documentation/technical/http-protocol.txt
+++ b/Documentation/technical/http-protocol.txt
@@ -338,11 +338,11 @@ server advertises capability `allow-tip-sha1-in-want` or
request_end
request_end = "" / "done"
- want_list = PKT-LINE(want NUL cap_list LF)
+ want_list = PKT-LINE(want SP cap_list LF)
*(want_pkt)
want_pkt = PKT-LINE(want LF)
want = "want" SP id
- cap_list = *(SP capability) SP
+ cap_list = capability *(SP capability)
have_list = *PKT-LINE("have" SP id LF)
--
2.18.0
[PATCH] builtin/send-pack: populate the default configs
builtin/send-pack didn't call git_default_config, and because of this git push --signed didn't respect the username and email in gitconfig in the HTTP transport. Signed-off-by: Masaya Suzuki --- builtin/send-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 4923b1058..42fd8d1a3 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -121,7 +121,7 @@ static int send_pack_config(const char *k, const char *v, void *cb) } } } - return 0; + return git_default_config(k, v, cb); } int cmd_send_pack(int argc, const char **argv, const char *prefix) -- 2.18.0.rc1.242.g61856ae69a-goog
