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

["Ray Chuan" <rctay89@gmail.com>]

Hi,

On Sat, Jan 17, 2009 at 2:13 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> 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).

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

>> 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.)

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