[PATCH 0/4] Consolidate ref parsing code

[PATCH 0/4] Consolidate ref parsing code

[Nguyễn Thái Ngọc Duy]

On Thu, Jul 24, 2014 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>  - .... three places now knows what a
>    textual symref looks like (i.e. begins with "ref:", zero or more
>    whitespaces, the target ref and then zero or more trailing
>    whitespaces).  Perhaps we need to consolidate the code further,
>    so that this knowledge does not leak out of refs.c?

I started on top of nd/multiple-work-trees but it conflicts badly with
rs/ref-transaction-0 because this is basically code move. So I think
we should make it a separate topic instead, based on latest 'master'.
Junio, you still hit conflicts when merging this with nd/multiple-work-trees,
but that's simpler to resolve (git_snpath -> strbuf_git_path). I
promise to replace the "ref:" code in checkout.c later when both
topics graduate.

So.. first cut. The end result looks nice.

Nguyễn Thái Ngọc Duy (4):
  strbuf.c: keep errno in strbuf_read_file()
  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   | 332 ++++++++++++++++++++++++++++++++-------------------------------
 strbuf.c |   7 +-
 3 files changed, 188 insertions(+), 163 deletions(-)

-- 
1.9.1.346.ga2b5940

[PATCH 1/4] strbuf.c: keep errno in strbuf_read_file()

[Nguyễn Thái Ngọc Duy]

This function is used to replaced some code in the next patch that
does this (i.e. keep the errno when read() fails)

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

diff --git a/strbuf.c b/strbuf.c
index 33018d8..61d685d 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -454,15 +454,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;
 }
-- 
1.9.1.346.ga2b5940

Re: [PATCH 1/4] strbuf.c: keep errno in strbuf_read_file()

[Eric Sunshine]

On Fri, Jul 25, 2014 at 6:43 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> This function is used to replaced some code in the next patch that

s/replaced/replace/

> does this (i.e. keep the errno when read() fails)
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  strbuf.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index 33018d8..61d685d 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -454,15 +454,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;
>  }
> --
> 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

Re: [PATCH 1/4] strbuf.c: keep errno in strbuf_read_file()

[Michael Haggerty]

On 07/25/2014 12:43 PM, Nguyễn Thái Ngọc Duy wrote:
> This function is used to replaced some code in the next patch that
> does this (i.e. keep the errno when read() fails)
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  strbuf.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/strbuf.c b/strbuf.c
> index 33018d8..61d685d 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -454,15 +454,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);

Theoretically close() can fail, though it seems a little far-fetched
(and also uninteresting) if it fails for a file opened read-only. But if
it did, you would not notice the error.

So I grepped through our code to see whether we typically bother to
check the return value when close()ing a read-only file. And I found
that we rarely even check its return value when *writing* to a file.
(Many of those places are probably bugs.)

So, carry on and forget I said anything :-)

> -	if (len < 0)
> +	if (len < 0) {
> +		errno = saved_errno;
>  		return -1;
> +	}
>  
>  	return len;
>  }
> 

Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

[PATCH 2/4] refs.c: refactor resolve_ref_unsafe() to use strbuf internally

[Nguyễn Thái Ngọc Duy]

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

Re: [PATCH 2/4] refs.c: refactor resolve_ref_unsafe() to use strbuf internally

[Eric Sunshine]

On Fri, Jul 25, 2014 at 6:43 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> 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>
> ---
>  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
> +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;

return strbuf_detach(&buf, NULL);

> +       else {
> +               strbuf_release(&buf);
> +               return NULL;
> +       }
>  }
>
>  /* The argument to filter_refs */
> --
> 1.9.1.346.ga2b5940

Re: [PATCH 2/4] refs.c: refactor resolve_ref_unsafe() to use strbuf internally

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>>  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;
>
> return strbuf_detach(&buf, NULL);

Yeah, the end result is the same, but it is a very good discipline.

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

[Nguyễn Thái Ngọc Duy]

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

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

[Ronnie Sahlberg]

Nice.

On Fri, Jul 25, 2014 at 3:43 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> 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)

Can you make this function static?
It is not used by anything outside of this series and thus making it
static avoids growing the public refs api.


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

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

[Duy Nguyen]

On Fri, Jul 25, 2014 at 11:12 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
>> 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)
>
> Can you make this function static?
> It is not used by anything outside of this series and thus making it
> static avoids growing the public refs api.

It's to be used by builtin/checkout.c in nd/multiple-work-trees. I
could mark it static now and unmark it later, but I'd need to add the
static prototype back in refs.c because in the next patch
resolve_gitlink_ref() uses this function and resolve_gitlink_ref() is
before parse_ref().
-- 
Duy

[PATCH 4/4] refs.c: rewrite resolve_gitlink_ref() to use parse_ref()

[Nguyễn Thái Ngọc Duy]

resolve_gitlink_ref_recursive() is less strict than the old
resolve_ref_unsafe() (which is now parse_ref()). It also has another
random limit 128 bytes for symrefs.

This brings MAXREFLEN check to resolve_ref* family. It looks like an
arbitrary limit though (added in 0ebde32 (Add 'resolve_gitlink_ref()'
helper function - 2007-04-09)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 68 +++++++++++++++++++++++++-----------------------------------------
 1 file changed, 26 insertions(+), 42 deletions(-)

diff --git a/refs.c b/refs.c
index 2769f20..24503e5 100644
--- a/refs.c
+++ b/refs.c
@@ -1436,48 +1436,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 || strlen(refname) > MAXREFLEN)
-		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), retval = 0;
+	int depth = MAXDEPTH;
 	char *submodule;
 	struct ref_cache *refs;
 
@@ -1489,8 +1452,24 @@ 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);
+	while (!retval) {
+		if (--depth < 0) {
+			errno = ELOOP;
+			retval = -1;
+			break;
+		}
+		path = *refs->name
+			? git_path_submodule(refs->name, "%s", result.buf)
+			: git_path("%s", result.buf);
+		retval = parse_ref(path, &result, sha1, NULL);
+		if (retval == -2) {
+			retval = resolve_gitlink_packed_ref(refs, result.buf, sha1);
+			retval = retval ? -1 : 1;
+		}
+	}
+	strbuf_release(&result);
+	return retval > 0 ? 0 : -1;
 }
 
 /*
@@ -1540,6 +1519,11 @@ int parse_ref(const char *path, struct strbuf *ref,
 	struct stat st;
 	const char *buf;
 
+	if (ref->len > MAXREFLEN) {
+		errno = ENAMETOOLONG;
+		return -1;
+	}
+
 	/*
 	 * 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
-- 
1.9.1.346.ga2b5940