Re: [PATCH 2/3] http-push: when sending objects, don't PUT before MOVE

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 17, 2009, 06:13 UTC
Message-ID: <alpine.DEB.1.00.0901170706450.3586@pacific.mpi-cbg.de>
In-Reply-To: <be6fef0d0901161859g4e5d749et78b6b0231eb915ee@mail.gmail.com>

Hi,

On Sat, 17 Jan 2009, Ray Chuan wrote:

> since 753bc911f489748a837ecb5ea4b5216220b24845, the opaquelocktocken

It would be nice to use the form <abbrev-sha1>(<oneline>) instead of a non-abbreviated SHA-1 that everybody who is interested has to look up, wasting time.

Show 5 quoted lines

> URI isn't used, so it doesn't make sense to PUT then MOVE.
> 
> currently, git PUTs to
> 
> /repo.git/objects/1a/1a2b..._opaquelocktoken:1234-....

First you say that the opaquelocktoken URI is not used, but here it looks like one?

> on some platforms, ':' isn't allowed in filenames so this fails
> (assuming the server doesn't recognize it as opaquelocktoken scheme).
> in fact, i don't think this is the correct implementation of the
> scheme; 'opaquelocktoken: ' should come in front of the path.

It would be nice to make that a fact-backed commit message. IOW there has to be some documentation about the subject which you can quote, and which would give you a definitive answer to the question if it should be a prefix or not.

Show 32 quoted lines

> diff --git a/src/git-1.6.1/http-push.c b/src/git-1.6.1/http-push.c
> index a646a49..838ff6f 100644
> --- a/src/git-1.6.1/http-push.c
> +++ b/src/git-1.6.1/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"
> @@ -104,7 +103,6 @@ enum transfer_state {
>  	NEED_PUSH,
>  	RUN_MKCOL,
>  	RUN_PUT,
> -	RUN_MOVE,
>  	ABORTED,
>  	COMPLETE,
>  };
> @@ -528,11 +526,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);
> 
>  	slot = get_active_slot();
>  	slot->callback_func = process_response;

Color me puzzled again. Why is this code no longer needed? Is this the lock you were talking about?

Show 13 quoted lines

> @@ -705,23 +672,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;
> -		}

... and here comes my first doubt that it would be a good idea to avoid "put && move"; what if "put" fails? Then you end up with a corrupt repository.

Ciao, Dscho