threads / patch / 17219

patch, 3 partshttp-push: when sending objects, don't PUT before MOVE

Subject: [PATCH 2/3] http-push: when sending objects, don't PUT before MOVE

## tl;dr

3 messages between Jan 17, 2009 and Jan 17, 2009. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Ray Chuan· Jan 17, 2009, 02:59 UTC · lore

since 753bc911f489748a837ecb5ea4b5216220b24845, the opaquelocktocken URI isn't used, so it doesn't make sense to PUT then MOVE.

currently, git PUTs to
/repo.git/objects/1a/1a2b..._opaquelocktoken:1234-....

on some platforms, ':' isn't allowed in filenames so this fails (assuming the server doesn't recognize it as opaquelocktoken scheme). in fact, i don't think this is the correct implementation of the scheme; 'opaquelocktoken: ' should come in front of the path.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 src/git-1.6.1/http-push.c |   45 +--------------------------------------------
 1 files changed, 1 insertions(+), 44 deletions(-)
Show changes to src/git-1.6.1/http-push.c +1 −44
diff --git a/src/git-1.6.1/http-push.c b/src/git-1.6.1/http-push.c
index a646a49..838ff6f 100644
--- a/src/git-1.6.1/http-push.c
+++ b/src/git-1.6.1/http-push.c
@@ -31,7 +31,6 @@ enum XML_Status {
 /* DAV methods */
 #define DAV_LOCK "LOCK"
 #define DAV_MKCOL "MKCOL"
-#define DAV_MOVE "MOVE"
 #define DAV_PROPFIND "PROPFIND"
 #define DAV_PUT "PUT"
 #define DAV_UNLOCK "UNLOCK"
@@ -104,7 +103,6 @@ enum transfer_state {
 	NEED_PUSH,
 	RUN_MKCOL,
 	RUN_PUT,
-	RUN_MOVE,
 	ABORTED,
 	COMPLETE,
 };
@@ -528,11 +526,6 @@ static void start_put(struct transfer_request *request)
 	posn += 2;
 	*(posn++) = '/';
 	strcpy(posn, hex + 2);
-	request->dest = xmalloc(strlen(request->url) + 14);
-	sprintf(request->dest, "Destination: %s", request->url);
-	posn += 38;
-	*(posn++) = '_';
-	strcpy(posn, request->lock->token);

 	slot = get_active_slot();
 	slot->callback_func = process_response;
@@ -557,32 +550,6 @@ static void start_put(struct transfer_request *request)
 	}
 }

-static void start_move(struct transfer_request *request)
-{
-	struct active_request_slot *slot;
-	struct curl_slist *dav_headers = NULL;
-
-	slot = get_active_slot();
-	slot->callback_func = process_response;
-	slot->callback_data = request;
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); /* undo PUT setup */
-	curl_easy_setopt(slot->curl, CURLOPT_CUSTOMREQUEST, DAV_MOVE);
-	dav_headers = curl_slist_append(dav_headers, request->dest);
-	dav_headers = curl_slist_append(dav_headers, "Overwrite: T");
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
-	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_null);
-	curl_easy_setopt(slot->curl, CURLOPT_URL, request->url);
-
-	if (start_active_slot(slot)) {
-		request->slot = slot;
-		request->state = RUN_MOVE;
-	} else {
-		request->state = ABORTED;
-		free(request->url);
-		request->url = NULL;
-	}
-}
-
 static int refresh_lock(struct remote_lock *lock)
 {
 	struct active_request_slot *slot;
@@ -705,23 +672,13 @@ static void finish_request(struct
transfer_request *request)
 		}
 	} else if (request->state == RUN_PUT) {
 		if (request->curl_result == CURLE_OK) {
-			start_move(request);
-		} else {
-			fprintf(stderr,	"PUT %s failed, aborting (%d/%ld)\n",
-				sha1_to_hex(request->obj->sha1),
-				request->curl_result, request->http_code);
-			request->state = ABORTED;
-			aborted = 1;
-		}
-	} else if (request->state == RUN_MOVE) {
-		if (request->curl_result == CURLE_OK) {
 			if (push_verbosely)
 				fprintf(stderr, "    sent %s\n",
 					sha1_to_hex(request->obj->sha1));
 			request->obj->flags |= REMOTE;
 			release_request(request);
 		} else {
-			fprintf(stderr, "MOVE %s failed, aborting (%d/%ld)\n",
+			fprintf(stderr,	"PUT %s failed, aborting (%d/%ld)\n",
 				sha1_to_hex(request->obj->sha1),
 				request->curl_result, request->http_code);
 			request->state = ABORTED;

-- 
1.6.0.4
Johannes Schindelin· Jan 17, 2009, 06:13 UTC · re: Ray Chuan · lore

Re: [PATCH 2/3] http-push: when sending objects, don't PUT before MOVE

Hi,
On Sat, 17 Jan 2009, Ray Chuan wrote:
> since 753bc911f489748a837ecb5ea4b5216220b24845, the opaquelocktocken

It would be nice to use the form <abbrev-sha1>(<oneline>) instead of a non-abbreviated SHA-1 that everybody who is interested has to look up, wasting time.

Show 5 quoted lines
> URI isn't used, so it doesn't make sense to PUT then MOVE.
> 
> currently, git PUTs to
> 
> /repo.git/objects/1a/1a2b..._opaquelocktoken:1234-....

First you say that the opaquelocktoken URI is not used, but here it looks like one?

> on some platforms, ':' isn't allowed in filenames so this fails
> (assuming the server doesn't recognize it as opaquelocktoken scheme).
> in fact, i don't think this is the correct implementation of the
> scheme; 'opaquelocktoken: ' should come in front of the path.

It would be nice to make that a fact-backed commit message. IOW there has to be some documentation about the subject which you can quote, and which would give you a definitive answer to the question if it should be a prefix or not.

Show 32 quoted lines
> diff --git a/src/git-1.6.1/http-push.c b/src/git-1.6.1/http-push.c
> index a646a49..838ff6f 100644
> --- a/src/git-1.6.1/http-push.c
> +++ b/src/git-1.6.1/http-push.c
> @@ -31,7 +31,6 @@ enum XML_Status {
>  /* DAV methods */
>  #define DAV_LOCK "LOCK"
>  #define DAV_MKCOL "MKCOL"
> -#define DAV_MOVE "MOVE"
>  #define DAV_PROPFIND "PROPFIND"
>  #define DAV_PUT "PUT"
>  #define DAV_UNLOCK "UNLOCK"
> @@ -104,7 +103,6 @@ enum transfer_state {
>  	NEED_PUSH,
>  	RUN_MKCOL,
>  	RUN_PUT,
> -	RUN_MOVE,
>  	ABORTED,
>  	COMPLETE,
>  };
> @@ -528,11 +526,6 @@ static void start_put(struct transfer_request *request)
>  	posn += 2;
>  	*(posn++) = '/';
>  	strcpy(posn, hex + 2);
> -	request->dest = xmalloc(strlen(request->url) + 14);
> -	sprintf(request->dest, "Destination: %s", request->url);
> -	posn += 38;
> -	*(posn++) = '_';
> -	strcpy(posn, request->lock->token);
> 
>  	slot = get_active_slot();
>  	slot->callback_func = process_response;

Color me puzzled again. Why is this code no longer needed? Is this the lock you were talking about?

Show 13 quoted lines
> @@ -705,23 +672,13 @@ static void finish_request(struct
> transfer_request *request)
>  		}
>  	} else if (request->state == RUN_PUT) {
>  		if (request->curl_result == CURLE_OK) {
> -			start_move(request);
> -		} else {
> -			fprintf(stderr,	"PUT %s failed, aborting (%d/%ld)\n",
> -				sha1_to_hex(request->obj->sha1),
> -				request->curl_result, request->http_code);
> -			request->state = ABORTED;
> -			aborted = 1;
> -		}

... and here comes my first doubt that it would be a good idea to avoid "put && move"; what if "put" fails? Then you end up with a corrupt repository.

Ciao, Dscho

Ray Chuan· Jan 17, 2009, 12:08 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/3] http-push: when sending objects, don't PUT before MOVE

Hi,

On Sat, Jan 17, 2009 at 2:13 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 9 quoted lines
> Hi,
>
> On Sat, 17 Jan 2009, Ray Chuan wrote:
>
>> since 753bc911f489748a837ecb5ea4b5216220b24845, the opaquelocktocken
>
> It would be nice to use the form <abbrev-sha1>(<oneline>) instead of a
> non-abbreviated SHA-1 that everybody who is interested has to look up,
> wasting time.

sorry, i was referring to 753bc91 (Remove the requirement opaquelocktoken uri scheme).

Show 18 quoted lines
>> URI isn't used, so it doesn't make sense to PUT then MOVE.
>>
>> currently, git PUTs to
>>
>> /repo.git/objects/1a/1a2b..._opaquelocktoken:1234-....
>
> First you say that the opaquelocktoken URI is not used, but here it looks
> like one?
>
>> on some platforms, ':' isn't allowed in filenames so this fails
>> (assuming the server doesn't recognize it as opaquelocktoken scheme).
>> in fact, i don't think this is the correct implementation of the
>> scheme; 'opaquelocktoken: ' should come in front of the path.
>
> It would be nice to make that a fact-backed commit message.  IOW there has
> to be some documentation about the subject which you can quote, and which
> would give you a definitive answer to the question if it should be a
> prefix or not.
According to the opaquelocktoken URI scheme in RFC2518
 OpaqueLockToken-URI = "opaquelocktoken:" UUID [Extension]
if i'm not wrong, then the URI should read
 opaquelocktoken:1234-....:/repo.git/objects/1a/1a2b...
rather than (currently):
 /repo.git/objects/1a/1a2b..._opaquelocktoken:1234-....

this means either the opaquelocktoken URI scheme wasn't meant to be implemented, or it is an incorrect implementation.

my belief is of the former, since the URI scheme is meant to represent
the lock token currently held when communicating to the server; in
addition, the lock held doesn't apply to the PUT path; it is
'refs/heads/abranch' rather than 'objects/' that is locked, so it
doesn't make sense to pass the lock token to the server while
accessing 'objects/'.
- Show quoted text -
Show 35 quoted lines
>> diff --git a/src/git-1.6.1/http-push.c b/src/git-1.6.1/http-push.c
>> index a646a49..838ff6f 100644
>> --- a/src/git-1.6.1/http-push.c
>> +++ b/src/git-1.6.1/http-push.c
>> @@ -31,7 +31,6 @@ enum XML_Status {
>>  /* DAV methods */
>>  #define DAV_LOCK "LOCK"
>>  #define DAV_MKCOL "MKCOL"
>> -#define DAV_MOVE "MOVE"
>>  #define DAV_PROPFIND "PROPFIND"
>>  #define DAV_PUT "PUT"
>>  #define DAV_UNLOCK "UNLOCK"
>> @@ -104,7 +103,6 @@ enum transfer_state {
>>       NEED_PUSH,
>>       RUN_MKCOL,
>>       RUN_PUT,
>> -     RUN_MOVE,
>>       ABORTED,
>>       COMPLETE,
>>  };
>> @@ -528,11 +526,6 @@ static void start_put(struct transfer_request *request)
>>       posn += 2;
>>       *(posn++) = '/';
>>       strcpy(posn, hex + 2);
>> -     request->dest = xmalloc(strlen(request->url) + 14);
>> -     sprintf(request->dest, "Destination: %s", request->url);
>> -     posn += 38;
>> -     *(posn++) = '_';
>> -     strcpy(posn, request->lock->token);
>>
>>       slot = get_active_slot();
>>       slot->callback_func = process_response;
>
> Color me puzzled again.  Why is this code no longer needed?  Is this the
> lock you were talking about?

the first two hunks remove MOVE-specific flags and status codes, while the last hunk removes code that generates the "Destination: <url>" header needed for a MOVE, which isn't needed by any other DAV requests.

it isn't related to locks, although the "source" url would contain the word lock in the current implementation.

for example, currently, a PUT path appended with a opaquelocktoken is followed by a MOVE request:

PUT /git/test_repo.git/objects/50/b820aea6d3503362343cdc0e699b760c700b2b_opaquelocktoken:6960ad7a-84b0-9b4e-85cc-b9f15652c658 MOVE /git/test_repo.git/objects/50/b820aea6d3503362343cdc0e699b760c700b2b

(actually, it is the request header 'Destination: ' that contains the destination path, not the request url; i replaced it for demonstrative purposes.)

Show 17 quoted lines
>> @@ -705,23 +672,13 @@ static void finish_request(struct
>> transfer_request *request)
>>               }
>>       } else if (request->state == RUN_PUT) {
>>               if (request->curl_result == CURLE_OK) {
>> -                     start_move(request);
>> -             } else {
>> -                     fprintf(stderr, "PUT %s failed, aborting (%d/%ld)\n",
>> -                             sha1_to_hex(request->obj->sha1),
>> -                             request->curl_result, request->http_code);
>> -                     request->state = ABORTED;
>> -                     aborted = 1;
>> -             }
>
> ... and here comes my first doubt that it would be a good idea to avoid
> "put && move"; what if "put" fails?  Then you end up with a corrupt
> repository.

if we take the "put && move" procedure as a guard against failure (as opposed to "put"), then how does one explain the fact that this procedure isn't applied when updating a branch file (eg. refs/heads/mybranch)?

in any case, "put && move" isn't an effective guard after failure during put. the PUT sends an object/revision to the repository, however, the repository doesn't yet know that such an object/revision exists, cos the ref file for the branch to be updated in the remote repository (eg. refs/heads/mybranch) has yet to be updated; it is updated only if the PUT was successful.

thus the repository won't be corrupt if a PUT request fails.
> Ciao,
> Dscho
>
>
-- 
Cheers,
Ray Chuan

← back to recent threads