[PATCH] don't append 'opaquelocktoken:' in PUT and MOVE

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 <redacted>
---

 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

Hi,

On Sat, 7 Feb 2009, Tay Ray Chuan 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.

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

Hi,

On Sun, Feb 8, 2009 at 1:03 AM, Johannes Schindelin
[off-list ref] 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

Hi,

On Sun, 8 Feb 2009, Tay Ray Chuan wrote:

On Sun, Feb 8, 2009 at 1:03 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

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

Hi,

On Sun, Feb 8, 2009 at 2:04 AM, Johannes Schindelin
[off-list ref] 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

Hi,

On Sun, 8 Feb 2009, Tay Ray Chuan wrote:

On Sun, Feb 8, 2009 at 2:04 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

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

Hi,

On Sun, Feb 8, 2009 at 2:57 AM, Johannes Schindelin
[off-list ref] 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