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

From: Andy Parkins <andyparkins@gmail.com>
Date: Oct 3, 2007, 07:50 UTC
Message-ID: <200710030850.19614.andyparkins@gmail.com>
In-Reply-To: <20071002191104.GA7901@coredump.intra.peff.net>

On Tuesday 2007 October 02, Jeff King wrote:

Show 9 quoted lines

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

Show 14 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Well spotted.  Fixed.

Show 8 quoted lines

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

Show 9 quoted lines

> 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