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.
> - 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...
> - 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.
> - 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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
