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

[Andy Parkins <andyparkins@gmail.com>]

On Tuesday 2007 October 02, Jeff King wrote:

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

Actually, I felt that was an improvement.  Personally I always like paths 
shown to a user to have the trailing slash so that they can be clear that it 
is a path not a file.

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

It's been a while, but at the time I did it I think I checked 
safe_create_dir() to be sure that it was happy with trailing slashes.

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

Okay.

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

It is.   A patch to fix these is in my pending list.

> > -	strcpy(posn, "/objects/");
> > +	strcpy(posn, "/" PATH_OBJECTS);
> >  	posn += 9;
>
> should be posn += 1 + STRLEN_PATH_OBJECTS ?

Well spotted.  Fixed.

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

I put a comment above the other changes like this in the same file, but 
thought I was being overly verbose putting it in every single time.  I'm 
happy to copy the comment around in the file though.

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

I was trying, as far as I could, to leave the code unchanged.  If strbuf would 
be better I think I'd rather do it with another patch after this.

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

It really was wasn't it?  :-)   While I was trying to find that bug that you 
caught yesterday I thought I was going blind.  I have this to say though: 
thank heaven for git's colourised diffs.  Those that think coloured output is 
just for prettiness sake should try that review with and without.

Thank you for expending so much effort on this, it is appreciated.



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