{"thread":{"id":"17218","subject":"[PATCH 1/3] http-push: append slash if possible for directories","startedAt":"2009-01-17T02:53:14Z","lastAt":"2009-01-17T15:11:51Z","messageCount":5,"participants":["Ray Chuan","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"100790","messageId":"be6fef0d0901161853y6784310dgf94dadb2218001a1@mail.gmail.com","threadId":"17218","inReplyTo":null,"subject":"[PATCH 1/3] http-push: append slash if possible for directories","fromName":"Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-01-17T02:53:14Z","receivedAt":"2009-01-17T02:53:14Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"the lock_remote currently sends MKCOL requests to leading directories\nto make sure they exist; however, it doesn't put a forward slash '/'\nbehind the path, so if the path is a directory, the server sends a 301\nredirect.\n\nby appending a '/' we can save the server this additional step.\n\nin addition, it seems that curl doesn't re-send the authentication\ncredentials when it follows a 301 redirect, so skipping (unnecessary)\nredirects can also be seen as a workaround for this issue. (i'm using\n7.16.3)\n\nSigned-off-by: Tay Ray Chuan \n---\n src/git-1.6.1/http-push.c | 21 ++++++++++++++++-----\n 1 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/src/git-1.6.1/http-push.c b/src/git-1.6.1/http-push.c\nindex 7c64609..25b655d 100644\n--- a/src/git-1.6.1/http-push.c\n+++ b/src/git-1.6.1/http-push.c\n@@ -1189,6 +1189,7 @@ static struct remote_lock *lock_remote(const\nchar *path, long timeout)\n \tstruct strbuf in_buffer = STRBUF_INIT;\n \tchar *url;\n \tchar *ep;\n+\tchar ep_old;\n \tchar timeout_header[25];\n \tstruct remote_lock *lock = NULL;\n \tstruct curl_slist *dav_headers = NULL;\n@@ -1198,9 +1199,18 @@ static struct remote_lock *lock_remote(const\nchar *path, long timeout)\n \tsprintf(url, \"%s%s\", remote->url, path);\n\n \t/* Make sure leading directories exist for the remote ref */\n-\tep = strchr(url + strlen(remote->url) + 1, '/');\n-\twhile (ep) {\n-\t\t*ep = 0;\n+\tep = url + strlen(remote->url) + 1;\n+\tint has_fs = 0;\n+\twhile (1) {\n+\t\tep = strchr(ep + 1, '/');\n+\t\tif(ep) {\n+\t\t\tep++;\n+\t\t\tep_old=*ep;\n+\t\t\t*ep = 0;\n+\t\t\thas_fs = 1;\n+\t\t} else {\n+\t\t\tbreak;\n+\t\t}\n \t\tslot = get_active_slot();\n \t\tslot->results = &results;\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n@@ -1222,8 +1232,9 @@ static struct remote_lock *lock_remote(const\nchar *path, long timeout)\n \t\t\tfree(url);\n \t\t\treturn NULL;\n \t\t}\n-\t\t*ep = '/';\n-\t\tep = strchr(ep + 1, '/');\n+\t\tif(has_fs) {\n+\t\t\t*ep = ep_old;\n+\t\t}\n \t}\n\n \tstrbuf_addf(&out_buffer.buf, LOCK_REQUEST, git_default_email);\n-- \n1.6.0.4\n"},{"id":"100804","messageId":"alpine.DEB.1.00.0901170618540.3586@pacific.mpi-cbg.de","threadId":"17218","inReplyTo":"be6fef0d0901161853y6784310dgf94dadb2218001a1@mail.gmail.com","subject":"Re: [PATCH 1/3] http-push: append slash if possible for directories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-17T05:19:56Z","receivedAt":"2009-01-17T05:19:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 17 Jan 2009, Ray Chuan wrote:\n\n> src/git-1.6.1/http-push.c | 21 ++++++++++++++++-----\n\nThat is a, uhm, creative way of using Git...\n\nCiao,\nDscho\n"},{"id":"100808","messageId":"alpine.DEB.1.00.0901170654540.3586@pacific.mpi-cbg.de","threadId":"17218","inReplyTo":"be6fef0d0901161853y6784310dgf94dadb2218001a1@mail.gmail.com","subject":"Re: [PATCH 1/3] http-push: append slash if possible for directories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-17T06:02:44Z","receivedAt":"2009-01-17T06:02:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nmore comments:\n\nOn Sat, 17 Jan 2009, Ray Chuan wrote:\n\n> diff --git a/src/git-1.6.1/http-push.c b/src/git-1.6.1/http-push.c\n> index 7c64609..25b655d 100644\n> --- a/src/git-1.6.1/http-push.c\n> +++ b/src/git-1.6.1/http-push.c\n> @@ -1189,6 +1189,7 @@ static struct remote_lock *lock_remote(const\n> char *path, long timeout)\n> \tstruct strbuf in_buffer = STRBUF_INIT;\n> \tchar *url;\n> \tchar *ep;\n> +\tchar ep_old;\n> \tchar timeout_header[25];\n> \tstruct remote_lock *lock = NULL;\n> \tstruct curl_slist *dav_headers = NULL;\n> @@ -1198,9 +1199,18 @@ static struct remote_lock *lock_remote(const\n> char *path, long timeout)\n> \tsprintf(url, \"%s%s\", remote->url, path);\n> \n> \t/* Make sure leading directories exist for the remote ref */\n> -\tep = strchr(url + strlen(remote->url) + 1, '/');\n> -\twhile (ep) {\n> -\t\t*ep = 0;\n> +\tep = url + strlen(remote->url) + 1;\n> +\tint has_fs = 0;\n\ndecl-after-statement.\n\nAnd name-not-meaningful. What does \"has_fs\" stand for?\n\n> +\twhile (1) {\n> +\t\tep = strchr(ep + 1, '/');\n> +\t\tif(ep) {\n> +\t\t\tep++;\n> +\t\t\tep_old=*ep;\n\nOkay, you succeeded in fooling me. It took fully five minutes until I \nrealized that ep_old is not the old value of ep, but of *ep.\n\nAnd now I know what has_fs does, but the name is an even bigger puzzle.\n\nAlmost as big as the puzzle why you did not do a much less intrusive \nchange:\n\n- after the \"while (ep) {\" you could say \"char saved_character = ep[1];\n\n- then you replace the \"*ep = 0\" by \"ep[1] = '\\0';\"\n\n- at the end of the loop, you replace the \"*ep = '/'\" with \"ep[1] = \n saved_character;\"\n\nThat way, not only would the patch be much smaller, it would also not have \nbeen as difficult to review as it was.\n\nCiao,\nDscho\n"},{"id":"100812","messageId":"be6fef0d0901170028t3b418220wa024880235776037@mail.gmail.com","threadId":"17218","inReplyTo":"alpine.DEB.1.00.0901170654540.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH 1/3] http-push: append slash if possible for directories","fromName":"Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-01-17T08:28:56Z","receivedAt":"2009-01-17T08:28:56Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"let me rewrite the patch:\n----------------------------------------------------\nthe lock_remote currently sends MKCOL requests to leading directories\nto make sure they exist; however, it doesn't put a forward slash '/'\nbehind the path, so if the path is a directory, the server sends a 301\nredirect.\n\nby appending a '/' we can save the server this additional step.\n\nin addition, it seems that curl doesn't re-send the authentication\ncredentials when it follows a 301 redirect, so skipping (unnecessary)\nredirects can also be seen as a workaround for this issue. (i'm using\n7.16.3)\n\nSigned-off-by: Tay Ray Chuan \n---\n http-push.c | 20 +++++++++++++++-----\n 1 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 7c64609..9218c7a 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1187,8 +1187,10 @@ static struct remote_lock *lock_remote(const\nchar *path, long timeout)\n \tstruct slot_results results;\n \tstruct buffer out_buffer = { STRBUF_INIT, 0 };\n \tstruct strbuf in_buffer = STRBUF_INIT;\n+\tint has_forwardslash = 0;\n \tchar *url;\n \tchar *ep;\n+\tchar saved_character;\n \tchar timeout_header[25];\n \tstruct remote_lock *lock = NULL;\n \tstruct curl_slist *dav_headers = NULL;\n@@ -1198,9 +1200,16 @@ static struct remote_lock *lock_remote(const\nchar *path, long timeout)\n \tsprintf(url, \"%s%s\", remote->url, path);\n\n \t/* Make sure leading directories exist for the remote ref */\n-\tep = strchr(url + strlen(remote->url) + 1, '/');\n-\twhile (ep) {\n-\t\t*ep = 0;\n+\tep = url + strlen(remote->url) + 1;\n+\twhile (1) {\n+\t\tep = strchr(ep + 1, '/');\n+\t\tif(ep) {\n+\t\t\tsaved_character=ep[1];\n+\t\t\tep[1]=0;\n+\t\t\thas_forwardslash = 1;\n+\t\t} else {\n+\t\t\tbreak;\n+\t\t}\n \t\tslot = get_active_slot();\n \t\tslot->results = &results;\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n@@ -1222,8 +1231,9 @@ static struct remote_lock *lock_remote(const\nchar *path, long timeout)\n \t\t\tfree(url);\n \t\t\treturn NULL;\n \t\t}\n-\t\t*ep = '/';\n-\t\tep = strchr(ep + 1, '/');\n+\t\tif(has_forwardslash) {\n+\t\t\tep[1] = saved_character;\n+\t\t}\n \t}\n\n \tstrbuf_addf(&out_buffer.buf, LOCK_REQUEST, git_default_email);\n-- \n1.6.0.4\n"},{"id":"100832","messageId":"alpine.DEB.1.00.0901171606190.3586@pacific.mpi-cbg.de","threadId":"17218","inReplyTo":"be6fef0d0901170028t3b418220wa024880235776037@mail.gmail.com","subject":"[PATCH] http-push: when making directories, have a trailing slash in the path name","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-17T15:11:51Z","receivedAt":"2009-01-17T15:11:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThe function lock_remote() sends MKCOL requests to make leading\ndirectories; However, if it does not put a forward slash '/' at the end of\nthe path, the server sends a 301 redirect.\n\nBy leaving the '/' in place, we can avoid this additional step.\n\nIncidentally, at least one version of Curl (7.16.3) does not resend\ncredentials when it follows a 301 redirect, so this commit also fixes\na bug.\n\nOriginal patch by Tay Ray Chuan .\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tOn Sat, 17 Jan 2009, Ray Chuan wrote:\n\n\t> http-push.c | 20 +++++++++++++++-----\n\t> 1 files changed, 15 insertions(+), 5 deletions(-)\n\n\tActually, I had more something like this in mind. Please note \n\tthat I am not the best patch writer of the world, and I know it. \n\tHowever, a few things I do try to follow:\n\n\t- our commit messages are not really uniform, but they do share a \n\t certain likeliness, e.g. all sentences start with a capital, we \n\t try to use a less personal form than \"I\", and we avoid \n\t abbreviations like \"don't\" and write \"do not\" instead.\n\n\t- if you can come up with a patch that is shorter, but still clear \n\t in what it does (the commit message might need to help there), \n\t it is preferred: the less lines changed, the less lines for bugs \n\t to hide in.\n\n\t- this patch includes a test case.\n\n\tThe latter part cannot be stressed enough: my first idea was to \n\ttest for 301, but I realized then and there that _my_ Apache does not \n\tcare, but always returns a 201.\n\n\tHowever, I remember that I had an Apache something like 2 years \n\tago which cared, and therefore I think this patch should go in.\n\n http-push.c | 5 +++--\n t/lib-httpd/apache.conf | 2 ++\n t/t5540-http-push.sh | 6 ++++++\n 3 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex bbe7a8e..9fcccee 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1201,7 +1201,8 @@ static struct remote_lock *lock_remote(const char *path, long timeout)\n \t/* Make sure leading directories exist for the remote ref */\n \tep = strchr(url + strlen(remote->url) + 1, '/');\n \twhile (ep) {\n-\t\t*ep = 0;\n+\t\tchar saved_character = ep[1];\n+\t\tep[1] = '\\0';\n \t\tslot = get_active_slot();\n \t\tslot->results = &results;\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n@@ -1223,7 +1224,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)\n \t\t\tfree(url);\n \t\t\treturn NULL;\n \t\t}\n-\t\t*ep = '/';\n+\t\tep[1] = saved_character;\n \t\tep = strchr(ep + 1, '/');\n \t}\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 4717c2d..fdb19a5 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -1,6 +1,8 @@\n ServerName dummy\n PidFile httpd.pid\n DocumentRoot www\n+LogFormat \"%h %l %u %t \\\"%r\\\" %>s %b\" common\n+CustomLog access.log common\n ErrorLog error.log\n \n \ndiff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh\nindex da95886..22cfbb6 100755\n--- a/t/t5540-http-push.sh\n+++ b/t/t5540-http-push.sh\n@@ -76,6 +76,12 @@ test_expect_failure 'create and delete remote branch' '\n \ttest_must_fail git show-ref --verify refs/remotes/origin/dev\n '\n \n+test_expect_success 'MKCOL sends directory names with trailing slashes' '\n+\n+\t! grep \"\\\"MKCOL.*[^/] HTTP/[^ ]*\\\"\" < \"$HTTPD_ROOT_PATH\"/access.log\n+\n+'\n+\n stop_httpd\n \n test_done\n-- \n1.6.1.325.g062d4\n"}]}