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

From: Junio C Hamano <junkio@cox.net>
Date: Feb 19, 2007, 20:07 UTC
Message-ID: <7vlkit7vy5.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <200702191839.05784.andyparkins@gmail.com>

Andy Parkins <andyparkins@gmail.com> writes:

Show 7 quoted lines

> Changed repeated use of the same constants for the ref paths to be
> symbolic constants.  I've defined them in refs.h
>
>   refs/ is now PATH_REFS
>   refs/heads/ is now PATH_REFS_HEADS
>   refs/tags/ is now PATH_REFS_TAGS
>   refs/remotes/ is now PATH_REFS_REMOTES

Your example:

Show 5 quoted lines

> ...  This has clarified the code in some places; for
> example:
>
>  - len = strlen(refs[i]) + 11;
>  + len = strlen(refs[i]) + STRLEN_PATH_REFS_TAGS + 1;

shows that you've carefully looked at what the code does, instead of mindlessly replacing, which is a very good sign, but how much testing has this seen, I wonder.

Show 13 quoted lines

> diff --git a/builtin-describe.c b/builtin-describe.c
> index bcc6456..0f78363 100644
> --- a/builtin-describe.c
> +++ b/builtin-describe.c
> @@ -52,7 +52,7 @@ static int get_name(const char *path, const unsigned char *sha1, int flag, void
>  	 * If --tags, then any tags are used.
>  	 * Otherwise only annotated tags are used.
>  	 */
> -	if (!strncmp(path, "refs/tags/", 10)) {
> +	if (!strncmp(path, PATH_TAGS, STRLEN_PATH_TAGS)) {
>  		if (object->type == OBJ_TAG)
>  			prio = 2;
>  		else

This is PATH_REFS_TAGS isn't it?

Show 9 quoted lines

> @@ -231,7 +232,7 @@ static int create_default_files(const char *git_dir, const char *template_path)
>  	strcpy(path + len, "HEAD");
>  	reinit = !read_ref("HEAD", sha1);
>  	if (!reinit) {
> -		if (create_symref("HEAD", "refs/heads/master", NULL) < 0)
> +		if (create_symref("HEAD", PATH_REFS_HEADS "master", NULL) < 0)
>  			exit(1);
>  	}
>

I mildly mind this one, as it hurts grep-ability. I know this is one of the the only two places in git that 'master' branch is treated specially (and I think we would like to keep it that way --- that's why I want to be able to grep for "refs/heads/master" and see very few hits), so introducing PATH_REFS_HEADS_MASTER is probably not very productive either, but... hmmmm.

Show 10 quoted lines

> diff --git a/builtin-pack-refs.c b/builtin-pack-refs.c
> index 3de9b3e..ac7543d 100644
> --- a/builtin-pack-refs.c
> +++ b/builtin-pack-refs.c
> @@ -36,7 +36,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
>  	/* Do not pack the symbolic refs */
>  	if ((flags & REF_ISSYMREF))
>  		return 0;
> -	is_tag_ref = !strncmp(path, "refs/tags/", 10);
> +	is_tag_ref = !strncmp(path, PATH_REFS_TAGS, STRLEN_PATH_REFS_TAGS);

These repeated strncmp(p, X, STRLEN_X) almost makes me wonder if we want to introduce:

	inline int prefixcmp(a, b)
        {
        	return (strncmp(a, b, strlen(b));
        }

with clever preprocessor optimization to have compiler do strlen() when b is a string literal.