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 <pclo...@gmail.com>
---
 cache.h |   1 +
 refs.c  | 122 +++++++++++++++++++++++++++++++++++-----------------------------
 2 files changed, 68 insertions(+), 55 deletions(-)

diff --git a/cache.h b/cache.h
index fcb511d..5ffbafb 100644
--- a/cache.h
+++ b/cache.h
@@ -1002,6 +1002,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 84b9070..bec2bb1 100644
--- a/refs.c
+++ b/refs.c
@@ -1506,10 +1506,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;
 
@@ -1522,45 +1522,53 @@ 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;
        }
 }
 
-/* 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];
+               const char *ref = result->buf;
                struct stat st;
-               char *buf;
-               int fd;
+               const char *buf;
 
                if (--depth < 0) {
                        errno = ELOOP;
-                       return NULL;
+                       break;
                }
 
-               git_snpath(path, sizeof(path), "%s", refname);
+               git_snpath(path, sizeof(path), "%s", ref);
 
                /*
                 * We might have to loop back here to avoid a race
@@ -1574,27 +1582,25 @@ 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
-                               return NULL;
+                               ret = handle_missing_loose_ref(ref, 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;
@@ -1604,69 +1610,75 @@ 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);
 
                /*
                 * Is it a symbolic ref?
                 */
-               if (!starts_with(buffer, "ref:")) {
+               if (!skip_prefix(buffer.buf, "ref:", &buf)) {
                        /*
                         * 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;
                }
                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;
+                       break;
                }
-               refname = strcpy(refname_buffer, buf);
+               strbuf_reset(result);
+               strbuf_addstr(result, buf);
        }
+       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))
+               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 buf.buf;
+       else {
+               strbuf_release(&buf);
+               return NULL;
+       }
 }
 
 /* The argument to filter_refs */
-- 
1.9.1.346.ga2b5940

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