[PATCH v2 00/11] Consolidate ref parsing code

[PATCH v2 00/11] Consolidate ref parsing code

[Michael Haggerty]

This is a rif on Duy's oldish patch series [1]. I started reviewing
his patch series, but found that some of his patches did multiple
things, making it harder to review. I started pulling it apart into
smaller changes to aid my review, and I guess I got carried away :-/

As far as I know, Duy isn't actively working on this, so I hope my
reroll is not unwelcome.

As far as I can tell, Duy's patch series was correct aside from a
couple of minor cosmetic blemishes [2]. So if you want to accept Duy's
original patch series, I hereby endorse it.

This version does the following things beyond Duy's original:

* Split commits up into smaller pieces.

* Get rid of the MAXREFLEN limitation.

* Rename the parse_ref() parameter from "ref" to "refname".

* Inline resolve_gitlink_packed_ref() and handle_missing_loose_ref().

* Invert the "if" statement for dealing with symbolic references in
  parse_ref() to make the logic flow more linear.

* Change a couple of "while" loops to "do..while".

* Change resolve_refdup() to return strbuf_detach(&buf, NULL) instead
  of buf.buf directly (as suggested by Eric Sunshine).

I retained Duy as author on commits that are derived straightforwardly
from his. I hope I haven't broken any of them. I am calling this patch
series v2 because I propose it as a successor to Duy's version.

[1] http://thread.gmane.org/gmane.comp.version-control.git/254203/focus=254203

[2] Aside from the couple of things pointed out on the mailing list,

    * The parse_ref() parameter should be named "refname" instead of
      "ref", for consistency with other refs code.

    * The local variable "ref = result->buf" in resolve_ref() just
      obscures things and should be inlined.

Michael Haggerty (5):
  resolve_ref_unsafe(): reverse the logic of the symref conditional
  handle_missing_loose_ref(): inline function
  resolve_gitlink_ref_recursive(): drop arbitrary refname length limit
  resove_gitlink_packed_ref(): inline function
  resolve_gitlink_ref(): remove redundant test

Nguyễn Thái Ngọc Duy (6):
  strbuf_read_file(): preserve errno on failure
  handle_missing_loose_ref(): return an int
  resolve_ref_unsafe(): use skip_prefix() to skip over "ref:"
  refs.c: refactor resolve_ref_unsafe() to use strbuf internally
  refs.c: move ref parsing code out of resolve_ref()
  refs.c: rewrite resolve_gitlink_ref() to use parse_ref()

 cache.h  |  12 +++
 refs.c   | 371 +++++++++++++++++++++++++++++++--------------------------------
 strbuf.c |   7 +-
 3 files changed, 200 insertions(+), 190 deletions(-)

-- 
2.1.1

[PATCH v2 01/11] strbuf_read_file(): preserve errno on failure

[Michael Haggerty]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

This will allow the function to be used in a later patch.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 strbuf.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index 0346e74..f1fec58 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -482,15 +482,18 @@ int strbuf_getwholeline_fd(struct strbuf *sb, int fd, int term)
 
 int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
 {
-	int fd, len;
+	int fd, len, saved_errno;
 
 	fd = open(path, O_RDONLY);
 	if (fd < 0)
 		return -1;
 	len = strbuf_read(sb, fd, hint);
+	saved_errno = errno;
 	close(fd);
-	if (len < 0)
+	if (len < 0) {
+		errno = saved_errno;
 		return -1;
+	}
 
 	return len;
 }
-- 
2.1.1

[PATCH v2 02/11] handle_missing_loose_ref(): return an int

[Michael Haggerty]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

The return value of handle_missing_loose_ref() was either NULL to
signify an error or the input parameter refname on success. So instead
of returning a string, just return a 0 on success or -1 on error, so
the reader doesn't have to wonder what string the return value points
at.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/refs.c b/refs.c
index ffd45e9..ceba23c 100644
--- a/refs.c
+++ b/refs.c
@@ -1370,10 +1370,10 @@ static struct ref_entry *get_packed_ref(const char *refname)
  * A loose ref file doesn't exist; check for a packed ref.  The
  * options are forwarded from resolve_safe_unsafe().
  */
-static const char *handle_missing_loose_ref(const char *refname,
-					    unsigned char *sha1,
-					    int reading,
-					    int *flag)
+static int handle_missing_loose_ref(const char *refname,
+				    unsigned char *sha1,
+				    int reading,
+				    int *flag)
 {
 	struct ref_entry *entry;
 
@@ -1386,14 +1386,14 @@ static const char *handle_missing_loose_ref(const char *refname,
 		hashcpy(sha1, entry->u.value.sha1);
 		if (flag)
 			*flag |= REF_ISPACKED;
-		return refname;
+		return 0;
 	}
 	/* The reference is not a packed reference, either. */
 	if (reading) {
-		return NULL;
+		return -1;
 	} else {
 		hashclr(sha1);
-		return refname;
+		return 0;
 	}
 }
 
@@ -1437,10 +1437,12 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 		 */
 	stat_ref:
 		if (lstat(path, &st) < 0) {
-			if (errno == ENOENT)
-				return handle_missing_loose_ref(refname, sha1,
-								reading, flag);
-			else
+			if (errno == ENOENT) {
+				if (handle_missing_loose_ref(refname, sha1,
+							     reading, flag))
+					return NULL;
+				return refname;
+			} else
 				return NULL;
 		}
 
-- 
2.1.1

[PATCH v2 03/11] resolve_ref_unsafe(): reverse the logic of the symref conditional

[Michael Haggerty]

It is clearer if the check whether a loose reference file is a symref
is followed immediately by the code to handle the symref, rather than
the current code, which has the if statement the other way around.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/refs.c b/refs.c
index ceba23c..771941b 100644
--- a/refs.c
+++ b/refs.c
@@ -1497,35 +1497,35 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 			len--;
 		buffer[len] = '\0';
 
-		/*
-		 * Is it a symbolic ref?
-		 */
-		if (!starts_with(buffer, "ref:")) {
-			/*
-			 * Please note that FETCH_HEAD has a second
-			 * line containing other data.
-			 */
-			if (get_sha1_hex(buffer, sha1) ||
-			    (buffer[40] != '\0' && !isspace(buffer[40]))) {
+		if (starts_with(buffer, "ref:")) {
+			/* It is a symbolic ref */
+			if (flag)
+				*flag |= REF_ISSYMREF;
+			buf = buffer + 4;
+			while (isspace(*buf))
+				buf++;
+			if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
 				if (flag)
 					*flag |= REF_ISBROKEN;
 				errno = EINVAL;
 				return NULL;
 			}
-			return refname;
+			refname = strcpy(refname_buffer, buf);
+			continue;
 		}
-		if (flag)
-			*flag |= REF_ISSYMREF;
-		buf = buffer + 4;
-		while (isspace(*buf))
-			buf++;
-		if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
+
+		/*
+		 * It must be a normal ref. Please note that
+		 * FETCH_HEAD has a second line containing other data.
+		 */
+		if (get_sha1_hex(buffer, sha1) ||
+		    (buffer[40] != '\0' && !isspace(buffer[40]))) {
 			if (flag)
 				*flag |= REF_ISBROKEN;
 			errno = EINVAL;
 			return NULL;
 		}
-		refname = strcpy(refname_buffer, buf);
+		return refname;
 	}
 }
 
-- 
2.1.1

[PATCH v2 04/11] resolve_ref_unsafe(): use skip_prefix() to skip over "ref:"

[Michael Haggerty]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

This requires buf to be declared (const char *), which is how it is
used anyway.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 771941b..020ee3f 100644
--- a/refs.c
+++ b/refs.c
@@ -1416,7 +1416,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 	for (;;) {
 		char path[PATH_MAX];
 		struct stat st;
-		char *buf;
+		const char *buf;
 		int fd;
 
 		if (--depth < 0) {
@@ -1497,11 +1497,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 			len--;
 		buffer[len] = '\0';
 
-		if (starts_with(buffer, "ref:")) {
+		if (skip_prefix(buffer, "ref:", &buf)) {
 			/* It is a symbolic ref */
 			if (flag)
 				*flag |= REF_ISSYMREF;
-			buf = buffer + 4;
 			while (isspace(*buf))
 				buf++;
 			if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
-- 
2.1.1

[PATCH v2 05/11] refs.c: refactor resolve_ref_unsafe() to use strbuf internally

[Michael Haggerty]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

In the beginning, we had resolve_ref() that returns a buffer owned by
this function. Then we started to move away from that direction because
the buffer could be overwritten by the next resolve_ref() call and
introduced two new functions: resolve_ref_unsafe() and resolve_refdup().
The static buffer is still kept internally.

This patch makes the core of resolve_ref use a strbuf instead of static
buffer. Which makes resolve_refdup() more efficient (no need to copy
from the static buffer to a new buffer). It also removes the (random?)
256 char limit. In future, resolve_ref() could be used directly without
going through resolve_refdup() wrapper.

A minor bonus. resolve_ref(dup) are now more thread-friendly (although I'm
not 100% sure if they are thread-safe yet).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 cache.h |   1 +
 refs.c  | 115 +++++++++++++++++++++++++++++++++++++---------------------------
 2 files changed, 67 insertions(+), 49 deletions(-)

diff --git a/cache.h b/cache.h
index 3e6a914..e36084d 100644
--- a/cache.h
+++ b/cache.h
@@ -1004,6 +1004,7 @@ extern int read_ref(const char *refname, unsigned char *sha1);
  */
 extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag);
 extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);
+extern int resolve_ref(const char *refname, struct strbuf *result, unsigned char *sha1, int reading, int *flag);
 
 extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
 extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
diff --git a/refs.c b/refs.c
index 020ee3f..29ea7e0 100644
--- a/refs.c
+++ b/refs.c
@@ -1397,34 +1397,41 @@ static int handle_missing_loose_ref(const char *refname,
 	}
 }
 
-/* This function needs to return a meaningful errno on failure */
-const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)
+/*
+ * 'result' content will be destroyed. Its value may be undefined if
+ * resolve_ref returns -1.
+ *
+ * This function needs to return a meaningful errno on failure
+ */
+int resolve_ref(const char *refname, struct strbuf *result,
+		unsigned char *sha1, int reading, int *flag)
 {
+	struct strbuf buffer = STRBUF_INIT;
 	int depth = MAXDEPTH;
-	ssize_t len;
-	char buffer[256];
-	static char refname_buffer[256];
+	int ret = -1;
 
 	if (flag)
 		*flag = 0;
 
 	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
 		errno = EINVAL;
-		return NULL;
+		return -1;
 	}
 
+	strbuf_reset(result);
+	strbuf_addstr(result, refname);
+
 	for (;;) {
 		char path[PATH_MAX];
 		struct stat st;
 		const char *buf;
-		int fd;
 
 		if (--depth < 0) {
 			errno = ELOOP;
-			return NULL;
+			break;
 		}
 
-		git_snpath(path, sizeof(path), "%s", refname);
+		git_snpath(path, sizeof(path), "%s", result->buf);
 
 		/*
 		 * We might have to loop back here to avoid a race
@@ -1437,30 +1444,26 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 		 */
 	stat_ref:
 		if (lstat(path, &st) < 0) {
-			if (errno == ENOENT) {
-				if (handle_missing_loose_ref(refname, sha1,
-							     reading, flag))
-					return NULL;
-				return refname;
-			} else
-				return NULL;
+			if (errno == ENOENT)
+				ret = handle_missing_loose_ref(result->buf, sha1,
+							       reading, flag);
+			break;
 		}
 
 		/* Follow "normalized" - ie "refs/.." symlinks by hand */
 		if (S_ISLNK(st.st_mode)) {
-			len = readlink(path, buffer, sizeof(buffer)-1);
-			if (len < 0) {
+			/* no need to reset buffer, strbuf_readlink does that */
+			if (strbuf_readlink(&buffer, path, 256) < 0) {
 				if (errno == ENOENT || errno == EINVAL)
 					/* inconsistent with lstat; retry */
 					goto stat_ref;
 				else
-					return NULL;
+					break;
 			}
-			buffer[len] = 0;
-			if (starts_with(buffer, "refs/") &&
-					!check_refname_format(buffer, 0)) {
-				strcpy(refname_buffer, buffer);
-				refname = refname_buffer;
+			if (starts_with(buffer.buf, "refs/") &&
+			    !check_refname_format(buffer.buf, 0)) {
+				strbuf_reset(result);
+				strbuf_addbuf(result, &buffer);
 				if (flag)
 					*flag |= REF_ISSYMREF;
 				continue;
@@ -1470,34 +1473,24 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 		/* Is it a directory? */
 		if (S_ISDIR(st.st_mode)) {
 			errno = EISDIR;
-			return NULL;
+			break;
 		}
 
 		/*
 		 * Anything else, just open it and try to use it as
 		 * a ref
 		 */
-		fd = open(path, O_RDONLY);
-		if (fd < 0) {
+		strbuf_reset(&buffer);
+		if (strbuf_read_file(&buffer, path, 256) < 0) {
 			if (errno == ENOENT)
 				/* inconsistent with lstat; retry */
 				goto stat_ref;
 			else
-				return NULL;
-		}
-		len = read_in_full(fd, buffer, sizeof(buffer)-1);
-		if (len < 0) {
-			int save_errno = errno;
-			close(fd);
-			errno = save_errno;
-			return NULL;
+				break;
 		}
-		close(fd);
-		while (len && isspace(buffer[len-1]))
-			len--;
-		buffer[len] = '\0';
+		strbuf_rtrim(&buffer);
 
-		if (skip_prefix(buffer, "ref:", &buf)) {
+		if (skip_prefix(buffer.buf, "ref:", &buf)) {
 			/* It is a symbolic ref */
 			if (flag)
 				*flag |= REF_ISSYMREF;
@@ -1507,9 +1500,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 				if (flag)
 					*flag |= REF_ISBROKEN;
 				errno = EINVAL;
-				return NULL;
+				break;
 			}
-			refname = strcpy(refname_buffer, buf);
+			strbuf_reset(result);
+			strbuf_add(result, buf, buffer.buf + buffer.len - buf);
 			continue;
 		}
 
@@ -1517,21 +1511,44 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 		 * It must be a normal ref. Please note that
 		 * FETCH_HEAD has a second line containing other data.
 		 */
-		if (get_sha1_hex(buffer, sha1) ||
-		    (buffer[40] != '\0' && !isspace(buffer[40]))) {
+		if (get_sha1_hex(buffer.buf, sha1) ||
+		    (buffer.buf[40] != '\0' && !isspace(buffer.buf[40]))) {
 			if (flag)
 				*flag |= REF_ISBROKEN;
 			errno = EINVAL;
-			return NULL;
-		}
-		return refname;
+		} else
+			ret = 0;
+		break;
 	}
+	strbuf_release(&buffer);
+	return ret;
+}
+
+const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)
+{
+	static struct strbuf buf = STRBUF_INIT;
+
+	if (!resolve_ref(refname, &buf, sha1, reading, flag))
+		/*
+		 * Please note: true to the "unsafe" name of this
+		 * function, we return a pointer to our internal
+		 * memory here rather than passing ownership to the
+		 * caller by calling strbuf_detach():
+		 */
+		return buf.buf;
+	else
+		return NULL;
 }
 
 char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)
 {
-	const char *ret = resolve_ref_unsafe(ref, sha1, reading, flag);
-	return ret ? xstrdup(ret) : NULL;
+	struct strbuf buf = STRBUF_INIT;
+	if (!resolve_ref(ref, &buf, sha1, reading, flag))
+		return strbuf_detach(&buf, NULL);
+	else {
+		strbuf_release(&buf);
+		return NULL;
+	}
 }
 
 /* The argument to filter_refs */
-- 
2.1.1

[PATCH v2 06/11] refs.c: move ref parsing code out of resolve_ref()

[Michael Haggerty]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

This function will soon have a second caller.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

I leave parse_ref() a public function for the reason explained by
Duy [1].

[1] http://article.gmane.org/gmane.comp.version-control.git/254274

 cache.h |  11 ++++
 refs.c  | 200 ++++++++++++++++++++++++++++++++++------------------------------
 2 files changed, 118 insertions(+), 93 deletions(-)

diff --git a/cache.h b/cache.h
index e36084d..d2b8399 100644
--- a/cache.h
+++ b/cache.h
@@ -1005,6 +1005,17 @@ extern int read_ref(const char *refname, unsigned char *sha1);
 extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag);
 extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);
 extern int resolve_ref(const char *refname, struct strbuf *result, unsigned char *sha1, int reading, int *flag);
+/*
+ * Given a ref in "ref" and its path, returns
+ *
+ * -2  failed to open with ENOENT, the caller is responsible for
+ *     checking missing loose ref (see resolve_ref for example)
+ * -1  if there's an error, "refname" can no longer be trusted, "flag" may
+ *     be set. errno is valid.
+ *  0  this is a symref, "refname" now contains the linked ref
+ * +1  normal ref, "sha1" is valid
+ */
+extern int parse_ref(const char *path, struct strbuf *refname, unsigned char *sha1, int *flag);
 
 extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
 extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
diff --git a/refs.c b/refs.c
index 29ea7e0..3acd83e 100644
--- a/refs.c
+++ b/refs.c
@@ -1397,6 +1397,104 @@ static int handle_missing_loose_ref(const char *refname,
 	}
 }
 
+int parse_ref(const char *path, struct strbuf *refname,
+	      unsigned char *sha1, int *flag)
+{
+	struct strbuf buffer = STRBUF_INIT;
+	struct stat st;
+	const char *buf;
+	int ret;
+
+	/*
+	 * We might have to loop back here to avoid a race condition:
+	 * first we lstat() the file, then we try to read it as a link
+	 * or as a file.  But if somebody changes the type of the file
+	 * (file <-> directory <-> symlink) between the lstat() and
+	 * reading, then we don't want to report that as an error but
+	 * rather try again starting with the lstat().
+	 */
+stat_ref:
+	if (lstat(path, &st) < 0)
+		return errno == ENOENT ? -2 : -1;
+
+	/* Follow "normalized" - ie "refs/.." symlinks by hand */
+	if (S_ISLNK(st.st_mode)) {
+		struct strbuf new_path = STRBUF_INIT;
+		if (strbuf_readlink(&new_path, path, 256) < 0) {
+			strbuf_release(&new_path);
+			if (errno == ENOENT || errno == EINVAL)
+				/* inconsistent with lstat; retry */
+				goto stat_ref;
+			else
+				return -1;
+		}
+		if (starts_with(new_path.buf, "refs/") &&
+		    !check_refname_format(new_path.buf, 0)) {
+			strbuf_reset(refname);
+			strbuf_addbuf(refname, &new_path);
+			if (flag)
+				*flag |= REF_ISSYMREF;
+			strbuf_release(&new_path);
+			return 0;
+		}
+		strbuf_release(&new_path);
+	}
+
+	/* Is it a directory? */
+	if (S_ISDIR(st.st_mode)) {
+		errno = EISDIR;
+		return -1;
+	}
+
+	/*
+	 * Anything else, just open it and try to use it as
+	 * a ref
+	 */
+	if (strbuf_read_file(&buffer, path, 256) < 0) {
+		strbuf_release(&buffer);
+		if (errno == ENOENT)
+			/* inconsistent with lstat; retry */
+			goto stat_ref;
+		else
+			return -1;
+	}
+	strbuf_rtrim(&buffer);
+
+	if (skip_prefix(buffer.buf, "ref:", &buf)) {
+		/* It is a symbolic ref */
+		if (flag)
+			*flag |= REF_ISSYMREF;
+		while (isspace(*buf))
+			buf++;
+		if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
+			if (flag)
+				*flag |= REF_ISBROKEN;
+			strbuf_release(&buffer);
+			errno = EINVAL;
+			return -1;
+		}
+		strbuf_reset(refname);
+		strbuf_add(refname, buf, buffer.buf + buffer.len - buf);
+		strbuf_release(&buffer);
+		return 0;
+	}
+
+	/*
+	 * Please note that FETCH_HEAD has a second line
+	 * containing other data.
+	 */
+	if (get_sha1_hex(buffer.buf, sha1) ||
+	    (buffer.buf[40] != '\0' && !isspace(buffer.buf[40]))) {
+		if (flag)
+			*flag |= REF_ISBROKEN;
+		errno = EINVAL;
+		ret = -1;
+	} else
+		ret = 1;
+	strbuf_release(&buffer);
+	return ret;
+}
+
 /*
  * 'result' content will be destroyed. Its value may be undefined if
  * resolve_ref returns -1.
@@ -1406,9 +1504,8 @@ static int handle_missing_loose_ref(const char *refname,
 int resolve_ref(const char *refname, struct strbuf *result,
 		unsigned char *sha1, int reading, int *flag)
 {
-	struct strbuf buffer = STRBUF_INIT;
 	int depth = MAXDEPTH;
-	int ret = -1;
+	int ret = 0;
 
 	if (flag)
 		*flag = 0;
@@ -1421,107 +1518,24 @@ int resolve_ref(const char *refname, struct strbuf *result,
 	strbuf_reset(result);
 	strbuf_addstr(result, refname);
 
-	for (;;) {
+	while (!ret) {
 		char path[PATH_MAX];
-		struct stat st;
-		const char *buf;
 
 		if (--depth < 0) {
 			errno = ELOOP;
+			ret = -1;
 			break;
 		}
 
 		git_snpath(path, sizeof(path), "%s", result->buf);
-
-		/*
-		 * We might have to loop back here to avoid a race
-		 * condition: first we lstat() the file, then we try
-		 * to read it as a link or as a file.  But if somebody
-		 * changes the type of the file (file <-> directory
-		 * <-> symlink) between the lstat() and reading, then
-		 * we don't want to report that as an error but rather
-		 * try again starting with the lstat().
-		 */
-	stat_ref:
-		if (lstat(path, &st) < 0) {
-			if (errno == ENOENT)
-				ret = handle_missing_loose_ref(result->buf, sha1,
-							       reading, flag);
-			break;
+		ret = parse_ref(path, result, sha1, flag);
+		if (ret == -2) {
+			ret = handle_missing_loose_ref(result->buf, sha1,
+						       reading, flag);
+			ret = ret ? -1 : 1;
 		}
-
-		/* Follow "normalized" - ie "refs/.." symlinks by hand */
-		if (S_ISLNK(st.st_mode)) {
-			/* no need to reset buffer, strbuf_readlink does that */
-			if (strbuf_readlink(&buffer, path, 256) < 0) {
-				if (errno == ENOENT || errno == EINVAL)
-					/* inconsistent with lstat; retry */
-					goto stat_ref;
-				else
-					break;
-			}
-			if (starts_with(buffer.buf, "refs/") &&
-			    !check_refname_format(buffer.buf, 0)) {
-				strbuf_reset(result);
-				strbuf_addbuf(result, &buffer);
-				if (flag)
-					*flag |= REF_ISSYMREF;
-				continue;
-			}
-		}
-
-		/* Is it a directory? */
-		if (S_ISDIR(st.st_mode)) {
-			errno = EISDIR;
-			break;
-		}
-
-		/*
-		 * Anything else, just open it and try to use it as
-		 * a ref
-		 */
-		strbuf_reset(&buffer);
-		if (strbuf_read_file(&buffer, path, 256) < 0) {
-			if (errno == ENOENT)
-				/* inconsistent with lstat; retry */
-				goto stat_ref;
-			else
-				break;
-		}
-		strbuf_rtrim(&buffer);
-
-		if (skip_prefix(buffer.buf, "ref:", &buf)) {
-			/* It is a symbolic ref */
-			if (flag)
-				*flag |= REF_ISSYMREF;
-			while (isspace(*buf))
-				buf++;
-			if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
-				if (flag)
-					*flag |= REF_ISBROKEN;
-				errno = EINVAL;
-				break;
-			}
-			strbuf_reset(result);
-			strbuf_add(result, buf, buffer.buf + buffer.len - buf);
-			continue;
-		}
-
-		/*
-		 * It must be a normal ref. Please note that
-		 * FETCH_HEAD has a second line containing other data.
-		 */
-		if (get_sha1_hex(buffer.buf, sha1) ||
-		    (buffer.buf[40] != '\0' && !isspace(buffer.buf[40]))) {
-			if (flag)
-				*flag |= REF_ISBROKEN;
-			errno = EINVAL;
-		} else
-			ret = 0;
-		break;
 	}
-	strbuf_release(&buffer);
-	return ret;
+	return ret > 0 ? 0 : -1;
 }
 
 const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)
-- 
2.1.1

[PATCH v2 07/11] handle_missing_loose_ref(): inline function

[Michael Haggerty]

It only had one remaining caller.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 70 ++++++++++++++++++++++++++++--------------------------------------
 1 file changed, 30 insertions(+), 40 deletions(-)

diff --git a/refs.c b/refs.c
index 3acd83e..9f2a0f8 100644
--- a/refs.c
+++ b/refs.c
@@ -1366,37 +1366,6 @@ static struct ref_entry *get_packed_ref(const char *refname)
 	return find_ref(get_packed_refs(&ref_cache), refname);
 }
 
-/*
- * A loose ref file doesn't exist; check for a packed ref.  The
- * options are forwarded from resolve_safe_unsafe().
- */
-static int handle_missing_loose_ref(const char *refname,
-				    unsigned char *sha1,
-				    int reading,
-				    int *flag)
-{
-	struct ref_entry *entry;
-
-	/*
-	 * The loose reference file does not exist; check for a packed
-	 * reference.
-	 */
-	entry = get_packed_ref(refname);
-	if (entry) {
-		hashcpy(sha1, entry->u.value.sha1);
-		if (flag)
-			*flag |= REF_ISPACKED;
-		return 0;
-	}
-	/* The reference is not a packed reference, either. */
-	if (reading) {
-		return -1;
-	} else {
-		hashclr(sha1);
-		return 0;
-	}
-}
-
 int parse_ref(const char *path, struct strbuf *refname,
 	      unsigned char *sha1, int *flag)
 {
@@ -1505,7 +1474,7 @@ int resolve_ref(const char *refname, struct strbuf *result,
 		unsigned char *sha1, int reading, int *flag)
 {
 	int depth = MAXDEPTH;
-	int ret = 0;
+	int ret;
 
 	if (flag)
 		*flag = 0;
@@ -1518,24 +1487,45 @@ int resolve_ref(const char *refname, struct strbuf *result,
 	strbuf_reset(result);
 	strbuf_addstr(result, refname);
 
-	while (!ret) {
+	do {
 		char path[PATH_MAX];
 
 		if (--depth < 0) {
 			errno = ELOOP;
-			ret = -1;
-			break;
+			return -1;
 		}
 
 		git_snpath(path, sizeof(path), "%s", result->buf);
 		ret = parse_ref(path, result, sha1, flag);
-		if (ret == -2) {
-			ret = handle_missing_loose_ref(result->buf, sha1,
-						       reading, flag);
-			ret = ret ? -1 : 1;
+	} while (!ret);
+
+	if (ret == 1) {
+		return 0;
+	} else if (ret == -2) {
+		/*
+		 * The loose reference file does not exist; check for a packed
+		 * reference.
+		 */
+		struct ref_entry *entry;
+
+		entry = get_packed_ref(result->buf);
+		if (entry) {
+			hashcpy(sha1, entry->u.value.sha1);
+			if (flag)
+				*flag |= REF_ISPACKED;
+			return 0;
+		}
+
+		/* The reference is not a packed reference, either. */
+		if (reading) {
+			return -1;
+		} else {
+			hashclr(sha1);
+			return 0;
 		}
+	} else {
+		return -1;
 	}
-	return ret > 0 ? 0 : -1;
 }
 
 const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)
-- 
2.1.1

[PATCH v2 08/11] resolve_gitlink_ref_recursive(): drop arbitrary refname length limit

[Michael Haggerty]

This limit was added in

    0ebde32 (Add 'resolve_gitlink_ref()' helper function - 2007-04-09)

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
Theoretically, somebody else might be relying on
resolve_gitlink_ref_recursive() to fail for too-long reference names
to prevent path.c's pitiful error handling from returning "/bad-path/"
and causing a nonsensical file lookup. I doubt it, but if somebody is
worried about it we could leave out this patch and instead build the
MAXREFLEN check into parse_ref().

Long-term, I think we should fix up path.c to remove its PATH_MAX
limits. I've started working on that.

 refs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 9f2a0f8..e1aa6a4 100644
--- a/refs.c
+++ b/refs.c
@@ -1279,7 +1279,6 @@ static struct ref_dir *get_loose_refs(struct ref_cache *refs)
 
 /* We allow "recursive" symbolic refs. Only within reason, though */
 #define MAXDEPTH 5
-#define MAXREFLEN (1024)
 
 /*
  * Called by resolve_gitlink_ref_recursive() after it failed to read
@@ -1308,7 +1307,7 @@ static int resolve_gitlink_ref_recursive(struct ref_cache *refs,
 	char buffer[128], *p;
 	char *path;
 
-	if (recursion > MAXDEPTH || strlen(refname) > MAXREFLEN)
+	if (recursion > MAXDEPTH)
 		return -1;
 	path = *refs->name
 		? git_path_submodule(refs->name, "%s", refname)
-- 
2.1.1

[PATCH v2 09/11] refs.c: rewrite resolve_gitlink_ref() to use parse_ref()

[Michael Haggerty]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

resolve_gitlink_ref_recursive() did about the same thing as
parse_ref(), but didn't know as many tricks. It also had another
random limit of 128 bytes for symrefs. So base resolve_gitlink_ref()
on parse_ref() instead.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 69 ++++++++++++++++++++++++++----------------------------------------
 1 file changed, 27 insertions(+), 42 deletions(-)

diff --git a/refs.c b/refs.c
index e1aa6a4..a7c8abd 100644
--- a/refs.c
+++ b/refs.c
@@ -1299,48 +1299,11 @@ static int resolve_gitlink_packed_ref(struct ref_cache *refs,
 	return 0;
 }
 
-static int resolve_gitlink_ref_recursive(struct ref_cache *refs,
-					 const char *refname, unsigned char *sha1,
-					 int recursion)
-{
-	int fd, len;
-	char buffer[128], *p;
-	char *path;
-
-	if (recursion > MAXDEPTH)
-		return -1;
-	path = *refs->name
-		? git_path_submodule(refs->name, "%s", refname)
-		: git_path("%s", refname);
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
-		return resolve_gitlink_packed_ref(refs, refname, sha1);
-
-	len = read(fd, buffer, sizeof(buffer)-1);
-	close(fd);
-	if (len < 0)
-		return -1;
-	while (len && isspace(buffer[len-1]))
-		len--;
-	buffer[len] = 0;
-
-	/* Was it a detached head or an old-fashioned symlink? */
-	if (!get_sha1_hex(buffer, sha1))
-		return 0;
-
-	/* Symref? */
-	if (strncmp(buffer, "ref:", 4))
-		return -1;
-	p = buffer + 4;
-	while (isspace(*p))
-		p++;
-
-	return resolve_gitlink_ref_recursive(refs, p, sha1, recursion+1);
-}
-
 int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1)
 {
-	int len = strlen(path), retval;
+	struct strbuf result = STRBUF_INIT;
+	int len = strlen(path), parseval, ret;
+	int depth = MAXDEPTH;
 	char *submodule;
 	struct ref_cache *refs;
 
@@ -1352,8 +1315,30 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sh
 	refs = get_ref_cache(submodule);
 	free(submodule);
 
-	retval = resolve_gitlink_ref_recursive(refs, refname, sha1, 0);
-	return retval;
+	strbuf_addstr(&result, refname);
+	do {
+		if (--depth < 0) {
+			errno = ELOOP;
+			ret = -1;
+			goto out;
+		}
+		path = *refs->name
+			? git_path_submodule(refs->name, "%s", result.buf)
+			: git_path("%s", result.buf);
+		parseval = parse_ref(path, &result, sha1, NULL);
+	} while (!parseval);
+
+	if (parseval == 1) {
+		ret = 0;
+	} else if (parseval == -2) {
+		ret = resolve_gitlink_packed_ref(refs, result.buf, sha1) ? -1 : 0;
+	} else {
+		ret = -1;
+	}
+
+out:
+	strbuf_release(&result);
+	return ret;
 }
 
 /*
-- 
2.1.1

[PATCH v2 10/11] resove_gitlink_packed_ref(): inline function

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/refs.c b/refs.c
index a7c8abd..8edcc3b 100644
--- a/refs.c
+++ b/refs.c
@@ -1280,25 +1280,6 @@ static struct ref_dir *get_loose_refs(struct ref_cache *refs)
 /* We allow "recursive" symbolic refs. Only within reason, though */
 #define MAXDEPTH 5
 
-/*
- * Called by resolve_gitlink_ref_recursive() after it failed to read
- * from the loose refs in ref_cache refs. Find <refname> in the
- * packed-refs file for the submodule.
- */
-static int resolve_gitlink_packed_ref(struct ref_cache *refs,
-				      const char *refname, unsigned char *sha1)
-{
-	struct ref_entry *ref;
-	struct ref_dir *dir = get_packed_refs(refs);
-
-	ref = find_ref(dir, refname);
-	if (ref == NULL)
-		return -1;
-
-	hashcpy(sha1, ref->u.value.sha1);
-	return 0;
-}
-
 int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1)
 {
 	struct strbuf result = STRBUF_INIT;
@@ -1331,7 +1312,17 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sh
 	if (parseval == 1) {
 		ret = 0;
 	} else if (parseval == -2) {
-		ret = resolve_gitlink_packed_ref(refs, result.buf, sha1) ? -1 : 0;
+		/* Loose ref doesn't exist; check for a packed ref */
+		struct ref_entry *entry;
+		struct ref_dir *dir = get_packed_refs(refs);
+
+		entry = find_ref(dir, result.buf);
+		if (entry) {
+			hashcpy(sha1, entry->u.value.sha1);
+			ret = 0;
+		} else {
+			ret = -1;
+		}
 	} else {
 		ret = -1;
 	}
-- 
2.1.1

[PATCH v2 11/11] resolve_gitlink_ref(): remove redundant test

[Michael Haggerty]

At this point we know that refs->name is a non-empty string, so we
know we don't have to look up the reference in the main repository.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 8edcc3b..3f4b95a 100644
--- a/refs.c
+++ b/refs.c
@@ -1303,9 +1303,7 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sh
 			ret = -1;
 			goto out;
 		}
-		path = *refs->name
-			? git_path_submodule(refs->name, "%s", result.buf)
-			: git_path("%s", result.buf);
+		path = git_path_submodule(refs->name, "%s", result.buf);
 		parseval = parse_ref(path, &result, sha1, NULL);
 	} while (!parseval);
 
-- 
2.1.1

Re: [PATCH v2 00/11] Consolidate ref parsing code

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> This is a rif on Duy's oldish patch series [1]. I started reviewing
> his patch series, but found that some of his patches did multiple
> things, making it harder to review. I started pulling it apart into
> smaller changes to aid my review, and I guess I got carried away :-/

Hmmm...

You are aware of another large change in flight in the same area,
and after having reviewed that series a few times I was hoping that
you would have a better understanding of how ready the other topic
is.  And then I see this series that conflicts with that other
topic.  Is this your way to say that the other topic is not quite
ready?

Re: [PATCH v2 00/11] Consolidate ref parsing code

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> This is a rif on Duy's oldish patch series [1]. I started reviewing
>> his patch series, but found that some of his patches did multiple
>> things, making it harder to review. I started pulling it apart into
>> smaller changes to aid my review, and I guess I got carried away :-/
>
> Hmmm...
>
> You are aware of another large change in flight in the same area,
> and after having reviewed that series a few times I was hoping that
> you would have a better understanding of how ready the other topic
> is.  And then I see this series that conflicts with that other
> topic.  Is this your way to say that the other topic is not quite
> ready?

The last one was a rhetorical question and I regret that it came out
a bit harsher than I intended to.  I just wanted to see a bit better
inter-developer coordination, that's all.

Thanks.

Re: [PATCH v2 00/11] Consolidate ref parsing code

[Michael Haggerty]

On 10/16/2014 10:47 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> This is a rif on Duy's oldish patch series [1]. I started reviewing
>> his patch series, but found that some of his patches did multiple
>> things, making it harder to review. I started pulling it apart into
>> smaller changes to aid my review, and I guess I got carried away :-/
> 
> Hmmm...
> 
> You are aware of another large change in flight in the same area,
> and after having reviewed that series a few times I was hoping that
> you would have a better understanding of how ready the other topic
> is.  And then I see this series that conflicts with that other
> topic.  Is this your way to say that the other topic is not quite
> ready?

No, not at all. To be honest, I thought that the changes in this patch
series were localized in an area different than Ronnie and Jonathan's
patches were touching, but stupidly I didn't check for conflicts. Sorry
about that.

There is nothing urgent in this patch series, so I suggest we just put
it back on ice and I will reroll after the dust has settled on
rs/ref-transactions. The conflicts shouldn't be super hard to resolve
(the series don't overlap much *conceptually*), but I'd rather not have
to do it multiple times.

Regarding rs/ref-transaction, I will reply on that thread.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v2 00/11] Consolidate ref parsing code

[Duy Nguyen]

On Wed, Oct 15, 2014 at 10:06 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> As far as I know, Duy isn't actively working on this, so I hope my
> reroll is not unwelcome.

I couldn't be happier that someone does the work for me and I still
benefit from it ;)
-- 
Duy