Re: [PATCH] Change "refs/" references to symbolic constants

[Jeff King <peff@peff.net>]

On Tue, Oct 02, 2007 at 07:16:43PM +0100, Andy Parkins wrote:

> Changed repeated use of the same constants for the ref paths to be
> symbolic constants.  I've defined them in refs.h

I've manually inspected the patch. Comments are below.

> -		if (prefixcmp(head, "refs/heads/"))
> -			die("HEAD not found below refs/heads!");
> -		head += 11;
> +		if (prefixcmp(head, PATH_REFS_HEADS))
> +			die("HEAD not found below " PATH_REFS_HEADS "!");
> +		head += STRLEN_PATH_REFS_HEADS;

This slightly changes the message (extra "/"), but I don't think that is
a big deal...

> -	strcpy(path + len, "refs");
> +	strcpy(path + len, PATH_REFS);
>  	safe_create_dir(path, 1);
> -	strcpy(path + len, "refs/heads");
> +	strcpy(path + len, PATH_REFS_HEADS);
>  	safe_create_dir(path, 1);
> -	strcpy(path + len, "refs/tags");
> +	strcpy(path + len, PATH_REFS_TAGS);
>  	safe_create_dir(path, 1);

...but here it's not immediately obvious if the extra trailing "/" is
OK. Looks like the path just gets handed off to system calls trhough
safe_create_dir, and they are happy with the trailing slash. But it is a
behavior change.

> -		strcpy(path + len, "refs");
> +		strcpy(path + len, PATH_REFS);
>  		adjust_shared_perm(path);
> -		strcpy(path + len, "refs/heads");
> +		strcpy(path + len, PATH_REFS_HEADS);
>  		adjust_shared_perm(path);
> -		strcpy(path + len, "refs/tags");
> +		strcpy(path + len, PATH_REFS_TAGS);
>  		adjust_shared_perm(path);

And of course ditto here.

> -		if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p)
> +		if (snprintf(ref, sizeof(ref), PATH_REFS_TAGS"%s", *p)

I find the 'PATH_REFS_TAGS "%s"' (with a space) you used earlier a
little easier to read.

> -	if (len < 5 || memcmp(name, "refs/", 5))
> +	if (len < STRLEN_PATH_REFS || memcmp(name, PATH_REFS, STRLEN_PATH_REFS))

I imagine this was one of the times you mentioned before where prefixcmp
would be more readable. I would agree.

> -	strcpy(posn, "/objects/");
> +	strcpy(posn, "/" PATH_OBJECTS);
>  	posn += 9;

should be posn += 1 + STRLEN_PATH_OBJECTS ?

> -	url = xmalloc(strlen(repo->base) + 64);
> -	sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
> +	url = xmalloc(strlen(repo->base) + STRLEN_PATH_OBJECTS + 56);
> +	sprintf(url, "%s/" PATH_OBJECTS "pack/pack-%s.idx", repo->base, hex);

The '56' is still quite hard to verify as correct ("/" + "pack/pack-" +
".idx" + "\0"). But I wonder if trying to fix that will just make it
harder to read (perhaps a comment is in order?).

Or maybe using a strbuf here would be much more obviously correct?

> -	url = xmalloc(strlen(base) + 31);
> -	sprintf(url, "%s/objects/info/http-alternates", base);
> +	url = xmalloc(strlen(base) + STRLEN_PATH_OBJECTS + 23);
> +	sprintf(url, "%s/" PATH_OBJECTS "info/http-alternates", base);

Also a potential strbuf. Ther are more of this same form, but I'm not
going to bother pointing out each one.

> -- 
> 1.5.3.rc5.11.g312e

Man that was tedious. But I think every other change is purely
syntactic, so there shouldn't be any bugs.

-Peff