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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 2, 2009, 07:26 UTC
Message-ID: <7v7i5edv7u.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1230879195-8567-1-git-send-email-catap@catap.ru>

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

Show 12 quoted lines

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