{"thread":{"id":"17628","subject":"[PATCH] don't append 'opaquelocktoken:' in PUT and MOVE","startedAt":"2009-02-07T15:12:57Z","lastAt":"2009-02-07T19:43:41Z","messageCount":8,"participants":["Tay Ray Chuan","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"103624","messageId":"498DA4F9.20104@gmail.com","threadId":"17628","inReplyTo":null,"subject":"[PATCH] don't append 'opaquelocktoken:' in PUT and MOVE","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-02-07T15:12:57Z","receivedAt":"2009-02-07T15:12:57Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"In 753bc91 (\"Remove the requirement opaquelocktoken uri scheme\"), the\nlock token is guaranteed to be prefixed with the string\n'opaquelocktoken:', which propagated down to file path creation\noperations in the remote repository, namely, in start_put (and\nconsequently start_move).\n\nThese file operations may not be successful, due to the colon ':'\ncharacter in the file path (specifically, in Windows).\n\nThis patch ensures that the lock token sans 'opaquelocktoken:' is used\ninstead in start_put.\n\nNote on tests: In the second grep, we check that Apache returns status\n20* (ie. the request was successful), but in the first we do not do\nso, since file creation by PUSH/MOVE is not guaranteed to succeed (see\nabove).\n\nSigned-off-by: Tay Ray Chuan \n---\n\n http-push.c | 2 +-\n t/t5540-http-push.sh | 9 +++++++++\n 2 files changed, 10 insertions(+), 1 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex eefd64c..10df94a 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -558,7 +558,7 @@ static void start_put(struct transfer_request *request)\n\n \tappend_remote_object_url(&buf, remote->url, hex, 0);\n \tstrbuf_addstr(&buf, \"_\");\n-\tstrbuf_addstr(&buf, request->lock->token);\n+\tstrbuf_addstr(&buf, request->lock->token + 16);\n \trequest->url = strbuf_detach(&buf, NULL);\n\n \tslot = get_active_slot();\ndiff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh\nindex c236b5e..544dea8 100755\n--- a/t/t5540-http-push.sh\n+++ b/t/t5540-http-push.sh\n@@ -94,6 +94,15 @@ test_expect_success 'MKCOL sends directory names with trailing slashes' '\n\n '\n\n+test_expect_success 'PUT and MOVE sends object to URLs without \"opaquelocktoken:\"' '\n+\n+\t!(grep -P \"\\\"(?:PUT|MOVE) .+objects/[\\da-z]{2}/[\\da-z]{38}_opaquelocktoken:[\\da-z\\-]{36} HTTP/[0-9.]+\\\"\" \\\n+\t< \"$HTTPD_ROOT_PATH\"/access.log) &&\n+\tgrep -P \"\\\"(?:PUT|MOVE) .+objects/[\\da-z]{2}/[\\da-z]{38}_[\\da-z\\-]{36} HTTP/[0-9.]+\\\" 20\\d\" \\\n+\t< \"$HTTPD_ROOT_PATH\"/access.log\n+\n+'\n+\n stop_httpd\n\n test_done\n-- \n1.6.1.2.278.g9a9e.dirty\n"},{"id":"103636","messageId":"alpine.DEB.1.00.0902071801570.10279@pacific.mpi-cbg.de","threadId":"17628","inReplyTo":"498DA4F9.20104@gmail.com","subject":"Re: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-07T17:03:19Z","receivedAt":"2009-02-07T17:03:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 7 Feb 2009, Tay Ray Chuan wrote:\n\n> diff --git a/http-push.c b/http-push.c\n> index eefd64c..10df94a 100644\n> --- a/http-push.c\n> +++ b/http-push.c\n> @@ -558,7 +558,7 @@ static void start_put(struct transfer_request *request)\n> \n> \tappend_remote_object_url(&buf, remote->url, hex, 0);\n> \tstrbuf_addstr(&buf, \"_\");\n> -\tstrbuf_addstr(&buf, request->lock->token);\n> +\tstrbuf_addstr(&buf, request->lock->token + 16);\n\nUmm. This \"16\" is a little bit too hardcoded for my liking. I mean, it \nis not even obvious from _this_ hunk why \"16\" should be correct.\n\nBesides, I have to wonder where request->lock->token is set, and if that \nwould not be the better place to fix the issue?\n\nCiao,\nDscho\n"},{"id":"103638","messageId":"be6fef0d0902070952l5818e4b7x3048c667661ce31@mail.gmail.com","threadId":"17628","inReplyTo":"alpine.DEB.1.00.0902071801570.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-02-07T17:52:27Z","receivedAt":"2009-02-07T17:52:27Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sun, Feb 8, 2009 at 1:03 AM, Johannes Schindelin\n wrote:\n> Umm. This \"16\" is a little bit too hardcoded for my liking. I mean, it\n> is not even obvious from _this_ hunk why \"16\" should be correct.\n\nAny solutions for this? Would a comment like \"skip 'opaquelocktoken:'\nprefix of length 16\" be sufficient? Or maybe in the commit message?\n\nI considered a set of strbuf functions, like for the remote object url\n(get_remote_object_url and append_remote_object_url), but I thought it\nwas a little overkill, since this is one of the only instances that I\ncan think of where including 'opaquelocktoken:' is unwanted.\n\n> Besides, I have to wonder where request->lock->token is set, and if that\n> would not be the better place to fix the issue?\n\nAre you suggesting perhaps that we revert commit 753bc91? Or perhaps\ncreate another way to access lock tokens, say, get_lock_token(int\nprepend_scheme)?\n\n-- \nCheers,\nRay Chuan\n"},{"id":"103639","messageId":"alpine.DEB.1.00.0902071903020.10279@pacific.mpi-cbg.de","threadId":"17628","inReplyTo":"be6fef0d0902070952l5818e4b7x3048c667661ce31@mail.gmail.com","subject":"Re: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-07T18:04:44Z","receivedAt":"2009-02-07T18:04:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 8 Feb 2009, Tay Ray Chuan wrote:\n\n> On Sun, Feb 8, 2009 at 1:03 AM, Johannes Schindelin\n> wrote:\n> \n> > Besides, I have to wonder where request->lock->token is set, and if \n> > that would not be the better place to fix the issue?\n> \n> Are you suggesting perhaps that we revert commit 753bc91? Or perhaps \n> create another way to access lock tokens, say, get_lock_token(int \n> prepend_scheme)?\n\nI do not suggest anything like that.\n\nBut I _refuse_ to go and investigate a thing that you should know already, \nand that you should be able to defend, indeed, something you should have \ndefended in the commit message:\n\nWhy does request->lock->token have the prefix at all?\n\nCiao,\nDscho\n"},{"id":"103642","messageId":"be6fef0d0902071038y3e6de33es33a73ab6a068f9ac@mail.gmail.com","threadId":"17628","inReplyTo":"alpine.DEB.1.00.0902071903020.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-02-07T18:38:39Z","receivedAt":"2009-02-07T18:38:39Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sun, Feb 8, 2009 at 2:04 AM, Johannes Schindelin\n wrote:\n> Why does request->lock->token have the prefix at all?\n\nWhile typing my reply, I realise it is unwarranted.\n\nThanks for taking the time to look through this.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"103643","messageId":"alpine.DEB.1.00.0902071956480.10279@pacific.mpi-cbg.de","threadId":"17628","inReplyTo":"be6fef0d0902071038y3e6de33es33a73ab6a068f9ac@mail.gmail.com","subject":"Re: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-02-07T18:57:18Z","receivedAt":"2009-02-07T18:57:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 8 Feb 2009, Tay Ray Chuan wrote:\n\n> On Sun, Feb 8, 2009 at 2:04 AM, Johannes Schindelin\n> wrote:\n> > Why does request->lock->token have the prefix at all?\n> \n> While typing my reply, I realise it is unwarranted.\n\nHuh? What does that mean? Does lock->token have that prefix (including \nthe colon) or not?\n\nCiao,\nDscho\n"},{"id":"103644","messageId":"be6fef0d0902071100o68cf0f81m2e666bb1dac9977a@mail.gmail.com","threadId":"17628","inReplyTo":"alpine.DEB.1.00.0902071956480.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-02-07T19:00:58Z","receivedAt":"2009-02-07T19:00:58Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sun, Feb 8, 2009 at 2:57 AM, Johannes Schindelin\n wrote:\n> Huh? What does that mean? Does lock->token have that prefix (including\n> the colon) or not?\n\nYes, it does have the prefix with colon, but it is not true that it is\nalways \"opaquelocktoken:\" -- it could be some other URI scheme.\n\nI submitting a new patch that accounts for this (other URI schemes).\n\n-- \nCheers,\nRay Chuan\n"},{"id":"103646","messageId":"7veiyavxqq.fsf@gitster.siamese.dyndns.org","threadId":"17628","inReplyTo":"498DA4F9.20104@gmail.com","subject":"Re: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-07T19:43:41Z","receivedAt":"2009-02-07T19:43:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan writes:\n\n> In 753bc91 (\"Remove the requirement opaquelocktoken uri scheme\"), the\n> lock token is guaranteed to be prefixed with the string\n> 'opaquelocktoken:', which propagated down to file path creation\n> operations in the remote repository, namely, in start_put (and\n> consequently start_move).\n>\n> These file operations may not be successful, due to the colon ':'\n> character in the file path (specifically, in Windows).\n>\n> This patch ensures that the lock token sans 'opaquelocktoken:' is used\n> instead in start_put.\n\nI am not familiar with DAV so I have to wonder who guarantees in the\ncodepath that the string that follows the \"opaquelocktoken:\" (and any\nother lock token uri scheme) is without problematic characters, such as a\nslash (or colon)?\n"}]}