# [PATCH] Normalise directory names when pushing to some WebDAV servers 15 messages from 2010-11-19 to 2010-11-20. Participants: Gabriel Corona, Tay Ray Chuan, Jonathan Nieder, Junio C Hamano. Thread: https://gitlist.dev/t/25783 ## Gabriel Corona, 2010-11-19 00:06 Subject: [PATCH] Normalise directory names when pushing to some WebDAV servers Message-ID: <1290125163-31065-1-git-send-email-gabriel.corona@enst-bretagne.fr> URL: https://gitlist.dev/e/1290125163-31065-1-git-send-email-gabriel.corona%40enst-bretagne.fr ``` 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 ``` ## Tay Ray Chuan, 2010-11-19 04:02 Subject: Re: [PATCH] Normalise directory names when pushing to some WebDAV servers Message-ID: URL: https://gitlist.dev/e/AANLkTikM8mNv%2BGiVyDWEJxsf0o3FYaaJj%2BjdYyiuXnov%40mail.gmail.com In-Reply-To: <1290125163-31065-1-git-send-email-gabriel.corona@enst-bretagne.fr> ``` On Fri, Nov 19, 2010 at 8:06 AM, Gabriel Corona 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 ``` ## Jonathan Nieder, 2010-11-19 05:10 Subject: Re: [PATCH] Normalise directory names when pushing to some WebDAV servers Message-ID: <20101119051047.GA4201@burratino> URL: https://gitlist.dev/e/20101119051047.GA4201%40burratino In-Reply-To: ``` 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 ``` ## Tay Ray Chuan, 2010-11-19 12:46 Subject: [PATCH 0/6] http (dumb): end urls with slash Message-ID: <1290170790-2200-1-git-send-email-rctay89@gmail.com> URL: https://gitlist.dev/e/1290170790-2200-1-git-send-email-rctay89%40gmail.com In-Reply-To: <20101119051047.GA4201@burratino> ``` 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, 2010-11-19 12:46 Subject: [PATCH 1/6] shift end_url_with_slash() from http.[ch] to url.[ch] Message-ID: <1290170790-2200-2-git-send-email-rctay89@gmail.com> URL: https://gitlist.dev/e/1290170790-2200-2-git-send-email-rctay89%40gmail.com In-Reply-To: <1290170790-2200-1-git-send-email-rctay89@gmail.com> ``` 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 --- 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 ``` ## Tay Ray Chuan, 2010-11-19 12:46 Subject: [PATCH 2/6] url: add str wrapper for end_url_with_slash() Message-ID: <1290170790-2200-3-git-send-email-rctay89@gmail.com> URL: https://gitlist.dev/e/1290170790-2200-3-git-send-email-rctay89%40gmail.com In-Reply-To: <1290170790-2200-2-git-send-email-rctay89@gmail.com> ``` Signed-off-by: Tay Ray Chuan --- 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 ``` ## Tay Ray Chuan, 2010-11-19 12:46 Subject: [PATCH 3/6] http-push: Normalise directory names when pushing to some WebDAV servers Message-ID: <1290170790-2200-4-git-send-email-rctay89@gmail.com> URL: https://gitlist.dev/e/1290170790-2200-4-git-send-email-rctay89%40gmail.com In-Reply-To: <1290170790-2200-3-git-send-email-rctay89@gmail.com> ``` 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 Signed-off-by: Tay Ray Chuan --- 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 ``` ## Tay Ray Chuan, 2010-11-19 12:46 Subject: [PATCH 4/6] http-backend: use end_url_with_slash() Message-ID: <1290170790-2200-5-git-send-email-rctay89@gmail.com> URL: https://gitlist.dev/e/1290170790-2200-5-git-send-email-rctay89%40gmail.com In-Reply-To: <1290170790-2200-4-git-send-email-rctay89@gmail.com> ``` Signed-off-by: Tay Ray Chuan --- 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 ``` ## Tay Ray Chuan, 2010-11-19 12:46 Subject: [PATCH 5/6] http-push: check path length before using it Message-ID: <1290170790-2200-6-git-send-email-rctay89@gmail.com> URL: https://gitlist.dev/e/1290170790-2200-6-git-send-email-rctay89%40gmail.com In-Reply-To: <1290170790-2200-5-git-send-email-rctay89@gmail.com> ``` 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 --- 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 ``` ## Tay Ray Chuan, 2010-11-19 12:46 Subject: [PATCH 6/6] http-push: add trailing slash at arg-parse time, instead of later on Message-ID: <1290170790-2200-7-git-send-email-rctay89@gmail.com> URL: https://gitlist.dev/e/1290170790-2200-7-git-send-email-rctay89%40gmail.com In-Reply-To: <1290170790-2200-6-git-send-email-rctay89@gmail.com> ``` 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 --- 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 ``` ## Jonathan Nieder, 2010-11-19 18:33 Subject: Re: [PATCH 5/6] http-push: check path length before using it Message-ID: <20101119183339.GB26187@burratino> URL: https://gitlist.dev/e/20101119183339.GB26187%40burratino In-Reply-To: <1290170790-2200-6-git-send-email-rctay89@gmail.com> ``` 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. ``` ## Jonathan Nieder, 2010-11-19 19:18 Subject: Re: [PATCH 0/6] http (dumb): end urls with slash Message-ID: <20101119191810.GD26187@burratino> URL: https://gitlist.dev/e/20101119191810.GD26187%40burratino In-Reply-To: <1290170790-2200-1-git-send-email-rctay89@gmail.com> ``` 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, 2010-11-19 22:54 Subject: Re: [PATCH 0/6] http (dumb): end urls with slash Message-ID: <7vd3q0c4b3.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vd3q0c4b3.fsf%40alter.siamese.dyndns.org In-Reply-To: <1290170790-2200-1-git-send-email-rctay89@gmail.com> ``` 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, 2010-11-20 04:28 Subject: Re: [PATCH 5/6] http-push: check path length before using it Message-ID: URL: https://gitlist.dev/e/AANLkTikJz%2BSigzqFdeP%2BrZR3FS06U1exDb59AsRFTQAj%40mail.gmail.com In-Reply-To: <20101119183339.GB26187@burratino> ``` Hi, On Sat, Nov 20, 2010 at 2:33 AM, Jonathan Nieder 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 ``` ## Tay Ray Chuan, 2010-11-20 04:32 Subject: Re: [PATCH 0/6] http (dumb): end urls with slash Message-ID: URL: https://gitlist.dev/e/AANLkTimWz4meHUb6o0K8yNMmqyqpWbKrq0OmnzWAS300%40mail.gmail.com In-Reply-To: <7vd3q0c4b3.fsf@alter.siamese.dyndns.org> ``` Hi, On Sat, Nov 20, 2010 at 6:54 AM, Junio C Hamano 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 ```