# [PATCH] http-push and http-fetch: handle URLs without leading / 13 messages from 2008-01-19 to 2008-01-21. Participants: Grégoire Barbier, Mike Hommey, Johannes Schindelin, Junio C Hamano. Thread: https://gitlist.dev/t/11676 ## Grégoire Barbier, 2008-01-19 15:22 Subject: [PATCH] http-push: fix webdav lock leak. Message-ID: <1200756171-11696-1-git-send-email-gb@gbarbier.org> URL: https://gitlist.dev/e/1200756171-11696-1-git-send-email-gb%40gbarbier.org ``` Releasing webdav lock even if push fails because of bad (or no) reference on command line. To reproduce the issue that this patch fixes, you need a test git repository availlable over http+webdav, let's say at http://myhost/myrepo.git/ Then, you do this: $ git clone http://myhost/myrepo.git/ $ cd myrepo $ git push http Fetching remote heads... refs/ refs/heads/ refs/tags/ No refs in common and none specified; doing nothing. $ git push http Fetching remote heads... refs/ refs/heads/ refs/tags/ No refs in common and none specified; doing nothing. $ Finally, you look at the web server logs, and will find one LOCK query and no UNLOCK query, of course the second one will be in 423 return code instead of 200: 1.2.3.4 - gb [19/Jan/2008:14:24:56 +0100] "LOCK /myrepo.git/info/refs HTTP/1.1" 200 465 (...) 1.2.3.4 - gb [19/Jan/2008:14:25:10 +0100] "LOCK /myrepo.git/info/refs HTTP/1.1" 423 363 With this patch, there would have be two UNLOCKs in addition of the LOCKs From the user point of view: - If you realize that you should have typed e.g. "git push http master" instead of "git push http", you will have to wait for 10 minutes for the lock to expire by its own. - Furthermore, if somebody else is dumb enough to type "git push http" while you need to push "master" branch, then you'll need too to wait for 10 minutes too. Signed-off-by: Grégoire Barbier --- http-push.c | 13 ++++++++----- 1 files changed, 8 insertions(+), 5 deletions(-) diff --git a/http-push.c b/http-push.c index eef7674..2c4e91d 100644 --- a/http-push.c +++ b/http-push.c @@ -2264,11 +2264,14 @@ int main(int argc, char **argv) if (!remote_tail) remote_tail = &remote_refs; if (match_refs(local_refs, remote_refs, &remote_tail, - nr_refspec, (const char **) refspec, push_all)) - return -1; + nr_refspec, (const char **) refspec, push_all)) { + rc = -1; + goto cleanup; + } if (!remote_refs) { fprintf(stderr, "No refs in common and none specified; doing nothing.\n"); - return 0; + rc = 0; + goto cleanup; } new_refs = 0; @@ -2399,10 +2402,10 @@ int main(int argc, char **argv) fprintf(stderr, "Unable to update server info\n"); } } - if (info_ref_lock) - unlock_remote(info_ref_lock); cleanup: + if (info_ref_lock) + unlock_remote(info_ref_lock); free(remote); curl_slist_free_all(no_pragma_header); -- 1.5.4.rc3.52.g9a5bd-dirty ``` ## Grégoire Barbier, 2008-01-19 15:22 Subject: [PATCH] http-push: fail when info/refs exists and is already locked Message-ID: <1200756171-11696-2-git-send-email-gb@gbarbier.org> URL: https://gitlist.dev/e/1200756171-11696-2-git-send-email-gb%40gbarbier.org In-Reply-To: <1200756171-11696-1-git-send-email-gb@gbarbier.org> ``` Rationale: Failing instead of silently not updating remote refs makes the things cleare for the user when trying to push on a repository while another person do (or while a dandling locks are waiting for a 10 minutes timeout). When silently not updating remote refs, the user does not even know that git has pushed the objects but leaved the refs as they were before (e.g. a new bunch of commits on branch "master" is uploaded, however the branch by itsel still points on the previous head commit). --- http-push.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/http-push.c b/http-push.c index 2c4e91d..e1984d3 100644 --- a/http-push.c +++ b/http-push.c @@ -2243,6 +2243,11 @@ int main(int argc, char **argv) info_ref_lock = lock_remote("info/refs", LOCK_TIME); if (info_ref_lock) remote->can_update_info_refs = 1; + else { + fprintf(stderr, "Error: cannot lock existing info/refs\n"); + rc = 1; + goto cleanup; + } } if (remote->has_info_packs) fetch_indices(); -- 1.5.4.rc3.52.g9a5bd-dirty ``` ## Grégoire Barbier, 2008-01-19 15:22 Subject: [PATCH] http-push: more explicit error message with bad URL or password Message-ID: <1200756171-11696-3-git-send-email-gb@gbarbier.org> URL: https://gitlist.dev/e/1200756171-11696-3-git-send-email-gb%40gbarbier.org In-Reply-To: <1200756171-11696-2-git-send-email-gb@gbarbier.org> ``` Previously, when URL or password where not set correctly (or when some network errors occur), the error message was "no DAV locking support". --- http-push.c | 6 ++++++ http.c | 25 +++++++++++++++++++++++++ http.h | 1 + 3 files changed, 32 insertions(+), 0 deletions(-) diff --git a/http-push.c b/http-push.c index e1984d3..c984d84 100644 --- a/http-push.c +++ b/http-push.c @@ -2228,6 +2228,12 @@ int main(int argc, char **argv) no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:"); + if (!http_test_connection(remote->url)) { + fprintf(stderr, "Error: cannot access to remote URL (maybe malformed URL, network error or bad credentials)\n"); + rc = 1; + goto cleanup; + } + /* Verify DAV compliance/lock support */ if (!locking_available()) { fprintf(stderr, "Error: no DAV locking support on remote repo %s\n", remote->url); diff --git a/http.c b/http.c index d2c11ae..8b04ae9 100644 --- a/http.c +++ b/http.c @@ -634,3 +634,28 @@ int http_fetch_ref(const char *base, const char *ref, unsigned char *sha1) free(url); return ret; } + +int http_test_connection(const char *url) +{ + struct strbuf buffer = STRBUF_INIT; + struct active_request_slot *slot; + struct slot_results results; + int ret = 0; + + slot = get_active_slot(); + slot->results = &results; + curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer); + curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer); + curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL); + curl_easy_setopt(slot->curl, CURLOPT_URL, url); + if (start_active_slot(slot)) { + run_active_slot(slot); + if (results.curl_result == CURLE_OK) + ret = -1; + else + error("Cannot access to URL %s, return code %d", url, results.curl_result); + } else + error("Unable to start request"); + strbuf_release(&buffer); + return ret; +} diff --git a/http.h b/http.h index aeba930..b353007 100644 --- a/http.h +++ b/http.h @@ -77,6 +77,7 @@ extern void step_active_slots(void); extern void http_init(void); extern void http_cleanup(void); +extern int http_test_connection(const char *url); extern int data_received; extern int active_requests; -- 1.5.4.rc3.52.g9a5bd-dirty ``` ## Grégoire Barbier, 2008-01-19 15:22 Subject: [PATCH] http-push and http-fetch: handle URLs without leading / Message-ID: <1200756171-11696-4-git-send-email-gb@gbarbier.org> URL: https://gitlist.dev/e/1200756171-11696-4-git-send-email-gb%40gbarbier.org In-Reply-To: <1200756171-11696-3-git-send-email-gb@gbarbier.org> ``` Since HTTP/302 is not handled in the git code calling curl, URLs without leading / used to lead to frozen git-fetch or git-push with no error message. Furthermore, http-push freeze forces the user to interrupt it (^C) and therefore to leave a dandling webdav lock that makes the remote repository un-pushable for 10 minutes. The patch does not make curl calls handle HTTP/302 but instead adds a / at the end of URLs that does not have it yet. --- builtin-http-fetch.c | 10 ++++++++++ http-push.c | 11 +++++++++++ 2 files changed, 21 insertions(+), 0 deletions(-) diff --git a/builtin-http-fetch.c b/builtin-http-fetch.c index 4a50dbd..3c1ed08 100644 --- a/builtin-http-fetch.c +++ b/builtin-http-fetch.c @@ -9,6 +9,7 @@ int cmd_http_fetch(int argc, const char **argv, const char *prefix) const char **write_ref = NULL; char **commit_id; const char *url; + char *rewritten_url = NULL; int arg = 1; int rc = 0; int get_tree = 0; @@ -51,6 +52,12 @@ int cmd_http_fetch(int argc, const char **argv, const char *prefix) commits = 1; } url = argv[arg]; + if (url && url[strlen(url)-1] != '/') { + rewritten_url = malloc(strlen(url)+2); + strcpy(rewritten_url, url); + strcat(rewritten_url, "/"); + url = rewritten_url; + } walker = get_http_walker(url); walker->get_tree = get_tree; @@ -73,5 +80,8 @@ int cmd_http_fetch(int argc, const char **argv, const char *prefix) walker_free(walker); + if (rewritten_url) + free(rewritten_url); + return rc; } diff --git a/http-push.c b/http-push.c index c984d84..2c27105 100644 --- a/http-push.c +++ b/http-push.c @@ -2161,6 +2161,7 @@ int main(int argc, char **argv) int i; int new_refs; struct ref *ref; + char *rewritten_url = NULL; setup_git_directory(); @@ -2228,6 +2229,14 @@ int main(int argc, char **argv) no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:"); + if (remote->url && remote->url[strlen(remote->url)-1] != '/') { + rewritten_url = malloc(strlen(remote->url)+2); + strcpy(rewritten_url, remote->url); + strcat(rewritten_url, "/"); + remote->url = rewritten_url; + ++remote->path_len; + } + if (!http_test_connection(remote->url)) { fprintf(stderr, "Error: cannot access to remote URL (maybe malformed URL, network error or bad credentials)\n"); rc = 1; @@ -2415,6 +2424,8 @@ int main(int argc, char **argv) } cleanup: + if (rewritten_url) + free(rewritten_url); if (info_ref_lock) unlock_remote(info_ref_lock); free(remote); -- 1.5.4.rc3.52.g9a5bd-dirty ``` ## Grégoire Barbier, 2008-01-19 15:22 Subject: [PATCH] added #define DEFAULT_MAX_REQUESTS for USE_CURL_MULTI mode Message-ID: <1200756171-11696-5-git-send-email-gb@gbarbier.org> URL: https://gitlist.dev/e/1200756171-11696-5-git-send-email-gb%40gbarbier.org In-Reply-To: <1200756171-11696-4-git-send-email-gb@gbarbier.org> ``` From: Gregoire Barbier I'm not sure of which value is the good one, but 4 seems good since it's not very high which would lead to resouce consumption problems. --- http.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/http.c b/http.c index 8b04ae9..7b1bcb8 100644 --- a/http.c +++ b/http.c @@ -4,6 +4,7 @@ int data_received; int active_requests = 0; #ifdef USE_CURL_MULTI +#define DEFAULT_MAX_REQUESTS 4 static int max_requests = -1; static CURLM *curlm; #endif -- 1.5.4.rc3.52.g9a5bd-dirty ``` ## Mike Hommey, 2008-01-19 15:29 Subject: Re: [PATCH] http-push and http-fetch: handle URLs without leading / Message-ID: <20080119152947.GA26553@glandium.org> URL: https://gitlist.dev/e/20080119152947.GA26553%40glandium.org In-Reply-To: <1200756171-11696-4-git-send-email-gb@gbarbier.org> ``` On Sat, Jan 19, 2008 at 04:22:50PM +0100, Grégoire Barbier wrote: > Since HTTP/302 is not handled in the git code calling curl, URLs without > leading / used to lead to frozen git-fetch or git-push with no error message. > > Furthermore, http-push freeze forces the user to interrupt it (^C) and > therefore to leave a dandling webdav lock that makes the remote repository > un-pushable for 10 minutes. > > The patch does not make curl calls handle HTTP/302 but instead adds a / at > the end of URLs that does not have it yet. Actually, it would be much better to do just that, i.e. handle HTTP 302 return codes. I suspect there may be other cases leading to similar dead locks with other HTTP codes. But that might just be easier to deal with once my refactoring will be done ;) Mike ``` ## Johannes Schindelin, 2008-01-19 23:08 Subject: Re: [PATCH] http-push: fix webdav lock leak. Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0801192302340.5731%40racer.site In-Reply-To: <1200756171-11696-1-git-send-email-gb@gbarbier.org> ``` Hi, On Sat, 19 Jan 2008, Grégoire Barbier wrote: > diff --git a/http-push.c b/http-push.c > index eef7674..2c4e91d 100644 > --- a/http-push.c > +++ b/http-push.c > @@ -2264,11 +2264,14 @@ int main(int argc, char **argv) > if (!remote_tail) > remote_tail = &remote_refs; > if (match_refs(local_refs, remote_refs, &remote_tail, > - nr_refspec, (const char **) refspec, push_all)) > - return -1; > + nr_refspec, (const char **) refspec, push_all)) { > + rc = -1; > + goto cleanup; > + } > if (!remote_refs) { > fprintf(stderr, "No refs in common and none specified; doing nothing.\n"); > - return 0; > + rc = 0; > + goto cleanup; > } > > new_refs = 0; > @@ -2399,10 +2402,10 @@ int main(int argc, char **argv) > fprintf(stderr, "Unable to update server info\n"); > } > } > - if (info_ref_lock) > - unlock_remote(info_ref_lock); > > cleanup: > + if (info_ref_lock) > + unlock_remote(info_ref_lock); > free(remote); This late in the rc cycle, together with my unfamiliarity of the code and the code paths in http.c and http-push.c would make me feel _much_ better if you could insert the "if (info_ref_lock)" before the returns, instead of replacing the returns with "goto cleanup"s... Thanks, Dscho ``` ## Johannes Schindelin, 2008-01-19 23:14 Subject: Re: [PATCH] http-push and http-fetch: handle URLs without leading / Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0801192310290.5731%40racer.site In-Reply-To: <1200756171-11696-4-git-send-email-gb@gbarbier.org> ``` Hi, On Sat, 19 Jan 2008, Grégoire Barbier wrote: > Since HTTP/302 is not handled in the git code calling curl, URLs without > leading / used to lead to frozen git-fetch or git-push with no error message. JFYI these lines are a little bit too long; I would like to see them unwrapped with a 4-space indent on a 80-column display. I am not sure how easily Junio can fix them. > @@ -51,6 +52,12 @@ int cmd_http_fetch(int argc, const char **argv, const char *prefix) > commits = 1; > } > url = argv[arg]; > + if (url && url[strlen(url)-1] != '/') { > + rewritten_url = malloc(strlen(url)+2); > + strcpy(rewritten_url, url); > + strcat(rewritten_url, "/"); > + url = rewritten_url; > + } > > walker = get_http_walker(url); > walker->get_tree = get_tree; Please use strbuf, like so: struct strbuf rewritten_url = STRBUF_INIT; ... url = argv[arg]; if (url && url[strlen(url)-1] != '/') { strbuf_addstr(&rewritten_url, url); strbuf_addch(&rewritten_url, '/'); url = rewritten_url.buf; } ... strbuf_release(&rewritten_url); BTW it seems you indented using spaces, but we like the indentation as tabs in git.git. Other than that, I like your patch! Thanks, Dscho ``` ## Johannes Schindelin, 2008-01-19 23:16 Subject: Re: [PATCH] http-push and http-fetch: handle URLs without leading / Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0801192315260.5731%40racer.site In-Reply-To: <20080119152947.GA26553@glandium.org> ``` Hi, On Sat, 19 Jan 2008, Mike Hommey wrote: > On Sat, Jan 19, 2008 at 04:22:50PM +0100, Grégoire Barbier wrote: > > Since HTTP/302 is not handled in the git code calling curl, URLs > > without leading / used to lead to frozen git-fetch or git-push with no > > error message. > > > > Furthermore, http-push freeze forces the user to interrupt it (^C) and > > therefore to leave a dandling webdav lock that makes the remote > > repository un-pushable for 10 minutes. > > > > The patch does not make curl calls handle HTTP/302 but instead adds a > > / at the end of URLs that does not have it yet. > > Actually, it would be much better to do just that, i.e. handle HTTP 302 > return codes. I suspect there may be other cases leading to similar dead > locks with other HTTP codes. But that might just be easier to deal with > once my refactoring will be done ;) Independently, it seems a good idea to not try a URL we _know_ will fail (read: redirect). Ciao, Dscho ``` ## Junio C Hamano, 2008-01-20 23:00 Subject: Re: [PATCH] http-push: more explicit error message with bad URL or password Message-ID: <7v63xo6r15.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v63xo6r15.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1200756171-11696-3-git-send-email-gb@gbarbier.org> ``` Grégoire Barbier writes: > Previously, when URL or password where not set correctly (or > when some network errors occur), the error message was "no DAV > locking support". The standard "Sign-off?" comment aside,... I think something like this would be much less invasive and more to the point. -- >8 -- http-push: clarify the reason of error from the initial PROPFIND request The first thing http-push does is a PROPFIND to see if the other end supports locking. The failure message we give is always reported as "no DAV locking support at the remote repository", regardless of the reason why we ended up not finding the locking support on the other end. This moves the code to report "no DAV locking support" down the codepath so that the message is issued only when we successfully get a response to PROPFIND and the other end say it does not support locking. Other failures, such as connectivity glitches and credential mismatches, have their own error message issued and we will not issue "no DAV locking" error (we do not even know if the remote end supports it). --- http-push.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-) diff --git a/http-push.c b/http-push.c index eef7674..9f92cc1 100644 --- a/http-push.c +++ b/http-push.c @@ -1563,9 +1563,17 @@ static int locking_available(void) lock_flags = 0; } XML_ParserFree(parser); + if (!lock_flags) + error("Error: no DAV locking support on %s", + remote->url); + + } else { + error("Cannot access URL %s, return code %d", + remote->url, results.curl_result); + lock_flags = 0; } } else { - fprintf(stderr, "Unable to start PROPFIND request\n"); + error("Unable to start PROPFIND request on %s", remote->url); } strbuf_release(&out_buffer.buf); @@ -2230,7 +2238,6 @@ int main(int argc, char **argv) /* Verify DAV compliance/lock support */ if (!locking_available()) { - fprintf(stderr, "Error: no DAV locking support on remote repo %s\n", remote->url); rc = 1; goto cleanup; } ``` ## Junio C Hamano, 2008-01-21 00:13 Subject: Re: [PATCH] added #define DEFAULT_MAX_REQUESTS for USE_CURL_MULTI mode Message-ID: <7v1w8c6nnm.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1w8c6nnm.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1200756171-11696-5-git-send-email-gb@gbarbier.org> ``` Grégoire Barbier writes: > From: Gregoire Barbier > > I'm not sure of which value is the good one, but 4 seems good since it's > not very high which would lead to resouce consumption problems. > --- > http.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/http.c b/http.c > index 8b04ae9..7b1bcb8 100644 > --- a/http.c > +++ b/http.c > @@ -4,6 +4,7 @@ int data_received; > int active_requests = 0; > > #ifdef USE_CURL_MULTI > +#define DEFAULT_MAX_REQUESTS 4 > static int max_requests = -1; > static CURLM *curlm; > #endif Why is this needed? How does this interact with an existing #define in http.h that defines it to 5? ``` ## Grégoire Barbier, 2008-01-21 09:57 Subject: Re: [PATCH] added #define DEFAULT_MAX_REQUESTS for USE_CURL_MULTI mode Message-ID: <47946C95.4080607@gbarbier.org> URL: https://gitlist.dev/e/47946C95.4080607%40gbarbier.org In-Reply-To: <7v1w8c6nnm.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano a écrit : > Grégoire Barbier writes: > >> From: Gregoire Barbier >> >> I'm not sure of which value is the good one, but 4 seems good since it's >> not very high which would lead to resouce consumption problems. >> --- >> http.c | 1 + >> 1 files changed, 1 insertions(+), 0 deletions(-) >> >> diff --git a/http.c b/http.c >> index 8b04ae9..7b1bcb8 100644 >> --- a/http.c >> +++ b/http.c >> @@ -4,6 +4,7 @@ int data_received; >> int active_requests = 0; >> >> #ifdef USE_CURL_MULTI >> +#define DEFAULT_MAX_REQUESTS 4 >> static int max_requests = -1; >> static CURLM *curlm; >> #endif >> > > Why is this needed? > > How does this interact with an existing #define in http.h that > defines it to 5? > Ok, please forget my patch and forgive my stupidity, I've juste realized why I needed to define -DUSE_CURL_MULTI by hand in the Makefile, I'm so stupid... I will answers you other mail and Johannes' later. -- Grégoire Barbier - gb à gbarbier.org - +33 6 21 35 73 49 ``` ## Junio C Hamano, 2008-01-21 10:19 Subject: Re: [PATCH] added #define DEFAULT_MAX_REQUESTS for USE_CURL_MULTI mode Message-ID: <7vr6gbzdk7.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr6gbzdk7.fsf%40gitster.siamese.dyndns.org In-Reply-To: <47946C95.4080607@gbarbier.org> ``` Grégoire Barbier writes: > Ok, please forget my patch and forgive my stupidity, I've juste > realized why I needed to define -DUSE_CURL_MULTI by hand in the > Makefile, I'm so stupid... No, it is not your stupidity. The http.h and http-*.c files assume that USE_CURL_MULTI is internal and do not want the user to define that symbol from outside. It should be better documented and mistakes should be prevented. Perhaps we need something like like this. diff --git a/http.h b/http.h index aeba930..046b17f 100644 --- a/http.h +++ b/http.h @@ -8,6 +8,12 @@ #include "strbuf.h" +/* + * We detect based on the cURL version if multi-transfer is + * usable in this implementation and define this symbol accordingly. + */ +#undef USE_CURL_MULTI + #if LIBCURL_VERSION_NUM >= 0x071000 #define USE_CURL_MULTI #define DEFAULT_MAX_REQUESTS 5 ```