* [PATCH 1/3] http-push: append slash if possible for directories @ 2009-01-17 2:53 Ray Chuan 2009-01-17 5:19 ` Johannes Schindelin 2009-01-17 6:02 ` Johannes Schindelin 0 siblings, 2 replies; 5+ messages in thread From: Ray Chuan @ 2009-01-17 2:53 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] http-push: append slash if possible for directories 2009-01-17 2:53 [PATCH 1/3] http-push: append slash if possible for directories Ray Chuan @ 2009-01-17 5:19 ` Johannes Schindelin 2009-01-17 6:02 ` Johannes Schindelin 1 sibling, 0 replies; 5+ messages in thread From: Johannes Schindelin @ 2009-01-17 5:19 UTC (permalink / raw) To: Ray Chuan; +Cc: git 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] http-push: append slash if possible for directories 2009-01-17 2:53 [PATCH 1/3] http-push: append slash if possible for directories Ray Chuan 2009-01-17 5:19 ` Johannes Schindelin @ 2009-01-17 6:02 ` Johannes Schindelin 2009-01-17 8:28 ` Ray Chuan 1 sibling, 1 reply; 5+ messages in thread From: Johannes Schindelin @ 2009-01-17 6:02 UTC (permalink / raw) To: Ray Chuan; +Cc: git 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] http-push: append slash if possible for directories 2009-01-17 6:02 ` Johannes Schindelin @ 2009-01-17 8:28 ` Ray Chuan 2009-01-17 15:11 ` [PATCH] http-push: when making directories, have a trailing slash in the path name Johannes Schindelin 0 siblings, 1 reply; 5+ messages in thread From: Ray Chuan @ 2009-01-17 8:28 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] http-push: when making directories, have a trailing slash in the path name 2009-01-17 8:28 ` Ray Chuan @ 2009-01-17 15:11 ` Johannes Schindelin 0 siblings, 0 replies; 5+ messages in thread From: Johannes Schindelin @ 2009-01-17 15:11 UTC (permalink / raw) To: Ray Chuan; +Cc: git 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-01-17 15:12 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-01-17 2:53 [PATCH 1/3] http-push: append slash if possible for directories Ray Chuan 2009-01-17 5:19 ` Johannes Schindelin 2009-01-17 6:02 ` Johannes Schindelin 2009-01-17 8:28 ` Ray Chuan 2009-01-17 15:11 ` [PATCH] http-push: when making directories, have a trailing slash in the path name Johannes Schindelin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).