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

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

[Andy Parkins]

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

I've changed all references to them and made constants for the string
lengths as well.  This has clarified the code in some places; for
example:

 - len = strlen(refs[i]) + 11;
 + len = strlen(refs[i]) + STRLEN_PATH_REFS_TAGS + 1;

In this case 11 isn't STRLEN_PATH_REFS_HEADS, as it is in most other
cases, it's TAGS + 1.  With the change to symbolic constants it's much
clearer what is happening.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 builtin-branch.c        |   28 ++++++++++++++--------------
 builtin-describe.c      |    2 +-
 builtin-fmt-merge-msg.c |    5 +++--
 builtin-fsck.c          |    2 +-
 builtin-init-db.c       |   15 ++++++++-------
 builtin-name-rev.c      |   10 +++++-----
 builtin-pack-refs.c     |    2 +-
 builtin-push.c          |    8 ++++----
 builtin-show-branch.c   |   34 +++++++++++++++++-----------------
 builtin-show-ref.c      |    6 +++---
 connect.c               |   18 +++++++++---------
 fetch-pack.c            |    6 +++---
 http-fetch.c            |    7 ++++---
 http-push.c             |    4 ++--
 local-fetch.c           |    3 ++-
 path.c                  |    5 +++--
 receive-pack.c          |    4 ++--
 reflog-walk.c           |    6 +++---
 refs.c                  |   26 +++++++++++++-------------
 refs.h                  |   17 +++++++++++++++++
 setup.c                 |    5 +++--
 sha1_name.c             |   10 +++++-----
 wt-status.c             |    5 +++--
 23 files changed, 126 insertions(+), 102 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index d0e7209..928f1fe 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -85,12 +85,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 
 	switch (kinds) {
 	case REF_REMOTE_BRANCH:
-		fmt = "refs/remotes/%s";
+		fmt = PATH_REFS_REMOTES "%s";
 		remote = "remote ";
 		force = 1;
 		break;
 	case REF_LOCAL_BRANCH:
-		fmt = "refs/heads/%s";
+		fmt = PATH_REFS_HEADS "%s";
 		remote = "";
 		break;
 	default:
@@ -178,15 +178,15 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 	int len;
 
 	/* Detect kind */
-	if (!strncmp(refname, "refs/heads/", 11)) {
+	if (!strncmp(refname, PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS)) {
 		kind = REF_LOCAL_BRANCH;
-		refname += 11;
-	} else if (!strncmp(refname, "refs/remotes/", 13)) {
+		refname += STRLEN_PATH_REFS_HEADS;
+	} else if (!strncmp(refname, PATH_REFS_REMOTES, STRLEN_PATH_REFS_REMOTES)) {
 		kind = REF_REMOTE_BRANCH;
-		refname += 13;
-	} else if (!strncmp(refname, "refs/tags/", 10)) {
+		refname += STRLEN_PATH_REFS_REMOTES;
+	} else if (!strncmp(refname, PATH_REFS_TAGS, STRLEN_PATH_REFS_TAGS)) {
 		kind = REF_TAG;
-		refname += 10;
+		refname += STRLEN_PATH_REFS_TAGS;
 	}
 
 	/* Don't add types the caller doesn't want */
@@ -318,7 +318,7 @@ static void create_branch(const char *name, const char *start_name,
 	char ref[PATH_MAX], msg[PATH_MAX + 20];
 	int forcing = 0;
 
-	snprintf(ref, sizeof ref, "refs/heads/%s", name);
+	snprintf(ref, sizeof ref, PATH_REFS_HEADS "%s", name);
 	if (check_ref_format(ref))
 		die("'%s' is not a valid branch name.", name);
 
@@ -366,13 +366,13 @@ static void rename_branch(const char *oldname, const char *newname, int force)
 	if (!oldname)
 		die("cannot rename the current branch while not on any.");
 
-	if (snprintf(oldref, sizeof(oldref), "refs/heads/%s", oldname) > sizeof(oldref))
+	if (snprintf(oldref, sizeof(oldref), PATH_REFS_HEADS "%s", oldname) > sizeof(oldref))
 		die("Old branchname too long");
 
 	if (check_ref_format(oldref))
 		die("Invalid branch name: %s", oldref);
 
-	if (snprintf(newref, sizeof(newref), "refs/heads/%s", newname) > sizeof(newref))
+	if (snprintf(newref, sizeof(newref), PATH_REFS_HEADS "%s", newname) > sizeof(newref))
 		die("New branchname too long");
 
 	if (check_ref_format(newref))
@@ -476,9 +476,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		detached = 1;
 	}
 	else {
-		if (strncmp(head, "refs/heads/", 11))
-			die("HEAD not found below refs/heads!");
-		head += 11;
+		if (strncmp(head, PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS))
+			die("HEAD not found below " PATH_REFS_HEADS "!");
+		head += STRLEN_PATH_REFS_HEADS;
 	}
 
 	if (delete)
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
diff --git a/builtin-fmt-merge-msg.c b/builtin-fmt-merge-msg.c
index 87d3d63..d0615b5 100644
--- a/builtin-fmt-merge-msg.c
+++ b/builtin-fmt-merge-msg.c
@@ -4,6 +4,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "tag.h"
+#include "refs.h"
 
 static const char *fmt_merge_msg_usage =
 	"git-fmt-merge-msg [--summary] [--no-summary] [--file <file>]";
@@ -280,8 +281,8 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 	current_branch = resolve_ref("HEAD", head_sha1, 1, NULL);
 	if (!current_branch)
 		die("No current branch");
-	if (!strncmp(current_branch, "refs/heads/", 11))
-		current_branch += 11;
+	if (!strncmp(current_branch, PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS))
+		current_branch += STRLEN_PATH_REFS_HEADS;
 
 	while (fgets(line, sizeof(line), in)) {
 		i++;
diff --git a/builtin-fsck.c b/builtin-fsck.c
index 6da3814..0109816 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -546,7 +546,7 @@ static int fsck_head_link(void)
 
 	if (!head_points_at || !(flag & REF_ISSYMREF))
 		return error("HEAD is not a symbolic ref");
-	if (strncmp(head_points_at, "refs/heads/", 11))
+	if (strncmp(head_points_at, PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS))
 		return error("HEAD points to something strange (%s)",
 			     head_points_at);
 	if (is_null_sha1(sha1))
diff --git a/builtin-init-db.c b/builtin-init-db.c
index 12e43d0..4e5c881 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -5,6 +5,7 @@
  */
 #include "cache.h"
 #include "builtin.h"
+#include "refs.h"
 
 #ifndef DEFAULT_GIT_TEMPLATE_DIR
 #define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates/"
@@ -193,11 +194,11 @@ static int create_default_files(const char *git_dir, const char *template_path)
 	/*
 	 * Create .git/refs/{heads,tags}
 	 */
-	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);
 
 	/* First copy the templates -- we might have the default
@@ -216,11 +217,11 @@ static int create_default_files(const char *git_dir, const char *template_path)
 	if (shared_repository) {
 		path[len] = 0;
 		adjust_shared_perm(path);
-		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);
 	}
 
@@ -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);
 	}
 
diff --git a/builtin-name-rev.c b/builtin-name-rev.c
index 36f1ba6..47a0f6d 100644
--- a/builtin-name-rev.c
+++ b/builtin-name-rev.c
@@ -85,7 +85,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
 	struct name_ref_data *data = cb_data;
 	int deref = 0;
 
-	if (data->tags_only && strncmp(path, "refs/tags/", 10))
+	if (data->tags_only && strncmp(path, PATH_REFS_TAGS, STRLEN_PATH_REFS_TAGS))
 		return 0;
 
 	if (data->ref_filter && fnmatch(data->ref_filter, path, 0))
@@ -101,10 +101,10 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
 	if (o && o->type == OBJ_COMMIT) {
 		struct commit *commit = (struct commit *)o;
 
-		if (!strncmp(path, "refs/heads/", 11))
-			path = path + 11;
-		else if (!strncmp(path, "refs/", 5))
-			path = path + 5;
+		if (!strncmp(path, PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS))
+			path = path + STRLEN_PATH_REFS_HEADS;
+		else if (!strncmp(path, PATH_REFS, STRLEN_PATH_REFS))
+			path = path + STRLEN_PATH_REFS;
 
 		name_rev(commit, xstrdup(path), 0, 0, deref);
 	}
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);
 
 	/* ALWAYS pack refs that were already packed or are tags */
 	if (!cb->all && !is_tag_ref && !(flags & REF_ISPACKED))
diff --git a/builtin-push.c b/builtin-push.c
index c45649e..caeb9ae 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -30,9 +30,9 @@ static void add_refspec(const char *ref)
 static int expand_one_ref(const char *ref, const unsigned char *sha1, int flag, void *cb_data)
 {
 	/* Ignore the "refs/" at the beginning of the refname */
-	ref += 5;
+	ref += STRLEN_PATH_REFS;
 
-	if (!strncmp(ref, "tags/", 5))
+	if (!strncmp(ref, PATH_TAGS, STRLEN_PATH_TAGS))
 		add_refspec(xstrdup(ref));
 	return 0;
 }
@@ -123,9 +123,9 @@ static void set_refspecs(const char **refs, int nr)
 				int len;
 				if (nr <= ++i)
 					die("tag shorthand without <tag>");
-				len = strlen(refs[i]) + 11;
+				len = strlen(refs[i]) + STRLEN_PATH_REFS_TAGS + 1;
 				tag = xmalloc(len);
-				strcpy(tag, "refs/tags/");
+				strcpy(tag, PATH_REFS_TAGS);
 				strcat(tag, refs[i]);
 				ref = tag;
 			}
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 0d94e40..9995780 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -377,36 +377,36 @@ static int append_ref(const char *refname, const unsigned char *sha1,
 static int append_head_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	unsigned char tmp[20];
-	int ofs = 11;
-	if (strncmp(refname, "refs/heads/", ofs))
+	int ofs = STRLEN_PATH_REFS_HEADS;
+	if (strncmp(refname, PATH_REFS_HEADS, ofs))
 		return 0;
 	/* If both heads/foo and tags/foo exists, get_sha1 would
 	 * get confused.
 	 */
 	if (get_sha1(refname + ofs, tmp) || hashcmp(tmp, sha1))
-		ofs = 5;
+		ofs = STRLEN_PATH_REFS;
 	return append_ref(refname + ofs, sha1, 0);
 }
 
 static int append_remote_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	unsigned char tmp[20];
-	int ofs = 13;
-	if (strncmp(refname, "refs/remotes/", ofs))
+	int ofs = STRLEN_PATH_REFS_REMOTES;
+	if (strncmp(refname, PATH_REFS_REMOTES, ofs))
 		return 0;
 	/* If both heads/foo and tags/foo exists, get_sha1 would
 	 * get confused.
 	 */
 	if (get_sha1(refname + ofs, tmp) || hashcmp(tmp, sha1))
-		ofs = 5;
+		ofs = STRLEN_PATH_REFS;
 	return append_ref(refname + ofs, sha1, 0);
 }
 
 static int append_tag_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
-	if (strncmp(refname, "refs/tags/", 10))
+	if (strncmp(refname, PATH_REFS_TAGS, STRLEN_PATH_REFS_TAGS))
 		return 0;
-	return append_ref(refname + 5, sha1, 0);
+	return append_ref(refname + STRLEN_PATH_REFS, sha1, 0);
 }
 
 static const char *match_ref_pattern = NULL;
@@ -435,9 +435,9 @@ static int append_matching_ref(const char *refname, const unsigned char *sha1, i
 		return 0;
 	if (fnmatch(match_ref_pattern, tail, 0))
 		return 0;
-	if (!strncmp("refs/heads/", refname, 11))
+	if (!strncmp(PATH_REFS_HEADS, refname, STRLEN_PATH_REFS_HEADS))
 		return append_head_ref(refname, sha1, flag, cb_data);
-	if (!strncmp("refs/tags/", refname, 10))
+	if (!strncmp(PATH_REFS_TAGS, refname, STRLEN_PATH_REFS_TAGS))
 		return append_tag_ref(refname, sha1, flag, cb_data);
 	return append_ref(refname, sha1, 0);
 }
@@ -462,12 +462,12 @@ static int rev_is_head(char *head, int headlen, char *name,
 	if ((!head[0]) ||
 	    (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
 		return 0;
-	if (!strncmp(head, "refs/heads/", 11))
-		head += 11;
-	if (!strncmp(name, "refs/heads/", 11))
-		name += 11;
-	else if (!strncmp(name, "heads/", 6))
-		name += 6;
+	if (!strncmp(head, PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS))
+		head += STRLEN_PATH_REFS_HEADS;
+	if (!strncmp(name, PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS))
+		name += STRLEN_PATH_REFS_HEADS;
+	else if (!strncmp(name, PATH_HEADS, STRLEN_PATH_HEADS))
+		name += STRLEN_PATH_HEADS;
 	return !strcmp(head, name);
 }
 
@@ -777,7 +777,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				has_head++;
 		}
 		if (!has_head) {
-			int pfxlen = strlen("refs/heads/");
+			int pfxlen = STRLEN_PATH_REFS_HEADS;
 			append_one_rev(head + pfxlen);
 		}
 	}
diff --git a/builtin-show-ref.c b/builtin-show-ref.c
index 853f13f..2c20cde 100644
--- a/builtin-show-ref.c
+++ b/builtin-show-ref.c
@@ -28,8 +28,8 @@ static int show_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	if (tags_only || heads_only) {
 		int match;
 
-		match = heads_only && !strncmp(refname, "refs/heads/", 11);
-		match |= tags_only && !strncmp(refname, "refs/tags/", 10);
+		match = heads_only && !strncmp(refname, PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS);
+		match |= tags_only && !strncmp(refname, PATH_REFS_TAGS, STRLEN_PATH_REFS_TAGS);
 		if (!match)
 			return 0;
 	}
@@ -224,7 +224,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
 		unsigned char sha1[20];
 
 		while (*pattern) {
-			if (!strncmp(*pattern, "refs/", 5) &&
+			if (!strncmp(*pattern, PATH_REFS, STRLEN_PATH_REFS) &&
 			    resolve_ref(*pattern, sha1, 1, NULL)) {
 				if (!quiet)
 					show_one(*pattern, sha1);
diff --git a/connect.c b/connect.c
index 7844888..8701de0 100644
--- a/connect.c
+++ b/connect.c
@@ -11,23 +11,23 @@ static int check_ref(const char *name, int len, unsigned int flags)
 	if (!flags)
 		return 1;
 
-	if (len < 5 || memcmp(name, "refs/", 5))
+	if (len < STRLEN_PATH_REFS || memcmp(name, PATH_REFS, STRLEN_PATH_REFS))
 		return 0;
 
 	/* Skip the "refs/" part */
-	name += 5;
-	len -= 5;
+	name += STRLEN_PATH_REFS;
+	len -= STRLEN_PATH_REFS;
 
 	/* REF_NORMAL means that we don't want the magic fake tag refs */
 	if ((flags & REF_NORMAL) && check_ref_format(name) < 0)
 		return 0;
 
 	/* REF_HEADS means that we want regular branch heads */
-	if ((flags & REF_HEADS) && !memcmp(name, "heads/", 6))
+	if ((flags & REF_HEADS) && !memcmp(name, PATH_HEADS, STRLEN_PATH_HEADS))
 		return 1;
 
 	/* REF_TAGS means that we want tags */
-	if ((flags & REF_TAGS) && !memcmp(name, "tags/", 5))
+	if ((flags & REF_TAGS) && !memcmp(name, PATH_TAGS, STRLEN_PATH_TAGS))
 		return 1;
 
 	/* All type bits clear means that we are ok with anything */
@@ -196,8 +196,8 @@ static int count_refspec_match(const char *pattern,
 		 */
 		if (namelen != patlen &&
 		    patlen != namelen - 5 &&
-		    strncmp(name, "refs/heads/", 11) &&
-		    strncmp(name, "refs/tags/", 10)) {
+		    strncmp(name, PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS) &&
+		    strncmp(name, PATH_REFS_TAGS, STRLEN_PATH_REFS_TAGS)) {
 			/* We want to catch the case where only weak
 			 * matches are found and there are multiple
 			 * matches, and where more than one strong
@@ -284,7 +284,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
 		case 1:
 			break;
 		case 0:
-			if (!memcmp(rs[i].dst, "refs/", 5)) {
+			if (!memcmp(rs[i].dst, PATH_REFS, STRLEN_PATH_REFS)) {
 				int len = strlen(rs[i].dst) + 1;
 				matched_dst = xcalloc(1, sizeof(*dst) + len);
 				memcpy(matched_dst->name, rs[i].dst, len);
@@ -305,7 +305,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
 				errs = 1;
 				error("dst refspec %s does not match any "
 				      "existing ref on the remote and does "
-				      "not start with refs/.", rs[i].dst);
+				      "not start with " PATH_REFS ".", rs[i].dst);
 			}
 			break;
 		default:
diff --git a/fetch-pack.c b/fetch-pack.c
index c787106..14a74d2 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -342,11 +342,11 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
 
 	for (ref = *refs; ref; ref = next) {
 		next = ref->next;
-		if (!memcmp(ref->name, "refs/", 5) &&
-		    check_ref_format(ref->name + 5))
+		if (!memcmp(ref->name, PATH_REFS, STRLEN_PATH_REFS) &&
+		    check_ref_format(ref->name + STRLEN_PATH_REFS))
 			; /* trash */
 		else if (fetch_all &&
-			 (!depth || strncmp(ref->name, "refs/tags/", 10) )) {
+			 (!depth || strncmp(ref->name, PATH_REFS_TAGS, STRLEN_PATH_REFS_TAGS) )) {
 			*newtail = ref;
 			ref->next = NULL;
 			newtail = &ref->next;
diff --git a/http-fetch.c b/http-fetch.c
index 9f790a0..79ef85a 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -3,6 +3,7 @@
 #include "pack.h"
 #include "fetch.h"
 #include "http.h"
+#include "refs.h"
 
 #define PREV_BUF_SIZE 4096
 #define RANGE_HEADER_SIZE 30
@@ -938,14 +939,14 @@ static char *quote_ref_url(const char *base, const char *ref)
 	int len, baselen, ch;
 
 	baselen = strlen(base);
-	len = baselen + 6; /* "refs/" + NUL */
+	len = baselen + STRLEN_PATH_REFS + 1; /* "refs/" + NUL */
 	for (cp = ref; (ch = *cp) != 0; cp++, len++)
 		if (needs_quote(ch))
 			len += 2; /* extra two hex plus replacement % */
 	qref = xmalloc(len);
 	memcpy(qref, base, baselen);
-	memcpy(qref + baselen, "refs/", 5);
-	for (cp = ref, dp = qref + baselen + 5; (ch = *cp) != 0; cp++) {
+	memcpy(qref + baselen, PATH_REFS, STRLEN_PATH_REFS);
+	for (cp = ref, dp = qref + baselen + STRLEN_PATH_REFS; (ch = *cp) != 0; cp++) {
 		if (needs_quote(ch)) {
 			*dp++ = '%';
 			*dp++ = hex((ch >> 4) & 0xF);
diff --git a/http-push.c b/http-push.c
index b128c01..37d2f50 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1922,7 +1922,7 @@ static void get_local_heads(void)
 static void get_dav_remote_heads(void)
 {
 	remote_tail = &remote_refs;
-	remote_ls("refs/", (PROCESS_FILES | PROCESS_DIRS | RECURSIVE), process_ls_ref, NULL);
+	remote_ls(PATH_REFS, (PROCESS_FILES | PROCESS_DIRS | RECURSIVE), process_ls_ref, NULL);
 }
 
 static int is_zero_sha1(const unsigned char *sha1)
@@ -2066,7 +2066,7 @@ static void update_remote_info_refs(struct remote_lock *lock)
 	buffer.buffer = xcalloc(1, 4096);
 	buffer.size = 4096;
 	buffer.posn = 0;
-	remote_ls("refs/", (PROCESS_FILES | RECURSIVE),
+	remote_ls(PATH_REFS, (PROCESS_FILES | RECURSIVE),
 		  add_remote_info_ref, &buffer);
 	if (!aborted) {
 		if_header = xmalloc(strlen(lock->token) + 25);
diff --git a/local-fetch.c b/local-fetch.c
index 7cfe8b3..7b80a31 100644
--- a/local-fetch.c
+++ b/local-fetch.c
@@ -4,6 +4,7 @@
 #include "cache.h"
 #include "commit.h"
 #include "fetch.h"
+#include "refs.h"
 
 static int use_link;
 static int use_symlink;
@@ -174,7 +175,7 @@ int fetch_ref(char *ref, unsigned char *sha1)
 	int ifd;
 
 	if (ref_name_start < 0) {
-		sprintf(filename, "%s/refs/", path);
+		sprintf(filename, "%s/" PATH_REFS, path);
 		ref_name_start = strlen(filename);
 	}
 	strcpy(filename + ref_name_start, ref);
diff --git a/path.c b/path.c
index c5d25a4..dfbae16 100644
--- a/path.c
+++ b/path.c
@@ -11,6 +11,7 @@
  * which is what it's designed for.
  */
 #include "cache.h"
+#include "refs.h"
 
 static char bad_path[] = "/bad-path/";
 
@@ -103,7 +104,7 @@ int validate_headref(const char *path)
 	/* Make sure it is a "refs/.." symlink */
 	if (S_ISLNK(st.st_mode)) {
 		len = readlink(path, buffer, sizeof(buffer)-1);
-		if (len >= 5 && !memcmp("refs/", buffer, 5))
+		if (len >= 5 && !memcmp(PATH_REFS, buffer, STRLEN_PATH_REFS))
 			return 0;
 		return -1;
 	}
@@ -127,7 +128,7 @@ int validate_headref(const char *path)
 		len -= 4;
 		while (len && isspace(*buf))
 			buf++, len--;
-		if (len >= 5 && !memcmp("refs/", buf, 5))
+		if (len >= STRLEN_PATH_REFS && !memcmp(PATH_REFS, buf, STRLEN_PATH_REFS))
 			return 0;
 	}
 
diff --git a/receive-pack.c b/receive-pack.c
index 7311c82..6fd9f60 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -109,7 +109,7 @@ static int update(struct command *cmd)
 	struct ref_lock *lock;
 
 	cmd->error_string = NULL;
-	if (!strncmp(name, "refs/", 5) && check_ref_format(name + 5)) {
+	if (!strncmp(name, PATH_REFS, STRLEN_PATH_REFS) && check_ref_format(name + STRLEN_PATH_REFS)) {
 		cmd->error_string = "funny refname";
 		return error("refusing to create funny ref '%s' locally",
 			     name);
@@ -125,7 +125,7 @@ static int update(struct command *cmd)
 	}
 	if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
 	    !is_null_sha1(old_sha1) &&
-	    !strncmp(name, "refs/heads/", 11)) {
+	    !strncmp(name, PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS)) {
 		struct commit *old_commit, *new_commit;
 		struct commit_list *bases, *ent;
 
diff --git a/reflog-walk.c b/reflog-walk.c
index c983858..c05a404 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -55,11 +55,11 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)
 	}
 	if (reflogs->nr == 0) {
 		int len = strlen(ref);
-		char *refname = xmalloc(len + 12);
-		sprintf(refname, "refs/%s", ref);
+		char *refname = xmalloc(len + STRLEN_PATH_REFS_HEADS + 1);
+		sprintf(refname, PATH_REFS "%s", ref);
 		for_each_reflog_ent(refname, read_one_reflog, reflogs);
 		if (reflogs->nr == 0) {
-			sprintf(refname, "refs/heads/%s", ref);
+			sprintf(refname, PATH_REFS_HEADS "%s", ref);
 			for_each_reflog_ent(refname, read_one_reflog, reflogs);
 		}
 		free(refname);
diff --git a/refs.c b/refs.c
index 6387703..997b360 100644
--- a/refs.c
+++ b/refs.c
@@ -261,7 +261,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
 		/* Follow "normalized" - ie "refs/.." symlinks by hand */
 		if (S_ISLNK(st.st_mode)) {
 			len = readlink(path, buffer, sizeof(buffer)-1);
-			if (len >= 5 && !memcmp("refs/", buffer, 5)) {
+			if (len >= STRLEN_PATH_REFS && !memcmp(PATH_REFS, buffer, STRLEN_PATH_REFS)) {
 				buffer[len] = 0;
 				strcpy(ref_buffer, buffer);
 				ref = ref_buffer;
@@ -413,22 +413,22 @@ int head_ref(each_ref_fn fn, void *cb_data)
 
 int for_each_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/", fn, 0, cb_data);
+	return do_for_each_ref(PATH_REFS, fn, 0, cb_data);
 }
 
 int for_each_tag_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/tags/", fn, 10, cb_data);
+	return do_for_each_ref(PATH_REFS_TAGS, fn, STRLEN_PATH_REFS_TAGS, cb_data);
 }
 
 int for_each_branch_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/heads/", fn, 11, cb_data);
+	return do_for_each_ref(PATH_REFS_HEADS, fn, STRLEN_PATH_REFS_HEADS, cb_data);
 }
 
 int for_each_remote_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/remotes/", fn, 13, cb_data);
+	return do_for_each_ref(PATH_REFS_REMOTES, fn, STRLEN_PATH_REFS_REMOTES, cb_data);
 }
 
 /* NEEDSWORK: This is only used by ssh-upload and it should go; the
@@ -440,7 +440,7 @@ int get_ref_sha1(const char *ref, unsigned char *sha1)
 {
 	if (check_ref_format(ref))
 		return -1;
-	return read_ref(mkpath("refs/%s", ref), sha1);
+	return read_ref(mkpath(PATH_REFS "%s", ref), sha1);
 }
 
 /*
@@ -657,7 +657,7 @@ struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)
 	char refpath[PATH_MAX];
 	if (check_ref_format(ref))
 		return NULL;
-	strcpy(refpath, mkpath("refs/%s", ref));
+	strcpy(refpath, mkpath(PATH_REFS "%s", ref));
 	return lock_ref_sha1_basic(refpath, old_sha1, NULL);
 }
 
@@ -828,12 +828,12 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 		goto rollback;
 	}
 
-	if (!strncmp(oldref, "refs/heads/", 11) &&
-			!strncmp(newref, "refs/heads/", 11)) {
+	if (!strncmp(oldref, PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS) &&
+			!strncmp(newref, PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS)) {
 		char oldsection[1024], newsection[1024];
 
-		snprintf(oldsection, 1024, "branch.%s", oldref + 11);
-		snprintf(newsection, 1024, "branch.%s", newref + 11);
+		snprintf(oldsection, 1024, "branch.%s", oldref + STRLEN_PATH_REFS_HEADS);
+		snprintf(newsection, 1024, "branch.%s", newref + STRLEN_PATH_REFS_HEADS);
 		if (git_config_rename_section(oldsection, newsection) < 0)
 			return 1;
 	}
@@ -894,8 +894,8 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 	log_file = git_path("logs/%s", ref_name);
 
 	if (log_all_ref_updates &&
-	    (!strncmp(ref_name, "refs/heads/", 11) ||
-	     !strncmp(ref_name, "refs/remotes/", 13) ||
+	    (!strncmp(ref_name,  PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS) ||
+	     !strncmp(ref_name, PATH_REFS_REMOTES, STRLEN_PATH_REFS_HEADS) ||
 	     !strcmp(ref_name, "HEAD"))) {
 		if (safe_create_leading_directories(log_file) < 0)
 			return error("unable to create directory for %s",
diff --git a/refs.h b/refs.h
index acedffc..a986b42 100644
--- a/refs.h
+++ b/refs.h
@@ -13,6 +13,23 @@ struct ref_lock {
 #define REF_ISSYMREF 01
 #define REF_ISPACKED 02
 
+#define PATH_OBJECTS             "objects/"
+#define STRLEN_PATH_OBJECTS      8
+#define PATH_REFS                "refs/"
+#define STRLEN_PATH_REFS         5
+#define PATH_HEADS               "heads/"
+#define STRLEN_PATH_HEADS        6
+#define PATH_TAGS                "tags/"
+#define STRLEN_PATH_TAGS         5
+#define PATH_REMOTES             "remotes/"
+#define STRLEN_PATH_REMOTES      8
+#define PATH_REFS_HEADS          PATH_REFS PATH_HEADS
+#define STRLEN_PATH_REFS_HEADS   (STRLEN_PATH_REFS+STRLEN_PATH_HEADS)
+#define PATH_REFS_TAGS           PATH_REFS PATH_TAGS
+#define STRLEN_PATH_REFS_TAGS    (STRLEN_PATH_REFS+STRLEN_PATH_TAGS)
+#define PATH_REFS_REMOTES        PATH_REFS PATH_REMOTES
+#define STRLEN_PATH_REFS_REMOTES (STRLEN_PATH_REFS+STRLEN_PATH_REMOTES)
+
 /*
  * Calls the specified function for each ref file until it returns nonzero,
  * and returns the value
diff --git a/setup.c b/setup.c
index e9d3f5a..782fc07 100644
--- a/setup.c
+++ b/setup.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "refs.h"
 
 const char *prefix_path(const char *prefix, int len, const char *path)
 {
@@ -154,12 +155,12 @@ static int is_git_directory(const char *suspect)
 			return 0;
 	}
 	else {
-		strcpy(path + len, "/objects");
+		strcpy(path + len, "/" PATH_OBJECTS);
 		if (access(path, X_OK))
 			return 0;
 	}
 
-	strcpy(path + len, "/refs");
+	strcpy(path + len, "/" PATH_REFS);
 	if (access(path, X_OK))
 		return 0;
 
diff --git a/sha1_name.c b/sha1_name.c
index a7efa96..add6123 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -237,11 +237,11 @@ static int ambiguous_path(const char *path, int len)
 
 static const char *ref_fmt[] = {
 	"%.*s",
-	"refs/%.*s",
-	"refs/tags/%.*s",
-	"refs/heads/%.*s",
-	"refs/remotes/%.*s",
-	"refs/remotes/%.*s/HEAD",
+	PATH_REFS "%.*s",
+	PATH_REFS_TAGS "%.*s",
+	PATH_REFS_HEADS "%.*s",
+	PATH_REFS_REMOTES "%.*s",
+	PATH_REFS_REMOTES "%.*s/HEAD",
 	NULL
 };
 
diff --git a/wt-status.c b/wt-status.c
index 2879c3d..9f2705c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -7,6 +7,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "diffcore.h"
+#include "refs.h"
 
 int wt_status_use_color = 0;
 static char wt_status_colors[][COLOR_MAXLEN] = {
@@ -298,8 +299,8 @@ void wt_status_print(struct wt_status *s)
 	if (s->branch) {
 		const char *on_what = "On branch ";
 		const char *branch_name = s->branch;
-		if (!strncmp(branch_name, "refs/heads/", 11))
-			branch_name += 11;
+		if (!strncmp(branch_name, PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS))
+			branch_name += STRLEN_PATH_REFS_HEADS;
 		else if (!strcmp(branch_name, "HEAD")) {
 			branch_name = "";
 			on_what = "Not currently on any branch.";
-- 
1.5.0.rc4.gb4d2

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

[Bill Lear]

On Monday, February 19, 2007 at 18:39:05 (+0000) Andy Parkins writes:
>...
>+#define PATH_REMOTES             "remotes/"
>+#define STRLEN_PATH_REMOTES      8
>...

Would this be less error-prone, and just as efficient?:

#define PATH_REMOTES "remotes/"
#define LIT_STRLEN(S) ((sizeof(S) / sizeof(S[0])) -1)
#define STRLEN_PATH_REMOTES LIT_STRLEN(PATH_REMOTES)


Bill

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

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> 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.

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

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> > 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.

This may be worthwhile.  We use strncmp so often in Git that I
tend to write strncmp even when I mean strcmp.  Yes, my fingers are
trained to write strncmp, and only strncmp...  *sigh* At least the
compiler checks for me.  ;-)

-- 
Shawn.

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

[Krzysztof Halasa]

Bill Lear <rael@zopyra.com> writes:

> Would this be less error-prone, and just as efficient?:
>
> #define PATH_REMOTES "remotes/"
> #define LIT_STRLEN(S) ((sizeof(S) / sizeof(S[0])) -1)
> #define STRLEN_PATH_REMOTES LIT_STRLEN(PATH_REMOTES)

sizeof(char) is always 1.
-- 
Krzysztof Halasa

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

[Shawn O. Pearce]

Krzysztof Halasa <khc@pm.waw.pl> wrote:
> Bill Lear <rael@zopyra.com> writes:
> 
> > Would this be less error-prone, and just as efficient?:
> >
> > #define PATH_REMOTES "remotes/"
> > #define LIT_STRLEN(S) ((sizeof(S) / sizeof(S[0])) -1)
> > #define STRLEN_PATH_REMOTES LIT_STRLEN(PATH_REMOTES)
> 
> sizeof(char) is always 1.

Except when someone does something silly, like say:

	#define char double

:-)

-- 
Shawn.

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

[Johannes Schindelin]

Hi,

On Mon, 19 Feb 2007, Junio C Hamano wrote:

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

I am in favour of that. BTW I remember that Han-Wen suggested this some 
time ago, too.

Ciao,
Dscho

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

[Andy Parkins]

On Monday 2007 February 19 20:07, Junio C Hamano wrote:

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

Not huge quantities.  The patch is so wide-spread, that I've checked it 
compiles, and I've run with it for a bit.  However, I could not swear to 
having checked every path...


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

<shame> Yes.

> 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

The output of 
 $ git-grep "master" -- '*.c' '*.h'
Is not that big, so those special treatments can be easily found irrespective 
of my patch.

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

As I've said before, I'm against /any/ special treatment of master other than 
as a default branch name in a newly initialised repository.  
PATH_REFS_HEADS_MASTER is closer to master not being special than without so 
I'd be in favour of that.

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

Wow; that would be clever - regardless of whether this patch is acceptable or 
not.


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

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

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> As I've said before, I'm against /any/ special treatment of
> master other than as a default branch name in a newly
> initialised repository.  PATH_REFS_HEADS_MASTER is closer to
> master not being special than without so I'd be in favour of
> that.

Ok.

>> 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.
>
> Wow; that would be clever - regardless of whether this patch
> is acceptable or not.

Actually GCC seems to be clever enough.  It appears that I do
not even have to do prefixcmp_0() below; prefixcmp_1() seems to
generate good code, with GCC 4.1.2 prerelease on my x86_64.

-- >8 --
#include <string.h>
static inline int prefixcmp_0(const char *a, const char *b)
{
	if (__builtin_constant_p(b))
		return strncmp(a, b, sizeof(b) - 1);
	else
		return strncmp(a, b, strlen(b));
}

static inline prefixcmp_1(const char *a, const char *b)
{
	return strncmp(a, b, strlen(b));
}

void foo(const char *s, const char *t, int *a, int *b, int *c, int *d)
{
	*a = prefixcmp_0(s, "abcdefg");
	*b = prefixcmp_0(s, t);
	*c = prefixcmp_1(s, "ABCDEFGH");
	*d = prefixcmp_1(s, t);
}
-- 8< --

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

[Andy Parkins]

On Monday 2007 February 19 20:07, Junio C Hamano wrote:

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

I'm uncomfortable that this one managed to get through; in view of this 
mistake if you are thinking of applying this - don't.  I'm going to review 
the patch itself line by line.

Also - I should learn how to run the tests - is "make test" good enough or is 
there something special I should do?


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

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

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> On Monday 2007 February 19 20:07, Junio C Hamano wrote:
>
>> > -	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?
>
> I'm uncomfortable that this one managed to get through; in view of this 
> mistake if you are thinking of applying this - don't.  I'm going to review 
> the patch itself line by line.

I'd send the prefixcmp() patches first, as yours would touch the
same lines.

> Also - I should learn how to run the tests - is "make test"
> good enough or is there something special I should do?

Should be enough.

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

[Andy Parkins]

On Tuesday 2007 February 20 09:50, Junio C Hamano wrote:

> I'd send the prefixcmp() patches first, as yours would touch the
> same lines.

Okay.

Your prefixcmp() point about them being used so regularly made me wonder if 
the following would improve readability:

static inline ref_is_head(const char *a)
{
    return (prefixcmp(a, PATH_REFS_HEADS) == 0);
}
static inline ref_is_tag(const char *a)
{
    return (prefixcmp(a, PATH_REFS_TAGS) == 0);
}
static inline ref_is_remote(const char *a)
{
    return (prefixcmp(a, PATH_REFS_REMOTES) == 0);
}

which would in turn convert:

   if (!strncmp(head, "refs/heads/", 11))
       head += 11;

into

   if (ref_is_head(head))
       head += STRLEN_PATH_REFS_HEADS;

which expresses the intent of the code far more clearly.



Andy

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

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

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> Your prefixcmp() point about them being used so regularly made me wonder if 
> the following would improve readability:
>
> static inline ref_is_head(const char *a)
> {
>     return (prefixcmp(a, PATH_REFS_HEADS) == 0);
> }
> ...
>    if (ref_is_head(head))
>        head += STRLEN_PATH_REFS_HEADS;
>
> which expresses the intent of the code far more clearly.

If we _were_ doing the inline function, I would actually prefer:

        static inline ref_is_head(const char *ref)
        {
		return !prefixcmp(ref, PATH_REFS_HEADS);
        }

But at least to me,

	if (!prefixcmp(head, PATH_REFS_HEADS))
		head += strlen(PATH_REFS_HEADS);

is easier to follow than:

        if (ref_is_head(head))
                head += STRLEN_PATH_REFS_HEADS;

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

[Andy Parkins]

On Tuesday 2007 February 20 10:30, Junio C Hamano wrote:

> If we _were_ doing the inline function, I would actually prefer:
>
>         static inline ref_is_head(const char *ref)
>         {
> 		return !prefixcmp(ref, PATH_REFS_HEADS);
>         }

As you brought it up...

I've never really liked "!" on strcmp() lines (but I accept that that is the 
tradition in git) because it implies the the output of prefixcmp is boolean, 
but it's actually ternary.  strcmp() (I think), should be thought of as 
outputting

enum {
 STRING1_LESS_THAN_STRING2,
 STRINGS_EQUAL,
 STRING1_GREATER_THAN_STRING2
}

Given that, it makes me uncomfortable to use !strcmp().  Of course in the case 
of strcmp(), that form is so well known that it makes very little difference 
to the reader.

I have similar feelings about

 if( !something )

being incorrect when you meant

 if( something == NULL )

While they are identical in what they generate, they send a different message 
to someone reading the code.

Regardless, I'm not so stubborn as to refuse to go with the flow...

> But at least to me,
>
> 	if (!prefixcmp(head, PATH_REFS_HEADS))
> 		head += strlen(PATH_REFS_HEADS);
>
> is easier to follow than:
>
>         if (ref_is_head(head))
>                 head += STRLEN_PATH_REFS_HEADS;

Fine.  I don't really mind - and it's less work on my patch :-)

My argument in favour of the ref_is_head() method is that the prefixcmp() 
method requires knowledge from the caller about how you tell whether a given 
ref is a head - the second pushes that information further down the call 
tree, abstracting it out just a little more.

As I say though - it's not a problem for me.



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

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

[Johannes Schindelin]

Hi,

On Tue, 20 Feb 2007, Andy Parkins wrote:

> I've never really liked "!" on strcmp() lines (but I accept that that is the 
> tradition in git) because it implies the the output of prefixcmp is boolean, 
> but it's actually ternary.

Actually, it's not even ternary, but to the return value should only be 
handled in terms of >0, ==0, <0.

Ah, and if "!" implies a boolean, then why is "!!" a common construct? 
Because "!" really does not imply a boolean.

Ciao,
Dscho

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

[Simon 'corecode' Schubert]

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

Johannes Schindelin wrote:
>> I've never really liked "!" on strcmp() lines (but I accept that that is the 
>> tradition in git) because it implies the the output of prefixcmp is boolean, 
>> but it's actually ternary.
> 
> Actually, it's not even ternary, but to the return value should only be 
> handled in terms of >0, ==0, <0.
> 
> Ah, and if "!" implies a boolean, then why is "!!" a common construct? 
> Because "!" really does not imply a boolean.

Depends on how you look at it.  I code using semantics which use expressions only as boolean if they are really are.  So NULL pointers are not treated like a boolean, and neither are errno nor strcmp.  For me that's part of good, readable style, but people/groups of course are free to disagree.  Even after so many years of breathing C, I find   "if (!strcmp(foo, bar))" misleading, suggesting "not compare", which translates to "not equal".  Of course I know it, and can work with it, but in my own code I'd never write this.  I don't see any gain except some obfuscation.

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

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

[Johannes Schindelin]

Hi,

On Tue, 20 Feb 2007, Simon 'corecode' Schubert wrote:

> Johannes Schindelin wrote:
> > > I've never really liked "!" on strcmp() lines (but I accept that that is
> > > the tradition in git) because it implies the the output of prefixcmp is
> > > boolean, but it's actually ternary.
> > 
> > Actually, it's not even ternary, but to the return value should only be
> > handled in terms of >0, ==0, <0.
> > 
> > Ah, and if "!" implies a boolean, then why is "!!" a common construct?
> > Because "!" really does not imply a boolean.
> 
> Depends on how you look at it.  I code using semantics which use 
> expressions only as boolean if they are really are.

There are no booleans in C.

Also, you just state that the construct is not common for _you_. It is 
really quite common in C. Why? Because it is a short way to say _exactly_ 
what you want. Like when you say "BTW" instead of "by the way". It is not 
only quicker to type, it is also quicker to read.

> Even after so many years of breathing C, I find "if (!strcmp(foo, bar))" 
> misleading, suggesting "not compare", which translates to "not equal".

No, in plain English "!strcmp("nothing", u)" translates to "nothing 
compares to u.

In mathematics, which is the basis of computer languages, "not compare" 
means something completely different yet: "a" does not compare to "b" 
means that they cannot be compared at all, i.e. the statement "a<b" is 
neither true nor false.

So, we have -- as so often -- a case, where somebody says "it is obvious 
how this expression translates to English", but really, it is not. So, why 
not stay in the context, and interpret it like millions of programmers 
before us? Or do you want to start another Babel?

Ciao,
Dscho

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

[Andy Parkins]

On Tuesday 2007 February 20 11:37, Johannes Schindelin wrote:

> Actually, it's not even ternary, but to the return value should only be
> handled in terms of >0, ==0, <0.

Apologies, I'm not using ternary in the sense of the number 0, 1 and 2; I'm 
using it in the sense of there being three possible outcomes - which there 
are.  This is similar to how operators are categorised into unary, binary and 
ternary categories.  I use ternary to mean "having three states or elements", 
rather than "having the value 0, 1 or 2".

> Ah, and if "!" implies a boolean, then why is "!!" a common construct?
> Because "!" really does not imply a boolean.

Boolean expressions are those that have two possible states - "true" 
or "false".  There are no real booleans in C, so they are faked.  The two 
boolean states in C are represented by

 False = Equal to zero
 True = Not equal to zero

Now, the !! construction you suggest is perfectly in keeping with this 
definition.  Ironically, the only reason you need the !! construction is 
because of this integer-as-boolean trait of C.

So, to get a boolean in C you use a standard integer (say).  My contention is 
that to improve clarity you should not mix integer-as-boolean and 
integer-as-integer, even though C will accept it when you do.

 i = 10;
 while( i )
   i--;

This is bad, it abuses the fact that C will let you treat an integer as a 
boolean.  while() takes a boolean expression as it's argument, so I think 
that you should always hand it something that would be a boolean output (even 
though C doesn't care if you don't).

 i = 10;
 while( i > 0 )
  i--;

This makes it clear to the reader that i is not boolean.  Obviously this is a 
trivial example; no one would have any trouble understanding either of the 
two examples.  When things start to get bigger and more complicated though, 
more clarity is always better than less clarity.  The principle I try to 
follow is that code is write-once-read-many.  If you save yourself two 
keystrokes at the expense of the clarity you gain for the 100 times you read 
that code, then you have made a false economy.

In the end - I don't care - I was only countering Junio's "I prefer !strcmp", 
with my reasons why I don't like it.  I do not expect git to change to my 
preferred coding style, and I do try to keep to the coding style that the git 
project uses.  To my mind, inconsistency is a worse offence than anything 
else in a project, so it's always better to go with what is established.



Andy

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

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

[Nicolas Pitre]

On Tue, 20 Feb 2007, Andy Parkins wrote:

> On Tuesday 2007 February 20 10:30, Junio C Hamano wrote:
> 
> > But at least to me,
> >
> > 	if (!prefixcmp(head, PATH_REFS_HEADS))
> > 		head += strlen(PATH_REFS_HEADS);
> >
> > is easier to follow than:
> >
> >         if (ref_is_head(head))
> >                 head += STRLEN_PATH_REFS_HEADS;

Ditto for me.

> Fine.  I don't really mind - and it's less work on my patch :-)
> 
> My argument in favour of the ref_is_head() method is that the prefixcmp() 
> method requires knowledge from the caller about how you tell whether a given 
> ref is a head - the second pushes that information further down the call 
> tree, abstracting it out just a little more.

That's the problem though.  Too much abstraction hides away the purpose.  
With prefixcmp() it shows that the code cares about a string prefix.  
With ref_is_head() you don't know what is happening there since that 
might be many things like a pointer comparison, etc. and you have to 
look ref_is_head() implementation to be sure.


Nicolas

[PATCH 1/2] Change "refs/" references to symbolic constants

[Andy Parkins]

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

I've changed all references to them and made constants for the string
lengths as well.  This has clarified the code in some places; for
example:

 - len = strlen(refs[i]) + 11;
 + len = strlen(refs[i]) + STRLEN_PATH_REFS_TAGS + 1;

In this case 11 isn't STRLEN_PATH_REFS_HEADS, as it is in most other
cases, it's TAGS + 1.  With the change to symbolic constants it's much
clearer what is happening.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---

The primary purpose of this patch is to clarify the string math that's
done whenever "refs/" paths are used.  As with the HASH_WIDTH patch I
sent, when we're dealing with lines like:

 url = xmalloc(strlen(repo->base) + 64);

It's much easier to tell where that 64 came from when it says:

  url = xmalloc(strlen(repo->base) + STRLEN_PATH_OBJECTS + HASH_WIDTH_ASCII
     + 16);

There is more to be done really because that 16 is really

  STRLEN_PATH_PACK + STRLEN_PACK_PREFIX + STRLEN_PACK_INDEX_EXTENSION

but I think that would perhaps be a bridge too far.

 builtin-branch.c        |   28 ++++++++++++++--------------
 builtin-describe.c      |    2 +-
 builtin-fetch--tool.c   |   10 +++++-----
 builtin-fmt-merge-msg.c |    5 +++--
 builtin-fsck.c          |    4 ++--
 builtin-init-db.c       |   15 ++++++++-------
 builtin-name-rev.c      |   14 +++++++-------
 builtin-pack-refs.c     |    2 +-
 builtin-push.c          |    6 +++---
 builtin-show-branch.c   |   34 +++++++++++++++++-----------------
 builtin-show-ref.c      |    6 +++---
 builtin-tag.c           |    4 ++--
 connect.c               |   10 +++++-----
 fetch-pack.c            |    6 +++---
 http-fetch.c            |   31 ++++++++++++++++---------------
 http-push.c             |   42 +++++++++++++++++++++---------------------
 local-fetch.c           |   13 +++++++------
 path.c                  |    5 +++--
 receive-pack.c          |    4 ++--
 reflog-walk.c           |    6 +++---
 refs.c                  |   18 +++++++++---------
 refs.h                  |   17 +++++++++++++++++
 remote.c                |   14 +++++++-------
 setup.c                 |    5 +++--
 sha1_name.c             |   10 +++++-----
 wt-status.c             |    5 +++--
 26 files changed, 170 insertions(+), 146 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 5f5c182..b203b2a 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -92,12 +92,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 
 	switch (kinds) {
 	case REF_REMOTE_BRANCH:
-		fmt = "refs/remotes/%s";
+		fmt = PATH_REFS_REMOTES "%s";
 		remote = "remote ";
 		force = 1;
 		break;
 	case REF_LOCAL_BRANCH:
-		fmt = "refs/heads/%s";
+		fmt = PATH_REFS_HEADS "%s";
 		remote = "";
 		break;
 	default:
@@ -189,15 +189,15 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 	int len;
 
 	/* Detect kind */
-	if (!prefixcmp(refname, "refs/heads/")) {
+	if (!prefixcmp(refname, PATH_REFS_HEADS)) {
 		kind = REF_LOCAL_BRANCH;
-		refname += 11;
-	} else if (!prefixcmp(refname, "refs/remotes/")) {
+		refname += STRLEN_PATH_REFS_HEADS;
+	} else if (!prefixcmp(refname, PATH_REFS_REMOTES)) {
 		kind = REF_REMOTE_BRANCH;
-		refname += 13;
-	} else if (!prefixcmp(refname, "refs/tags/")) {
+		refname += STRLEN_PATH_REFS_REMOTES;
+	} else if (!prefixcmp(refname, PATH_REFS_TAGS)) {
 		kind = REF_TAG;
-		refname += 10;
+		refname += STRLEN_PATH_REFS_TAGS;
 	}
 
 	/* Don't add types the caller doesn't want */
@@ -400,7 +400,7 @@ static void create_branch(const char *name, const char *start_name,
 	char *real_ref, ref[PATH_MAX], msg[PATH_MAX + 20];
 	int forcing = 0;
 
-	snprintf(ref, sizeof ref, "refs/heads/%s", name);
+	snprintf(ref, sizeof ref, PATH_REFS_HEADS "%s", name);
 	if (check_ref_format(ref))
 		die("'%s' is not a valid branch name.", name);
 
@@ -469,13 +469,13 @@ static void rename_branch(const char *oldname, const char *newname, int force)
 	if (!oldname)
 		die("cannot rename the current branch while not on any.");
 
-	if (snprintf(oldref, sizeof(oldref), "refs/heads/%s", oldname) > sizeof(oldref))
+	if (snprintf(oldref, sizeof(oldref), PATH_REFS_HEADS "%s", oldname) > sizeof(oldref))
 		die("Old branchname too long");
 
 	if (check_ref_format(oldref))
 		die("Invalid branch name: %s", oldref);
 
-	if (snprintf(newref, sizeof(newref), "refs/heads/%s", newname) > sizeof(newref))
+	if (snprintf(newref, sizeof(newref), PATH_REFS_HEADS "%s", newname) > sizeof(newref))
 		die("New branchname too long");
 
 	if (check_ref_format(newref))
@@ -602,9 +602,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		detached = 1;
 	}
 	else {
-		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;
 	}
 
 	if (delete)
diff --git a/builtin-describe.c b/builtin-describe.c
index 669110c..09ed8f8 100644
--- a/builtin-describe.c
+++ b/builtin-describe.c
@@ -53,7 +53,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 (!prefixcmp(path, "refs/tags/")) {
+	if (!prefixcmp(path, PATH_TAGS)) {
 		if (object->type == OBJ_TAG)
 			prio = 2;
 		else
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index 24c7e6f..521b5fc 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -76,7 +76,7 @@ static int update_local_ref(const char *name,
 		char *msg;
 	just_store:
 		/* new ref */
-		if (!strncmp(name, "refs/tags/", 10))
+		if (!prefixcmp(name, PATH_REFS_TAGS))
 			msg = "storing tag";
 		else
 			msg = "storing head";
@@ -94,7 +94,7 @@ static int update_local_ref(const char *name,
 		return 0;
 	}
 
-	if (!strncmp(name, "refs/tags/", 10)) {
+	if (!prefixcmp(name, PATH_REFS_TAGS)) {
 		fprintf(stderr, "* %s: updating with %s\n", name, note);
 		show_new(type, sha1_new);
 		return update_ref_env("updating tag", name, sha1_new, NULL);
@@ -151,15 +151,15 @@ static int append_fetch_head(FILE *fp,
 		kind = "";
 		what = "";
 	}
-	else if (!strncmp(remote_name, "refs/heads/", 11)) {
+	else if (!prefixcmp(remote_name, PATH_REFS_HEADS)) {
 		kind = "branch";
 		what = remote_name + 11;
 	}
-	else if (!strncmp(remote_name, "refs/tags/", 10)) {
+	else if (!prefixcmp(remote_name, PATH_REFS_TAGS)) {
 		kind = "tag";
 		what = remote_name + 10;
 	}
-	else if (!strncmp(remote_name, "refs/remotes/", 13)) {
+	else if (!prefixcmp(remote_name, PATH_REFS_REMOTES)) {
 		kind = "remote branch";
 		what = remote_name + 13;
 	}
diff --git a/builtin-fmt-merge-msg.c b/builtin-fmt-merge-msg.c
index ae60fcc..8700343 100644
--- a/builtin-fmt-merge-msg.c
+++ b/builtin-fmt-merge-msg.c
@@ -4,6 +4,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "tag.h"
+#include "refs.h"
 
 static const char *fmt_merge_msg_usage =
 	"git-fmt-merge-msg [--summary] [--no-summary] [--file <file>]";
@@ -282,8 +283,8 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 	current_branch = resolve_ref("HEAD", head_sha1, 1, NULL);
 	if (!current_branch)
 		die("No current branch");
-	if (!prefixcmp(current_branch, "refs/heads/"))
-		current_branch += 11;
+	if (!prefixcmp(current_branch, PATH_REFS_HEADS))
+		current_branch += STRLEN_PATH_REFS_HEADS;
 
 	while (fgets(line, sizeof(line), in)) {
 		i++;
diff --git a/builtin-fsck.c b/builtin-fsck.c
index 8d12287..83a2d0c 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -629,14 +629,14 @@ static int fsck_head_link(void)
 	if (!strcmp(head_points_at, "HEAD"))
 		/* detached HEAD */
 		null_is_error = 1;
-	else if (prefixcmp(head_points_at, "refs/heads/"))
+	else if (prefixcmp(head_points_at, PATH_REFS_HEADS))
 		return error("HEAD points to something strange (%s)",
 			     head_points_at);
 	if (is_null_sha1(sha1)) {
 		if (null_is_error)
 			return error("HEAD: detached HEAD points at nothing");
 		fprintf(stderr, "notice: HEAD points to an unborn branch (%s)\n",
-			head_points_at + 11);
+			head_points_at + STRLEN_PATH_REFS_HEADS);
 	}
 	return 0;
 }
diff --git a/builtin-init-db.c b/builtin-init-db.c
index 763fa55..1f3c0dd 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -5,6 +5,7 @@
  */
 #include "cache.h"
 #include "builtin.h"
+#include "refs.h"
 
 #ifndef DEFAULT_GIT_TEMPLATE_DIR
 #define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
@@ -194,11 +195,11 @@ static int create_default_files(const char *git_dir, const char *template_path)
 	/*
 	 * Create .git/refs/{heads,tags}
 	 */
-	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);
 
 	/* First copy the templates -- we might have the default
@@ -217,11 +218,11 @@ static int create_default_files(const char *git_dir, const char *template_path)
 	if (shared_repository) {
 		path[len] = 0;
 		adjust_shared_perm(path);
-		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);
 	}
 
@@ -232,7 +233,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);
 	}
 
diff --git a/builtin-name-rev.c b/builtin-name-rev.c
index 03083e9..56c13d0 100644
--- a/builtin-name-rev.c
+++ b/builtin-name-rev.c
@@ -96,7 +96,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
 	struct name_ref_data *data = cb_data;
 	int deref = 0;
 
-	if (data->tags_only && prefixcmp(path, "refs/tags/"))
+	if (data->tags_only && prefixcmp(path, PATH_REFS_TAGS))
 		return 0;
 
 	if (data->ref_filter && fnmatch(data->ref_filter, path, 0))
@@ -112,14 +112,14 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
 	if (o && o->type == OBJ_COMMIT) {
 		struct commit *commit = (struct commit *)o;
 
-		if (!prefixcmp(path, "refs/heads/"))
-			path = path + 11;
+		if (!prefixcmp(path, PATH_REFS_HEADS))
+			path = path + STRLEN_PATH_REFS_HEADS;
 		else if (data->tags_only
 		    && data->name_only
-		    && !prefixcmp(path, "refs/tags/"))
-			path = path + 10;
-		else if (!prefixcmp(path, "refs/"))
-			path = path + 5;
+		    && !prefixcmp(path, PATH_REFS_TAGS))
+			path = path + STRLEN_PATH_REFS_TAGS;
+		else if (!prefixcmp(path, PATH_REFS))
+			path = path + STRLEN_PATH_REFS;
 
 		name_rev(commit, xstrdup(path), 0, 0, deref);
 	}
diff --git a/builtin-pack-refs.c b/builtin-pack-refs.c
index 09df4e1..23b4c4e 100644
--- a/builtin-pack-refs.c
+++ b/builtin-pack-refs.c
@@ -39,7 +39,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 = !prefixcmp(path, "refs/tags/");
+	is_tag_ref = !prefixcmp(path, PATH_REFS_TAGS);
 
 	/* ALWAYS pack refs that were already packed or are tags */
 	if (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref && !(flags & REF_ISPACKED))
diff --git a/builtin-push.c b/builtin-push.c
index 88c5024..2fdae7a 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -33,9 +33,9 @@ static void set_refspecs(const char **refs, int nr)
 			int len;
 			if (nr <= ++i)
 				die("tag shorthand without <tag>");
-			len = strlen(refs[i]) + 11;
+			len = strlen(refs[i]) + STRLEN_PATH_REFS_TAGS + 1;
 			tag = xmalloc(len);
-			strcpy(tag, "refs/tags/");
+			strcpy(tag, PATH_REFS_TAGS);
 			strcat(tag, refs[i]);
 			ref = tag;
 		}
@@ -148,7 +148,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 		if (!strcmp(arg, "--tags")) {
-			add_refspec("refs/tags/*");
+			add_refspec(PATH_REFS_TAGS "*");
 			continue;
 		}
 		if (!strcmp(arg, "--force") || !strcmp(arg, "-f")) {
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 4fa87f6..7e39d60 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -380,36 +380,36 @@ static int append_ref(const char *refname, const unsigned char *sha1,
 static int append_head_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	unsigned char tmp[20];
-	int ofs = 11;
-	if (prefixcmp(refname, "refs/heads/"))
+	int ofs = STRLEN_PATH_REFS_HEADS;
+	if (prefixcmp(refname, PATH_REFS_HEADS))
 		return 0;
 	/* If both heads/foo and tags/foo exists, get_sha1 would
 	 * get confused.
 	 */
 	if (get_sha1(refname + ofs, tmp) || hashcmp(tmp, sha1))
-		ofs = 5;
+		ofs = STRLEN_PATH_REFS;
 	return append_ref(refname + ofs, sha1, 0);
 }
 
 static int append_remote_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	unsigned char tmp[20];
-	int ofs = 13;
-	if (prefixcmp(refname, "refs/remotes/"))
+	int ofs = STRLEN_PATH_REFS_REMOTES;
+	if (prefixcmp(refname, PATH_REFS_REMOTES))
 		return 0;
 	/* If both heads/foo and tags/foo exists, get_sha1 would
 	 * get confused.
 	 */
 	if (get_sha1(refname + ofs, tmp) || hashcmp(tmp, sha1))
-		ofs = 5;
+		ofs = STRLEN_PATH_REFS;
 	return append_ref(refname + ofs, sha1, 0);
 }
 
 static int append_tag_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
-	if (prefixcmp(refname, "refs/tags/"))
+	if (prefixcmp(refname, PATH_REFS_TAGS))
 		return 0;
-	return append_ref(refname + 5, sha1, 0);
+	return append_ref(refname + STRLEN_PATH_REFS, sha1, 0);
 }
 
 static const char *match_ref_pattern = NULL;
@@ -438,9 +438,9 @@ static int append_matching_ref(const char *refname, const unsigned char *sha1, i
 		return 0;
 	if (fnmatch(match_ref_pattern, tail, 0))
 		return 0;
-	if (!prefixcmp(refname, "refs/heads/"))
+	if (!prefixcmp(refname, PATH_REFS_HEADS))
 		return append_head_ref(refname, sha1, flag, cb_data);
-	if (!prefixcmp(refname, "refs/tags/"))
+	if (!prefixcmp(refname, PATH_REFS_TAGS))
 		return append_tag_ref(refname, sha1, flag, cb_data);
 	return append_ref(refname, sha1, 0);
 }
@@ -465,12 +465,12 @@ static int rev_is_head(char *head, int headlen, char *name,
 	if ((!head[0]) ||
 	    (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
 		return 0;
-	if (!prefixcmp(head, "refs/heads/"))
-		head += 11;
-	if (!prefixcmp(name, "refs/heads/"))
-		name += 11;
-	else if (!prefixcmp(name, "heads/"))
-		name += 6;
+	if (!prefixcmp(head, PATH_REFS_HEADS))
+		head += STRLEN_PATH_REFS_HEADS;
+	if (!prefixcmp(name, PATH_REFS_HEADS))
+		name += STRLEN_PATH_REFS_HEADS;
+	else if (!prefixcmp(name, PATH_HEADS))
+		name += STRLEN_PATH_HEADS;
 	return !strcmp(head, name);
 }
 
@@ -781,7 +781,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				has_head++;
 		}
 		if (!has_head) {
-			int pfxlen = strlen("refs/heads/");
+			int pfxlen = STRLEN_PATH_REFS_HEADS;
 			append_one_rev(head + pfxlen);
 		}
 	}
diff --git a/builtin-show-ref.c b/builtin-show-ref.c
index 65051d1..d463d80 100644
--- a/builtin-show-ref.c
+++ b/builtin-show-ref.c
@@ -29,8 +29,8 @@ static int show_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	if (tags_only || heads_only) {
 		int match;
 
-		match = heads_only && !prefixcmp(refname, "refs/heads/");
-		match |= tags_only && !prefixcmp(refname, "refs/tags/");
+		match = heads_only && !prefixcmp(refname, PATH_REFS_HEADS);
+		match |= tags_only && !prefixcmp(refname, PATH_REFS_TAGS);
 		if (!match)
 			return 0;
 	}
@@ -227,7 +227,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
 		while (*pattern) {
 			unsigned char sha1[20];
 
-			if (!prefixcmp(*pattern, "refs/") &&
+			if (!prefixcmp(*pattern, PATH_REFS) &&
 			    resolve_ref(*pattern, sha1, 1, NULL)) {
 				if (!quiet)
 					show_one(*pattern, sha1);
diff --git a/builtin-tag.c b/builtin-tag.c
index 3a9d2ee..800e823 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -146,7 +146,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
 	unsigned char sha1[20];
 
 	for (p = argv; *p; p++) {
-		if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p)
+		if (snprintf(ref, sizeof(ref), PATH_REFS_TAGS"%s", *p)
 					>= sizeof(ref)) {
 			error("tag name too long: %.*s...", 50, *p);
 			had_error = 1;
@@ -440,7 +440,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die("Failed to resolve '%s' as a valid ref.", object_ref);
 
-	if (snprintf(ref, sizeof(ref), "refs/tags/%s", tag) > sizeof(ref) - 1)
+	if (snprintf(ref, sizeof(ref), PATH_REFS_TAGS "%s", tag) > sizeof(ref) - 1)
 		die("tag name too long: %.*s...", 50, tag);
 	if (check_ref_format(ref))
 		die("'%s' is not a valid tag name.", tag);
diff --git a/connect.c b/connect.c
index 8b1e993..9d576bc 100644
--- a/connect.c
+++ b/connect.c
@@ -13,23 +13,23 @@ static int check_ref(const char *name, int len, unsigned int flags)
 	if (!flags)
 		return 1;
 
-	if (len < 5 || memcmp(name, "refs/", 5))
+	if (len < STRLEN_PATH_REFS || memcmp(name, PATH_REFS, STRLEN_PATH_REFS))
 		return 0;
 
 	/* Skip the "refs/" part */
-	name += 5;
-	len -= 5;
+	name += STRLEN_PATH_REFS;
+	len -= STRLEN_PATH_REFS;
 
 	/* REF_NORMAL means that we don't want the magic fake tag refs */
 	if ((flags & REF_NORMAL) && check_ref_format(name) < 0)
 		return 0;
 
 	/* REF_HEADS means that we want regular branch heads */
-	if ((flags & REF_HEADS) && !memcmp(name, "heads/", 6))
+	if ((flags & REF_HEADS) && !memcmp(name, PATH_HEADS, STRLEN_PATH_HEADS))
 		return 1;
 
 	/* REF_TAGS means that we want tags */
-	if ((flags & REF_TAGS) && !memcmp(name, "tags/", 5))
+	if ((flags & REF_TAGS) && !memcmp(name, PATH_TAGS, STRLEN_PATH_TAGS))
 		return 1;
 
 	/* All type bits clear means that we are ok with anything */
diff --git a/fetch-pack.c b/fetch-pack.c
index 9c81305..c3b7ef6 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -344,11 +344,11 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
 
 	for (ref = *refs; ref; ref = next) {
 		next = ref->next;
-		if (!memcmp(ref->name, "refs/", 5) &&
-		    check_ref_format(ref->name + 5))
+		if (!memcmp(ref->name, PATH_REFS, STRLEN_PATH_REFS) &&
+		    check_ref_format(ref->name + STRLEN_PATH_REFS))
 			; /* trash */
 		else if (fetch_all &&
-			 (!depth || prefixcmp(ref->name, "refs/tags/") )) {
+			 (!depth || prefixcmp(ref->name, PATH_REFS_TAGS) )) {
 			*newtail = ref;
 			ref->next = NULL;
 			newtail = &ref->next;
diff --git a/http-fetch.c b/http-fetch.c
index 202fae0..6dc32e8 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -3,6 +3,7 @@
 #include "pack.h"
 #include "fetch.h"
 #include "http.h"
+#include "refs.h"
 
 #define PREV_BUF_SIZE 4096
 #define RANGE_HEADER_SIZE 30
@@ -157,11 +158,11 @@ static void start_object_request(struct object_request *obj_req)
 
 	SHA1_Init(&obj_req->c);
 
-	url = xmalloc(strlen(obj_req->repo->base) + 51);
-	obj_req->url = xmalloc(strlen(obj_req->repo->base) + 51);
+	url = xmalloc(strlen(obj_req->repo->base) + STRLEN_PATH_OBJECTS + 43);
+	obj_req->url = xmalloc(strlen(obj_req->repo->base) + STRLEN_PATH_OBJECTS + 43);
 	strcpy(url, obj_req->repo->base);
 	posn = url + strlen(obj_req->repo->base);
-	strcpy(posn, "/objects/");
+	strcpy(posn, "/" PATH_OBJECTS);
 	posn += 9;
 	memcpy(posn, hex, 2);
 	posn += 2;
@@ -398,8 +399,8 @@ static int fetch_index(struct alt_base *repo, unsigned char *sha1)
 	if (get_verbosely)
 		fprintf(stderr, "Getting index for pack %s\n", hex);
 
-	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);
 
 	filename = sha1_pack_index_name(sha1);
 	snprintf(tmpfile, sizeof(tmpfile), "%s.temp", filename);
@@ -478,7 +479,7 @@ static void process_alternates_response(void *callback_data)
 
 			/* Try reusing the slot to get non-http alternates */
 			alt_req->http_specific = 0;
-			sprintf(alt_req->url, "%s/objects/info/alternates",
+			sprintf(alt_req->url, "%s/" PATH_OBJECTS "info/alternates",
 				base);
 			curl_easy_setopt(slot->curl, CURLOPT_URL,
 					 alt_req->url);
@@ -625,8 +626,8 @@ static void fetch_alternates(const char *base)
 	if (get_verbosely)
 		fprintf(stderr, "Getting alternates list for %s\n", base);
 
-	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);
 
 	/* Use a callback to process the result, since another request
 	   may fail and need to have alternates loaded before continuing */
@@ -675,8 +676,8 @@ static int fetch_indices(struct alt_base *repo)
 	if (get_verbosely)
 		fprintf(stderr, "Getting pack list for %s\n", repo->base);
 
-	url = xmalloc(strlen(repo->base) + 21);
-	sprintf(url, "%s/objects/info/packs", repo->base);
+	url = xmalloc(strlen(repo->base) + STRLEN_PATH_OBJECTS + 13);
+	sprintf(url, "%s/" PATH_OBJECTS "info/packs", repo->base);
 
 	slot = get_active_slot();
 	slot->results = &results;
@@ -757,8 +758,8 @@ static int fetch_pack(struct alt_base *repo, unsigned char *sha1)
 			sha1_to_hex(sha1));
 	}
 
-	url = xmalloc(strlen(repo->base) + 65);
-	sprintf(url, "%s/objects/pack/pack-%s.pack",
+	url = xmalloc(strlen(repo->base) + STRLEN_PATH_OBJECTS + 57);
+	sprintf(url, "%s/" PATH_OBJECTS "pack/pack-%s.pack",
 		repo->base, sha1_to_hex(target->sha1));
 
 	filename = sha1_pack_name(target->sha1);
@@ -930,14 +931,14 @@ static char *quote_ref_url(const char *base, const char *ref)
 	int len, baselen, ch;
 
 	baselen = strlen(base);
-	len = baselen + 7; /* "/refs/" + NUL */
+	len = baselen + STRLEN_PATH_REFS + 2; /* "/" + "refs/" + NUL */
 	for (cp = ref; (ch = *cp) != 0; cp++, len++)
 		if (needs_quote(ch))
 			len += 2; /* extra two hex plus replacement % */
 	qref = xmalloc(len);
 	memcpy(qref, base, baselen);
-	memcpy(qref + baselen, "/refs/", 6);
-	for (cp = ref, dp = qref + baselen + 6; (ch = *cp) != 0; cp++) {
+	memcpy(qref + baselen, PATH_REFS, STRLEN_PATH_REFS);
+	for (cp = ref, dp = qref + baselen + STRLEN_PATH_REFS; (ch = *cp) != 0; cp++) {
 		if (needs_quote(ch)) {
 			*dp++ = '%';
 			*dp++ = hex((ch >> 4) & 0xF);
diff --git a/http-push.c b/http-push.c
index 7c3720f..0c5e892 100644
--- a/http-push.c
+++ b/http-push.c
@@ -272,12 +272,12 @@ static void start_fetch_loose(struct transfer_request *request)
 
 	SHA1_Init(&request->c);
 
-	url = xmalloc(strlen(remote->url) + 50);
-	request->url = xmalloc(strlen(remote->url) + 50);
+	url = xmalloc(strlen(remote->url) + 42 + STRLEN_PATH_OBJECTS);
+	request->url = xmalloc(strlen(remote->url) + 42 + STRLEN_PATH_OBJECTS);
 	strcpy(url, remote->url);
 	posn = url + strlen(remote->url);
-	strcpy(posn, "objects/");
-	posn += 8;
+	strcpy(posn, PATH_OBJECTS);
+	posn += STRLEN_PATH_OBJECTS;
 	memcpy(posn, hex, 2);
 	posn += 2;
 	*(posn++) = '/';
@@ -357,11 +357,11 @@ static void start_mkcol(struct transfer_request *request)
 	struct active_request_slot *slot;
 	char *posn;
 
-	request->url = xmalloc(strlen(remote->url) + 13);
+	request->url = xmalloc(strlen(remote->url) + STRLEN_PATH_OBJECTS + 5);
 	strcpy(request->url, remote->url);
 	posn = request->url + strlen(remote->url);
-	strcpy(posn, "objects/");
-	posn += 8;
+	strcpy(posn, PATH_OBJECTS);
+	posn += STRLEN_PATH_OBJECTS;
 	memcpy(posn, hex, 2);
 	posn += 2;
 	strcpy(posn, "/");
@@ -415,8 +415,8 @@ static void start_fetch_packed(struct transfer_request *request)
 	snprintf(request->tmpfile, sizeof(request->tmpfile),
 		 "%s.temp", filename);
 
-	url = xmalloc(strlen(remote->url) + 64);
-	sprintf(url, "%sobjects/pack/pack-%s.pack",
+	url = xmalloc(strlen(remote->url) + STRLEN_PATH_OBJECTS + 56);
+	sprintf(url, "%s/" PATH_OBJECTS "pack/pack-%s.pack",
 		remote->url, sha1_to_hex(target->sha1));
 
 	/* Make sure there isn't another open request for this pack */
@@ -519,11 +519,11 @@ static void start_put(struct transfer_request *request)
 	request->buffer.posn = 0;
 
 	request->url = xmalloc(strlen(remote->url) +
-			       strlen(request->lock->token) + 51);
+			       strlen(request->lock->token) + STRLEN_PATH_OBJECTS + 43);
 	strcpy(request->url, remote->url);
 	posn = request->url + strlen(remote->url);
-	strcpy(posn, "objects/");
-	posn += 8;
+	strcpy(posn, PATH_OBJECTS);
+	posn += STRLEN_PATH_OBJECTS;
 	memcpy(posn, hex, 2);
 	posn += 2;
 	*(posn++) = '/';
@@ -922,8 +922,8 @@ static int fetch_index(unsigned char *sha1)
 	struct slot_results results;
 
 	/* Don't use the index if the pack isn't there */
-	url = xmalloc(strlen(remote->url) + 64);
-	sprintf(url, "%sobjects/pack/pack-%s.pack", remote->url, hex);
+	url = xmalloc(strlen(remote->url) + STRLEN_PATH_OBJECTS + 56);
+	sprintf(url, "%s" PATH_OBJECTS "pack/pack-%s.pack", remote->url, hex);
 	slot = get_active_slot();
 	slot->results = &results;
 	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
@@ -945,7 +945,7 @@ static int fetch_index(unsigned char *sha1)
 	if (push_verbosely)
 		fprintf(stderr, "Getting index for pack %s\n", hex);
 
-	sprintf(url, "%sobjects/pack/pack-%s.idx", remote->url, hex);
+	sprintf(url, "%s" PATH_OBJECTS "/pack/pack-%s.idx", remote->url, hex);
 
 	filename = sha1_pack_index_name(sha1);
 	snprintf(tmpfile, sizeof(tmpfile), "%s.temp", filename);
@@ -1029,8 +1029,8 @@ static int fetch_indices(void)
 	if (push_verbosely)
 		fprintf(stderr, "Getting pack list\n");
 
-	url = xmalloc(strlen(remote->url) + 20);
-	sprintf(url, "%sobjects/info/packs", remote->url);
+	url = xmalloc(strlen(remote->url) + STRLEN_PATH_OBJECTS + 12);
+	sprintf(url, "%s" PATH_OBJECTS "info/packs", remote->url);
 
 	slot = get_active_slot();
 	slot->results = &results;
@@ -1615,7 +1615,7 @@ static void remote_ls(const char *path, int flags,
 
 static void get_remote_object_list(unsigned char parent)
 {
-	char path[] = "objects/XX/";
+	char path[] = PATH_OBJECTS "XX/";
 	static const char hex[] = "0123456789abcdef";
 	unsigned int val = parent;
 
@@ -1925,7 +1925,7 @@ static void get_local_heads(void)
 static void get_dav_remote_heads(void)
 {
 	remote_tail = &remote_refs;
-	remote_ls("refs/", (PROCESS_FILES | PROCESS_DIRS | RECURSIVE), process_ls_ref, NULL);
+	remote_ls(PATH_REFS, (PROCESS_FILES | PROCESS_DIRS | RECURSIVE), process_ls_ref, NULL);
 }
 
 static int is_zero_sha1(const unsigned char *sha1)
@@ -2069,7 +2069,7 @@ static void update_remote_info_refs(struct remote_lock *lock)
 	buffer.buffer = xcalloc(1, 4096);
 	buffer.size = 4096;
 	buffer.posn = 0;
-	remote_ls("refs/", (PROCESS_FILES | RECURSIVE),
+	remote_ls(PATH_REFS, (PROCESS_FILES | RECURSIVE),
 		  add_remote_info_ref, &buffer);
 	if (!aborted) {
 		if_header = xmalloc(strlen(lock->token) + 25);
@@ -2375,7 +2375,7 @@ int main(int argc, char **argv)
 	/* Check whether the remote has server info files */
 	remote->can_update_info_refs = 0;
 	remote->has_info_refs = remote_exists("info/refs");
-	remote->has_info_packs = remote_exists("objects/info/packs");
+	remote->has_info_packs = remote_exists(PATH_OBJECTS "info/packs");
 	if (remote->has_info_refs) {
 		info_ref_lock = lock_remote("info/refs", LOCK_TIME);
 		if (info_ref_lock)
diff --git a/local-fetch.c b/local-fetch.c
index bf7ec6c..67dc065 100644
--- a/local-fetch.c
+++ b/local-fetch.c
@@ -4,6 +4,7 @@
 #include "cache.h"
 #include "commit.h"
 #include "fetch.h"
+#include "refs.h"
 
 static int use_link;
 static int use_symlink;
@@ -23,7 +24,7 @@ static void setup_index(unsigned char *sha1)
 	struct packed_git *new_pack;
 	char filename[PATH_MAX];
 	strcpy(filename, path);
-	strcat(filename, "/objects/pack/pack-");
+	strcat(filename, "/" PATH_OBJECTS "pack/pack-");
 	strcat(filename, sha1_to_hex(sha1));
 	strcat(filename, ".idx");
 	new_pack = parse_pack_index_file(sha1, filename);
@@ -37,7 +38,7 @@ static int setup_indices(void)
 	struct dirent *de;
 	char filename[PATH_MAX];
 	unsigned char sha1[20];
-	sprintf(filename, "%s/objects/pack/", path);
+	sprintf(filename, "%s/" PATH_OBJECTS "pack/", path);
 	dir = opendir(filename);
 	if (!dir)
 		return -1;
@@ -122,11 +123,11 @@ static int fetch_pack(const unsigned char *sha1)
 		fprintf(stderr, " which contains %s\n",
 			sha1_to_hex(sha1));
 	}
-	sprintf(filename, "%s/objects/pack/pack-%s.pack",
+	sprintf(filename, "%s/" PATH_OBJECTS "pack/pack-%s.pack",
 		path, sha1_to_hex(target->sha1));
 	copy_file(filename, sha1_pack_name(target->sha1),
 		  sha1_to_hex(target->sha1), 1);
-	sprintf(filename, "%s/objects/pack/pack-%s.idx",
+	sprintf(filename, "%s/" PATH_OBJECTS "pack/pack-%s.idx",
 		path, sha1_to_hex(target->sha1));
 	copy_file(filename, sha1_pack_index_name(target->sha1),
 		  sha1_to_hex(target->sha1), 1);
@@ -143,7 +144,7 @@ static int fetch_file(const unsigned char *sha1)
 
 	if (object_name_start < 0) {
 		strcpy(filename, path); /* e.g. git.git */
-		strcat(filename, "/objects/");
+		strcat(filename, "/" PATH_OBJECTS);
 		object_name_start = strlen(filename);
 	}
 	filename[object_name_start+0] = hex[0];
@@ -169,7 +170,7 @@ int fetch_ref(char *ref, unsigned char *sha1)
 	int ifd;
 
 	if (ref_name_start < 0) {
-		sprintf(filename, "%s/refs/", path);
+		sprintf(filename, "%s/" PATH_REFS, path);
 		ref_name_start = strlen(filename);
 	}
 	strcpy(filename + ref_name_start, ref);
diff --git a/path.c b/path.c
index 4260952..78511d7 100644
--- a/path.c
+++ b/path.c
@@ -11,6 +11,7 @@
  * which is what it's designed for.
  */
 #include "cache.h"
+#include "refs.h"
 
 static char bad_path[] = "/bad-path/";
 
@@ -99,7 +100,7 @@ int validate_headref(const char *path)
 	/* Make sure it is a "refs/.." symlink */
 	if (S_ISLNK(st.st_mode)) {
 		len = readlink(path, buffer, sizeof(buffer)-1);
-		if (len >= 5 && !memcmp("refs/", buffer, 5))
+		if (len >= 5 && !memcmp(PATH_REFS, buffer, STRLEN_PATH_REFS))
 			return 0;
 		return -1;
 	}
@@ -123,7 +124,7 @@ int validate_headref(const char *path)
 		len -= 4;
 		while (len && isspace(*buf))
 			buf++, len--;
-		if (len >= 5 && !memcmp("refs/", buf, 5))
+		if (len >= STRLEN_PATH_REFS && !memcmp(PATH_REFS, buf, STRLEN_PATH_REFS))
 			return 0;
 	}
 
diff --git a/receive-pack.c b/receive-pack.c
index d3c422b..114ea38 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -165,7 +165,7 @@ static const char *update(struct command *cmd)
 	unsigned char *new_sha1 = cmd->new_sha1;
 	struct ref_lock *lock;
 
-	if (!prefixcmp(name, "refs/") && check_ref_format(name + 5)) {
+	if (!prefixcmp(name, PATH_REFS) && check_ref_format(name + STRLEN_PATH_REFS)) {
 		error("refusing to create funny ref '%s' locally", name);
 		return "funny refname";
 	}
@@ -177,7 +177,7 @@ static const char *update(struct command *cmd)
 	}
 	if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
 	    !is_null_sha1(old_sha1) &&
-	    !prefixcmp(name, "refs/heads/")) {
+	    !prefixcmp(name, PATH_REFS_HEADS)) {
 		struct commit *old_commit, *new_commit;
 		struct commit_list *bases, *ent;
 
diff --git a/reflog-walk.c b/reflog-walk.c
index ee1456b..98cf8ef 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -55,11 +55,11 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)
 	}
 	if (reflogs->nr == 0) {
 		int len = strlen(ref);
-		char *refname = xmalloc(len + 12);
-		sprintf(refname, "refs/%s", ref);
+		char *refname = xmalloc(len + STRLEN_PATH_REFS_HEADS + 1);
+		sprintf(refname, PATH_REFS "%s", ref);
 		for_each_reflog_ent(refname, read_one_reflog, reflogs);
 		if (reflogs->nr == 0) {
-			sprintf(refname, "refs/heads/%s", ref);
+			sprintf(refname, PATH_REFS_HEADS "%s", ref);
 			for_each_reflog_ent(refname, read_one_reflog, reflogs);
 		}
 		free(refname);
diff --git a/refs.c b/refs.c
index 7fb3350..840a433 100644
--- a/refs.c
+++ b/refs.c
@@ -409,7 +409,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
 		/* Follow "normalized" - ie "refs/.." symlinks by hand */
 		if (S_ISLNK(st.st_mode)) {
 			len = readlink(path, buffer, sizeof(buffer)-1);
-			if (len >= 5 && !memcmp("refs/", buffer, 5)) {
+			if (len >= STRLEN_PATH_REFS && !memcmp(PATH_REFS, buffer, STRLEN_PATH_REFS)) {
 				buffer[len] = 0;
 				strcpy(ref_buffer, buffer);
 				ref = ref_buffer;
@@ -561,22 +561,22 @@ int head_ref(each_ref_fn fn, void *cb_data)
 
 int for_each_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/", fn, 0, cb_data);
+	return do_for_each_ref(PATH_REFS, fn, 0, cb_data);
 }
 
 int for_each_tag_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/tags/", fn, 10, cb_data);
+	return do_for_each_ref(PATH_REFS_TAGS, fn, STRLEN_PATH_REFS_TAGS, cb_data);
 }
 
 int for_each_branch_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/heads/", fn, 11, cb_data);
+	return do_for_each_ref(PATH_REFS_HEADS, fn, STRLEN_PATH_REFS_HEADS, cb_data);
 }
 
 int for_each_remote_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/remotes/", fn, 13, cb_data);
+	return do_for_each_ref(PATH_REFS_REMOTES, fn, STRLEN_PATH_REFS_REMOTES, cb_data);
 }
 
 /* NEEDSWORK: This is only used by ssh-upload and it should go; the
@@ -588,7 +588,7 @@ int get_ref_sha1(const char *ref, unsigned char *sha1)
 {
 	if (check_ref_format(ref))
 		return -1;
-	return read_ref(mkpath("refs/%s", ref), sha1);
+	return read_ref(mkpath(PATH_REFS "%s", ref), sha1);
 }
 
 /*
@@ -824,7 +824,7 @@ struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)
 	char refpath[PATH_MAX];
 	if (check_ref_format(ref))
 		return NULL;
-	strcpy(refpath, mkpath("refs/%s", ref));
+	strcpy(refpath, mkpath(PATH_REFS "%s", ref));
 	return lock_ref_sha1_basic(refpath, old_sha1, 0, NULL);
 }
 
@@ -1078,8 +1078,8 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 	log_file = git_path("logs/%s", ref_name);
 
 	if (log_all_ref_updates &&
-	    (!prefixcmp(ref_name, "refs/heads/") ||
-	     !prefixcmp(ref_name, "refs/remotes/") ||
+	    (!prefixcmp(ref_name,  PATH_REFS_HEADS) ||
+	     !prefixcmp(ref_name, PATH_REFS_REMOTES) ||
 	     !strcmp(ref_name, "HEAD"))) {
 		if (safe_create_leading_directories(log_file) < 0)
 			return error("unable to create directory for %s",
diff --git a/refs.h b/refs.h
index 6eb98a4..1025d04 100644
--- a/refs.h
+++ b/refs.h
@@ -13,6 +13,23 @@ struct ref_lock {
 #define REF_ISSYMREF 01
 #define REF_ISPACKED 02
 
+#define PATH_OBJECTS             "objects/"
+#define STRLEN_PATH_OBJECTS      8
+#define PATH_REFS                "refs/"
+#define STRLEN_PATH_REFS         5
+#define PATH_HEADS               "heads/"
+#define STRLEN_PATH_HEADS        6
+#define PATH_TAGS                "tags/"
+#define STRLEN_PATH_TAGS         5
+#define PATH_REMOTES             "remotes/"
+#define STRLEN_PATH_REMOTES      8
+#define PATH_REFS_HEADS          PATH_REFS PATH_HEADS
+#define STRLEN_PATH_REFS_HEADS   (STRLEN_PATH_REFS+STRLEN_PATH_HEADS)
+#define PATH_REFS_TAGS           PATH_REFS PATH_TAGS
+#define STRLEN_PATH_REFS_TAGS    (STRLEN_PATH_REFS+STRLEN_PATH_TAGS)
+#define PATH_REFS_REMOTES        PATH_REFS PATH_REMOTES
+#define STRLEN_PATH_REFS_REMOTES (STRLEN_PATH_REFS+STRLEN_PATH_REMOTES)
+
 /*
  * Calls the specified function for each ref file until it returns nonzero,
  * and returns the value
diff --git a/remote.c b/remote.c
index bb774d0..8d1e0a4 100644
--- a/remote.c
+++ b/remote.c
@@ -211,8 +211,8 @@ static void read_config(void)
 	current_branch = NULL;
 	head_ref = resolve_ref("HEAD", sha1, 0, &flag);
 	if (head_ref && (flag & REF_ISSYMREF) &&
-	    !prefixcmp(head_ref, "refs/heads/")) {
-		current_branch = head_ref + strlen("refs/heads/");
+	    !prefixcmp(head_ref, PATH_REFS_HEADS)) {
+		current_branch = head_ref + strlen(PATH_REFS_HEADS);
 		current_branch_len = strlen(current_branch);
 	}
 	git_config(handle_config);
@@ -398,9 +398,9 @@ static int count_refspec_match(const char *pattern,
 		 * at the remote site.
 		 */
 		if (namelen != patlen &&
-		    patlen != namelen - 5 &&
-		    prefixcmp(name, "refs/heads/") &&
-		    prefixcmp(name, "refs/tags/")) {
+		    patlen != namelen - STRLEN_PATH_REFS_HEADS &&
+		    prefixcmp(name, PATH_REFS_HEADS) &&
+		    prefixcmp(name, PATH_REFS_HEADS)) {
 			/* We want to catch the case where only weak
 			 * matches are found and there are multiple
 			 * matches, and where more than one strong
@@ -511,7 +511,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 	case 1:
 		break;
 	case 0:
-		if (!memcmp(dst_value, "refs/", 5))
+		if (!prefixcmp(dst_value, PATH_REFS))
 			matched_dst = make_linked_ref(dst_value, dst_tail);
 		else
 			error("dst refspec %s does not match any "
@@ -594,7 +594,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 			if (!pat)
 				continue;
 		}
-		else if (prefixcmp(src->name, "refs/heads/"))
+		else if (prefixcmp(src->name, PATH_REFS_HEADS))
 			/*
 			 * "matching refs"; traditionally we pushed everything
 			 * including refs outside refs/heads/ hierarchy, but
diff --git a/setup.c b/setup.c
index 06004f1..c8912d2 100644
--- a/setup.c
+++ b/setup.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "dir.h"
+#include "refs.h"
 
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
@@ -158,12 +159,12 @@ static int is_git_directory(const char *suspect)
 			return 0;
 	}
 	else {
-		strcpy(path + len, "/objects");
+		strcpy(path + len, "/" PATH_OBJECTS);
 		if (access(path, X_OK))
 			return 0;
 	}
 
-	strcpy(path + len, "/refs");
+	strcpy(path + len, "/" PATH_REFS);
 	if (access(path, X_OK))
 		return 0;
 
diff --git a/sha1_name.c b/sha1_name.c
index 2d727d5..649e438 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -241,11 +241,11 @@ static int ambiguous_path(const char *path, int len)
 
 static const char *ref_fmt[] = {
 	"%.*s",
-	"refs/%.*s",
-	"refs/tags/%.*s",
-	"refs/heads/%.*s",
-	"refs/remotes/%.*s",
-	"refs/remotes/%.*s/HEAD",
+	PATH_REFS "%.*s",
+	PATH_REFS_TAGS "%.*s",
+	PATH_REFS_HEADS "%.*s",
+	PATH_REFS_REMOTES "%.*s",
+	PATH_REFS_REMOTES "%.*s/HEAD",
 	NULL
 };
 
diff --git a/wt-status.c b/wt-status.c
index 10ce6ee..93dee72 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -7,6 +7,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "diffcore.h"
+#include "refs.h"
 
 int wt_status_use_color = 0;
 static char wt_status_colors[][COLOR_MAXLEN] = {
@@ -311,8 +312,8 @@ void wt_status_print(struct wt_status *s)
 	if (s->branch) {
 		const char *on_what = "On branch ";
 		const char *branch_name = s->branch;
-		if (!prefixcmp(branch_name, "refs/heads/"))
-			branch_name += 11;
+		if (!prefixcmp(branch_name, PATH_REFS_HEADS))
+			branch_name += STRLEN_PATH_REFS_HEADS;
 		else if (!strcmp(branch_name, "HEAD")) {
 			branch_name = "";
 			on_what = "Not currently on any branch.";
-- 
1.5.3.rc5.11.g312e

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

[Andy Parkins]

On Saturday 2007, September 29, 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

Please hold off on applying this.  I'm getting this when running the tests:

*** t5516-fetch-push.sh ***
*   ok 1: setup
*   ok 2: fetch without wildcard
*   ok 3: fetch with wildcard
*   ok 4: push without wildcard
*   ok 5: push with wildcard
*   ok 6: push with matching heads
*   ok 7: push with no ambiguity (1)
*   ok 8: push with no ambiguity (2)
*   ok 9: push with weak ambiguity (1)
*   ok 10: push with weak ambiguity (2)
*   ok 11: push with ambiguity (1)
* FAIL 12: push with ambiguity (2)

I'm having trouble seeing where the fault is at the moment though.



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

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

[Jeff King]

On Mon, Oct 01, 2007 at 09:41:43PM +0100, Andy Parkins wrote:

> Please hold off on applying this.  I'm getting this when running the tests:
> 
> *** t5516-fetch-push.sh ***
> *   ok 1: setup
> *   ok 2: fetch without wildcard
> *   ok 3: fetch with wildcard
> *   ok 4: push without wildcard
> *   ok 5: push with wildcard
> *   ok 6: push with matching heads
> *   ok 7: push with no ambiguity (1)
> *   ok 8: push with no ambiguity (2)
> *   ok 9: push with weak ambiguity (1)
> *   ok 10: push with weak ambiguity (2)
> *   ok 11: push with ambiguity (1)
> * FAIL 12: push with ambiguity (2)
> 
> I'm having trouble seeing where the fault is at the moment though.

>From your patch:

-		    patlen != namelen - 5 &&
-		    prefixcmp(name, "refs/heads/") &&
-		    prefixcmp(name, "refs/tags/")) {
+		    patlen != namelen - STRLEN_PATH_REFS_HEADS &&
+		    prefixcmp(name, PATH_REFS_HEADS) &&
+		    prefixcmp(name, PATH_REFS_HEADS)) {

This is totally bogus. You meant STRLEN_PATH_REFS, and the second path
should be PATH_REFS_TAGS. With those changes, t5516 passes.

I haven't combed through your patch in detail, so there might be similar
problems lurking. I did notice one or two spots where you call
strlen(PATH_REFS_*), which should of course also be changed to
STRLEN_PATH_REFS_*.

And as a final comment, your patch doesn't apply to next at all because
of the reorganization of the fetching API (e.g., fetch-pack.c doesn't
exist at all anymore). You should probably prepare a parallel patch for
next.

-Peff

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

[Andy Parkins]

On Tuesday 2007 October 02, Jeff King wrote:

> -		    patlen != namelen - 5 &&
> -		    prefixcmp(name, "refs/heads/") &&
> -		    prefixcmp(name, "refs/tags/")) {
> +		    patlen != namelen - STRLEN_PATH_REFS_HEADS &&
> +		    prefixcmp(name, PATH_REFS_HEADS) &&
> +		    prefixcmp(name, PATH_REFS_HEADS)) {
>
> This is totally bogus. You meant STRLEN_PATH_REFS, and the second path
> should be PATH_REFS_TAGS. With those changes, t5516 passes.

Excellent!  Well done.  I spent a couple of hours last night going through 
every changed line and have spotted the TAGS mistake but didn't spot the 
STRLEN being wrong.  Amazing how easy it is to become blind to these things.  
There were a couple of errors in "/" placement too, but I don't think they 
were causing any trouble, just doubled up "/" characters.

> I haven't combed through your patch in detail, so there might be similar
> problems lurking. I did notice one or two spots where you call
> strlen(PATH_REFS_*), which should of course also be changed to
> STRLEN_PATH_REFS_*.

I think I caught a few of them in my review.  I think I had originally left 
them like that on purpose.  My reasoning was that a patch like that should 
leave the resultant code identical.  So I did replacements like:

- strlen("refs/heads/");
+ strlen(PATH_REFS_HEADS);

However, I think it was just pedantry, so I've been correcting them too.

I noticed a couple of places where memcmp() has been used where prefixcmp() 
would work fine.  I'm tempted to change them too - what do you think?  
Perhaps a separate patch?

> And as a final comment, your patch doesn't apply to next at all because
> of the reorganization of the fetching API (e.g., fetch-pack.c doesn't
> exist at all anymore). You should probably prepare a parallel patch for
> next.

I'm happy to do prepare a patch against any revision, I was really waiting for 
feedback from Junio as to how he'd like to manage it.  Last time I submitted 
this patch he (quite correctly) asked that I delay until after the next point 
release; of course I promptly found other things to do and never 
resubmitted :-)


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

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

[Jeff King]

On Tue, Oct 02, 2007 at 09:41:03AM +0100, Andy Parkins wrote:

> Excellent!  Well done.  I spent a couple of hours last night going through 
> every changed line and have spotted the TAGS mistake but didn't spot the 
> STRLEN being wrong.  Amazing how easy it is to become blind to these things.  
> There were a couple of errors in "/" placement too, but I don't think they 
> were causing any trouble, just doubled up "/" characters.

I wonder if you could check the patch mechanically (i.e., write a script
to confirm that '5' got replaced by tokens equal to '5'), though that
might require some tricky parsing of C.

If you a post an updated version, I will try to read through it
carefully, as two eyes are better than one (er, four eyes, I guess).

> I noticed a couple of places where memcmp() has been used where prefixcmp() 
> would work fine.  I'm tempted to change them too - what do you think?  
> Perhaps a separate patch?

When in doubt, I would suggest a separate patch, as it makes the review
easier.

> I'm happy to do prepare a patch against any revision, I was really
> waiting for feedback from Junio as to how he'd like to manage it.
> Last time I submitted this patch he (quite correctly) asked that I
> delay until after the next point release; of course I promptly found
> other things to do and never resubmitted :-)

Yes, you should definitely listen to Junio on such issues, and not me.
:)

-Peff

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

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> I noticed a couple of places where memcmp() has been used where prefixcmp() 
> would work fine.  I'm tempted to change them too - what do you think?  
> Perhaps a separate patch?

In general, probably it is preferable to have a separate
"preliminary patch" to normalize the existing code without using
the new infrastructure (i.e. REF_* macros), and then to have the
main patch.  That way would make the main patch more about
mechanical conversion, which would be easier to verify
independently.

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

[Andy Parkins]

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

I've changed all references to them and made constants for the string
lengths as well.  This has clarified the code in some places; for
example:

 - len = strlen(refs[i]) + 11;
 + len = strlen(refs[i]) + STRLEN_PATH_REFS_TAGS + 1;

In this case 11 isn't STRLEN_PATH_REFS_HEADS, as it is in most other
cases, it's TAGS + 1.  With the change to symbolic constants it's much
clearer what is happening.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 builtin-branch.c        |   28 ++++++++++++++--------------
 builtin-describe.c      |    2 +-
 builtin-fetch--tool.c   |   10 +++++-----
 builtin-fmt-merge-msg.c |    5 +++--
 builtin-fsck.c          |    4 ++--
 builtin-init-db.c       |   15 ++++++++-------
 builtin-name-rev.c      |   14 +++++++-------
 builtin-pack-refs.c     |    2 +-
 builtin-push.c          |    6 +++---
 builtin-show-branch.c   |   34 +++++++++++++++++-----------------
 builtin-show-ref.c      |    6 +++---
 builtin-tag.c           |    4 ++--
 connect.c               |   10 +++++-----
 fetch-pack.c            |    6 +++---
 http-fetch.c            |   31 ++++++++++++++++---------------
 http-push.c             |   42 +++++++++++++++++++++---------------------
 local-fetch.c           |   13 +++++++------
 path.c                  |    5 +++--
 receive-pack.c          |    4 ++--
 reflog-walk.c           |    6 +++---
 refs.c                  |   18 +++++++++---------
 refs.h                  |   17 +++++++++++++++++
 remote.c                |   14 +++++++-------
 setup.c                 |    5 +++--
 sha1_name.c             |   10 +++++-----
 wt-status.c             |    5 +++--
 26 files changed, 170 insertions(+), 146 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 5f5c182..b203b2a 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -92,12 +92,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 
 	switch (kinds) {
 	case REF_REMOTE_BRANCH:
-		fmt = "refs/remotes/%s";
+		fmt = PATH_REFS_REMOTES "%s";
 		remote = "remote ";
 		force = 1;
 		break;
 	case REF_LOCAL_BRANCH:
-		fmt = "refs/heads/%s";
+		fmt = PATH_REFS_HEADS "%s";
 		remote = "";
 		break;
 	default:
@@ -189,15 +189,15 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 	int len;
 
 	/* Detect kind */
-	if (!prefixcmp(refname, "refs/heads/")) {
+	if (!prefixcmp(refname, PATH_REFS_HEADS)) {
 		kind = REF_LOCAL_BRANCH;
-		refname += 11;
-	} else if (!prefixcmp(refname, "refs/remotes/")) {
+		refname += STRLEN_PATH_REFS_HEADS;
+	} else if (!prefixcmp(refname, PATH_REFS_REMOTES)) {
 		kind = REF_REMOTE_BRANCH;
-		refname += 13;
-	} else if (!prefixcmp(refname, "refs/tags/")) {
+		refname += STRLEN_PATH_REFS_REMOTES;
+	} else if (!prefixcmp(refname, PATH_REFS_TAGS)) {
 		kind = REF_TAG;
-		refname += 10;
+		refname += STRLEN_PATH_REFS_TAGS;
 	}
 
 	/* Don't add types the caller doesn't want */
@@ -400,7 +400,7 @@ static void create_branch(const char *name, const char *start_name,
 	char *real_ref, ref[PATH_MAX], msg[PATH_MAX + 20];
 	int forcing = 0;
 
-	snprintf(ref, sizeof ref, "refs/heads/%s", name);
+	snprintf(ref, sizeof ref, PATH_REFS_HEADS "%s", name);
 	if (check_ref_format(ref))
 		die("'%s' is not a valid branch name.", name);
 
@@ -469,13 +469,13 @@ static void rename_branch(const char *oldname, const char *newname, int force)
 	if (!oldname)
 		die("cannot rename the current branch while not on any.");
 
-	if (snprintf(oldref, sizeof(oldref), "refs/heads/%s", oldname) > sizeof(oldref))
+	if (snprintf(oldref, sizeof(oldref), PATH_REFS_HEADS "%s", oldname) > sizeof(oldref))
 		die("Old branchname too long");
 
 	if (check_ref_format(oldref))
 		die("Invalid branch name: %s", oldref);
 
-	if (snprintf(newref, sizeof(newref), "refs/heads/%s", newname) > sizeof(newref))
+	if (snprintf(newref, sizeof(newref), PATH_REFS_HEADS "%s", newname) > sizeof(newref))
 		die("New branchname too long");
 
 	if (check_ref_format(newref))
@@ -602,9 +602,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		detached = 1;
 	}
 	else {
-		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;
 	}
 
 	if (delete)
diff --git a/builtin-describe.c b/builtin-describe.c
index 669110c..b6f5b45 100644
--- a/builtin-describe.c
+++ b/builtin-describe.c
@@ -53,7 +53,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 (!prefixcmp(path, "refs/tags/")) {
+	if (!prefixcmp(path, PATH_REFS_TAGS)) {
 		if (object->type == OBJ_TAG)
 			prio = 2;
 		else
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index 24c7e6f..521b5fc 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -76,7 +76,7 @@ static int update_local_ref(const char *name,
 		char *msg;
 	just_store:
 		/* new ref */
-		if (!strncmp(name, "refs/tags/", 10))
+		if (!prefixcmp(name, PATH_REFS_TAGS))
 			msg = "storing tag";
 		else
 			msg = "storing head";
@@ -94,7 +94,7 @@ static int update_local_ref(const char *name,
 		return 0;
 	}
 
-	if (!strncmp(name, "refs/tags/", 10)) {
+	if (!prefixcmp(name, PATH_REFS_TAGS)) {
 		fprintf(stderr, "* %s: updating with %s\n", name, note);
 		show_new(type, sha1_new);
 		return update_ref_env("updating tag", name, sha1_new, NULL);
@@ -151,15 +151,15 @@ static int append_fetch_head(FILE *fp,
 		kind = "";
 		what = "";
 	}
-	else if (!strncmp(remote_name, "refs/heads/", 11)) {
+	else if (!prefixcmp(remote_name, PATH_REFS_HEADS)) {
 		kind = "branch";
 		what = remote_name + 11;
 	}
-	else if (!strncmp(remote_name, "refs/tags/", 10)) {
+	else if (!prefixcmp(remote_name, PATH_REFS_TAGS)) {
 		kind = "tag";
 		what = remote_name + 10;
 	}
-	else if (!strncmp(remote_name, "refs/remotes/", 13)) {
+	else if (!prefixcmp(remote_name, PATH_REFS_REMOTES)) {
 		kind = "remote branch";
 		what = remote_name + 13;
 	}
diff --git a/builtin-fmt-merge-msg.c b/builtin-fmt-merge-msg.c
index ae60fcc..8700343 100644
--- a/builtin-fmt-merge-msg.c
+++ b/builtin-fmt-merge-msg.c
@@ -4,6 +4,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "tag.h"
+#include "refs.h"
 
 static const char *fmt_merge_msg_usage =
 	"git-fmt-merge-msg [--summary] [--no-summary] [--file <file>]";
@@ -282,8 +283,8 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 	current_branch = resolve_ref("HEAD", head_sha1, 1, NULL);
 	if (!current_branch)
 		die("No current branch");
-	if (!prefixcmp(current_branch, "refs/heads/"))
-		current_branch += 11;
+	if (!prefixcmp(current_branch, PATH_REFS_HEADS))
+		current_branch += STRLEN_PATH_REFS_HEADS;
 
 	while (fgets(line, sizeof(line), in)) {
 		i++;
diff --git a/builtin-fsck.c b/builtin-fsck.c
index 8d12287..83a2d0c 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -629,14 +629,14 @@ static int fsck_head_link(void)
 	if (!strcmp(head_points_at, "HEAD"))
 		/* detached HEAD */
 		null_is_error = 1;
-	else if (prefixcmp(head_points_at, "refs/heads/"))
+	else if (prefixcmp(head_points_at, PATH_REFS_HEADS))
 		return error("HEAD points to something strange (%s)",
 			     head_points_at);
 	if (is_null_sha1(sha1)) {
 		if (null_is_error)
 			return error("HEAD: detached HEAD points at nothing");
 		fprintf(stderr, "notice: HEAD points to an unborn branch (%s)\n",
-			head_points_at + 11);
+			head_points_at + STRLEN_PATH_REFS_HEADS);
 	}
 	return 0;
 }
diff --git a/builtin-init-db.c b/builtin-init-db.c
index 763fa55..1f3c0dd 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -5,6 +5,7 @@
  */
 #include "cache.h"
 #include "builtin.h"
+#include "refs.h"
 
 #ifndef DEFAULT_GIT_TEMPLATE_DIR
 #define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
@@ -194,11 +195,11 @@ static int create_default_files(const char *git_dir, const char *template_path)
 	/*
 	 * Create .git/refs/{heads,tags}
 	 */
-	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);
 
 	/* First copy the templates -- we might have the default
@@ -217,11 +218,11 @@ static int create_default_files(const char *git_dir, const char *template_path)
 	if (shared_repository) {
 		path[len] = 0;
 		adjust_shared_perm(path);
-		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);
 	}
 
@@ -232,7 +233,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);
 	}
 
diff --git a/builtin-name-rev.c b/builtin-name-rev.c
index 03083e9..56c13d0 100644
--- a/builtin-name-rev.c
+++ b/builtin-name-rev.c
@@ -96,7 +96,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
 	struct name_ref_data *data = cb_data;
 	int deref = 0;
 
-	if (data->tags_only && prefixcmp(path, "refs/tags/"))
+	if (data->tags_only && prefixcmp(path, PATH_REFS_TAGS))
 		return 0;
 
 	if (data->ref_filter && fnmatch(data->ref_filter, path, 0))
@@ -112,14 +112,14 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
 	if (o && o->type == OBJ_COMMIT) {
 		struct commit *commit = (struct commit *)o;
 
-		if (!prefixcmp(path, "refs/heads/"))
-			path = path + 11;
+		if (!prefixcmp(path, PATH_REFS_HEADS))
+			path = path + STRLEN_PATH_REFS_HEADS;
 		else if (data->tags_only
 		    && data->name_only
-		    && !prefixcmp(path, "refs/tags/"))
-			path = path + 10;
-		else if (!prefixcmp(path, "refs/"))
-			path = path + 5;
+		    && !prefixcmp(path, PATH_REFS_TAGS))
+			path = path + STRLEN_PATH_REFS_TAGS;
+		else if (!prefixcmp(path, PATH_REFS))
+			path = path + STRLEN_PATH_REFS;
 
 		name_rev(commit, xstrdup(path), 0, 0, deref);
 	}
diff --git a/builtin-pack-refs.c b/builtin-pack-refs.c
index 09df4e1..23b4c4e 100644
--- a/builtin-pack-refs.c
+++ b/builtin-pack-refs.c
@@ -39,7 +39,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 = !prefixcmp(path, "refs/tags/");
+	is_tag_ref = !prefixcmp(path, PATH_REFS_TAGS);
 
 	/* ALWAYS pack refs that were already packed or are tags */
 	if (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref && !(flags & REF_ISPACKED))
diff --git a/builtin-push.c b/builtin-push.c
index 88c5024..2fdae7a 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -33,9 +33,9 @@ static void set_refspecs(const char **refs, int nr)
 			int len;
 			if (nr <= ++i)
 				die("tag shorthand without <tag>");
-			len = strlen(refs[i]) + 11;
+			len = strlen(refs[i]) + STRLEN_PATH_REFS_TAGS + 1;
 			tag = xmalloc(len);
-			strcpy(tag, "refs/tags/");
+			strcpy(tag, PATH_REFS_TAGS);
 			strcat(tag, refs[i]);
 			ref = tag;
 		}
@@ -148,7 +148,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 		if (!strcmp(arg, "--tags")) {
-			add_refspec("refs/tags/*");
+			add_refspec(PATH_REFS_TAGS "*");
 			continue;
 		}
 		if (!strcmp(arg, "--force") || !strcmp(arg, "-f")) {
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 4fa87f6..7e39d60 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -380,36 +380,36 @@ static int append_ref(const char *refname, const unsigned char *sha1,
 static int append_head_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	unsigned char tmp[20];
-	int ofs = 11;
-	if (prefixcmp(refname, "refs/heads/"))
+	int ofs = STRLEN_PATH_REFS_HEADS;
+	if (prefixcmp(refname, PATH_REFS_HEADS))
 		return 0;
 	/* If both heads/foo and tags/foo exists, get_sha1 would
 	 * get confused.
 	 */
 	if (get_sha1(refname + ofs, tmp) || hashcmp(tmp, sha1))
-		ofs = 5;
+		ofs = STRLEN_PATH_REFS;
 	return append_ref(refname + ofs, sha1, 0);
 }
 
 static int append_remote_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	unsigned char tmp[20];
-	int ofs = 13;
-	if (prefixcmp(refname, "refs/remotes/"))
+	int ofs = STRLEN_PATH_REFS_REMOTES;
+	if (prefixcmp(refname, PATH_REFS_REMOTES))
 		return 0;
 	/* If both heads/foo and tags/foo exists, get_sha1 would
 	 * get confused.
 	 */
 	if (get_sha1(refname + ofs, tmp) || hashcmp(tmp, sha1))
-		ofs = 5;
+		ofs = STRLEN_PATH_REFS;
 	return append_ref(refname + ofs, sha1, 0);
 }
 
 static int append_tag_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
-	if (prefixcmp(refname, "refs/tags/"))
+	if (prefixcmp(refname, PATH_REFS_TAGS))
 		return 0;
-	return append_ref(refname + 5, sha1, 0);
+	return append_ref(refname + STRLEN_PATH_REFS, sha1, 0);
 }
 
 static const char *match_ref_pattern = NULL;
@@ -438,9 +438,9 @@ static int append_matching_ref(const char *refname, const unsigned char *sha1, i
 		return 0;
 	if (fnmatch(match_ref_pattern, tail, 0))
 		return 0;
-	if (!prefixcmp(refname, "refs/heads/"))
+	if (!prefixcmp(refname, PATH_REFS_HEADS))
 		return append_head_ref(refname, sha1, flag, cb_data);
-	if (!prefixcmp(refname, "refs/tags/"))
+	if (!prefixcmp(refname, PATH_REFS_TAGS))
 		return append_tag_ref(refname, sha1, flag, cb_data);
 	return append_ref(refname, sha1, 0);
 }
@@ -465,12 +465,12 @@ static int rev_is_head(char *head, int headlen, char *name,
 	if ((!head[0]) ||
 	    (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
 		return 0;
-	if (!prefixcmp(head, "refs/heads/"))
-		head += 11;
-	if (!prefixcmp(name, "refs/heads/"))
-		name += 11;
-	else if (!prefixcmp(name, "heads/"))
-		name += 6;
+	if (!prefixcmp(head, PATH_REFS_HEADS))
+		head += STRLEN_PATH_REFS_HEADS;
+	if (!prefixcmp(name, PATH_REFS_HEADS))
+		name += STRLEN_PATH_REFS_HEADS;
+	else if (!prefixcmp(name, PATH_HEADS))
+		name += STRLEN_PATH_HEADS;
 	return !strcmp(head, name);
 }
 
@@ -781,7 +781,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				has_head++;
 		}
 		if (!has_head) {
-			int pfxlen = strlen("refs/heads/");
+			int pfxlen = STRLEN_PATH_REFS_HEADS;
 			append_one_rev(head + pfxlen);
 		}
 	}
diff --git a/builtin-show-ref.c b/builtin-show-ref.c
index 65051d1..d463d80 100644
--- a/builtin-show-ref.c
+++ b/builtin-show-ref.c
@@ -29,8 +29,8 @@ static int show_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	if (tags_only || heads_only) {
 		int match;
 
-		match = heads_only && !prefixcmp(refname, "refs/heads/");
-		match |= tags_only && !prefixcmp(refname, "refs/tags/");
+		match = heads_only && !prefixcmp(refname, PATH_REFS_HEADS);
+		match |= tags_only && !prefixcmp(refname, PATH_REFS_TAGS);
 		if (!match)
 			return 0;
 	}
@@ -227,7 +227,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
 		while (*pattern) {
 			unsigned char sha1[20];
 
-			if (!prefixcmp(*pattern, "refs/") &&
+			if (!prefixcmp(*pattern, PATH_REFS) &&
 			    resolve_ref(*pattern, sha1, 1, NULL)) {
 				if (!quiet)
 					show_one(*pattern, sha1);
diff --git a/builtin-tag.c b/builtin-tag.c
index 3a9d2ee..800e823 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -146,7 +146,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
 	unsigned char sha1[20];
 
 	for (p = argv; *p; p++) {
-		if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p)
+		if (snprintf(ref, sizeof(ref), PATH_REFS_TAGS"%s", *p)
 					>= sizeof(ref)) {
 			error("tag name too long: %.*s...", 50, *p);
 			had_error = 1;
@@ -440,7 +440,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (get_sha1(object_ref, object))
 		die("Failed to resolve '%s' as a valid ref.", object_ref);
 
-	if (snprintf(ref, sizeof(ref), "refs/tags/%s", tag) > sizeof(ref) - 1)
+	if (snprintf(ref, sizeof(ref), PATH_REFS_TAGS "%s", tag) > sizeof(ref) - 1)
 		die("tag name too long: %.*s...", 50, tag);
 	if (check_ref_format(ref))
 		die("'%s' is not a valid tag name.", tag);
diff --git a/connect.c b/connect.c
index 8b1e993..9d576bc 100644
--- a/connect.c
+++ b/connect.c
@@ -13,23 +13,23 @@ static int check_ref(const char *name, int len, unsigned int flags)
 	if (!flags)
 		return 1;
 
-	if (len < 5 || memcmp(name, "refs/", 5))
+	if (len < STRLEN_PATH_REFS || memcmp(name, PATH_REFS, STRLEN_PATH_REFS))
 		return 0;
 
 	/* Skip the "refs/" part */
-	name += 5;
-	len -= 5;
+	name += STRLEN_PATH_REFS;
+	len -= STRLEN_PATH_REFS;
 
 	/* REF_NORMAL means that we don't want the magic fake tag refs */
 	if ((flags & REF_NORMAL) && check_ref_format(name) < 0)
 		return 0;
 
 	/* REF_HEADS means that we want regular branch heads */
-	if ((flags & REF_HEADS) && !memcmp(name, "heads/", 6))
+	if ((flags & REF_HEADS) && !memcmp(name, PATH_HEADS, STRLEN_PATH_HEADS))
 		return 1;
 
 	/* REF_TAGS means that we want tags */
-	if ((flags & REF_TAGS) && !memcmp(name, "tags/", 5))
+	if ((flags & REF_TAGS) && !memcmp(name, PATH_TAGS, STRLEN_PATH_TAGS))
 		return 1;
 
 	/* All type bits clear means that we are ok with anything */
diff --git a/fetch-pack.c b/fetch-pack.c
index 9c81305..c3b7ef6 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -344,11 +344,11 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
 
 	for (ref = *refs; ref; ref = next) {
 		next = ref->next;
-		if (!memcmp(ref->name, "refs/", 5) &&
-		    check_ref_format(ref->name + 5))
+		if (!memcmp(ref->name, PATH_REFS, STRLEN_PATH_REFS) &&
+		    check_ref_format(ref->name + STRLEN_PATH_REFS))
 			; /* trash */
 		else if (fetch_all &&
-			 (!depth || prefixcmp(ref->name, "refs/tags/") )) {
+			 (!depth || prefixcmp(ref->name, PATH_REFS_TAGS) )) {
 			*newtail = ref;
 			ref->next = NULL;
 			newtail = &ref->next;
diff --git a/http-fetch.c b/http-fetch.c
index 202fae0..85c46a8 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -3,6 +3,7 @@
 #include "pack.h"
 #include "fetch.h"
 #include "http.h"
+#include "refs.h"
 
 #define PREV_BUF_SIZE 4096
 #define RANGE_HEADER_SIZE 30
@@ -157,11 +158,11 @@ static void start_object_request(struct object_request *obj_req)
 
 	SHA1_Init(&obj_req->c);
 
-	url = xmalloc(strlen(obj_req->repo->base) + 51);
-	obj_req->url = xmalloc(strlen(obj_req->repo->base) + 51);
+	url = xmalloc(strlen(obj_req->repo->base) + STRLEN_PATH_OBJECTS + 43);
+	obj_req->url = xmalloc(strlen(obj_req->repo->base) + STRLEN_PATH_OBJECTS + 43);
 	strcpy(url, obj_req->repo->base);
 	posn = url + strlen(obj_req->repo->base);
-	strcpy(posn, "/objects/");
+	strcpy(posn, "/" PATH_OBJECTS);
 	posn += 9;
 	memcpy(posn, hex, 2);
 	posn += 2;
@@ -398,8 +399,8 @@ static int fetch_index(struct alt_base *repo, unsigned char *sha1)
 	if (get_verbosely)
 		fprintf(stderr, "Getting index for pack %s\n", hex);
 
-	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);
 
 	filename = sha1_pack_index_name(sha1);
 	snprintf(tmpfile, sizeof(tmpfile), "%s.temp", filename);
@@ -478,7 +479,7 @@ static void process_alternates_response(void *callback_data)
 
 			/* Try reusing the slot to get non-http alternates */
 			alt_req->http_specific = 0;
-			sprintf(alt_req->url, "%s/objects/info/alternates",
+			sprintf(alt_req->url, "%s/" PATH_OBJECTS "info/alternates",
 				base);
 			curl_easy_setopt(slot->curl, CURLOPT_URL,
 					 alt_req->url);
@@ -625,8 +626,8 @@ static void fetch_alternates(const char *base)
 	if (get_verbosely)
 		fprintf(stderr, "Getting alternates list for %s\n", base);
 
-	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);
 
 	/* Use a callback to process the result, since another request
 	   may fail and need to have alternates loaded before continuing */
@@ -675,8 +676,8 @@ static int fetch_indices(struct alt_base *repo)
 	if (get_verbosely)
 		fprintf(stderr, "Getting pack list for %s\n", repo->base);
 
-	url = xmalloc(strlen(repo->base) + 21);
-	sprintf(url, "%s/objects/info/packs", repo->base);
+	url = xmalloc(strlen(repo->base) + STRLEN_PATH_OBJECTS + 13);
+	sprintf(url, "%s/" PATH_OBJECTS "info/packs", repo->base);
 
 	slot = get_active_slot();
 	slot->results = &results;
@@ -757,8 +758,8 @@ static int fetch_pack(struct alt_base *repo, unsigned char *sha1)
 			sha1_to_hex(sha1));
 	}
 
-	url = xmalloc(strlen(repo->base) + 65);
-	sprintf(url, "%s/objects/pack/pack-%s.pack",
+	url = xmalloc(strlen(repo->base) + STRLEN_PATH_OBJECTS + 57);
+	sprintf(url, "%s/" PATH_OBJECTS "pack/pack-%s.pack",
 		repo->base, sha1_to_hex(target->sha1));
 
 	filename = sha1_pack_name(target->sha1);
@@ -930,14 +931,14 @@ static char *quote_ref_url(const char *base, const char *ref)
 	int len, baselen, ch;
 
 	baselen = strlen(base);
-	len = baselen + 7; /* "/refs/" + NUL */
+	len = baselen + STRLEN_PATH_REFS + 2; /* "/" + "refs/" + NUL */
 	for (cp = ref; (ch = *cp) != 0; cp++, len++)
 		if (needs_quote(ch))
 			len += 2; /* extra two hex plus replacement % */
 	qref = xmalloc(len);
 	memcpy(qref, base, baselen);
-	memcpy(qref + baselen, "/refs/", 6);
-	for (cp = ref, dp = qref + baselen + 6; (ch = *cp) != 0; cp++) {
+	memcpy(qref + baselen, "/" PATH_REFS, STRLEN_PATH_REFS + 1);
+	for (cp = ref, dp = qref + baselen + STRLEN_PATH_REFS + 1; (ch = *cp) != 0; cp++) {
 		if (needs_quote(ch)) {
 			*dp++ = '%';
 			*dp++ = hex((ch >> 4) & 0xF);
diff --git a/http-push.c b/http-push.c
index 7c3720f..1d26b9b 100644
--- a/http-push.c
+++ b/http-push.c
@@ -272,12 +272,12 @@ static void start_fetch_loose(struct transfer_request *request)
 
 	SHA1_Init(&request->c);
 
-	url = xmalloc(strlen(remote->url) + 50);
-	request->url = xmalloc(strlen(remote->url) + 50);
+	url = xmalloc(strlen(remote->url) + 42 + STRLEN_PATH_OBJECTS);
+	request->url = xmalloc(strlen(remote->url) + 42 + STRLEN_PATH_OBJECTS);
 	strcpy(url, remote->url);
 	posn = url + strlen(remote->url);
-	strcpy(posn, "objects/");
-	posn += 8;
+	strcpy(posn, PATH_OBJECTS);
+	posn += STRLEN_PATH_OBJECTS;
 	memcpy(posn, hex, 2);
 	posn += 2;
 	*(posn++) = '/';
@@ -357,11 +357,11 @@ static void start_mkcol(struct transfer_request *request)
 	struct active_request_slot *slot;
 	char *posn;
 
-	request->url = xmalloc(strlen(remote->url) + 13);
+	request->url = xmalloc(strlen(remote->url) + STRLEN_PATH_OBJECTS + 5);
 	strcpy(request->url, remote->url);
 	posn = request->url + strlen(remote->url);
-	strcpy(posn, "objects/");
-	posn += 8;
+	strcpy(posn, PATH_OBJECTS);
+	posn += STRLEN_PATH_OBJECTS;
 	memcpy(posn, hex, 2);
 	posn += 2;
 	strcpy(posn, "/");
@@ -415,8 +415,8 @@ static void start_fetch_packed(struct transfer_request *request)
 	snprintf(request->tmpfile, sizeof(request->tmpfile),
 		 "%s.temp", filename);
 
-	url = xmalloc(strlen(remote->url) + 64);
-	sprintf(url, "%sobjects/pack/pack-%s.pack",
+	url = xmalloc(strlen(remote->url) + STRLEN_PATH_OBJECTS + 56);
+	sprintf(url, "%s" PATH_OBJECTS "pack/pack-%s.pack",
 		remote->url, sha1_to_hex(target->sha1));
 
 	/* Make sure there isn't another open request for this pack */
@@ -519,11 +519,11 @@ static void start_put(struct transfer_request *request)
 	request->buffer.posn = 0;
 
 	request->url = xmalloc(strlen(remote->url) +
-			       strlen(request->lock->token) + 51);
+			       strlen(request->lock->token) + STRLEN_PATH_OBJECTS + 43);
 	strcpy(request->url, remote->url);
 	posn = request->url + strlen(remote->url);
-	strcpy(posn, "objects/");
-	posn += 8;
+	strcpy(posn, PATH_OBJECTS);
+	posn += STRLEN_PATH_OBJECTS;
 	memcpy(posn, hex, 2);
 	posn += 2;
 	*(posn++) = '/';
@@ -922,8 +922,8 @@ static int fetch_index(unsigned char *sha1)
 	struct slot_results results;
 
 	/* Don't use the index if the pack isn't there */
-	url = xmalloc(strlen(remote->url) + 64);
-	sprintf(url, "%sobjects/pack/pack-%s.pack", remote->url, hex);
+	url = xmalloc(strlen(remote->url) + STRLEN_PATH_OBJECTS + 56);
+	sprintf(url, "%s" PATH_OBJECTS "pack/pack-%s.pack", remote->url, hex);
 	slot = get_active_slot();
 	slot->results = &results;
 	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
@@ -945,7 +945,7 @@ static int fetch_index(unsigned char *sha1)
 	if (push_verbosely)
 		fprintf(stderr, "Getting index for pack %s\n", hex);
 
-	sprintf(url, "%sobjects/pack/pack-%s.idx", remote->url, hex);
+	sprintf(url, "%s" PATH_OBJECTS "pack/pack-%s.idx", remote->url, hex);
 
 	filename = sha1_pack_index_name(sha1);
 	snprintf(tmpfile, sizeof(tmpfile), "%s.temp", filename);
@@ -1029,8 +1029,8 @@ static int fetch_indices(void)
 	if (push_verbosely)
 		fprintf(stderr, "Getting pack list\n");
 
-	url = xmalloc(strlen(remote->url) + 20);
-	sprintf(url, "%sobjects/info/packs", remote->url);
+	url = xmalloc(strlen(remote->url) + STRLEN_PATH_OBJECTS + 12);
+	sprintf(url, "%s" PATH_OBJECTS "info/packs", remote->url);
 
 	slot = get_active_slot();
 	slot->results = &results;
@@ -1615,7 +1615,7 @@ static void remote_ls(const char *path, int flags,
 
 static void get_remote_object_list(unsigned char parent)
 {
-	char path[] = "objects/XX/";
+	char path[] = PATH_OBJECTS "XX/";
 	static const char hex[] = "0123456789abcdef";
 	unsigned int val = parent;
 
@@ -1925,7 +1925,7 @@ static void get_local_heads(void)
 static void get_dav_remote_heads(void)
 {
 	remote_tail = &remote_refs;
-	remote_ls("refs/", (PROCESS_FILES | PROCESS_DIRS | RECURSIVE), process_ls_ref, NULL);
+	remote_ls(PATH_REFS, (PROCESS_FILES | PROCESS_DIRS | RECURSIVE), process_ls_ref, NULL);
 }
 
 static int is_zero_sha1(const unsigned char *sha1)
@@ -2069,7 +2069,7 @@ static void update_remote_info_refs(struct remote_lock *lock)
 	buffer.buffer = xcalloc(1, 4096);
 	buffer.size = 4096;
 	buffer.posn = 0;
-	remote_ls("refs/", (PROCESS_FILES | RECURSIVE),
+	remote_ls(PATH_REFS, (PROCESS_FILES | RECURSIVE),
 		  add_remote_info_ref, &buffer);
 	if (!aborted) {
 		if_header = xmalloc(strlen(lock->token) + 25);
@@ -2375,7 +2375,7 @@ int main(int argc, char **argv)
 	/* Check whether the remote has server info files */
 	remote->can_update_info_refs = 0;
 	remote->has_info_refs = remote_exists("info/refs");
-	remote->has_info_packs = remote_exists("objects/info/packs");
+	remote->has_info_packs = remote_exists(PATH_OBJECTS "info/packs");
 	if (remote->has_info_refs) {
 		info_ref_lock = lock_remote("info/refs", LOCK_TIME);
 		if (info_ref_lock)
diff --git a/local-fetch.c b/local-fetch.c
index bf7ec6c..67dc065 100644
--- a/local-fetch.c
+++ b/local-fetch.c
@@ -4,6 +4,7 @@
 #include "cache.h"
 #include "commit.h"
 #include "fetch.h"
+#include "refs.h"
 
 static int use_link;
 static int use_symlink;
@@ -23,7 +24,7 @@ static void setup_index(unsigned char *sha1)
 	struct packed_git *new_pack;
 	char filename[PATH_MAX];
 	strcpy(filename, path);
-	strcat(filename, "/objects/pack/pack-");
+	strcat(filename, "/" PATH_OBJECTS "pack/pack-");
 	strcat(filename, sha1_to_hex(sha1));
 	strcat(filename, ".idx");
 	new_pack = parse_pack_index_file(sha1, filename);
@@ -37,7 +38,7 @@ static int setup_indices(void)
 	struct dirent *de;
 	char filename[PATH_MAX];
 	unsigned char sha1[20];
-	sprintf(filename, "%s/objects/pack/", path);
+	sprintf(filename, "%s/" PATH_OBJECTS "pack/", path);
 	dir = opendir(filename);
 	if (!dir)
 		return -1;
@@ -122,11 +123,11 @@ static int fetch_pack(const unsigned char *sha1)
 		fprintf(stderr, " which contains %s\n",
 			sha1_to_hex(sha1));
 	}
-	sprintf(filename, "%s/objects/pack/pack-%s.pack",
+	sprintf(filename, "%s/" PATH_OBJECTS "pack/pack-%s.pack",
 		path, sha1_to_hex(target->sha1));
 	copy_file(filename, sha1_pack_name(target->sha1),
 		  sha1_to_hex(target->sha1), 1);
-	sprintf(filename, "%s/objects/pack/pack-%s.idx",
+	sprintf(filename, "%s/" PATH_OBJECTS "pack/pack-%s.idx",
 		path, sha1_to_hex(target->sha1));
 	copy_file(filename, sha1_pack_index_name(target->sha1),
 		  sha1_to_hex(target->sha1), 1);
@@ -143,7 +144,7 @@ static int fetch_file(const unsigned char *sha1)
 
 	if (object_name_start < 0) {
 		strcpy(filename, path); /* e.g. git.git */
-		strcat(filename, "/objects/");
+		strcat(filename, "/" PATH_OBJECTS);
 		object_name_start = strlen(filename);
 	}
 	filename[object_name_start+0] = hex[0];
@@ -169,7 +170,7 @@ int fetch_ref(char *ref, unsigned char *sha1)
 	int ifd;
 
 	if (ref_name_start < 0) {
-		sprintf(filename, "%s/refs/", path);
+		sprintf(filename, "%s/" PATH_REFS, path);
 		ref_name_start = strlen(filename);
 	}
 	strcpy(filename + ref_name_start, ref);
diff --git a/path.c b/path.c
index 4260952..d330bbc 100644
--- a/path.c
+++ b/path.c
@@ -11,6 +11,7 @@
  * which is what it's designed for.
  */
 #include "cache.h"
+#include "refs.h"
 
 static char bad_path[] = "/bad-path/";
 
@@ -99,7 +100,7 @@ int validate_headref(const char *path)
 	/* Make sure it is a "refs/.." symlink */
 	if (S_ISLNK(st.st_mode)) {
 		len = readlink(path, buffer, sizeof(buffer)-1);
-		if (len >= 5 && !memcmp("refs/", buffer, 5))
+		if (len >= STRLEN_PATH_REFS && !memcmp(PATH_REFS, buffer, STRLEN_PATH_REFS))
 			return 0;
 		return -1;
 	}
@@ -123,7 +124,7 @@ int validate_headref(const char *path)
 		len -= 4;
 		while (len && isspace(*buf))
 			buf++, len--;
-		if (len >= 5 && !memcmp("refs/", buf, 5))
+		if (len >= STRLEN_PATH_REFS && !memcmp(PATH_REFS, buf, STRLEN_PATH_REFS))
 			return 0;
 	}
 
diff --git a/receive-pack.c b/receive-pack.c
index d3c422b..114ea38 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -165,7 +165,7 @@ static const char *update(struct command *cmd)
 	unsigned char *new_sha1 = cmd->new_sha1;
 	struct ref_lock *lock;
 
-	if (!prefixcmp(name, "refs/") && check_ref_format(name + 5)) {
+	if (!prefixcmp(name, PATH_REFS) && check_ref_format(name + STRLEN_PATH_REFS)) {
 		error("refusing to create funny ref '%s' locally", name);
 		return "funny refname";
 	}
@@ -177,7 +177,7 @@ static const char *update(struct command *cmd)
 	}
 	if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
 	    !is_null_sha1(old_sha1) &&
-	    !prefixcmp(name, "refs/heads/")) {
+	    !prefixcmp(name, PATH_REFS_HEADS)) {
 		struct commit *old_commit, *new_commit;
 		struct commit_list *bases, *ent;
 
diff --git a/reflog-walk.c b/reflog-walk.c
index ee1456b..98cf8ef 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -55,11 +55,11 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)
 	}
 	if (reflogs->nr == 0) {
 		int len = strlen(ref);
-		char *refname = xmalloc(len + 12);
-		sprintf(refname, "refs/%s", ref);
+		char *refname = xmalloc(len + STRLEN_PATH_REFS_HEADS + 1);
+		sprintf(refname, PATH_REFS "%s", ref);
 		for_each_reflog_ent(refname, read_one_reflog, reflogs);
 		if (reflogs->nr == 0) {
-			sprintf(refname, "refs/heads/%s", ref);
+			sprintf(refname, PATH_REFS_HEADS "%s", ref);
 			for_each_reflog_ent(refname, read_one_reflog, reflogs);
 		}
 		free(refname);
diff --git a/refs.c b/refs.c
index 7fb3350..840a433 100644
--- a/refs.c
+++ b/refs.c
@@ -409,7 +409,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
 		/* Follow "normalized" - ie "refs/.." symlinks by hand */
 		if (S_ISLNK(st.st_mode)) {
 			len = readlink(path, buffer, sizeof(buffer)-1);
-			if (len >= 5 && !memcmp("refs/", buffer, 5)) {
+			if (len >= STRLEN_PATH_REFS && !memcmp(PATH_REFS, buffer, STRLEN_PATH_REFS)) {
 				buffer[len] = 0;
 				strcpy(ref_buffer, buffer);
 				ref = ref_buffer;
@@ -561,22 +561,22 @@ int head_ref(each_ref_fn fn, void *cb_data)
 
 int for_each_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/", fn, 0, cb_data);
+	return do_for_each_ref(PATH_REFS, fn, 0, cb_data);
 }
 
 int for_each_tag_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/tags/", fn, 10, cb_data);
+	return do_for_each_ref(PATH_REFS_TAGS, fn, STRLEN_PATH_REFS_TAGS, cb_data);
 }
 
 int for_each_branch_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/heads/", fn, 11, cb_data);
+	return do_for_each_ref(PATH_REFS_HEADS, fn, STRLEN_PATH_REFS_HEADS, cb_data);
 }
 
 int for_each_remote_ref(each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref("refs/remotes/", fn, 13, cb_data);
+	return do_for_each_ref(PATH_REFS_REMOTES, fn, STRLEN_PATH_REFS_REMOTES, cb_data);
 }
 
 /* NEEDSWORK: This is only used by ssh-upload and it should go; the
@@ -588,7 +588,7 @@ int get_ref_sha1(const char *ref, unsigned char *sha1)
 {
 	if (check_ref_format(ref))
 		return -1;
-	return read_ref(mkpath("refs/%s", ref), sha1);
+	return read_ref(mkpath(PATH_REFS "%s", ref), sha1);
 }
 
 /*
@@ -824,7 +824,7 @@ struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)
 	char refpath[PATH_MAX];
 	if (check_ref_format(ref))
 		return NULL;
-	strcpy(refpath, mkpath("refs/%s", ref));
+	strcpy(refpath, mkpath(PATH_REFS "%s", ref));
 	return lock_ref_sha1_basic(refpath, old_sha1, 0, NULL);
 }
 
@@ -1078,8 +1078,8 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
 	log_file = git_path("logs/%s", ref_name);
 
 	if (log_all_ref_updates &&
-	    (!prefixcmp(ref_name, "refs/heads/") ||
-	     !prefixcmp(ref_name, "refs/remotes/") ||
+	    (!prefixcmp(ref_name,  PATH_REFS_HEADS) ||
+	     !prefixcmp(ref_name, PATH_REFS_REMOTES) ||
 	     !strcmp(ref_name, "HEAD"))) {
 		if (safe_create_leading_directories(log_file) < 0)
 			return error("unable to create directory for %s",
diff --git a/refs.h b/refs.h
index 6eb98a4..1025d04 100644
--- a/refs.h
+++ b/refs.h
@@ -13,6 +13,23 @@ struct ref_lock {
 #define REF_ISSYMREF 01
 #define REF_ISPACKED 02
 
+#define PATH_OBJECTS             "objects/"
+#define STRLEN_PATH_OBJECTS      8
+#define PATH_REFS                "refs/"
+#define STRLEN_PATH_REFS         5
+#define PATH_HEADS               "heads/"
+#define STRLEN_PATH_HEADS        6
+#define PATH_TAGS                "tags/"
+#define STRLEN_PATH_TAGS         5
+#define PATH_REMOTES             "remotes/"
+#define STRLEN_PATH_REMOTES      8
+#define PATH_REFS_HEADS          PATH_REFS PATH_HEADS
+#define STRLEN_PATH_REFS_HEADS   (STRLEN_PATH_REFS+STRLEN_PATH_HEADS)
+#define PATH_REFS_TAGS           PATH_REFS PATH_TAGS
+#define STRLEN_PATH_REFS_TAGS    (STRLEN_PATH_REFS+STRLEN_PATH_TAGS)
+#define PATH_REFS_REMOTES        PATH_REFS PATH_REMOTES
+#define STRLEN_PATH_REFS_REMOTES (STRLEN_PATH_REFS+STRLEN_PATH_REMOTES)
+
 /*
  * Calls the specified function for each ref file until it returns nonzero,
  * and returns the value
diff --git a/remote.c b/remote.c
index bb774d0..b8922c7 100644
--- a/remote.c
+++ b/remote.c
@@ -211,8 +211,8 @@ static void read_config(void)
 	current_branch = NULL;
 	head_ref = resolve_ref("HEAD", sha1, 0, &flag);
 	if (head_ref && (flag & REF_ISSYMREF) &&
-	    !prefixcmp(head_ref, "refs/heads/")) {
-		current_branch = head_ref + strlen("refs/heads/");
+	    !prefixcmp(head_ref, PATH_REFS_HEADS)) {
+		current_branch = head_ref + STRLEN_PATH_REFS_HEADS;
 		current_branch_len = strlen(current_branch);
 	}
 	git_config(handle_config);
@@ -398,9 +398,9 @@ static int count_refspec_match(const char *pattern,
 		 * at the remote site.
 		 */
 		if (namelen != patlen &&
-		    patlen != namelen - 5 &&
-		    prefixcmp(name, "refs/heads/") &&
-		    prefixcmp(name, "refs/tags/")) {
+		    patlen != namelen - STRLEN_PATH_REFS &&
+		    prefixcmp(name, PATH_REFS_HEADS) &&
+		    prefixcmp(name, PATH_REFS_TAGS)) {
 			/* We want to catch the case where only weak
 			 * matches are found and there are multiple
 			 * matches, and where more than one strong
@@ -511,7 +511,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 	case 1:
 		break;
 	case 0:
-		if (!memcmp(dst_value, "refs/", 5))
+		if (!prefixcmp(dst_value, PATH_REFS))
 			matched_dst = make_linked_ref(dst_value, dst_tail);
 		else
 			error("dst refspec %s does not match any "
@@ -594,7 +594,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 			if (!pat)
 				continue;
 		}
-		else if (prefixcmp(src->name, "refs/heads/"))
+		else if (prefixcmp(src->name, PATH_REFS_HEADS))
 			/*
 			 * "matching refs"; traditionally we pushed everything
 			 * including refs outside refs/heads/ hierarchy, but
diff --git a/setup.c b/setup.c
index 06004f1..c8912d2 100644
--- a/setup.c
+++ b/setup.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "dir.h"
+#include "refs.h"
 
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
@@ -158,12 +159,12 @@ static int is_git_directory(const char *suspect)
 			return 0;
 	}
 	else {
-		strcpy(path + len, "/objects");
+		strcpy(path + len, "/" PATH_OBJECTS);
 		if (access(path, X_OK))
 			return 0;
 	}
 
-	strcpy(path + len, "/refs");
+	strcpy(path + len, "/" PATH_REFS);
 	if (access(path, X_OK))
 		return 0;
 
diff --git a/sha1_name.c b/sha1_name.c
index 2d727d5..649e438 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -241,11 +241,11 @@ static int ambiguous_path(const char *path, int len)
 
 static const char *ref_fmt[] = {
 	"%.*s",
-	"refs/%.*s",
-	"refs/tags/%.*s",
-	"refs/heads/%.*s",
-	"refs/remotes/%.*s",
-	"refs/remotes/%.*s/HEAD",
+	PATH_REFS "%.*s",
+	PATH_REFS_TAGS "%.*s",
+	PATH_REFS_HEADS "%.*s",
+	PATH_REFS_REMOTES "%.*s",
+	PATH_REFS_REMOTES "%.*s/HEAD",
 	NULL
 };
 
diff --git a/wt-status.c b/wt-status.c
index 10ce6ee..93dee72 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -7,6 +7,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "diffcore.h"
+#include "refs.h"
 
 int wt_status_use_color = 0;
 static char wt_status_colors[][COLOR_MAXLEN] = {
@@ -311,8 +312,8 @@ void wt_status_print(struct wt_status *s)
 	if (s->branch) {
 		const char *on_what = "On branch ";
 		const char *branch_name = s->branch;
-		if (!prefixcmp(branch_name, "refs/heads/"))
-			branch_name += 11;
+		if (!prefixcmp(branch_name, PATH_REFS_HEADS))
+			branch_name += STRLEN_PATH_REFS_HEADS;
 		else if (!strcmp(branch_name, "HEAD")) {
 			branch_name = "";
 			on_what = "Not currently on any branch.";
-- 
1.5.3.rc5.11.g312e

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

        die("HEAD not found below %.*%s!",
             PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS-1)

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

Even though we all know that PATH_REFS_* do not have any '%' in
them, it is somewhat unnerving to see such an opaque string in
the format specifier part of _any_printf() function.  It just
makes you think twice, disrupting the flow of thoughts.

This applies to die() and friends as well; see my above rewrite.

To me, the valid reasons for this kind of rewrite are if:

 - it makes typo harder to make and easier to spot
   (e.g. "refs/head/");

 - it makes miscount harder to make and easier to spot (e.g.
   what is this magic constant 11? Is it strlen("refs/heads/")?);

 - it makes reviewing the resulting code, and more importantly,
   future patches on the resulting code, easier.

 - it makes it easier for us to later revamp the strings
   wholesale (e.g. "refs/heads/" => "refs/branches/").

 - it saves us repeated instances of the same string constant;
   using C literal string as values for PATH_REFS_HEADS would
   not help and you would need (const char []) strings instead,
   but the compiler may be clever enough to do so.

Unquestionably, this series helps on the first two counts.

It however actively hurts on the third count.  These long
constants in CAPITAL_LETTERS_WITH_UNDERSCORE shout too loudly to
the eye, overwhelming the surrounding code.  I wonder if we can
do anything about this point to resurrect the first two
benefits, which I like very much.

The forth is a myth we shouldn't care about.  If we later would
want to change refs/heads to refs/branches, we would want to
rename PATH_REFS_HEADS to PATH_REFS_BRANCHES at the same time as
well, so the kind of rewrite this patch does does not buy us
anything there.  More importantly, such a change would need to
be made in a backward compatible way (e.g. "if we have heads
then keep using them but in new repositories we favor
branches"), so it won't be straight token replacement anyway.

And the fifth do not apply to us.  This matters only if we were
an embedded application on memory starved machine and string
constants are far smaller matter compared to the amount of other
data we use in-core.

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

[Jeff King]

On Tue, Oct 02, 2007 at 12:47:59PM -0700, Junio C Hamano wrote:

>  - it makes typo harder to make and easier to spot
>    (e.g. "refs/head/");
> 
>  - it makes miscount harder to make and easier to spot (e.g.
>    what is this magic constant 11? Is it strlen("refs/heads/")?);
> 
>  - it makes reviewing the resulting code, and more importantly,
>    future patches on the resulting code, easier.
> [...]
> It however actively hurts on the third count.  These long

Yes, I find some of the substitutions more readable, but some are a bit
less readable. The parts of the patch I found the _most_ improved are
the ones that get rid of a memcmp in favor of a prefixcmp (i.e.,
removing the count entirely).

Perhaps a better quest would be to eliminate all of those counts
entirely with code that is obviously correct. I think it is much more
readable to replace:

  url = xmalloc(strlen(repo->base) + 64);
  sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);

with something like:

  strbuf_init(&url);
  strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);

which has the same number of lines, but no magic numbers at all. Or
since most of the uses of things like PATH_OBJECTS are more or less the
same, maybe something like:

  mkpath_object(&url, "pack/pack-%s.idx", hex);

i.e., rather than fiddling with string constants, wrap them
functionally.

> constants in CAPITAL_LETTERS_WITH_UNDERSCORE shout too loudly to

Part of the problem is also that they're long. Perhaps REFS_HEADS, while
being less unique in the C namespace, would look better?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Perhaps a better quest would be to eliminate all of those counts
> entirely with code that is obviously correct. I think it is much more
> readable to replace:
>
>   url = xmalloc(strlen(repo->base) + 64);
>   sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
>
> with something like:
>
>   strbuf_init(&url);
>   strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);

Ugh, this typically calls snprintf() twice doesn't it?

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

[Jeff King]

On Tue, Oct 02, 2007 at 05:22:23PM -0700, Junio C Hamano wrote:

> >   strbuf_init(&url);
> >   strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
> 
> Ugh, this typically calls snprintf() twice doesn't it?

Yes, it probably does. However, I think it is considerably easier to
read and more maintainable. Are you "ugh"ing because of the performance
impact (which should be negligible unless this is in a tight loop) or
because of the portability problems associated with va_copy?

-Peff

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

[Johannes Schindelin]

Hi,

On Tue, 2 Oct 2007, Jeff King wrote:

> On Tue, Oct 02, 2007 at 05:22:23PM -0700, Junio C Hamano wrote:
> 
> > >   strbuf_init(&url);
> > >   strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
> > 
> > Ugh, this typically calls snprintf() twice doesn't it?
> 
> Yes, it probably does. However, I think it is considerably easier to
> read and more maintainable. Are you "ugh"ing because of the performance
> impact (which should be negligible unless this is in a tight loop) or
> because of the portability problems associated with va_copy?

I wonder, I wonder, if

	strbuf_addstr(&url, repo->base);
	strbuf_addstr(&url, "/objects/pack/pack-");
	strbuf_addstr(&url, hex);
	strbuf_addstr(&url, ".idx");

would make anybody else but me happy...

Ciao,
Dscho

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

[Jeff King]

On Wed, Oct 03, 2007 at 05:05:15AM +0100, Johannes Schindelin wrote:

> I wonder, I wonder, if
> 
> 	strbuf_addstr(&url, repo->base);
> 	strbuf_addstr(&url, "/objects/pack/pack-");
> 	strbuf_addstr(&url, hex);
> 	strbuf_addstr(&url, ".idx");
> 
> would make anybody else but me happy...

I actually wrote that originally, and then switched to the formatted
version for readability. But I would be happy with that, as well, if we
are truly concerned about the cost of 2 snprintfs.

-Peff

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

[Andy Parkins]

On Tuesday 2007 October 02, Jeff King wrote:

> Perhaps a better quest would be to eliminate all of those counts
> entirely with code that is obviously correct. I think it is much more
> readable to replace:

I've got a patch replacing every appropriate memcmp() with prefixcmp(), but it 
goes on top of this one, so wanted to get this through review to save 
constantly spamming the list with the same patch slightly modified because of 
changes in a different patch.

>   url = xmalloc(strlen(repo->base) + 64);
>   sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
>
> with something like:
>
>   strbuf_init(&url);
>   strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);

I've not been following the strbuf() changes, so have missed the appearance of 
these handy new functions.  They would appear to be an improvement for cases 
just like this.

> > constants in CAPITAL_LETTERS_WITH_UNDERSCORE shout too loudly to
>
> Part of the problem is also that they're long. Perhaps REFS_HEADS, while
> being less unique in the C namespace, would look better?

I completely agree with the length and loudness concerns, but my worry was 
polluting the namespace while maintaining some sort of rationality between 
PATH_REFS_HEADS and STRLEN_PATH_REFS_HEADS.  My reasoning was that

 "refs/heads" -> PATH_REFS_HEADS

is only three extra characters, and

 strlen("refs/heads/") -> STRLEN_PATH_REFS_HEADS

is only one extra character.

However I have no strong feelings about changing them.



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

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

[Andy Parkins]

On Tuesday 2007 October 02, Junio C Hamano wrote:
> Andy Parkins <andyparkins@gmail.com> writes:
> > I noticed a couple of places where memcmp() has been used where
> > prefixcmp() would work fine.  I'm tempted to change them too - what do
> > you think? Perhaps a separate patch?
>
> In general, probably it is preferable to have a separate
> "preliminary patch" to normalize the existing code without using
> the new infrastructure (i.e. REF_* macros), and then to have the
> main patch.  That way would make the main patch more about
> mechanical conversion, which would be easier to verify
> independently.

I only noticed them memcmp() use while I was reviewing the PATH_REFS patch :-)  
So making it preliminary would have meant travelling backwards in time.

I imagine I can do a bit of rebase-interactive to rearrange should that be the 
route you'd like me to go.  Your call.


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

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

[Andy Parkins]

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

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

[Andy Parkins]

On Wednesday 2007 October 03, Andy Parkins wrote:

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

I'm a liar; I remember writing them, but they're not in the patch.  Perhaps 
they are still in my working tree at home.  Anyway; you're point was correct, 
I'll find the explanatory comments or write new ones and add them to the 
patch.



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

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

[Andreas Ericsson]

Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 2 Oct 2007, Jeff King wrote:
> 
>> On Tue, Oct 02, 2007 at 05:22:23PM -0700, Junio C Hamano wrote:
>>
>>>>   strbuf_init(&url);
>>>>   strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
>>> Ugh, this typically calls snprintf() twice doesn't it?
>> Yes, it probably does. However, I think it is considerably easier to
>> read and more maintainable. Are you "ugh"ing because of the performance
>> impact (which should be negligible unless this is in a tight loop) or
>> because of the portability problems associated with va_copy?
> 
> I wonder, I wonder, if
> 
> 	strbuf_addstr(&url, repo->base);
> 	strbuf_addstr(&url, "/objects/pack/pack-");
> 	strbuf_addstr(&url, hex);
> 	strbuf_addstr(&url, ".idx");
> 
> would make anybody else but me happy...

strbuf_addstr_many(&url, repo->base, "/objects/pack/pack-", hex, ".idx", NULL);

is what I'd prefer. It's not overly complicated, requires no *printf(), and doesn't
introduce any new portability issues (va_arg() is C89).

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231