Re: [PATCH 3/4] refs.c: move ref parsing code out of resolve_ref()

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  cache.h |  11 ++++
>  refs.c  | 204 ++++++++++++++++++++++++++++++++++------------------------------
>  2 files changed, 120 insertions(+), 95 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 5ffbafb..40a63d9 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1003,6 +1003,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, "ref" can no longer be trusted, "flag" may
> + *     be set. errno is valid.
> + *  0  this is a symref, "ref" now contains the linked ref
> + * +1  normal ref, "sha1" is valid
> + */
> +extern int parse_ref(const char *path, struct strbuf *ref, 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 bec2bb1..2769f20 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1533,6 +1533,105 @@ static int handle_missing_loose_ref(const char *refname,
>         }
>  }
>
> +int parse_ref(const char *path, struct strbuf *ref,
> +             unsigned char *sha1, int *flag)

> +{
> +       struct strbuf buffer = STRBUF_INIT;
> +       struct stat st;
> +       const char *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)
> +               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(ref);
> +                       strbuf_addbuf(ref, &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);
> +
> +       /*
> +        * Is it a symbolic ref?
> +        */
> +       if (!skip_prefix(buffer.buf, "ref:", &buf)) {
> +               int ret;
> +               /*
> +                * 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;
> +       }
> +       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(ref);
> +       strbuf_addstr(ref, buf);
> +       strbuf_release(&buffer);
> +       return 0;
> +}
> +
>  /*
>   * 'result' content will be destroyed. Its value may be undefined if
>   * resolve_ref returns -1.
> @@ -1542,9 +1641,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;
> @@ -1557,108 +1655,24 @@ int resolve_ref(const char *refname, struct strbuf *result,
>         strbuf_reset(result);
>         strbuf_addstr(result, refname);
>
> -       for (;;) {
> +       while (!ret) {
>                 char path[PATH_MAX];
> -               const char *ref = result->buf;
> -               struct stat st;
> -               const char *buf;
>
>                 if (--depth < 0) {
>                         errno = ELOOP;
> +                       ret = -1;
>                         break;
>                 }
>
> -               git_snpath(path, sizeof(path), "%s", ref);
> -
> -               /*
> -                * 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(ref, sha1,
> -                                                              reading, flag);
> -                       break;
> +               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;
>                 }
> -
> -               /* 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);
> -
> -               /*
> -                * Is it a symbolic 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.buf, sha1) ||
> -                           (buffer.buf[40] != '\0' && !isspace(buffer.buf[40]))) {
> -                               if (flag)
> -                                       *flag |= REF_ISBROKEN;
> -                               errno = EINVAL;
> -                       } else
> -                               ret = 0;
> -                       break;
> -               }
> -               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_addstr(result, buf);
>         }
> -       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)
> --
> 1.9.1.346.ga2b5940
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html