Re: [PATCH v5 25/27] refs: add LMDB refs storage backend

@@ -1407,6 +1407,95 @@ static int resolve_missing_loose_ref(const char *refname,
 	}
 }
 
+static const char *continue_normal_ref = "read_ref returns a normal ref";
+static const char *continue_symlink = "read_ref returns a symlink";
+
+/*
+ * Read a ref from backend. Returning any values except
+ * continue_normal_ref or continue_symlink ends resolve_ref_1()
+ * execution. If the return value is not NULL, sha1 and flags must be
+ * updated correctly. except REF_ISBROKEN which is set by
+ * resolve_ref_1().
+ *
+ * If continue_* is returned, sb_contents must contain the ref data.
+ */
+static const char *parse_ref(const char *refname,
+			     int resolve_flags,
+			     unsigned char *sha1,
+			     int *flags,
+			     struct strbuf *sb_path,
+			     struct strbuf *sb_contents)
+{
+	const char *path;
+	struct stat st;
+	int fd;
+
+	strbuf_reset(sb_path);
+	strbuf_git_path(sb_path, "%s", refname);
+	path = sb_path->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)
+			return NULL;
+		if (resolve_missing_loose_ref(refname, resolve_flags,
+					      sha1, flags))
+			return NULL;
+		return refname;
+	}
+
+	/* Follow "normalized" - ie "refs/.." symlinks by hand */
+	if (S_ISLNK(st.st_mode)) {
+		strbuf_reset(sb_contents);
+		if (strbuf_readlink(sb_contents, path, 0) < 0) {
+			if (errno == ENOENT || errno == EINVAL)
+				/* inconsistent with lstat; retry */
+				goto stat_ref;
+			else
+				return NULL;
+		}
+		return continue_symlink;
+	}
+
+	/* Is it a directory? */
+	if (S_ISDIR(st.st_mode)) {
+		errno = EISDIR;
+		return NULL;
+	}
+
+	/*
+	 * Anything else, just open it and try to use it as
+	 * a ref
+	 */
+	fd = open(path, O_RDONLY);
+	if (fd < 0) {
+		if (errno == ENOENT)
+			/* inconsistent with lstat; retry */
+			goto stat_ref;
+		else
+			return NULL;
+	}
+	strbuf_reset(sb_contents);
+	if (strbuf_read(sb_contents, fd, 256) < 0) {
+		int save_errno = errno;
+		close(fd);
+		errno = save_errno;
+		return NULL;
+	}
+	close(fd);
+
+	return continue_normal_ref;
+}
+
 /* This function needs to return a meaningful errno on failure */
 static const char *resolve_ref_1(const char *refname,
 				 int resolve_flags,

@@ -1442,54 +1531,18 @@ static const char *resolve_ref_1(const char *refname,
 		bad_name = 1;
 	}
 	for (;;) {
-		const char *path;
-		struct stat st;
+		const char *ret;
 		char *buf;
-		int fd;
 
 		if (--depth < 0) {
 			errno = ELOOP;
 			return NULL;
 		}
 
-		strbuf_reset(sb_path);
-		strbuf_git_path(sb_path, "%s", refname);
-		path = sb_path->buf;
+		ret = parse_ref(refname, resolve_flags, sha1,
+				flags, sb_path, sb_contents);
 
-		/*
-		 * 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)
-				return NULL;
-			if (resolve_missing_loose_ref(refname, resolve_flags,
-						      sha1, flags))
-				return NULL;
-			if (bad_name) {
-				hashclr(sha1);
-				if (flags)
-					*flags |= REF_ISBROKEN;
-			}
-			return refname;
-		}
-
-		/* Follow "normalized" - ie "refs/.." symlinks by hand */
-		if (S_ISLNK(st.st_mode)) {
-			strbuf_reset(sb_contents);
-			if (strbuf_readlink(sb_contents, path, 0) < 0) {
-				if (errno == ENOENT || errno == EINVAL)
-					/* inconsistent with lstat; retry */
-					goto stat_ref;
-				else
-					return NULL;
-			}
+		if (ret == continue_symlink) {
 			if (starts_with(sb_contents->buf, "refs/") &&
 			    !check_refname_format(sb_contents->buf, 0)) {
 				strbuf_swap(sb_refname, sb_contents);

@@ -1502,35 +1555,10 @@ static const char *resolve_ref_1(const char *refname,
 				}
 				continue;
 			}
-		}
-
-		/* Is it a directory? */
-		if (S_ISDIR(st.st_mode)) {
-			errno = EISDIR;
-			return NULL;
-		}
-
-		/*
-		 * Anything else, just open it and try to use it as
-		 * a ref
-		 */
-		fd = open(path, O_RDONLY);
-		if (fd < 0) {
-			if (errno == ENOENT)
-				/* inconsistent with lstat; retry */
-				goto stat_ref;
-			else
-				return NULL;
-		}
-		strbuf_reset(sb_contents);
-		if (strbuf_read(sb_contents, fd, 256) < 0) {
-			int save_errno = errno;
-			close(fd);
-			errno = save_errno;
-			return NULL;
-		}
-		close(fd);
-		strbuf_rtrim(sb_contents);
+		} else if (ret == refname)
+			break;
+		else if (ret != continue_normal_ref)
+			return ret;
 
 		/*
 		 * Is it a symbolic ref?

@@ -1547,12 +1575,7 @@ static const char *resolve_ref_1(const char *refname,
 				errno = EINVAL;
 				return NULL;
 			}
-			if (bad_name) {
-				hashclr(sha1);
-				if (flags)
-					*flags |= REF_ISBROKEN;
-			}
-			return refname;
+			break;
 		}
 		if (flags)
 			*flags |= REF_ISSYMREF;

@@ -1578,6 +1601,13 @@ static const char *resolve_ref_1(const char *refname,
 			bad_name = 1;
 		}
 	}
+
+	if (bad_name) {
+		hashclr(sha1);
+		if (flags)
+			*flags |= REF_ISBROKEN;
+	}
+	return refname;
 }
 
 static const char *files_resolve_ref_unsafe(const char *refname,

@@ -1424,7 +1424,8 @@ static const char *parse_ref(const char *refname,
 			     unsigned char *sha1,
 			     int *flags,
 			     struct strbuf *sb_path,
-			     struct strbuf *sb_contents)
+			     struct strbuf *sb_contents,
+			     void *cb_data)
 {
 	const char *path;
 	struct stat st;

@@ -1497,13 +1498,21 @@ stat_ref:
 }
 
 /* This function needs to return a meaningful errno on failure */
-static const char *resolve_ref_1(const char *refname,
-				 int resolve_flags,
-				 unsigned char *sha1,
-				 int *flags,
-				 struct strbuf *sb_refname,
-				 struct strbuf *sb_path,
-				 struct strbuf *sb_contents)
+const char *resolve_ref_1(const char *refname,
+			  int resolve_flags,
+			  unsigned char *sha1,
+			  int *flags,
+			  struct strbuf *sb_refname,
+			  struct strbuf *sb_path,
+			  struct strbuf *sb_contents,
+			  const char *(*parse_ref)(const char *refname,
+						   int resolve_flags,
+						   unsigned char *sha1,
+						   int *flags,
+						   struct strbuf *sb_path,
+						   struct strbuf *sb_contents,
+						   void *cb_data),
+			  void *cb_data)
 {
 	int depth = MAXDEPTH;
 	int bad_name = 0;

@@ -1540,7 +1549,8 @@ static const char *resolve_ref_1(const char *refname,
 		}
 
 		ret = parse_ref(refname, resolve_flags, sha1,
-				flags, sb_path, sb_contents);
+				flags, sb_path, sb_contents,
+				cb_data);
 
 		if (ret == continue_symlink) {
 			if (starts_with(sb_contents->buf, "refs/") &&

@@ -1621,7 +1631,8 @@ static const char *files_resolve_ref_unsafe(const char *refname,
 	const char *ret;
 
 	ret = resolve_ref_1(refname, resolve_flags, sha1, flags,
-			    &sb_refname, &sb_path, &sb_contents);
+			    &sb_refname, &sb_path, &sb_contents,
+			    parse_ref, NULL);
 	strbuf_release(&sb_path);
 	strbuf_release(&sb_contents);
 	return ret;

@@ -1407,7 +1407,7 @@ static int resolve_missing_loose_ref(const char *refname,
 	}
 }
 
-static const char *continue_normal_ref = "read_ref returns a normal ref";
+const char *continue_normal_ref = "read_ref returns a normal ref";
 static const char *continue_symlink = "read_ref returns a symlink";
 
 /*

@@ -544,74 +544,64 @@ done:
 	return ret;
 }
 
+extern const char *continue_normal_ref;
+
+static const char *retrieve_ref(const char *refname,
+				int resolve_flags,
+				unsigned char *sha1,
+				int *flags,
+				struct strbuf *sb_path,
+				struct strbuf *sb_contents,
+				void *cb_data)
+{
+	struct lmdb_transaction *transaction = cb_data;
+	MDB_val key, val;
+	int needs_free;		/* dont care, leak */
+
+	key.mv_data = (void *)refname;
+	key.mv_size = strlen(refname) + 1;
+
+	val.mv_data = NULL;
+	val.mv_size = 0;
+
+	if (mdb_get_or_die(transaction, &key, &val, &needs_free)) {
+		struct strbuf err = STRBUF_INIT;
+
+		if (resolve_flags & RESOLVE_REF_READING)
+			return NULL;
+
+		if (verify_refname_available_txn(transaction,
+						 refname, NULL, NULL, &err)) {
+			error("%s", err.buf);
+			strbuf_release(&err);
+			return NULL;
+		}
+
+		hashclr(sha1);
+		return refname;
+	}
+
+	strbuf_reset(sb_contents);
+	strbuf_add(sb_contents, val.mv_data, val.mv_size);
+	return continue_normal_ref;
+}
+
 static const char *resolve_ref_unsafe_txn(struct lmdb_transaction *transaction,
 					  const char *refname,
 					  int resolve_flags,
 					  unsigned char *sha1,
 					  int *flags)
 {
-	int bad_name = 0;
-	char *ref_data;
-	struct MDB_val key, val;
-	struct strbuf err = STRBUF_INIT;
-	int needs_free = 0;
+	static struct strbuf sb_refname = STRBUF_INIT;
+	struct strbuf sb_contents = STRBUF_INIT;
+	struct strbuf sb_path = STRBUF_INIT;
 	const char *ret;
 
-	val.mv_size = 0;
-	val.mv_data = NULL;
-
-	if (flags)
-		*flags = 0;
-
-	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-		if (flags)
-			*flags |= REF_BAD_NAME;
-
-		if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
-		    !refname_is_safe(refname)) {
-			errno = EINVAL;
-			return NULL;
-		}
-		/*
-		 * dwim_ref() uses REF_ISBROKEN to distinguish between
-		 * missing refs and refs that were present but invalid,
-		 * to complain about the latter to stderr.
-		 *
-		 * We don't know whether the ref exists, so don't set
-		 * REF_ISBROKEN yet.
-		 */
-		bad_name = 1;
-	}
-
-	key.mv_data = (void *)refname;
-	key.mv_size = strlen(refname) + 1;
-	if (mdb_get_or_die(transaction, &key, &val, &needs_free)) {
-		if (bad_name) {
-			hashclr(sha1);
-			if (flags)
-				*flags |= REF_ISBROKEN;
-		}
-
-		if (resolve_flags & RESOLVE_REF_READING)
-			return NULL;
-
-		if (verify_refname_available_txn(transaction, refname, NULL, NULL, &err)) {
-			error("%s", err.buf);
-			strbuf_release(&err);
-			return NULL;
-		}
-
-		hashclr(sha1);
-		return refname;
-	}
-
-	ref_data = val.mv_data;
-	assert(ref_data[val.mv_size - 1] == 0);
-
-	ret = parse_ref_data(transaction, refname, ref_data, sha1,
-			     resolve_flags, flags, bad_name);
-	if (needs_free)
-		free(ref_data);
+	ret = resolve_ref_1(refname, resolve_flags, sha1, flags,
+			    &sb_refname, &sb_path, &sb_contents,
+			    retrieve_ref, transaction);
+	strbuf_release(&sb_path);
+	strbuf_release(&sb_contents);
 	return ret;
 }