[PATCH] http-push: fix webdav lock leak.

[PATCH] http-push: fix webdav lock leak.

[Grégoire Barbier]

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 <gb@gbarbier.org>
---
 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

[PATCH] http-push: fail when info/refs exists and is already locked

[Grégoire Barbier]

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

[PATCH] http-push: more explicit error message with bad URL or password

[Grégoire Barbier]

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

[PATCH] http-push and http-fetch: handle URLs without leading /

[Grégoire Barbier]

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

[PATCH] added #define DEFAULT_MAX_REQUESTS for USE_CURL_MULTI mode

[Grégoire Barbier]

From: Gregoire Barbier <gb@gbarbier.org>

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

Re: [PATCH] added #define DEFAULT_MAX_REQUESTS for USE_CURL_MULTI mode

[Junio C Hamano]

Grégoire Barbier <gb@gbarbier.org> writes:

> From: Gregoire Barbier <gb@gbarbier.org>
>
> 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?

Re: [PATCH] added #define DEFAULT_MAX_REQUESTS for USE_CURL_MULTI mode

[Grégoire Barbier]

Junio C Hamano a écrit :
> Grégoire Barbier <gb@gbarbier.org> writes:
>   
>> From: Gregoire Barbier <gb@gbarbier.org>
>>
>> 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

Re: [PATCH] added #define DEFAULT_MAX_REQUESTS for USE_CURL_MULTI mode

[Junio C Hamano]

Grégoire Barbier <gb@gbarbier.org> 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

Re: [PATCH] http-push and http-fetch: handle URLs without leading /

[Mike Hommey]

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

Re: [PATCH] http-push and http-fetch: handle URLs without leading /

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 977 bytes --]

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

Re: [PATCH] http-push and http-fetch: handle URLs without leading /

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1275 bytes --]

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

Re: [PATCH] http-push: more explicit error message with bad URL or password

[Junio C Hamano]

Grégoire Barbier <gb@gbarbier.org> 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;
 	}

Re: [PATCH] http-push: fix webdav lock leak.

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1260 bytes --]

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