[PATCH 1/3] http-push: append slash if possible for directories

[PATCH 1/3] http-push: append slash if possible for directories

[Ray Chuan]

the lock_remote currently sends MKCOL requests to leading directories
to make sure they exist; however, it doesn't put a forward slash '/'
behind the path, so if the path is a directory, the server sends a 301
redirect.

by appending a '/' we can save the server this additional step.

in addition, it seems that curl doesn't re-send the authentication
credentials when it follows a 301 redirect, so skipping (unnecessary)
redirects can also be seen as a workaround for this issue. (i'm using
7.16.3)

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 src/git-1.6.1/http-push.c |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/src/git-1.6.1/http-push.c b/src/git-1.6.1/http-push.c
index 7c64609..25b655d 100644
--- a/src/git-1.6.1/http-push.c
+++ b/src/git-1.6.1/http-push.c
@@ -1189,6 +1189,7 @@ static struct remote_lock *lock_remote(const
char *path, long timeout)
 	struct strbuf in_buffer = STRBUF_INIT;
 	char *url;
 	char *ep;
+	char ep_old;
 	char timeout_header[25];
 	struct remote_lock *lock = NULL;
 	struct curl_slist *dav_headers = NULL;
@@ -1198,9 +1199,18 @@ static struct remote_lock *lock_remote(const
char *path, long timeout)
 	sprintf(url, "%s%s", remote->url, path);

 	/* Make sure leading directories exist for the remote ref */
-	ep = strchr(url + strlen(remote->url) + 1, '/');
-	while (ep) {
-		*ep = 0;
+	ep = url + strlen(remote->url) + 1;
+	int has_fs = 0;
+	while (1) {
+		ep = strchr(ep + 1, '/');
+		if(ep) {
+			ep++;
+			ep_old=*ep;
+			*ep = 0;
+			has_fs = 1;
+		} else {
+			break;
+		}
 		slot = get_active_slot();
 		slot->results = &results;
 		curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
@@ -1222,8 +1232,9 @@ static struct remote_lock *lock_remote(const
char *path, long timeout)
 			free(url);
 			return NULL;
 		}
-		*ep = '/';
-		ep = strchr(ep + 1, '/');
+		if(has_fs) {
+			*ep = ep_old;
+		}
 	}

 	strbuf_addf(&out_buffer.buf, LOCK_REQUEST, git_default_email);
-- 
1.6.0.4

Re: [PATCH 1/3] http-push: append slash if possible for directories

[Johannes Schindelin]

Hi,

On Sat, 17 Jan 2009, Ray Chuan wrote:

>  src/git-1.6.1/http-push.c |   21 ++++++++++++++++-----

That is a, uhm, creative way of using Git...

Ciao,
Dscho

Re: [PATCH 1/3] http-push: append slash if possible for directories

[Johannes Schindelin]

Hi,

more comments:

On Sat, 17 Jan 2009, Ray Chuan wrote:

> diff --git a/src/git-1.6.1/http-push.c b/src/git-1.6.1/http-push.c
> index 7c64609..25b655d 100644
> --- a/src/git-1.6.1/http-push.c
> +++ b/src/git-1.6.1/http-push.c
> @@ -1189,6 +1189,7 @@ static struct remote_lock *lock_remote(const
> char *path, long timeout)
>  	struct strbuf in_buffer = STRBUF_INIT;
>  	char *url;
>  	char *ep;
> +	char ep_old;
>  	char timeout_header[25];
>  	struct remote_lock *lock = NULL;
>  	struct curl_slist *dav_headers = NULL;
> @@ -1198,9 +1199,18 @@ static struct remote_lock *lock_remote(const
> char *path, long timeout)
>  	sprintf(url, "%s%s", remote->url, path);
> 
>  	/* Make sure leading directories exist for the remote ref */
> -	ep = strchr(url + strlen(remote->url) + 1, '/');
> -	while (ep) {
> -		*ep = 0;
> +	ep = url + strlen(remote->url) + 1;
> +	int has_fs = 0;

decl-after-statement.

And name-not-meaningful.  What does "has_fs" stand for?

> +	while (1) {
> +		ep = strchr(ep + 1, '/');
> +		if(ep) {
> +			ep++;
> +			ep_old=*ep;

Okay, you succeeded in fooling me.  It took fully five minutes until I 
realized that ep_old is not the old value of ep, but of *ep.

And now I know what has_fs does, but the name is an even bigger puzzle.

Almost as big as the puzzle why you did not do a much less intrusive 
change:

- after the "while (ep) {" you could say "char saved_character = ep[1];

- then you replace the "*ep = 0" by "ep[1] = '\0';"

- at the end of the loop, you replace the "*ep = '/'" with "ep[1] = 
  saved_character;"

That way, not only would the patch be much smaller, it would also not have 
been as difficult to review as it was.

Ciao,
Dscho

Re: [PATCH 1/3] http-push: append slash if possible for directories

[Ray Chuan]

let me rewrite the patch:
----------------------------------------------------
the lock_remote currently sends MKCOL requests to leading directories
to make sure they exist; however, it doesn't put a forward slash '/'
behind the path, so if the path is a directory, the server sends a 301
redirect.

by appending a '/' we can save the server this additional step.

in addition, it seems that curl doesn't re-send the authentication
credentials when it follows a 301 redirect, so skipping (unnecessary)
redirects can also be seen as a workaround for this issue. (i'm using
7.16.3)

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 http-push.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/http-push.c b/http-push.c
index 7c64609..9218c7a 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1187,8 +1187,10 @@ static struct remote_lock *lock_remote(const
char *path, long timeout)
 	struct slot_results results;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
 	struct strbuf in_buffer = STRBUF_INIT;
+	int has_forwardslash = 0;
 	char *url;
 	char *ep;
+	char saved_character;
 	char timeout_header[25];
 	struct remote_lock *lock = NULL;
 	struct curl_slist *dav_headers = NULL;
@@ -1198,9 +1200,16 @@ static struct remote_lock *lock_remote(const
char *path, long timeout)
 	sprintf(url, "%s%s", remote->url, path);

 	/* Make sure leading directories exist for the remote ref */
-	ep = strchr(url + strlen(remote->url) + 1, '/');
-	while (ep) {
-		*ep = 0;
+	ep = url + strlen(remote->url) + 1;
+	while (1) {
+		ep = strchr(ep + 1, '/');
+		if(ep) {
+			saved_character=ep[1];
+			ep[1]=0;
+			has_forwardslash = 1;
+		} else {
+			break;
+		}
 		slot = get_active_slot();
 		slot->results = &results;
 		curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
@@ -1222,8 +1231,9 @@ static struct remote_lock *lock_remote(const
char *path, long timeout)
 			free(url);
 			return NULL;
 		}
-		*ep = '/';
-		ep = strchr(ep + 1, '/');
+		if(has_forwardslash) {
+			ep[1] = saved_character;
+		}
 	}

 	strbuf_addf(&out_buffer.buf, LOCK_REQUEST, git_default_email);
-- 
1.6.0.4

[PATCH] http-push: when making directories, have a trailing slash in the path name

[Johannes Schindelin]

The function lock_remote() sends MKCOL requests to make leading
directories; However, if it does not put a forward slash '/' at the end of
the path, the server sends a 301 redirect.

By leaving the '/' in place, we can avoid this additional step.

Incidentally, at least one version of Curl (7.16.3) does not resend
credentials when it follows a 301 redirect, so this commit also fixes
a bug.

Original patch by Tay Ray Chuan <rctay89@gmail.com>.

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

	On Sat, 17 Jan 2009, Ray Chuan wrote:

	>  http-push.c |   20 +++++++++++++++-----
	>  1 files changed, 15 insertions(+), 5 deletions(-)

	Actually, I had more something like this in mind.  Please note 
	that I am not the best patch writer of the world, and I know it.  
	However, a few things I do try to follow:

	- our commit messages are not really uniform, but they do share a 
	  certain likeliness, e.g. all sentences start with a capital, we 
	  try to use a less personal form than "I", and we avoid 
	  abbreviations like "don't" and write "do not" instead.

	- if you can come up with a patch that is shorter, but still clear 
	  in what it does (the commit message might need to help there), 
	  it is preferred: the less lines changed, the less lines for bugs 
	  to hide in.

	- this patch includes a test case.

	The latter part cannot be stressed enough: my first idea was to 
	test for 301, but I realized then and there that _my_ Apache does not 
	care, but always returns a 201.

	However, I remember that I had an Apache something like 2 years 
	ago which cared, and therefore I think this patch should go in.

 http-push.c             |    5 +++--
 t/lib-httpd/apache.conf |    2 ++
 t/t5540-http-push.sh    |    6 ++++++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/http-push.c b/http-push.c
index bbe7a8e..9fcccee 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1201,7 +1201,8 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	/* Make sure leading directories exist for the remote ref */
 	ep = strchr(url + strlen(remote->url) + 1, '/');
 	while (ep) {
-		*ep = 0;
+		char saved_character = ep[1];
+		ep[1] = '\0';
 		slot = get_active_slot();
 		slot->results = &results;
 		curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
@@ -1223,7 +1224,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 			free(url);
 			return NULL;
 		}
-		*ep = '/';
+		ep[1] = saved_character;
 		ep = strchr(ep + 1, '/');
 	}
 
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 4717c2d..fdb19a5 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -1,6 +1,8 @@
 ServerName dummy
 PidFile httpd.pid
 DocumentRoot www
+LogFormat "%h %l %u %t \"%r\" %>s %b" common
+CustomLog access.log common
 ErrorLog error.log
 
 <IfDefine SSL>
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index da95886..22cfbb6 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -76,6 +76,12 @@ test_expect_failure 'create and delete remote branch' '
 	test_must_fail git show-ref --verify refs/remotes/origin/dev
 '
 
+test_expect_success 'MKCOL sends directory names with trailing slashes' '
+
+	! grep "\"MKCOL.*[^/] HTTP/[^ ]*\"" < "$HTTPD_ROOT_PATH"/access.log
+
+'
+
 stop_httpd
 
 test_done
-- 
1.6.1.325.g062d4