[PATCH] Normalise directory names when pushing to some WebDAV servers

[PATCH] Normalise directory names when pushing to some WebDAV servers

[Gabriel Corona]

Fix a bug when pushing to WebDAV servers which do not use a trailing
slash for collection names. The previous implementation fails to see
that the requested resource "refs/" is the same resource as "refs"
and loads every reference twice (once for refs/ and once for refs).

This implementation normalises every collection name by appending a
trailing slash if necessary.

This can be tested with old versions of Apache (such as the WebDAV
server of GMX, Apache 2.0.63).
---
 http-push.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/http-push.c b/http-push.c
index c9bcd11..aeb37ab 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1083,6 +1083,18 @@ static void process_ls_ref(struct remote_ls_ctx *ls)
 		one_remote_ref(ls->dentry_name);
 }
 
+static void normalize_dirname(char** name) {
+	char* res;
+	int len = strlen(*name);
+	if((*name)[len-1]=='/') return;
+	res = malloc(len+2);
+	strcpy(res, *name);
+	res[len]='/';
+	res[len+1]='\0';
+	free(*name);
+	*name = res;
+}
+
 static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
 {
 	struct remote_ls_ctx *ls = (struct remote_ls_ctx *)ctx->userData;
@@ -1090,6 +1102,7 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
 	if (tag_closed) {
 		if (!strcmp(ctx->name, DAV_PROPFIND_RESP) && ls->dentry_name) {
 			if (ls->dentry_flags & IS_DIR) {
+				normalize_dirname(&ls->dentry_name);
 				if (ls->flags & PROCESS_DIRS) {
 					ls->userFunc(ls);
 				}
-- 
1.7.2.3

Re: [PATCH] Normalise directory names when pushing to some WebDAV servers

[Tay Ray Chuan]

On Fri, Nov 19, 2010 at 8:06 AM, Gabriel Corona
<gabriel.corona@enst-bretagne.fr> wrote:
>  http-push.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/http-push.c b/http-push.c
> index c9bcd11..aeb37ab 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1083,6 +1083,18 @@ static void process_ls_ref(struct remote_ls_ctx *ls)
>                one_remote_ref(ls->dentry_name);
>  }
>
> +static void normalize_dirname(char** name) {
> +       char* res;
> +       int len = strlen(*name);
> +       if((*name)[len-1]=='/') return;
> +       res = malloc(len+2);
> +       strcpy(res, *name);
> +       res[len]='/';
> +       res[len+1]='\0';
> +       free(*name);
> +       *name = res;
> +}
> +
>  static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
>  {
>        struct remote_ls_ctx *ls = (struct remote_ls_ctx *)ctx->userData;
> @@ -1090,6 +1102,7 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
>        if (tag_closed) {
>                if (!strcmp(ctx->name, DAV_PROPFIND_RESP) && ls->dentry_name) {
>                        if (ls->dentry_flags & IS_DIR) {
> +                               normalize_dirname(&ls->dentry_name);
>                                if (ls->flags & PROCESS_DIRS) {
>                                        ls->userFunc(ls);
>                                }
> --

Hmm, we already have the logic to do this in end_url_with_slash(). It
works on strbufs though. How about this?

-->8--

diff --git a/http-push.c b/http-push.c
index c9bcd11..645c2b5 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1086,10 +1086,15 @@ static void process_ls_ref(struct remote_ls_ctx *ls)
 static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
 {
        struct remote_ls_ctx *ls = (struct remote_ls_ctx *)ctx->userData;
+       struct strbuf buf = STRBUF_INIT;

        if (tag_closed) {
                if (!strcmp(ctx->name, DAV_PROPFIND_RESP) && ls->dentry_name) {
                        if (ls->dentry_flags & IS_DIR) {
+                               end_url_with_slash(&buf, ls->dentry_name);
+                               free(ls->dentry_name);
+                               ls->dentry_name = strbuf_detach(&buf, NULL);
+
                                if (ls->flags & PROCESS_DIRS) {
                                        ls->userFunc(ls);
                                }

-- 
Cheers,
Ray Chuan

Re: [PATCH] Normalise directory names when pushing to some WebDAV servers

[Jonathan Nieder]

Tay Ray Chuan wrote:

> --- a/http-push.c
> +++ b/http-push.c
> @@ -1086,10 +1086,15 @@ static void process_ls_ref(struct remote_ls_ctx *ls)
>  static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
>  {
>         struct remote_ls_ctx *ls = (struct remote_ls_ctx *)ctx->userData;
> +       struct strbuf buf = STRBUF_INIT;
> 
>         if (tag_closed) {
>                 if (!strcmp(ctx->name, DAV_PROPFIND_RESP) && ls->dentry_name) {
>                         if (ls->dentry_flags & IS_DIR) {
> +                               end_url_with_slash(&buf, ls->dentry_name);
> +                               free(ls->dentry_name);
> +                               ls->dentry_name = strbuf_detach(&buf, NULL);
> +
>                                 if (ls->flags & PROCESS_DIRS) {

Nit: might make sense to narrow the scope of buf, to avoid tempting
people to reuse or release it.

Thanks,
Jonathan

[PATCH 0/6] http (dumb): end urls with slash

[Tay Ray Chuan]

This patch series contains Gabriel's fix, as well as some related work I did in
the area while looking through the code paths.

Contents:
--
[PATCH 1/6] shift end_url_with_slash() from http.[ch] to url.[ch]
[PATCH 2/6] url: add str wrapper for end_url_with_slash()
[PATCH 3/6] http-push: Normalise directory names when pushing to some WebDAV servers
[PATCH 4/6] http-backend: use end_url_with_slash()
[PATCH 5/6] http-push: check path length before using it
[PATCH 6/6] http-push: add trailing slash at arg-parse time, instead of later on

--
1.7.3.67.g2a10b

[PATCH 1/6] shift end_url_with_slash() from http.[ch] to url.[ch]

[Tay Ray Chuan]

This allows non-http/curl users to access it too (eg. http-backend.c).

Update include headers in end_url_with_slash() users too.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 Makefile |    2 +-
 http.c   |    8 +-------
 http.h   |    1 -
 url.c    |    7 +++++++
 url.h    |    2 ++
 5 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index ca1d1fe..e02b0f4 100644
--- a/Makefile
+++ b/Makefile
@@ -1922,7 +1922,7 @@ builtin/tar-tree.o archive-tar.o: tar.h
 builtin/pack-objects.o: thread-utils.h
 connect.o transport.o http-backend.o: url.h
 http-fetch.o http-walker.o remote-curl.o transport.o walker.o: walker.h
-http.o http-walker.o http-push.o http-fetch.o remote-curl.o: http.h
+http.o http-walker.o http-push.o http-fetch.o remote-curl.o: http.h url.h
 
 xdiff-interface.o $(XDIFF_OBJS): \
 	xdiff/xinclude.h xdiff/xmacros.h xdiff/xdiff.h xdiff/xtypes.h \
diff --git a/http.c b/http.c
index 17bcf19..310dd86 100644
--- a/http.c
+++ b/http.c
@@ -2,6 +2,7 @@
 #include "pack.h"
 #include "sideband.h"
 #include "run-command.h"
+#include "url.h"
 
 int data_received;
 int active_requests;
@@ -733,13 +734,6 @@ static inline int hex(int v)
 		return 'A' + v - 10;
 }
 
-void end_url_with_slash(struct strbuf *buf, const char *url)
-{
-	strbuf_addstr(buf, url);
-	if (buf->len && buf->buf[buf->len - 1] != '/')
-		strbuf_addstr(buf, "/");
-}
-
 static char *quote_ref_url(const char *base, const char *ref)
 {
 	struct strbuf buf = STRBUF_INIT;
diff --git a/http.h b/http.h
index 173f74c..10de0a5 100644
--- a/http.h
+++ b/http.h
@@ -117,7 +117,6 @@ extern void append_remote_object_url(struct strbuf *buf, const char *url,
 				     int only_two_digit_prefix);
 extern char *get_remote_object_url(const char *url, const char *hex,
 				   int only_two_digit_prefix);
-extern void end_url_with_slash(struct strbuf *buf, const char *url);
 
 /* Options for http_request_*() */
 #define HTTP_NO_CACHE		1
diff --git a/url.c b/url.c
index cd8f74f..7cebc64 100644
--- a/url.c
+++ b/url.c
@@ -125,3 +125,10 @@ char *url_decode_parameter_value(const char **query)
 	struct strbuf out = STRBUF_INIT;
 	return url_decode_internal(query, "&", &out, 1);
 }
+
+void end_url_with_slash(struct strbuf *buf, const char *url)
+{
+	strbuf_addstr(buf, url);
+	if (buf->len && buf->buf[buf->len - 1] != '/')
+		strbuf_addstr(buf, "/");
+}
diff --git a/url.h b/url.h
index 15817f8..8cb74d4 100644
--- a/url.h
+++ b/url.h
@@ -7,4 +7,6 @@ extern char *url_decode(const char *url);
 extern char *url_decode_parameter_name(const char **query);
 extern char *url_decode_parameter_value(const char **query);
 
+extern void end_url_with_slash(struct strbuf *buf, const char *url);
+
 #endif /* URL_H */
-- 
1.7.3.67.g2a10b

[PATCH 2/6] url: add str wrapper for end_url_with_slash()

[Tay Ray Chuan]

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 url.c |    8 ++++++++
 url.h |    1 +
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/url.c b/url.c
index 7cebc64..138396c 100644
--- a/url.c
+++ b/url.c
@@ -132,3 +132,11 @@ void end_url_with_slash(struct strbuf *buf, const char *url)
 	if (buf->len && buf->buf[buf->len - 1] != '/')
 		strbuf_addstr(buf, "/");
 }
+
+void str_end_url_with_slash(const char *url, char **dest) {
+	struct strbuf buf = STRBUF_INIT;
+	end_url_with_slash(&buf, url);
+	if (dest)
+		free(*dest);
+	*dest = strbuf_detach(&buf, NULL);
+}
diff --git a/url.h b/url.h
index 8cb74d4..7100e32 100644
--- a/url.h
+++ b/url.h
@@ -8,5 +8,6 @@ extern char *url_decode_parameter_name(const char **query);
 extern char *url_decode_parameter_value(const char **query);
 
 extern void end_url_with_slash(struct strbuf *buf, const char *url);
+extern void str_end_url_with_slash(const char *url, char **dest);
 
 #endif /* URL_H */
-- 
1.7.3.67.g2a10b

[PATCH 3/6] http-push: Normalise directory names when pushing to some WebDAV servers

[Tay Ray Chuan]

Fix a bug when pushing to WebDAV servers which do not use a trailing
slash for collection names. The previous implementation fails to see
that the requested resource "refs/" is the same resource as "refs"
and loads every reference twice (once for refs/ and once for refs).

This implementation normalises every collection name by appending a
trailing slash if necessary.

This can be tested with old versions of Apache (such as the WebDAV
server of GMX, Apache 2.0.63).

Based-on-patch-by: Gabriel Corona <gabriel.corona@enst-bretagne.fr>
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 http-push.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/http-push.c b/http-push.c
index c9bcd11..565e580 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1090,6 +1090,10 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
 	if (tag_closed) {
 		if (!strcmp(ctx->name, DAV_PROPFIND_RESP) && ls->dentry_name) {
 			if (ls->dentry_flags & IS_DIR) {
+
+				/* ensure collection names end with slash */
+				str_end_url_with_slash(ls->dentry_name, &ls->dentry_name);
+
 				if (ls->flags & PROCESS_DIRS) {
 					ls->userFunc(ls);
 				}
-- 
1.7.3.67.g2a10b

[PATCH 4/6] http-backend: use end_url_with_slash()

[Tay Ray Chuan]

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 http-backend.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index 14c90c2..8501504 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -510,9 +510,7 @@ static char* getdir(void)
 			die("GIT_PROJECT_ROOT is set but PATH_INFO is not");
 		if (daemon_avoid_alias(pathinfo))
 			die("'%s': aliased", pathinfo);
-		strbuf_addstr(&buf, root);
-		if (buf.buf[buf.len - 1] != '/')
-			strbuf_addch(&buf, '/');
+		end_url_with_slash(&buf, root);
 		if (pathinfo[0] == '/')
 			pathinfo++;
 		strbuf_addstr(&buf, pathinfo);
-- 
1.7.3.67.g2a10b

[PATCH 5/6] http-push: check path length before using it

[Tay Ray Chuan]

We use path_len to skip the base url/path, but we do not know for sure
if path does indeed contain the base url/path. Check if this is so.

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

diff --git a/http-push.c b/http-push.c
index 565e580..1c34b00 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1116,8 +1116,14 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
 				}
 			}
 			if (path) {
-				path += repo->path_len;
-				ls->dentry_name = xstrdup(path);
+				if (strncmp(path,
+					    (repo->path ? repo->path : repo->url),
+					    repo->path_len) == 0) {
+					path += repo->path_len;
+					ls->dentry_name = xstrdup(path);
+				} else
+					error("Parsed path '%s' does not match url: '%s'\n",
+					      path, (repo->path ? repo->path : repo->url));
 			}
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_COLLECTION)) {
 			ls->dentry_flags |= IS_DIR;
-- 
1.7.3.67.g2a10b

[PATCH 6/6] http-push: add trailing slash at arg-parse time, instead of later on

[Tay Ray Chuan]

That way, we don't have to update repo->path and repo->path_len again
after adding the trailing slash.

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

diff --git a/http-push.c b/http-push.c
index 1c34b00..a10891e 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1799,7 +1799,6 @@ int main(int argc, char **argv)
 	int new_refs;
 	struct ref *ref, *local_refs;
 	struct remote *remote;
-	char *rewritten_url = NULL;
 
 	git_extract_argv0_path(argv[0]);
 
@@ -1845,8 +1844,8 @@ int main(int argc, char **argv)
 		}
 		if (!repo->url) {
 			char *path = strstr(arg, "//");
-			repo->url = arg;
-			repo->path_len = strlen(arg);
+			str_end_url_with_slash(arg, &repo->url);
+			repo->path_len = strlen(repo->url);
 			if (path) {
 				repo->path = strchr(path+2, '/');
 				if (repo->path)
@@ -1882,15 +1881,6 @@ int main(int argc, char **argv)
 	remote->url[remote->url_nr++] = repo->url;
 	http_init(remote);
 
-	if (repo->url && repo->url[strlen(repo->url)-1] != '/') {
-		rewritten_url = xmalloc(strlen(repo->url)+2);
-		strcpy(rewritten_url, repo->url);
-		strcat(rewritten_url, "/");
-		repo->path = rewritten_url + (repo->path - repo->url);
-		repo->path_len++;
-		repo->url = rewritten_url;
-	}
-
 #ifdef USE_CURL_MULTI
 	is_running_queue = 0;
 #endif
@@ -2098,7 +2088,6 @@ int main(int argc, char **argv)
 	}
 
  cleanup:
-	free(rewritten_url);
 	if (info_ref_lock)
 		unlock_remote(info_ref_lock);
 	free(repo);
-- 
1.7.3.67.g2a10b

Re: [PATCH 5/6] http-push: check path length before using it

[Jonathan Nieder]

Tay Ray Chuan wrote:

> We use path_len to skip the base url/path, but we do not know for sure
> if path does indeed contain the base url/path. Check if this is so.
[...]
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1116,8 +1116,14 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
>  				}
>  			}
>  			if (path) {
> -				path += repo->path_len;
> -				ls->dentry_name = xstrdup(path);
> +				if (strncmp(path,
> +					    (repo->path ? repo->path : repo->url),
> +					    repo->path_len) == 0) {

Nits: the usual idiom is

 if (!strncmp(path, ...

to make it is obvious from looking at the left side that this is a
test for equality.  The parentheses around the second parameter are
not needed here...


> +					path += repo->path_len;
> +					ls->dentry_name = xstrdup(path);
> +				} else
> +					error("Parsed path '%s' does not match url: '%s'\n",
> +					      path, (repo->path ? repo->path : repo->url));

... or here.  So perhaps:

			if (path) {
				const char *url = repo->path;
				if (!url)
					url = repo->url;
				if (strncmp(path, url, repo->path_len)) {
					error("Parsed path '%s' does not match url: '%s'\n",
						path, url);
				} else {
					path += ...

By the way, is the error behavior correct?  This prints a message with
"error: " and then continues anyway.

Re: [PATCH 5/6] http-push: check path length before using it

[Tay Ray Chuan]

Hi,

On Sat, Nov 20, 2010 at 2:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> [snip]
> ... or here.  So perhaps:
>
>                        if (path) {
>                                const char *url = repo->path;
>                                if (!url)
>                                        url = repo->url;
>                                if (strncmp(path, url, repo->path_len)) {
>                                        error("Parsed path '%s' does not match url: '%s'\n",
>                                                path, url);
>                                } else {
>                                        path += ...

Looks much cleaner, thanks.

> By the way, is the error behavior correct?  This prints a message with
> "error: " and then continues anyway.

Yeah, this fits in with the exiting xml code paths - they just
indicate errors and continue.

-- 
Cheers,
Ray Chuan

Re: [PATCH 0/6] http (dumb): end urls with slash

[Jonathan Nieder]

Hi Ray Chuan,

Tay Ray Chuan wrote:

> This patch series contains Gabriel's fix, as well as some related work I did in
> the area while looking through the code paths.

Thanks!  The code is quite readable.  I don't know enough to say
anything about the functional changes, but I assume you have that
covered.

Re: [PATCH 0/6] http (dumb): end urls with slash

[Junio C Hamano]

Didn't find anything suspicious during my cursory read (except for the
ones Jonathan pointed out, which I missed), but how does this series
relate to your earlier commits around d8fab07 (remote-curl: ensure that
URLs have a trailing slash, 2010-04-08)?  Same topic, tying loose ends?
Same topic, different approach?  Unrelated changes with similar-sounding
names?

Re: [PATCH 0/6] http (dumb): end urls with slash

[Tay Ray Chuan]

Hi,

On Sat, Nov 20, 2010 at 6:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Didn't find anything suspicious during my cursory read (except for the
> ones Jonathan pointed out, which I missed), but how does this series
> relate to your earlier commits around d8fab07 (remote-curl: ensure that
> URLs have a trailing slash, 2010-04-08)?  Same topic, tying loose ends?
> Same topic, different approach?  Unrelated changes with similar-sounding
> names?

Johnathan and Junio, thanks for taking the time.

Sorry for the confusion. Well, it's all about slashes, hence the
title, but there are some refactors and related work in the http area
in this series.

-- 
Cheers,
Ray Chuan