threads / patch / 25783

patchNormalise directory names when pushing to some WebDAV servers

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

## tl;dr

15 messages between Nov 19, 2010 and Nov 20, 2010. Diffs are folded; open one to read it.

replies: 14people: 4as markdown or json

Gabriel Corona· Nov 19, 2010, 00:06 UTC · lore

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(-)
Show changes to http-push.c +13 −0
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
Tay Ray Chuan· Nov 19, 2010, 04:02 UTC · re: Gabriel Corona · lore

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

On Fri, Nov 19, 2010 at 8:06 AM, Gabriel Corona <gabriel.corona@enst-bretagne.fr> wrote:

Show 35 quoted lines
>  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--
Show changes to http-push.c +5 −0
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
Jonathan Nieder· Nov 19, 2010, 05:10 UTC · re: Tay Ray Chuan · lore

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

Tay Ray Chuan wrote:
Show 16 quoted lines
> --- 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

Tay Ray Chuan· Nov 19, 2010, 12:46 UTC · re: Jonathan Nieder · lore

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

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

Tay Ray Chuan· Nov 19, 2010, 12:46 UTC · re: Tay Ray Chuan · lore

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

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(-)
Show changes to 5 files +11 −9

Makefile, http.c, http.h, url.c, url.h

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
Tay Ray Chuan· Nov 19, 2010, 12:46 UTC · re: Tay Ray Chuan · lore

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

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

url.c, url.h

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
Tay Ray Chuan· Nov 19, 2010, 12:46 UTC · re: Tay Ray Chuan · lore

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

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(-)
Show changes to http-push.c +4 −0
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
Tay Ray Chuan· Nov 19, 2010, 12:46 UTC · re: Tay Ray Chuan · lore

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

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 http-backend.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
Show changes to http-backend.c +1 −3
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
Tay Ray Chuan· Nov 19, 2010, 12:46 UTC · re: Tay Ray Chuan · lore

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

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(-)
Show changes to http-push.c +8 −2
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
Tay Ray Chuan· Nov 19, 2010, 12:46 UTC · re: Tay Ray Chuan · lore

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

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(-)
Show changes to http-push.c +2 −13
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
Jonathan Nieder· Nov 19, 2010, 18:33 UTC · re: Tay Ray Chuan · lore

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

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.
[...]
Show 11 quoted lines
> --- 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...

Show 5 quoted lines
> +					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.

Tay Ray Chuan· Nov 20, 2010, 04:28 UTC · re: Jonathan Nieder · lore

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

Hi,
On Sat, Nov 20, 2010 at 2:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 12 quoted lines
> [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
Jonathan Nieder· Nov 19, 2010, 19:18 UTC · re: Tay Ray Chuan · lore

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

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.

Junio C Hamano· Nov 19, 2010, 22:54 UTC · re: Tay Ray Chuan · lore

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

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?

Tay Ray Chuan· Nov 20, 2010, 04:32 UTC · re: Junio C Hamano · lore

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

Hi,
On Sat, Nov 20, 2010 at 6:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> 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

← back to recent threads