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

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

[Ray Chuan]

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

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

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

[Junio C Hamano]

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

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

[Ray Chuan]

Hi,

On Sun, Jan 18, 2009 at 12:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "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.

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

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

[Johannes Schindelin]

Hi,

On Sun, 18 Jan 2009, Ray Chuan wrote:

> 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

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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?

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

[Johannes Schindelin]

Hi,

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.

Ciao,
Dscho

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

[Junio C Hamano]

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 ;-)?

I'd take slow but reliable any day over fast and mostly works but
unreliable.

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

[Johannes Schindelin]

Hi,

On Sun, 18 Jan 2009, Junio C Hamano wrote:

> 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

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

[Sverre Rabbelier]

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

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

[Junio C Hamano]

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