{"thread":{"id":"17229","subject":"[PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server","startedAt":"2009-01-17T20:24:47Z","lastAt":"2009-01-19T03:07:10Z","messageCount":10,"participants":["Ray Chuan","Junio C Hamano","Johannes Schindelin","Sverre Rabbelier"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"100880","messageId":"be6fef0d0901171224y35c3d95cn2d38639ac03c3b8f@mail.gmail.com","threadId":"17229","inReplyTo":null,"subject":"[PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server","fromName":"Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-01-17T20:24:47Z","receivedAt":"2009-01-17T20:24:47Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Currently, git PUTs to\n\n /repo.git/objects/1a/1a2b...9z_opaquelocktoken:1234-....\n\nthen MOVEs to\n\n /repo.git/objects/1a/1a2b...9z\n\nThis is needless. In fact, the only time MOVE requests are sent is for\nthis sole purpose (ie. of renaming an object).\n\nA concern raised was repository corruption in the event of failure\nduring PUT. \"put && move\" won't afford any more protection than using\nsimply \"put\", since info/refs is not updated if a PUT fails, so there\nis no cause for concern.\n\nSigned-off-by: Tay Ray Chuan \n---\n http-push.c | 45 +--------------------------------------------\n 1 files changed, 1 insertions(+), 44 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 4517cf2..d21fd3b 100644\n--- a/http-push.c\n+++ b/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@@ -105,7 +104,6 @@ enum transfer_state {\n \tNEED_PUSH,\n \tRUN_MKCOL,\n \tRUN_PUT,\n-\tRUN_MOVE,\n \tABORTED,\n \tCOMPLETE,\n };\n@@ -531,11 +529,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 \tif_header = xmalloc(strlen(request->lock->token) + 25);\n \tsprintf(if_header, \"If: <%s>\", request->lock->token);\n@@ -565,32 +558,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@@ -713,23 +680,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.5.6.3\n"},{"id":"100896","messageId":"7viqod5thi.fsf@gitster.siamese.dyndns.org","threadId":"17229","inReplyTo":"be6fef0d0901171224y35c3d95cn2d38639ac03c3b8f@mail.gmail.com","subject":"Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-18T00:48:57Z","receivedAt":"2009-01-18T00:48:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ray Chuan\" writes:\n\n> Currently, git PUTs to\n>\n> /repo.git/objects/1a/1a2b...9z_opaquelocktoken:1234-....\n>\n> then MOVEs to\n>\n> /repo.git/objects/1a/1a2b...9z\n>\n> This is needless. In fact, the only time MOVE requests are sent is for\n> this sole purpose (ie. of renaming an object).\n>\n> A concern raised was repository corruption in the event of failure\n> during PUT. \"put && move\" won't afford any more protection than using\n> simply \"put\", since info/refs is not updated if a PUT fails, so there\n> is no cause for concern.\n\nThat's a completely bogus reasoning. Normal operation inside the\nrepository that was pushed into won't look at info/refs at all.\n\nThe true reason you want to avoid put-then-move is...?\n"},{"id":"100913","messageId":"be6fef0d0901171919ub28dda7ref6443abec3627aa@mail.gmail.com","threadId":"17229","inReplyTo":"7viqod5thi.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server","fromName":"Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-01-18T03:19:01Z","receivedAt":"2009-01-18T03:19:01Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sun, Jan 18, 2009 at 12:48 AM, Junio C Hamano wrote:\n> \"Ray Chuan\" writes:\n>\n>> Currently, git PUTs to\n>>\n>> /repo.git/objects/1a/1a2b...9z_opaquelocktoken:1234-....\n>>\n>> then MOVEs to\n>>\n>> /repo.git/objects/1a/1a2b...9z\n>>\n>> This is needless. In fact, the only time MOVE requests are sent is for\n>> this sole purpose (ie. of renaming an object).\n\nthis would be my \"true\" reason. i do believe PUTting to a \"temporary\"\nfile (ie with '__opaquelocktoken:1234-....' appended) to be a needless\nstep.\n\n>> A concern raised was repository corruption in the event of failure\n>> during PUT. \"put && move\" won't afford any more protection than using\n>> simply \"put\", since info/refs is not updated if a PUT fails, so there\n>> is no cause for concern.\n>\n> That's a completely bogus reasoning. Normal operation inside the\n> repository that was pushed into won't look at info/refs at all.\n>\n> The true reason you want to avoid put-then-move is...?\n\ni mentioned this \"repository corruption\" issue as it was raised\npreviously by Johannes (towards the bottom):\n\n http://article.gmane.org/gmane.comp.version-control.git/106031\n\n-- \nCheers,\nRay Chuan\n"},{"id":"100963","messageId":"alpine.DEB.1.00.0901181425420.3586@pacific.mpi-cbg.de","threadId":"17229","inReplyTo":"be6fef0d0901171919ub28dda7ref6443abec3627aa@mail.gmail.com","subject":"Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-18T13:32:30Z","receivedAt":"2009-01-18T13:32:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 18 Jan 2009, Ray Chuan wrote:\n\n> On Sun, Jan 18, 2009 at 12:48 AM, Junio C Hamano wrote:\n>\n> > \"Ray Chuan\" writes:\n> >\n> >> A concern raised was repository corruption in the event of failure \n> >> during PUT. \"put && move\" won't afford any more protection than using \n> >> simply \"put\", since info/refs is not updated if a PUT fails, so there \n> >> is no cause for concern.\n> >\n> > That's a completely bogus reasoning. Normal operation inside the \n> > repository that was pushed into won't look at info/refs at all.\n> \n> i mentioned this \"repository corruption\" issue as it was raised\n> previously by Johannes (towards the bottom):\n> \n> http://article.gmane.org/gmane.comp.version-control.git/106031\n\n,.. and Junio just raised a new concern. The repository as it is on the \nserver could very well be served by other means, i.e. git:// and rsync://, \nand there could be cron jobs and interactive Git sessions trying to work \nwith it.\n\nThe point is: the repository inside the document root of the web server is \nstill a valid repository.\n\nAnd the assumption is that whenever you have a file that looks like a \nvalid object/pack inside a valid repository, that it does not need \nreplacing.\n\nSo even when optimizing the uncommon (HTTP push is 2nd class citizen), we \nhave to keep the common workflow intact (1st class citizens _are_ push by \nfile system, ssh or git://).\n\nWhich unfortunately means that put && move must stay.\n\nThe same reasoning explains why http-push has to ignore the info/refs file \nwhen looking for common refs, BTW.\n\nCiao,\nDscho\n"},{"id":"101051","messageId":"7vocy4wfb2.fsf@gitster.siamese.dyndns.org","threadId":"17229","inReplyTo":"alpine.DEB.1.00.0901181425420.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-18T20:05:21Z","receivedAt":"2009-01-18T20:05:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The point is: the repository inside the document root of the web server is \n> still a valid repository.\n>\n> And the assumption is that whenever you have a file that looks like a \n> valid object/pack inside a valid repository, that it does not need \n> replacing.\n>\n> So even when optimizing the uncommon (HTTP push is 2nd class citizen), we \n> have to keep the common workflow intact (1st class citizens _are_ push by \n> file system, ssh or git://).\n\nThe first class citizens are \"local use\", not \"copying out of the area\nthat looks like a repository only to http:// transport users\".\n\n> Which unfortunately means that put && move must stay.\n\nI still do not understand why it is unfortunate.\n\nAs far as I understand, Ray's issue was that his filesystem did not like\nthe temporary file name that is used for the initial \"put\" phase because\nit contained a character it did not like (colon, perhaps). Why isn't the\npatch about changing _that_ single issue?\n"},{"id":"101060","messageId":"alpine.DEB.1.00.0901182214180.3586@pacific.mpi-cbg.de","threadId":"17229","inReplyTo":"7vocy4wfb2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-18T21:14:44Z","receivedAt":"2009-01-18T21:14:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 18 Jan 2009, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Which unfortunately means that put && move must stay.\n> \n> I still do not understand why it is unfortunate.\n\nBecause it is slow.\n\nCiao,\nDscho\n"},{"id":"101066","messageId":"7vvdscthmq.fsf@gitster.siamese.dyndns.org","threadId":"17229","inReplyTo":"alpine.DEB.1.00.0901182214180.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-18T21:43:25Z","receivedAt":"2009-01-18T21:43:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Sun, 18 Jan 2009, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>> \n>> > Which unfortunately means that put && move must stay.\n>> \n>> I still do not understand why it is unfortunate.\n>\n> Because it is slow.\n\nIf \"slow\" is a problem, why are you using http to begin with ;-)?\n\nI'd take slow but reliable any day over fast and mostly works but\nunreliable.\n"},{"id":"101068","messageId":"alpine.DEB.1.00.0901182258330.3586@pacific.mpi-cbg.de","threadId":"17229","inReplyTo":"7vvdscthmq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-18T21:58:54Z","receivedAt":"2009-01-18T21:58:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 18 Jan 2009, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > On Sun, 18 Jan 2009, Junio C Hamano wrote:\n> >\n> >> Johannes Schindelin writes:\n> >> \n> >> > Which unfortunately means that put && move must stay.\n> >> \n> >> I still do not understand why it is unfortunate.\n> >\n> > Because it is slow.\n> \n> If \"slow\" is a problem, why are you using http to begin with ;-)?\n\nHeh, indeed.\n\n> I'd take slow but reliable any day over fast and mostly works but \n> unreliable.\n\nRight. Issue settled,\nDscho\n"},{"id":"101070","messageId":"bd6139dc0901181612p5181c36eub337a339cd777da0@mail.gmail.com","threadId":"17229","inReplyTo":"7vvdscthmq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-01-19T00:12:44Z","receivedAt":"2009-01-19T00:12:44Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Sun, Jan 18, 2009 at 22:43, Junio C Hamano wrote:\n> If \"slow\" is a problem, why are you using http to begin with ;-)?\n\nBecause http might be the only available protocol?\n\n> I'd take slow but reliable any day over fast and mostly works but\n> unreliable.\n\nYes, but if we want, say, git support at code.google.com (which I do,\nI totally detest having to use svn because of this), it would be nice\nto not dismiss http as \"being slow anyway, so who cares about not\nmaking it faster\"?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"101087","messageId":"7v3afgt2n5.fsf@gitster.siamese.dyndns.org","threadId":"17229","inReplyTo":"bd6139dc0901181612p5181c36eub337a339cd777da0@mail.gmail.com","subject":"Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-19T03:07:10Z","receivedAt":"2009-01-19T03:07:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Sverre Rabbelier\" writes:\n\n> I totally detest having to use svn because of this), it would be nice\n> to not dismiss http as \"being slow anyway, so who cares about not\n> making it faster\"?\n\nNobody is arguing for \"keeping it slow at all costs\".\n\nBut \"replace put && move with a single put, and we do not care if that\ncorrupts the resulting repository if it gets interrupted\" is a different\nstory.\n"}]}