threads / patch / 17229

patch, 2 partshttp-push: remove MOVE step after PUT when sending objects to server

Subject: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server

## tl;dr

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

replies: 9people: 4as markdown or json

Ray Chuan· Jan 17, 2009, 20:24 UTC · lore
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 <rctay89@gmail.com>
---
 http-push.c |   45 +--------------------------------------------
 1 files changed, 1 insertions(+), 44 deletions(-)
Show changes to http-push.c +1 −44
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· Jan 18, 2009, 00:48 UTC · re: Ray Chuan · lore

Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server

"Ray Chuan" <rctay89@gmail.com> writes:
Show 15 quoted lines
> 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· Jan 18, 2009, 03:19 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server

Hi,
On Sun, Jan 18, 2009 at 12:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> "Ray Chuan" <rctay89@gmail.com> 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.

Show 9 quoted lines
>> 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· Jan 18, 2009, 13:32 UTC · re: Ray Chuan · lore

Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server

Hi,
On Sun, 18 Jan 2009, Ray Chuan wrote:
Show 16 quoted lines
> On Sun, Jan 18, 2009 at 12:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> > "Ray Chuan" <rctay89@gmail.com> 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· Jan 18, 2009, 20:05 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 10 quoted lines
> 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· Jan 18, 2009, 21:14 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server

Hi,
On Sun, 18 Jan 2009, Junio C Hamano wrote:
Show 5 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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· Jan 18, 2009, 21:43 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 9 quoted lines
> On Sun, 18 Jan 2009, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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· Jan 18, 2009, 21:58 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server

Hi,
On Sun, 18 Jan 2009, Junio C Hamano wrote:
Show 13 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Sun, 18 Jan 2009, Junio C Hamano wrote:
> >
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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· Jan 19, 2009, 00:12 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server

On Sun, Jan 18, 2009 at 22:43, Junio C Hamano <gitster@pobox.com> 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· Jan 19, 2009, 03:07 UTC · re: Sverre Rabbelier · lore

Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server

"Sverre Rabbelier" <srabbelier@gmail.com> 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.

← back to recent threads