threads / patch / 17628

patchdon't append 'opaquelocktoken:' in PUT and MOVE

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

## tl;dr

8 messages between Feb 7, 2009 and Feb 7, 2009. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Tay Ray Chuan· Feb 7, 2009, 15:12 UTC · lore

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 <rctay89@gmail.com>
---
 http-push.c          |    2 +-
 t/t5540-http-push.sh |    9 +++++++++
 2 files changed, 10 insertions(+), 1 deletions(-)
Show changes to 2 files +10 −1

http-push.c, t/t5540-http-push.sh

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· Feb 7, 2009, 17:03 UTC · re: Tay Ray Chuan · lore

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

Hi,
On Sat, 7 Feb 2009, Tay Ray Chuan wrote:
Show 10 quoted lines
> 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· Feb 7, 2009, 17:52 UTC · re: Johannes Schindelin · lore

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

Hi,

On Sun, Feb 8, 2009 at 1:03 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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· Feb 7, 2009, 18:04 UTC · re: Tay Ray Chuan · lore

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

Hi,
On Sun, 8 Feb 2009, Tay Ray Chuan wrote:
Show 9 quoted lines
> On Sun, Feb 8, 2009 at 1:03 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> 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· Feb 7, 2009, 18:38 UTC · re: Johannes Schindelin · lore

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

Hi,

On Sun, Feb 8, 2009 at 2:04 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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· Feb 7, 2009, 18:57 UTC · re: Tay Ray Chuan · lore

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

Hi,
On Sun, 8 Feb 2009, Tay Ray Chuan wrote:
Show 5 quoted lines
> On Sun, Feb 8, 2009 at 2:04 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> 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· Feb 7, 2009, 19:00 UTC · re: Johannes Schindelin · lore

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

Hi,

On Sun, Feb 8, 2009 at 2:57 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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· Feb 7, 2009, 19:43 UTC · re: Tay Ray Chuan · lore

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

Tay Ray Chuan <rctay89@gmail.com> writes:
Show 11 quoted lines
> 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)?

← back to recent threads