Refactor resolve_ref_1 in terms of a new function read_raw_ref, which
is responsible for reading ref data from the ref storage.

Later, we will make read_raw_ref a pluggable backend function, and make
resolve_ref_unsafe common.

Testing done: Hacked in code to run both old and new version of
resolve_ref_1 and compare all outputs, failing dramatically if outputs
differed.  Ran test suite.

Signed-off-by: David Turner <dtur...@twopensource.com>
Helped-by: Duy Nguyen <pclo...@gmail.com>
---
 refs/files-backend.c | 265 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 159 insertions(+), 106 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index fd664d6..ef5f28d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1377,10 +1377,9 @@ 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().
+ * options are forwarded from resolve_ref_unsafe().
  */
 static int resolve_missing_loose_ref(const char *refname,
-                                    int resolve_flags,
                                     unsigned char *sha1,
                                     int *flags)
 {
@@ -1398,64 +1397,48 @@ static int resolve_missing_loose_ref(const char 
*refname,
                return 0;
        }
        /* The reference is not a packed reference, either. */
-       if (resolve_flags & RESOLVE_REF_READING) {
-               errno = ENOENT;
-               return -1;
-       } else {
-               hashclr(sha1);
-               return 0;
-       }
+       errno = ENOENT;
+       return -1;
 }
 
-/* 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)
+/*
+ * Read a raw ref from the filesystem or packed refs file.
+ *
+ * If the ref is a sha1, fill in sha1 and return 0.
+ *
+ * If the ref is symbolic, fill in *symref with the referrent
+ * (e.g. "refs/heads/master") and return 0.  The caller is responsible
+ * for validating the referrent.  Set REF_ISSYMREF in flags.
+ *
+ * If the ref is neither a symbolic ref nor a sha1, it is broken.  Set
+ * REF_ISBROKEN in flags, set errno to EINVAL, and return -1.
+ *
+ * If the ref doesn't exist, set errno to ENOENT and return -1.
+ *
+ * If there is another error reading the ref, set errno appropriately and
+ * return -1.
+ *
+ * Backend-specific flags might be set in flags as well, regardless of
+ * outcome.
+ *
+ * sb_path is workspace: the caller should allocate and free it.
+ */
+static int read_raw_ref(const char *refname, unsigned char *sha1,
+                       struct strbuf *symref, struct strbuf *sb_path,
+                       unsigned int *flags)
 {
-       int depth = MAXDEPTH;
-       int bad_name = 0;
-
-       if (flags)
-               *flags = 0;
+       struct strbuf sb_contents = STRBUF_INIT;
+       int ret = -1;
+       const char *path;
+       const char *buf;
 
-       if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-               if (flags)
-                       *flags |= REF_BAD_NAME;
+       strbuf_reset(sb_path);
+       strbuf_git_path(sb_path, "%s", refname);
+       path = sb_path->buf;
 
-               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;
-       }
-       for (;;) {
-               const char *path;
+       for(;;) {
                struct stat st;
-               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;
-
                /*
                 * We might have to loop back here to avoid a race
                 * condition: first we lstat() the file, then we try
@@ -1465,49 +1448,45 @@ static const char *resolve_ref_1(const char *refname,
                 * 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;
+                               break;
+                       if (resolve_missing_loose_ref(refname, sha1, flags)) 
+                               break;
+                       ret = 0;
+                       break;
                }
 
                /* 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) {
+                       strbuf_reset(&sb_contents);
+                       if (strbuf_readlink(&sb_contents, path, 0) < 0) {
                                if (errno == ENOENT || errno == EINVAL)
                                        /* inconsistent with lstat; retry */
-                                       goto stat_ref;
+                                       continue;
                                else
-                                       return NULL;
+                                       break;
                        }
-                       if (starts_with(sb_contents->buf, "refs/") &&
-                           !check_refname_format(sb_contents->buf, 0)) {
-                               strbuf_swap(sb_refname, sb_contents);
-                               refname = sb_refname->buf;
+                       if (starts_with(sb_contents.buf, "refs/") &&
+                           !check_refname_format(sb_contents.buf, 0)) {
+                               strbuf_swap(&sb_contents, symref);
                                if (flags)
                                        *flags |= REF_ISSYMREF;
-                               if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
-                                       hashclr(sha1);
-                                       return refname;
-                               }
-                               continue;
+                               ret = 0;
+                               break;
+                       } else {
+                               /* bogus symlink ref  */
+                               if (flags)
+                                       *flags |= REF_ISBROKEN;
+                               break;
                        }
                }
 
                /* Is it a directory? */
                if (S_ISDIR(st.st_mode)) {
                        errno = EISDIR;
-                       return NULL;
+                       break;
                }
 
                /*
@@ -1518,35 +1497,110 @@ static const char *resolve_ref_1(const char *refname,
                if (fd < 0) {
                        if (errno == ENOENT)
                                /* inconsistent with lstat; retry */
-                               goto stat_ref;
+                               continue;
                        else
-                               return NULL;
+                               break;
                }
-               strbuf_reset(sb_contents);
-               if (strbuf_read(sb_contents, fd, 256) < 0) {
+               strbuf_reset(&sb_contents);
+               if (strbuf_read(&sb_contents, fd, 256) < 0) {
                        int save_errno = errno;
                        close(fd);
                        errno = save_errno;
-                       return NULL;
+                       break;
                }
                close(fd);
-               strbuf_rtrim(sb_contents);
+               strbuf_rtrim(&sb_contents);
+               buf = sb_contents.buf;
+               if (starts_with(buf, "ref:")) {
+                       buf += 4;
+                       while (isspace(*buf))
+                               buf++;
+
+                       strbuf_reset(symref);
+                       strbuf_addstr(symref, buf);
+                       if (flags)
+                               *flags |= REF_ISSYMREF;
+                       ret = 0;
+                       break;
+               }
 
                /*
-                * Is it a symbolic ref?
+                * Please note that FETCH_HEAD has additional
+                * data after the sha.
                 */
-               if (!starts_with(sb_contents->buf, "ref:")) {
-                       /*
-                        * Please note that FETCH_HEAD has a second
-                        * line containing other data.
-                        */
-                       if (get_sha1_hex(sb_contents->buf, sha1) ||
-                           (sb_contents->buf[40] != '\0' && 
!isspace(sb_contents->buf[40]))) {
+               if (get_sha1_hex(buf, sha1) ||
+                   (buf[40] != '\0' && !isspace(buf[40]))) {
+                       if (flags)
+                               *flags |= REF_ISBROKEN;
+                       errno = EINVAL;
+                       break;
+               }
+               ret = 0;
+               break;
+       }
+
+       strbuf_release(&sb_contents);
+       return ret;
+}
+
+/* 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)
+{
+       int bad_name = 0;
+       int symref_count;
+
+       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;
+       }
+
+       for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
+               int read_flags = 0;
+
+               if (read_raw_ref(refname, sha1, sb_refname, sb_path, 
&read_flags)) {
+                       int saved_errno = errno;
+                       if (flags)
+                               *flags |= read_flags;
+                       errno = saved_errno;
+                       if (bad_name) {
+                               hashclr(sha1);
                                if (flags)
                                        *flags |= REF_ISBROKEN;
-                               errno = EINVAL;
+                       }
+                       if (resolve_flags & RESOLVE_REF_READING || errno != 
ENOENT) {
                                return NULL;
+                       } else {
+                               hashclr(sha1);
+                               return refname;
                        }
+               }
+               if (flags)
+                       *flags |= read_flags;
+
+               if (!(read_flags & REF_ISSYMREF)) {
                        if (bad_name) {
                                hashclr(sha1);
                                if (flags)
@@ -1554,44 +1608,43 @@ static const char *resolve_ref_1(const char *refname,
                        }
                        return refname;
                }
-               if (flags)
-                       *flags |= REF_ISSYMREF;
-               buf = sb_contents->buf + 4;
-               while (isspace(*buf))
-                       buf++;
-               strbuf_reset(sb_refname);
-               strbuf_addstr(sb_refname, buf);
+
                refname = sb_refname->buf;
                if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
                        hashclr(sha1);
+                       if (bad_name && flags)
+                               *flags |= REF_ISBROKEN;
                        return refname;
                }
-               if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
+
+               if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
                        if (flags)
                                *flags |= REF_ISBROKEN;
-
                        if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
-                           !refname_is_safe(buf)) {
+                           !refname_is_safe(refname)) {
                                errno = EINVAL;
                                return NULL;
                        }
                        bad_name = 1;
                }
        }
+
+       if (flags)
+               *flags |= REF_ISBROKEN;
+       return NULL;
 }
 
 const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
                               unsigned char *sha1, int *flags)
 {
        static struct strbuf sb_refname = STRBUF_INIT;
-       struct strbuf sb_contents = STRBUF_INIT;
        struct strbuf sb_path = STRBUF_INIT;
        const char *ret;
 
        ret = resolve_ref_1(refname, resolve_flags, sha1, flags,
-                           &sb_refname, &sb_path, &sb_contents);
+                           &sb_refname, &sb_path);
+
        strbuf_release(&sb_path);
-       strbuf_release(&sb_contents);
        return ret;
 }
 
-- 
2.4.2.767.g62658d5-twtrsrc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to