Andy Parkins <andyparkins@xxxxxxxxx> writes:
> Changed repeated use of the same constants for the ref paths to be
> symbolic constants. I've defined them in refs.h
>
> refs/ is now PATH_REFS
> refs/heads/ is now PATH_REFS_HEADS
> refs/tags/ is now PATH_REFS_TAGS
> refs/remotes/ is now PATH_REFS_REMOTES
Your example:
> ... This has clarified the code in some places; for
> example:
>
> - 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.
> 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?
> @@ -231,7 +232,7 @@ static int create_default_files(const char *git_dir, const char *template_path)
> strcpy(path + len, "HEAD");
> reinit = !read_ref("HEAD", sha1);
> if (!reinit) {
> - if (create_symref("HEAD", "refs/heads/master", NULL) < 0)
> + if (create_symref("HEAD", PATH_REFS_HEADS "master", NULL) < 0)
> exit(1);
> }
>
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
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.
> diff --git a/builtin-pack-refs.c b/builtin-pack-refs.c
> index 3de9b3e..ac7543d 100644
> --- a/builtin-pack-refs.c
> +++ b/builtin-pack-refs.c
> @@ -36,7 +36,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
> /* Do not pack the symbolic refs */
> if ((flags & REF_ISSYMREF))
> return 0;
> - is_tag_ref = !strncmp(path, "refs/tags/", 10);
> + is_tag_ref = !strncmp(path, PATH_REFS_TAGS, STRLEN_PATH_REFS_TAGS);
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.
-
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