[PATCH] http-push: support full URI in handle_remote_ls_ctx()

[PATCH] http-push: support full URI in handle_remote_ls_ctx()

[Kirill A. Korinskiy]

The program calls remote_ls() to get list of files from the server
over HTTP; handle_remote_ls_ctx() is used to parse its response to
populate "struct remote_ls_ctx" that is returned from remote_ls().

The handle_remote_ls_ctx() function assumed that the server returns a
local path in href field, but RFC 4918 (14.7) demand of support full
URI (e.g. "http://localhost:8080/repo.git").

This resulted in push failure (e.g. git-http-push issues a PROPFIND
request to "/repo.git/alhost:8080/repo.git/refs/" to the server).

Signed-off-by: Kirill A. Korinskiy <catap@catap.ru>
---
 http-push.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/http-push.c b/http-push.c
index 7c6460919bf3eba10c46cede11ffdd9c53fd2dd2..2cb925a9ad857b6d79858d5187f14072167282e7 100644
--- a/http-push.c
+++ b/http-push.c
@@ -87,6 +87,7 @@ static struct object_list *objects;
 struct repo
 {
 	char *url;
+	char *path;
 	int path_len;
 	int has_info_refs;
 	int can_update_info_refs;
@@ -1424,9 +1425,18 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
 				ls->userFunc(ls);
 			}
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_NAME) && ctx->cdata) {
-			ls->dentry_name = xmalloc(strlen(ctx->cdata) -
+			char *path = ctx->cdata;
+			if (!strcmp(ctx->cdata, "http://")) {
+				path = strchr(path + sizeof("http://") - 1, '/');
+			} else if (!strcmp(ctx->cdata, "https://")) {
+				path = strchr(path + sizeof("https://") - 1, '/');
+			}
+
+			path += remote->path_len;
+
+			ls->dentry_name = xmalloc(strlen(path) -
 						  remote->path_len + 1);
-			strcpy(ls->dentry_name, ctx->cdata + remote->path_len);
+			strcpy(ls->dentry_name, path + remote->path_len);
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_COLLECTION)) {
 			ls->dentry_flags |= IS_DIR;
 		}
@@ -2206,10 +2216,11 @@ int main(int argc, char **argv)
 		if (!remote->url) {
 			char *path = strstr(arg, "//");
 			remote->url = arg;
+			remote->path_len = strlen(arg);
 			if (path) {
-				path = strchr(path+2, '/');
-				if (path)
-					remote->path_len = strlen(path);
+				remote->path = strchr(path+2, '/');
+				if (remote->path)
+					remote->path_len = strlen(remote->path);
 			}
 			continue;
 		}
@@ -2238,8 +2249,9 @@ int main(int argc, char **argv)
 		rewritten_url = xmalloc(strlen(remote->url)+2);
 		strcpy(rewritten_url, remote->url);
 		strcat(rewritten_url, "/");
+		remote->path = rewritten_url + (remote->path - remote->url);
+		remote->path_len++;
 		remote->url = rewritten_url;
-		++remote->path_len;
 	}
 
 	/* Verify DAV compliance/lock support */
-- 
1.5.6.5

Re: [PATCH] http-push: support full URI in handle_remote_ls_ctx()

[Johannes Schindelin]

Hi,

On Sun, 18 Jan 2009, Kirill A. Korinskiy wrote:

> The program calls remote_ls() to get list of files from the server
> over HTTP; handle_remote_ls_ctx() is used to parse its response to
> populate "struct remote_ls_ctx" that is returned from remote_ls().
> 
> The handle_remote_ls_ctx() function assumed that the server returns a
> local path in href field, but RFC 4918 (14.7) demand of support full
> URI (e.g. "http://localhost:8080/repo.git").
> 
> This resulted in push failure (e.g. git-http-push issues a PROPFIND
> request to "/repo.git/alhost:8080/repo.git/refs/" to the server).
> 
> Signed-off-by: Kirill A. Korinskiy <catap@catap.ru>
> ---
>  http-push.c |   24 ++++++++++++++++++------
>  1 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/http-push.c b/http-push.c
> index 7c6460919bf3eba10c46cede11ffdd9c53fd2dd2..2cb925a9ad857b6d79858d5187f14072167282e7 100644

You mean this patch is not vs 
junio/next(bbe7a8ed3dac72b7a1372cd92f68f47965c10100) or junio/master or 
junio/maint(both a4b7d08663504a57008f66a39fffe293f62c1d08) but 
tags/v1.6.1-rc4~13?

Ciao,
Dscho

Re: [PATCH] http-push: support full URI in handle_remote_ls_ctx()

[Junio C Hamano]

"Kirill A. Korinskiy" <catap@catap.ru> writes:

> @@ -1424,9 +1425,18 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
>  				ls->userFunc(ls);
>  			}
>  		} else if (!strcmp(ctx->name, DAV_PROPFIND_NAME) && ctx->cdata) {
> -			ls->dentry_name = xmalloc(strlen(ctx->cdata) -
> +			char *path = ctx->cdata;
> +			if (!strcmp(ctx->cdata, "http://")) {
> +				path = strchr(path + sizeof("http://") - 1, '/');
> +			} else if (!strcmp(ctx->cdata, "https://")) {
> +				path = strchr(path + sizeof("https://") - 1, '/');
> +			}
> +
> +			path += remote->path_len;

I see you chose to address the issue I pointed out in:

    http://thread.gmane.org/gmane.comp.version-control.git/103804/focus=104363

by being more strict.  That's the only change I can spot compared to
e1f33ef (http-push: support full URI in handle_remote_ls_ctx(),
2008-12-23) that is already in maint.

Could you make this into an incremental patch?

Re: [PATCH] handle_remote_ls_ctx can parsing href starting at http://

[Junio C Hamano]

"Kirill A. Korinskiy" <catap@catap.ru> writes:

> The program call remote_ls() to get remote objects over http;
> handle_remote_ls_ctx() is used to parse it's response to populated
> "struct remote_ls_ctx" that is returned from remote_ls().
>
> The handle_remote_ls_ctx() function assumed that the server will
> returned local path in href field, but RFC 4918 demand of support full
> URI (http://localhost/repo.git for example).
>
> This resulted in push failure (git-http-push ask server
> PROPFIND /repo.git/alhost:8080/repo.git/refs/) when a server returned
> full URI.

Thanks.

Do you mean PROPFIND was made to that garbage with :8080 in it when the
server returned a full URI http://localhost/repo.git as in the example in
the previous paragraph, or are you using a different example here?

I am contemplating of munging your commit log message like this...

commit e1f33efe07b9a520505fccd71bea1292fc9448dd
Author: Kirill A. Korinskiy <catap@catap.ru>
Date:   Tue Dec 23 11:31:15 2008 +0300

    http-push: support full URI in handle_remote_ls_ctx()
    
    The program calls remote_ls() to get list of files from the server over
    HTTP; handle_remote_ls_ctx() is used to parse its response to populate
    "struct remote_ls_ctx" that is returned from remote_ls().
    
    The handle_remote_ls_ctx() function assumed that the server returns a
    local path in href field, but RFC 4918 (14.7) demand of support full URI
    (e.g. "http://localhost:8080/repo.git").
    
    This resulted in push failure (e.g. git-http-push issues a PROPFIND
    request to "/repo.git/alhost:8080/repo.git/refs/" to the server).
    
    Signed-off-by: Kirill A. Korinskiy <catap@catap.ru>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

[PATCH] http-push: support full URI in handle_remote_ls_ctx()

[Kirill A. Korinskiy]

The program calls remote_ls() to get list of files from the server
over HTTP; handle_remote_ls_ctx() is used to parse its response to
populate "struct remote_ls_ctx" that is returned from remote_ls().

The handle_remote_ls_ctx() function assumed that the server returns a
local path in href field, but RFC 4918 (14.7) demand of support full
URI (e.g. "http://localhost:8080/repo.git").

This resulted in push failure (e.g. git-http-push issues a PROPFIND
request to "/repo.git/alhost:8080/repo.git/refs/" to the server).

Signed-off-by: Kirill A. Korinskiy <catap@catap.ru>
---
 http-push.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/http-push.c b/http-push.c
index 7c6460919bf3eba10c46cede11ffdd9c53fd2dd2..a4b7d08663504a57008f66a39fffe293f62c1d08 100644
--- a/http-push.c
+++ b/http-push.c
@@ -87,6 +87,7 @@ static struct object_list *objects;
 struct repo
 {
 	char *url;
+	char *path;
 	int path_len;
 	int has_info_refs;
 	int can_update_info_refs;
@@ -1424,9 +1425,19 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
 				ls->userFunc(ls);
 			}
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_NAME) && ctx->cdata) {
-			ls->dentry_name = xmalloc(strlen(ctx->cdata) -
+			char *path = ctx->cdata;
+			if (*ctx->cdata == 'h') {
+				path = strstr(path, "//");
+				if (path) {
+					path = strchr(path+2, '/');
+				}
+			}
+			if (path) {
+				path += remote->path_len;
+			}
+			ls->dentry_name = xmalloc(strlen(path) -
 						  remote->path_len + 1);
-			strcpy(ls->dentry_name, ctx->cdata + remote->path_len);
+			strcpy(ls->dentry_name, path + remote->path_len);
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_COLLECTION)) {
 			ls->dentry_flags |= IS_DIR;
 		}
@@ -2206,10 +2217,11 @@ int main(int argc, char **argv)
 		if (!remote->url) {
 			char *path = strstr(arg, "//");
 			remote->url = arg;
+			remote->path_len = strlen(arg);
 			if (path) {
-				path = strchr(path+2, '/');
-				if (path)
-					remote->path_len = strlen(path);
+				remote->path = strchr(path+2, '/');
+				if (remote->path)
+					remote->path_len = strlen(remote->path);
 			}
 			continue;
 		}
@@ -2238,8 +2250,9 @@ int main(int argc, char **argv)
 		rewritten_url = xmalloc(strlen(remote->url)+2);
 		strcpy(rewritten_url, remote->url);
 		strcat(rewritten_url, "/");
+		remote->path = rewritten_url + (remote->path - remote->url);
+		remote->path_len++;
 		remote->url = rewritten_url;
-		++remote->path_len;
 	}
 
 	/* Verify DAV compliance/lock support */
-- 
1.5.6.5

Re: [PATCH] http-push: support full URI in handle_remote_ls_ctx()

[Mike Hommey]

On Mon, Dec 29, 2008 at 05:32:15AM +0300, Kirill A. Korinskiy wrote:
> The program calls remote_ls() to get list of files from the server
> over HTTP; handle_remote_ls_ctx() is used to parse its response to
> populate "struct remote_ls_ctx" that is returned from remote_ls().
> 
> The handle_remote_ls_ctx() function assumed that the server returns a
> local path in href field, but RFC 4918 (14.7) demand of support full
> URI (e.g. "http://localhost:8080/repo.git").
> 
> This resulted in push failure (e.g. git-http-push issues a PROPFIND
> request to "/repo.git/alhost:8080/repo.git/refs/" to the server).
> 
> Signed-off-by: Kirill A. Korinskiy <catap@catap.ru>
> ---
>  http-push.c |   25 +++++++++++++++++++------
>  1 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/http-push.c b/http-push.c
> index 7c6460919bf3eba10c46cede11ffdd9c53fd2dd2..a4b7d08663504a57008f66a39fffe293f62c1d08 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -87,6 +87,7 @@ static struct object_list *objects;
>  struct repo
>  {
>  	char *url;
> +	char *path;
>  	int path_len;
>  	int has_info_refs;
>  	int can_update_info_refs;
> @@ -1424,9 +1425,19 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
>  				ls->userFunc(ls);
>  			}
>  		} else if (!strcmp(ctx->name, DAV_PROPFIND_NAME) && ctx->cdata) {
> -			ls->dentry_name = xmalloc(strlen(ctx->cdata) -
> +			char *path = ctx->cdata;
> +			if (*ctx->cdata == 'h') {
> +				path = strstr(path, "//");

I would make that a "://"

> +				if (path) {
> +					path = strchr(path+2, '/');

and s/2/3/, accordingly.

But I realize the existing code already does something like what your
are doing...

Mike

Re: [PATCH] http-push: support full URI in handle_remote_ls_ctx()

[Junio C Hamano]

Thanks; already applied ;-)

[PATCH] http-push: support full URI in handle_remote_ls_ctx()

[Kirill A. Korinskiy]

The program calls remote_ls() to get list of files from the server
over HTTP; handle_remote_ls_ctx() is used to parse its response to
populate "struct remote_ls_ctx" that is returned from remote_ls().

The handle_remote_ls_ctx() function assumed that the server returns a
local path in href field, but RFC 4918 (14.7) demand of support full
URI (e.g. "http://localhost:8080/repo.git").

This resulted in push failure (e.g. git-http-push issues a PROPFIND
request to "/repo.git/alhost:8080/repo.git/refs/" to the server).

Signed-off-by: Kirill A. Korinskiy <catap@catap.ru>
---
 http-push.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/http-push.c b/http-push.c
index 7c6460919bf3eba10c46cede11ffdd9c53fd2dd2..d1749fe2ffd6a59a4eea514997a62b5d7c80b438 100644
--- a/http-push.c
+++ b/http-push.c
@@ -87,6 +87,7 @@ static struct object_list *objects;
 struct repo
 {
 	char *url;
+	char *path;
 	int path_len;
 	int has_info_refs;
 	int can_update_info_refs;
@@ -1424,9 +1425,19 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
 				ls->userFunc(ls);
 			}
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_NAME) && ctx->cdata) {
-			ls->dentry_name = xmalloc(strlen(ctx->cdata) -
+			char *path = ctx->cdata;
+			if (*ctx->cdata == 'h') {
+				path = strstr(path, "://");
+				if (path) {
+					path = strchr(path+3, '/');
+				}
+			}
+			if (path) {
+				path += remote->path_len;
+			}
+			ls->dentry_name = xmalloc(strlen(path) -
 						  remote->path_len + 1);
-			strcpy(ls->dentry_name, ctx->cdata + remote->path_len);
+			strcpy(ls->dentry_name, path + remote->path_len);
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_COLLECTION)) {
 			ls->dentry_flags |= IS_DIR;
 		}
@@ -2206,10 +2217,11 @@ int main(int argc, char **argv)
 		if (!remote->url) {
 			char *path = strstr(arg, "//");
 			remote->url = arg;
+			remote->path_len = strlen(arg);
 			if (path) {
-				path = strchr(path+2, '/');
-				if (path)
-					remote->path_len = strlen(path);
+				remote->path = strchr(path+2, '/');
+				if (remote->path)
+					remote->path_len = strlen(remote->path);
 			}
 			continue;
 		}
@@ -2238,8 +2250,9 @@ int main(int argc, char **argv)
 		rewritten_url = xmalloc(strlen(remote->url)+2);
 		strcpy(rewritten_url, remote->url);
 		strcat(rewritten_url, "/");
+		remote->path = rewritten_url + (remote->path - remote->url);
+		remote->path_len++;
 		remote->url = rewritten_url;
-		++remote->path_len;
 	}
 
 	/* Verify DAV compliance/lock support */
-- 
1.5.6.5

Re: [PATCH] http-push: support full URI in handle_remote_ls_ctx()

[Junio C Hamano]

"Kirill A. Korinskiy" <catap@catap.ru> writes:

> @@ -1424,9 +1425,19 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
>  				ls->userFunc(ls);
>  			}
>  		} else if (!strcmp(ctx->name, DAV_PROPFIND_NAME) && ctx->cdata) {
> -			ls->dentry_name = xmalloc(strlen(ctx->cdata) -
> +			char *path = ctx->cdata;
> +			if (*ctx->cdata == 'h') {
> +				path = strstr(path, "://");
> +				if (path) {
> +					path = strchr(path+3, '/');
> +				}
> +			}

Is this "://" (and +3) the only change from the previous one that has
already been queued?  I didn't have a problem with the old "//" one.

The check to see if it begins with 'h' bothers me much much more.

If you want to be defensively tight, you should be checking if it begins
with either "http://" or "https://", the only two protocols you are
prepared to handle, and nothing else, so that you won't trigger this
codepath when the other end gave you "hqrt://..", on the basis that your
code won't know if hqrt:// protocol works the same way as http and https.

On the other hand, if you want to be optimistically loose, expecting
whatever people would implement that can be handled with the existing DAV
code would behave the same way as http and https, you shouldn't be
limiting yourself to an unknown protocol name that happens to begin with
an 'h', only accepting "hqrt://" but not "ittp://" URLs.

Your "first byte of the protocol name must be 'h'" does not do either.