# [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE 8 messages from 2009-02-07 to 2009-02-07. Participants: Tay Ray Chuan, Johannes Schindelin, Junio C Hamano. Thread: https://gitlist.dev/t/17628 ## Tay Ray Chuan, 2009-02-07 15:12 Subject: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE Message-ID: <498DA4F9.20104@gmail.com> URL: https://gitlist.dev/e/498DA4F9.20104%40gmail.com ``` In 753bc91 ("Remove the requirement opaquelocktoken uri scheme"), the lock token is guaranteed to be prefixed with the string 'opaquelocktoken:', which propagated down to file path creation operations in the remote repository, namely, in start_put (and consequently start_move). These file operations may not be successful, due to the colon ':' character in the file path (specifically, in Windows). This patch ensures that the lock token sans 'opaquelocktoken:' is used instead in start_put. Note on tests: In the second grep, we check that Apache returns status 20* (ie. the request was successful), but in the first we do not do so, since file creation by PUSH/MOVE is not guaranteed to succeed (see above). Signed-off-by: Tay Ray Chuan --- http-push.c | 2 +- t/t5540-http-push.sh | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletions(-) diff --git a/http-push.c b/http-push.c index eefd64c..10df94a 100644 --- a/http-push.c +++ b/http-push.c @@ -558,7 +558,7 @@ static void start_put(struct transfer_request *request) append_remote_object_url(&buf, remote->url, hex, 0); strbuf_addstr(&buf, "_"); - strbuf_addstr(&buf, request->lock->token); + strbuf_addstr(&buf, request->lock->token + 16); request->url = strbuf_detach(&buf, NULL); slot = get_active_slot(); diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh index c236b5e..544dea8 100755 --- a/t/t5540-http-push.sh +++ b/t/t5540-http-push.sh @@ -94,6 +94,15 @@ test_expect_success 'MKCOL sends directory names with trailing slashes' ' ' +test_expect_success 'PUT and MOVE sends object to URLs without "opaquelocktoken:"' ' + + !(grep -P "\"(?:PUT|MOVE) .+objects/[\da-z]{2}/[\da-z]{38}_opaquelocktoken:[\da-z\-]{36} HTTP/[0-9.]+\"" \ + < "$HTTPD_ROOT_PATH"/access.log) && + grep -P "\"(?:PUT|MOVE) .+objects/[\da-z]{2}/[\da-z]{38}_[\da-z\-]{36} HTTP/[0-9.]+\" 20\d" \ + < "$HTTPD_ROOT_PATH"/access.log + +' + stop_httpd test_done -- 1.6.1.2.278.g9a9e.dirty ``` ## Johannes Schindelin, 2009-02-07 17:03 Subject: Re: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902071801570.10279%40pacific.mpi-cbg.de In-Reply-To: <498DA4F9.20104@gmail.com> ``` Hi, On Sat, 7 Feb 2009, Tay Ray Chuan wrote: > diff --git a/http-push.c b/http-push.c > index eefd64c..10df94a 100644 > --- a/http-push.c > +++ b/http-push.c > @@ -558,7 +558,7 @@ static void start_put(struct transfer_request *request) > > append_remote_object_url(&buf, remote->url, hex, 0); > strbuf_addstr(&buf, "_"); > - strbuf_addstr(&buf, request->lock->token); > + strbuf_addstr(&buf, request->lock->token + 16); Umm. This "16" is a little bit too hardcoded for my liking. I mean, it is not even obvious from _this_ hunk why "16" should be correct. Besides, I have to wonder where request->lock->token is set, and if that would not be the better place to fix the issue? Ciao, Dscho ``` ## Tay Ray Chuan, 2009-02-07 17:52 Subject: Re: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE Message-ID: URL: https://gitlist.dev/e/be6fef0d0902070952l5818e4b7x3048c667661ce31%40mail.gmail.com In-Reply-To: ``` Hi, On Sun, Feb 8, 2009 at 1:03 AM, Johannes Schindelin wrote: > Umm. This "16" is a little bit too hardcoded for my liking. I mean, it > is not even obvious from _this_ hunk why "16" should be correct. Any solutions for this? Would a comment like "skip 'opaquelocktoken:' prefix of length 16" be sufficient? Or maybe in the commit message? I considered a set of strbuf functions, like for the remote object url (get_remote_object_url and append_remote_object_url), but I thought it was a little overkill, since this is one of the only instances that I can think of where including 'opaquelocktoken:' is unwanted. > Besides, I have to wonder where request->lock->token is set, and if that > would not be the better place to fix the issue? Are you suggesting perhaps that we revert commit 753bc91? Or perhaps create another way to access lock tokens, say, get_lock_token(int prepend_scheme)? -- Cheers, Ray Chuan ``` ## Johannes Schindelin, 2009-02-07 18:04 Subject: Re: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902071903020.10279%40pacific.mpi-cbg.de In-Reply-To: ``` Hi, On Sun, 8 Feb 2009, Tay Ray Chuan wrote: > On Sun, Feb 8, 2009 at 1:03 AM, Johannes Schindelin > wrote: > > > Besides, I have to wonder where request->lock->token is set, and if > > that would not be the better place to fix the issue? > > Are you suggesting perhaps that we revert commit 753bc91? Or perhaps > create another way to access lock tokens, say, get_lock_token(int > prepend_scheme)? I do not suggest anything like that. But I _refuse_ to go and investigate a thing that you should know already, and that you should be able to defend, indeed, something you should have defended in the commit message: Why does request->lock->token have the prefix at all? Ciao, Dscho ``` ## Tay Ray Chuan, 2009-02-07 18:38 Subject: Re: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE Message-ID: URL: https://gitlist.dev/e/be6fef0d0902071038y3e6de33es33a73ab6a068f9ac%40mail.gmail.com In-Reply-To: ``` Hi, On Sun, Feb 8, 2009 at 2:04 AM, Johannes Schindelin wrote: > Why does request->lock->token have the prefix at all? While typing my reply, I realise it is unwarranted. Thanks for taking the time to look through this. -- Cheers, Ray Chuan ``` ## Johannes Schindelin, 2009-02-07 18:57 Subject: Re: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0902071956480.10279%40pacific.mpi-cbg.de In-Reply-To: ``` Hi, On Sun, 8 Feb 2009, Tay Ray Chuan wrote: > On Sun, Feb 8, 2009 at 2:04 AM, Johannes Schindelin > wrote: > > Why does request->lock->token have the prefix at all? > > While typing my reply, I realise it is unwarranted. Huh? What does that mean? Does lock->token have that prefix (including the colon) or not? Ciao, Dscho ``` ## Tay Ray Chuan, 2009-02-07 19:00 Subject: Re: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE Message-ID: URL: https://gitlist.dev/e/be6fef0d0902071100o68cf0f81m2e666bb1dac9977a%40mail.gmail.com In-Reply-To: ``` Hi, On Sun, Feb 8, 2009 at 2:57 AM, Johannes Schindelin wrote: > Huh? What does that mean? Does lock->token have that prefix (including > the colon) or not? Yes, it does have the prefix with colon, but it is not true that it is always "opaquelocktoken:" -- it could be some other URI scheme. I submitting a new patch that accounts for this (other URI schemes). -- Cheers, Ray Chuan ``` ## Junio C Hamano, 2009-02-07 19:43 Subject: Re: [PATCH] don't append 'opaquelocktoken:' in PUT and MOVE Message-ID: <7veiyavxqq.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7veiyavxqq.fsf%40gitster.siamese.dyndns.org In-Reply-To: <498DA4F9.20104@gmail.com> ``` Tay Ray Chuan writes: > In 753bc91 ("Remove the requirement opaquelocktoken uri scheme"), the > lock token is guaranteed to be prefixed with the string > 'opaquelocktoken:', which propagated down to file path creation > operations in the remote repository, namely, in start_put (and > consequently start_move). > > These file operations may not be successful, due to the colon ':' > character in the file path (specifically, in Windows). > > This patch ensures that the lock token sans 'opaquelocktoken:' is used > instead in start_put. I am not familiar with DAV so I have to wonder who guarantees in the codepath that the string that follows the "opaquelocktoken:" (and any other lock token uri scheme) is without problematic characters, such as a slash (or colon)? ```