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

From: Andy Parkins <andyparkins@gmail.com>
Date: Feb 20, 2007, 08:41 UTC
Message-ID: <200702200841.26578.andyparkins@gmail.com>
In-Reply-To: <7vlkit7vy5.fsf@assigned-by-dhcp.cox.net>

On Monday 2007 February 19 20:07, Junio C Hamano wrote:

Show 6 quoted lines

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

Not huge quantities. The patch is so wide-spread, that I've checked it compiles, and I've run with it for a bit. However, I could not swear to having checked every path...

Show 15 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?

<shame> Yes.

> 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

The output of 
 $ git-grep "master" -- '*.c' '*.h'
Is not that big, so those special treatments can be easily found irrespective 
of my patch.

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

As I've said before, I'm against /any/ special treatment of master other than as a default branch name in a newly initialised repository. PATH_REFS_HEADS_MASTER is closer to master not being special than without so I'd be in favour of that.

Show 10 quoted lines

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

Wow; that would be clever - regardless of whether this patch is acceptable or not.

Andy

-- 
Dr Andy Parkins, M Eng (hons), MIEE
andyparkins@gmail.com