Re: [PATCH] Change "refs/" references to symbolic constants
- From
Jeff King <peff@peff.net>
- Date
- Oct 2, 2007, 19:11 UTC
- Message-ID
- <20071002191104.GA7901@coredump.intra.peff.net>
- In-Reply-To
- <200710021916.44388.andyparkins@gmail.com>
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.
Show 6 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...
Show 9 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.
Show 9 quoted lines
> - 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