Re: [PATCH] handle http urls with query string ("?foo") correctly

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 5, 2008, 00:12 UTC
Message-ID: <alpine.DEB.1.00.0806050103520.21190@racer>
In-Reply-To: <200806050128.33467.bombe@pterodactylus.net>

Hi,

On Thu, 5 Jun 2008, David ‘Bombe’ Roden wrote:

> From 1f55b9176248673b78c77ff292fb5c6c988a7f8b Mon Sep 17 00:00:00 2001
> From: =?utf-8?q?David=20=E2=80=98Bombe=E2=80=99=20Roden?= <bombe@pterodactylus.net>
> Date: Thu, 5 Jun 2008 00:53:56 +0200
> Subject: [PATCH] handle http urls with query string ("?foo") correctly

Please follow the hints in Documentation/SubmittingPatches, or just imitate others' mails with patches.

Show 6 quoted lines

> I'm a developer for the Freenet Project[1] and tried to get git running 
> with Freenet's http-gateway. For several reasons I have to use URLs like 
> "http://host:8888/USK@<lots-of-stuff>/project.git/4?type=text/plain". 
> This breaks the way git creates http URLs to retrieve. The following 
> patch remedys this situation by inserting the git-generated part of the 
> URL (like "/info/refs") before the query string.

I think that this paragraph should be rewritten into a less "I did" form, and be the commit message.

Show 18 quoted lines

> diff --git a/http-walker.c b/http-walker.c
> index 99f397e..711e55b 100644
> --- a/http-walker.c
> +++ b/http-walker.c
> @@ -100,7 +100,6 @@ static void start_object_request(struct walker *walker,
>  	char *hex = sha1_to_hex(obj_req->sha1);
>  	char prevfile[PATH_MAX];
>  	char *url;
> -	char *posn;
>  	int prevlocal;
>  	unsigned char prev_buf[PREV_BUF_SIZE];
>  	ssize_t prev_read = 0;
> @@ -146,16 +145,11 @@ static void start_object_request(struct walker *walker,
>  
>  	SHA1_Init(&obj_req->c);
>  
> -	url = xmalloc(strlen(obj_req->repo->base) + 51);
> +	char *suffix = xmalloc(51);

I guess you are a Java programmer? In C, this would be "char suffix[51];".

Also, we like to have C89 compatibility, especially when it is too simple: 
declarations have to come _before_ statements.

Show 12 quoted lines

>  	obj_req->url = xmalloc(strlen(obj_req->repo->base) + 51);
> -	strcpy(url, obj_req->repo->base);
> -	posn = url + strlen(obj_req->repo->base);
> -	strcpy(posn, "/objects/");
> -	posn += 9;
> -	memcpy(posn, hex, 2);
> -	posn += 2;
> -	*(posn++) = '/';
> -	strcpy(posn, hex + 2);
> +	strcpy(suffix, "/objects/");
> +	sprintf(suffix + 9, "%c%c/%s", hex[0], hex[1], hex + 2);
> +	url = transform_url(obj_req->repo->base, suffix);

This part of Git's source code predates the strbuf work, and therefore it is understandable that strbufs are not used there. However, I think that your changes just cry for want of strbufs.

Also, the strcpy() could be merged into the sprintf().

Show 9 quoted lines

> @@ -384,8 +378,9 @@ static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned ch
>  	if (walker->get_verbosely)
>  		fprintf(stderr, "Getting index for pack %s\n", hex);
>  
> -	url = xmalloc(strlen(repo->base) + 64);
> -	sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
> +	char *index_url = xmalloc(64);
> +	sprintf(index_url, "/objects/pack/pack-%s.idx", hex);
> +	url = transform_url(repo->base, index_url);

Again, declaration-after-statement, and again an opportunity for an strbuf.

Maybe you want to rewrite your patch before I keep on repeating these two comments? ;-)

Thanks, Dscho