{"thread":{"id":"17219","subject":"[PATCH 2/3] http-push: when sending objects, don't PUT before MOVE","startedAt":"2009-01-17T02:59:07Z","lastAt":"2009-01-17T12:08:33Z","messageCount":3,"participants":["Ray Chuan","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"100791","messageId":"be6fef0d0901161859g4e5d749et78b6b0231eb915ee@mail.gmail.com","threadId":"17219","inReplyTo":null,"subject":"[PATCH 2/3] http-push: when sending objects, don't PUT before MOVE","fromName":"Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-01-17T02:59:07Z","receivedAt":"2009-01-17T02:59:07Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"since 753bc911f489748a837ecb5ea4b5216220b24845, the opaquelocktocken\nURI isn't used, so it doesn't make sense to PUT then MOVE.\n\ncurrently, git PUTs to\n\n/repo.git/objects/1a/1a2b..._opaquelocktoken:1234-....\n\non some platforms, ':' isn't allowed in filenames so this fails\n(assuming the server doesn't recognize it as opaquelocktoken scheme).\nin fact, i don't think this is the correct implementation of the\nscheme; 'opaquelocktoken: ' should come in front of the path.\n\nSigned-off-by: Tay Ray Chuan <rctay89@gmail.com>\n---\n src/git-1.6.1/http-push.c |   45 +--------------------------------------------\n 1 files changed, 1 insertions(+), 44 deletions(-)\n\ndiff --git a/src/git-1.6.1/http-push.c b/src/git-1.6.1/http-push.c\nindex a646a49..838ff6f 100644\n--- a/src/git-1.6.1/http-push.c\n+++ b/src/git-1.6.1/http-push.c\n@@ -31,7 +31,6 @@ enum XML_Status {\n /* DAV methods */\n #define DAV_LOCK \"LOCK\"\n #define DAV_MKCOL \"MKCOL\"\n-#define DAV_MOVE \"MOVE\"\n #define DAV_PROPFIND \"PROPFIND\"\n #define DAV_PUT \"PUT\"\n #define DAV_UNLOCK \"UNLOCK\"\n@@ -104,7 +103,6 @@ enum transfer_state {\n \tNEED_PUSH,\n \tRUN_MKCOL,\n \tRUN_PUT,\n-\tRUN_MOVE,\n \tABORTED,\n \tCOMPLETE,\n };\n@@ -528,11 +526,6 @@ static void start_put(struct transfer_request *request)\n \tposn += 2;\n \t*(posn++) = '/';\n \tstrcpy(posn, hex + 2);\n-\trequest->dest = xmalloc(strlen(request->url) + 14);\n-\tsprintf(request->dest, \"Destination: %s\", request->url);\n-\tposn += 38;\n-\t*(posn++) = '_';\n-\tstrcpy(posn, request->lock->token);\n\n \tslot = get_active_slot();\n \tslot->callback_func = process_response;\n@@ -557,32 +550,6 @@ static void start_put(struct transfer_request *request)\n \t}\n }\n\n-static void start_move(struct transfer_request *request)\n-{\n-\tstruct active_request_slot *slot;\n-\tstruct curl_slist *dav_headers = NULL;\n-\n-\tslot = get_active_slot();\n-\tslot->callback_func = process_response;\n-\tslot->callback_data = request;\n-\tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); /* undo PUT setup */\n-\tcurl_easy_setopt(slot->curl, CURLOPT_CUSTOMREQUEST, DAV_MOVE);\n-\tdav_headers = curl_slist_append(dav_headers, request->dest);\n-\tdav_headers = curl_slist_append(dav_headers, \"Overwrite: T\");\n-\tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_null);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_URL, request->url);\n-\n-\tif (start_active_slot(slot)) {\n-\t\trequest->slot = slot;\n-\t\trequest->state = RUN_MOVE;\n-\t} else {\n-\t\trequest->state = ABORTED;\n-\t\tfree(request->url);\n-\t\trequest->url = NULL;\n-\t}\n-}\n-\n static int refresh_lock(struct remote_lock *lock)\n {\n \tstruct active_request_slot *slot;\n@@ -705,23 +672,13 @@ static void finish_request(struct\ntransfer_request *request)\n \t\t}\n \t} else if (request->state == RUN_PUT) {\n \t\tif (request->curl_result == CURLE_OK) {\n-\t\t\tstart_move(request);\n-\t\t} else {\n-\t\t\tfprintf(stderr,\t\"PUT %s failed, aborting (%d/%ld)\\n\",\n-\t\t\t\tsha1_to_hex(request->obj->sha1),\n-\t\t\t\trequest->curl_result, request->http_code);\n-\t\t\trequest->state = ABORTED;\n-\t\t\taborted = 1;\n-\t\t}\n-\t} else if (request->state == RUN_MOVE) {\n-\t\tif (request->curl_result == CURLE_OK) {\n \t\t\tif (push_verbosely)\n \t\t\t\tfprintf(stderr, \"    sent %s\\n\",\n \t\t\t\t\tsha1_to_hex(request->obj->sha1));\n \t\t\trequest->obj->flags |= REMOTE;\n \t\t\trelease_request(request);\n \t\t} else {\n-\t\t\tfprintf(stderr, \"MOVE %s failed, aborting (%d/%ld)\\n\",\n+\t\t\tfprintf(stderr,\t\"PUT %s failed, aborting (%d/%ld)\\n\",\n \t\t\t\tsha1_to_hex(request->obj->sha1),\n \t\t\t\trequest->curl_result, request->http_code);\n \t\t\trequest->state = ABORTED;\n-- \n1.6.0.4\n"},{"id":"100809","messageId":"alpine.DEB.1.00.0901170706450.3586@pacific.mpi-cbg.de","threadId":"17219","inReplyTo":"be6fef0d0901161859g4e5d749et78b6b0231eb915ee@mail.gmail.com","subject":"Re: [PATCH 2/3] http-push: when sending objects, don't PUT before MOVE","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-17T06:13:15Z","receivedAt":"2009-01-17T06:13:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 17 Jan 2009, Ray Chuan wrote:\n\n> since 753bc911f489748a837ecb5ea4b5216220b24845, the opaquelocktocken\n\nIt would be nice to use the form <abbrev-sha1>(<oneline>) instead of a \nnon-abbreviated SHA-1 that everybody who is interested has to look up, \nwasting time.\n\n> URI isn't used, so it doesn't make sense to PUT then MOVE.\n> \n> currently, git PUTs to\n> \n> /repo.git/objects/1a/1a2b..._opaquelocktoken:1234-....\n\nFirst you say that the opaquelocktoken URI is not used, but here it looks \nlike one?\n\n> on some platforms, ':' isn't allowed in filenames so this fails\n> (assuming the server doesn't recognize it as opaquelocktoken scheme).\n> in fact, i don't think this is the correct implementation of the\n> scheme; 'opaquelocktoken: ' should come in front of the path.\n\nIt would be nice to make that a fact-backed commit message.  IOW there has \nto be some documentation about the subject which you can quote, and which \nwould give you a definitive answer to the question if it should be a \nprefix or not.\n\n> diff --git a/src/git-1.6.1/http-push.c b/src/git-1.6.1/http-push.c\n> index a646a49..838ff6f 100644\n> --- a/src/git-1.6.1/http-push.c\n> +++ b/src/git-1.6.1/http-push.c\n> @@ -31,7 +31,6 @@ enum XML_Status {\n>  /* DAV methods */\n>  #define DAV_LOCK \"LOCK\"\n>  #define DAV_MKCOL \"MKCOL\"\n> -#define DAV_MOVE \"MOVE\"\n>  #define DAV_PROPFIND \"PROPFIND\"\n>  #define DAV_PUT \"PUT\"\n>  #define DAV_UNLOCK \"UNLOCK\"\n> @@ -104,7 +103,6 @@ enum transfer_state {\n>  \tNEED_PUSH,\n>  \tRUN_MKCOL,\n>  \tRUN_PUT,\n> -\tRUN_MOVE,\n>  \tABORTED,\n>  \tCOMPLETE,\n>  };\n> @@ -528,11 +526,6 @@ static void start_put(struct transfer_request *request)\n>  \tposn += 2;\n>  \t*(posn++) = '/';\n>  \tstrcpy(posn, hex + 2);\n> -\trequest->dest = xmalloc(strlen(request->url) + 14);\n> -\tsprintf(request->dest, \"Destination: %s\", request->url);\n> -\tposn += 38;\n> -\t*(posn++) = '_';\n> -\tstrcpy(posn, request->lock->token);\n> \n>  \tslot = get_active_slot();\n>  \tslot->callback_func = process_response;\n\nColor me puzzled again.  Why is this code no longer needed?  Is this the \nlock you were talking about?\n\n> @@ -705,23 +672,13 @@ static void finish_request(struct\n> transfer_request *request)\n>  \t\t}\n>  \t} else if (request->state == RUN_PUT) {\n>  \t\tif (request->curl_result == CURLE_OK) {\n> -\t\t\tstart_move(request);\n> -\t\t} else {\n> -\t\t\tfprintf(stderr,\t\"PUT %s failed, aborting (%d/%ld)\\n\",\n> -\t\t\t\tsha1_to_hex(request->obj->sha1),\n> -\t\t\t\trequest->curl_result, request->http_code);\n> -\t\t\trequest->state = ABORTED;\n> -\t\t\taborted = 1;\n> -\t\t}\n\n... and here comes my first doubt that it would be a good idea to avoid \n\"put && move\"; what if \"put\" fails?  Then you end up with a corrupt \nrepository.\n\nCiao,\nDscho\n"},{"id":"100822","messageId":"be6fef0d0901170408g48139ceai2831978b764b805d@mail.gmail.com","threadId":"17219","inReplyTo":"alpine.DEB.1.00.0901170706450.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH 2/3] http-push: when sending objects, don't PUT before MOVE","fromName":"Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-01-17T12:08:33Z","receivedAt":"2009-01-17T12:08:33Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sat, Jan 17, 2009 at 2:13 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi,\n>\n> On Sat, 17 Jan 2009, Ray Chuan wrote:\n>\n>> since 753bc911f489748a837ecb5ea4b5216220b24845, the opaquelocktocken\n>\n> It would be nice to use the form <abbrev-sha1>(<oneline>) instead of a\n> non-abbreviated SHA-1 that everybody who is interested has to look up,\n> wasting time.\n\nsorry, i was referring to 753bc91 (Remove the requirement\nopaquelocktoken uri scheme).\n\n>> URI isn't used, so it doesn't make sense to PUT then MOVE.\n>>\n>> currently, git PUTs to\n>>\n>> /repo.git/objects/1a/1a2b..._opaquelocktoken:1234-....\n>\n> First you say that the opaquelocktoken URI is not used, but here it looks\n> like one?\n>\n>> on some platforms, ':' isn't allowed in filenames so this fails\n>> (assuming the server doesn't recognize it as opaquelocktoken scheme).\n>> in fact, i don't think this is the correct implementation of the\n>> scheme; 'opaquelocktoken: ' should come in front of the path.\n>\n> It would be nice to make that a fact-backed commit message.  IOW there has\n> to be some documentation about the subject which you can quote, and which\n> would give you a definitive answer to the question if it should be a\n> prefix or not.\n\nAccording to the opaquelocktoken URI scheme in RFC2518\n\n OpaqueLockToken-URI = \"opaquelocktoken:\" UUID [Extension]\n\nif i'm not wrong, then the URI should read\n\n opaquelocktoken:1234-....:/repo.git/objects/1a/1a2b...\n\nrather than (currently):\n\n /repo.git/objects/1a/1a2b..._opaquelocktoken:1234-....\n\nthis means either the opaquelocktoken URI scheme wasn't meant to be\nimplemented, or it is an incorrect implementation.\n\nmy belief is of the former, since the URI scheme is meant to represent\nthe lock token currently held when communicating to the server; in\naddition, the lock held doesn't apply to the PUT path; it is\n'refs/heads/abranch' rather than 'objects/' that is locked, so it\ndoesn't make sense to pass the lock token to the server while\naccessing 'objects/'.\n- Show quoted text -\n\n>> diff --git a/src/git-1.6.1/http-push.c b/src/git-1.6.1/http-push.c\n>> index a646a49..838ff6f 100644\n>> --- a/src/git-1.6.1/http-push.c\n>> +++ b/src/git-1.6.1/http-push.c\n>> @@ -31,7 +31,6 @@ enum XML_Status {\n>>  /* DAV methods */\n>>  #define DAV_LOCK \"LOCK\"\n>>  #define DAV_MKCOL \"MKCOL\"\n>> -#define DAV_MOVE \"MOVE\"\n>>  #define DAV_PROPFIND \"PROPFIND\"\n>>  #define DAV_PUT \"PUT\"\n>>  #define DAV_UNLOCK \"UNLOCK\"\n>> @@ -104,7 +103,6 @@ enum transfer_state {\n>>       NEED_PUSH,\n>>       RUN_MKCOL,\n>>       RUN_PUT,\n>> -     RUN_MOVE,\n>>       ABORTED,\n>>       COMPLETE,\n>>  };\n>> @@ -528,11 +526,6 @@ static void start_put(struct transfer_request *request)\n>>       posn += 2;\n>>       *(posn++) = '/';\n>>       strcpy(posn, hex + 2);\n>> -     request->dest = xmalloc(strlen(request->url) + 14);\n>> -     sprintf(request->dest, \"Destination: %s\", request->url);\n>> -     posn += 38;\n>> -     *(posn++) = '_';\n>> -     strcpy(posn, request->lock->token);\n>>\n>>       slot = get_active_slot();\n>>       slot->callback_func = process_response;\n>\n> Color me puzzled again.  Why is this code no longer needed?  Is this the\n> lock you were talking about?\n\nthe first two hunks remove MOVE-specific flags and status codes, while\nthe last hunk removes code that generates the \"Destination: <url>\"\nheader needed for a MOVE, which isn't needed by any other DAV\nrequests.\n\nit isn't related to locks, although the \"source\" url would contain the\nword lock in the current implementation.\n\nfor example, currently, a PUT path appended with a opaquelocktoken is\nfollowed by a MOVE request:\n\nPUT /git/test_repo.git/objects/50/b820aea6d3503362343cdc0e699b760c700b2b_opaquelocktoken:6960ad7a-84b0-9b4e-85cc-b9f15652c658\nMOVE /git/test_repo.git/objects/50/b820aea6d3503362343cdc0e699b760c700b2b\n\n(actually, it is the request header 'Destination: ' that contains the\ndestination path, not the request url; i replaced it for demonstrative\npurposes.)\n\n>> @@ -705,23 +672,13 @@ static void finish_request(struct\n>> transfer_request *request)\n>>               }\n>>       } else if (request->state == RUN_PUT) {\n>>               if (request->curl_result == CURLE_OK) {\n>> -                     start_move(request);\n>> -             } else {\n>> -                     fprintf(stderr, \"PUT %s failed, aborting (%d/%ld)\\n\",\n>> -                             sha1_to_hex(request->obj->sha1),\n>> -                             request->curl_result, request->http_code);\n>> -                     request->state = ABORTED;\n>> -                     aborted = 1;\n>> -             }\n>\n> ... and here comes my first doubt that it would be a good idea to avoid\n> \"put && move\"; what if \"put\" fails?  Then you end up with a corrupt\n> repository.\n\nif we take the \"put && move\" procedure as a guard against failure (as\nopposed to \"put\"), then how does one explain the fact that this\nprocedure isn't applied when updating a branch file (eg.\nrefs/heads/mybranch)?\n\nin any case, \"put && move\" isn't an effective guard after failure\nduring put. the PUT sends an object/revision to the repository,\nhowever, the repository doesn't yet know that such an object/revision\nexists, cos the ref file for the branch to be updated in the remote\nrepository (eg. refs/heads/mybranch) has yet to be updated; it is\nupdated only if the PUT was successful.\n\nthus the repository won't be corrupt if a PUT request fails.\n\n\n> Ciao,\n> Dscho\n>\n>\n\n\n-- \nCheers,\nRay Chuan\n"}]}