# [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server 10 messages from 2009-01-17 to 2009-01-19. Participants: Ray Chuan, Junio C Hamano, Johannes Schindelin, Sverre Rabbelier. Thread: https://gitlist.dev/t/17229 ## Ray Chuan, 2009-01-17 20:24 Subject: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server Message-ID: URL: https://gitlist.dev/e/be6fef0d0901171224y35c3d95cn2d38639ac03c3b8f%40mail.gmail.com ``` Currently, git PUTs to /repo.git/objects/1a/1a2b...9z_opaquelocktoken:1234-.... then MOVEs to /repo.git/objects/1a/1a2b...9z This is needless. In fact, the only time MOVE requests are sent is for this sole purpose (ie. of renaming an object). A concern raised was repository corruption in the event of failure during PUT. "put && move" won't afford any more protection than using simply "put", since info/refs is not updated if a PUT fails, so there is no cause for concern. Signed-off-by: Tay Ray Chuan --- http-push.c | 45 +-------------------------------------------- 1 files changed, 1 insertions(+), 44 deletions(-) diff --git a/http-push.c b/http-push.c index 4517cf2..d21fd3b 100644 --- a/http-push.c +++ b/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" @@ -105,7 +104,6 @@ enum transfer_state { NEED_PUSH, RUN_MKCOL, RUN_PUT, - RUN_MOVE, ABORTED, COMPLETE, }; @@ -531,11 +529,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); if_header = xmalloc(strlen(request->lock->token) + 25); sprintf(if_header, "If: <%s>", request->lock->token); @@ -565,32 +558,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; @@ -713,23 +680,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.5.6.3 ``` ## Junio C Hamano, 2009-01-18 00:48 Subject: Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server Message-ID: <7viqod5thi.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7viqod5thi.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` "Ray Chuan" writes: > Currently, git PUTs to > > /repo.git/objects/1a/1a2b...9z_opaquelocktoken:1234-.... > > then MOVEs to > > /repo.git/objects/1a/1a2b...9z > > This is needless. In fact, the only time MOVE requests are sent is for > this sole purpose (ie. of renaming an object). > > A concern raised was repository corruption in the event of failure > during PUT. "put && move" won't afford any more protection than using > simply "put", since info/refs is not updated if a PUT fails, so there > is no cause for concern. That's a completely bogus reasoning. Normal operation inside the repository that was pushed into won't look at info/refs at all. The true reason you want to avoid put-then-move is...? ``` ## Ray Chuan, 2009-01-18 03:19 Subject: Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server Message-ID: URL: https://gitlist.dev/e/be6fef0d0901171919ub28dda7ref6443abec3627aa%40mail.gmail.com In-Reply-To: <7viqod5thi.fsf@gitster.siamese.dyndns.org> ``` Hi, On Sun, Jan 18, 2009 at 12:48 AM, Junio C Hamano wrote: > "Ray Chuan" writes: > >> Currently, git PUTs to >> >> /repo.git/objects/1a/1a2b...9z_opaquelocktoken:1234-.... >> >> then MOVEs to >> >> /repo.git/objects/1a/1a2b...9z >> >> This is needless. In fact, the only time MOVE requests are sent is for >> this sole purpose (ie. of renaming an object). this would be my "true" reason. i do believe PUTting to a "temporary" file (ie with '__opaquelocktoken:1234-....' appended) to be a needless step. >> A concern raised was repository corruption in the event of failure >> during PUT. "put && move" won't afford any more protection than using >> simply "put", since info/refs is not updated if a PUT fails, so there >> is no cause for concern. > > That's a completely bogus reasoning. Normal operation inside the > repository that was pushed into won't look at info/refs at all. > > The true reason you want to avoid put-then-move is...? i mentioned this "repository corruption" issue as it was raised previously by Johannes (towards the bottom): http://article.gmane.org/gmane.comp.version-control.git/106031 -- Cheers, Ray Chuan ``` ## Johannes Schindelin, 2009-01-18 13:32 Subject: Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901181425420.3586%40pacific.mpi-cbg.de In-Reply-To: ``` Hi, On Sun, 18 Jan 2009, Ray Chuan wrote: > On Sun, Jan 18, 2009 at 12:48 AM, Junio C Hamano wrote: > > > "Ray Chuan" writes: > > > >> A concern raised was repository corruption in the event of failure > >> during PUT. "put && move" won't afford any more protection than using > >> simply "put", since info/refs is not updated if a PUT fails, so there > >> is no cause for concern. > > > > That's a completely bogus reasoning. Normal operation inside the > > repository that was pushed into won't look at info/refs at all. > > i mentioned this "repository corruption" issue as it was raised > previously by Johannes (towards the bottom): > > http://article.gmane.org/gmane.comp.version-control.git/106031 ,.. and Junio just raised a new concern. The repository as it is on the server could very well be served by other means, i.e. git:// and rsync://, and there could be cron jobs and interactive Git sessions trying to work with it. The point is: the repository inside the document root of the web server is still a valid repository. And the assumption is that whenever you have a file that looks like a valid object/pack inside a valid repository, that it does not need replacing. So even when optimizing the uncommon (HTTP push is 2nd class citizen), we have to keep the common workflow intact (1st class citizens _are_ push by file system, ssh or git://). Which unfortunately means that put && move must stay. The same reasoning explains why http-push has to ignore the info/refs file when looking for common refs, BTW. Ciao, Dscho ``` ## Junio C Hamano, 2009-01-18 20:05 Subject: Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server Message-ID: <7vocy4wfb2.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vocy4wfb2.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin writes: > The point is: the repository inside the document root of the web server is > still a valid repository. > > And the assumption is that whenever you have a file that looks like a > valid object/pack inside a valid repository, that it does not need > replacing. > > So even when optimizing the uncommon (HTTP push is 2nd class citizen), we > have to keep the common workflow intact (1st class citizens _are_ push by > file system, ssh or git://). The first class citizens are "local use", not "copying out of the area that looks like a repository only to http:// transport users". > Which unfortunately means that put && move must stay. I still do not understand why it is unfortunate. As far as I understand, Ray's issue was that his filesystem did not like the temporary file name that is used for the initial "put" phase because it contained a character it did not like (colon, perhaps). Why isn't the patch about changing _that_ single issue? ``` ## Johannes Schindelin, 2009-01-18 21:14 Subject: Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901182214180.3586%40pacific.mpi-cbg.de In-Reply-To: <7vocy4wfb2.fsf@gitster.siamese.dyndns.org> ``` Hi, On Sun, 18 Jan 2009, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Which unfortunately means that put && move must stay. > > I still do not understand why it is unfortunate. Because it is slow. Ciao, Dscho ``` ## Junio C Hamano, 2009-01-18 21:43 Subject: Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server Message-ID: <7vvdscthmq.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvdscthmq.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin writes: > On Sun, 18 Jan 2009, Junio C Hamano wrote: > >> Johannes Schindelin writes: >> >> > Which unfortunately means that put && move must stay. >> >> I still do not understand why it is unfortunate. > > Because it is slow. If "slow" is a problem, why are you using http to begin with ;-)? I'd take slow but reliable any day over fast and mostly works but unreliable. ``` ## Johannes Schindelin, 2009-01-18 21:58 Subject: Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901182258330.3586%40pacific.mpi-cbg.de In-Reply-To: <7vvdscthmq.fsf@gitster.siamese.dyndns.org> ``` Hi, On Sun, 18 Jan 2009, Junio C Hamano wrote: > Johannes Schindelin writes: > > > On Sun, 18 Jan 2009, Junio C Hamano wrote: > > > >> Johannes Schindelin writes: > >> > >> > Which unfortunately means that put && move must stay. > >> > >> I still do not understand why it is unfortunate. > > > > Because it is slow. > > If "slow" is a problem, why are you using http to begin with ;-)? Heh, indeed. > I'd take slow but reliable any day over fast and mostly works but > unreliable. Right. Issue settled, Dscho ``` ## Sverre Rabbelier, 2009-01-19 00:12 Subject: Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server Message-ID: URL: https://gitlist.dev/e/bd6139dc0901181612p5181c36eub337a339cd777da0%40mail.gmail.com In-Reply-To: <7vvdscthmq.fsf@gitster.siamese.dyndns.org> ``` On Sun, Jan 18, 2009 at 22:43, Junio C Hamano wrote: > If "slow" is a problem, why are you using http to begin with ;-)? Because http might be the only available protocol? > I'd take slow but reliable any day over fast and mostly works but > unreliable. Yes, but if we want, say, git support at code.google.com (which I do, I totally detest having to use svn because of this), it would be nice to not dismiss http as "being slow anyway, so who cares about not making it faster"? -- Cheers, Sverre Rabbelier ``` ## Junio C Hamano, 2009-01-19 03:07 Subject: Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server Message-ID: <7v3afgt2n5.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v3afgt2n5.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` "Sverre Rabbelier" writes: > I totally detest having to use svn because of this), it would be nice > to not dismiss http as "being slow anyway, so who cares about not > making it faster"? Nobody is arguing for "keeping it slow at all costs". But "replace put && move with a single put, and we do not care if that corrupts the resulting repository if it gets interrupted" is a different story. ```