git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 15/24] read_raw_ref(): manage own scratch space

From
David Turner <dturner@twopensource.com>
Date
Apr 7, 2016, 19:03 UTC
Message-ID
<1460055791-23313-16-git-send-email-dturner@twopensource.com>
In-Reply-To
<1460055791-23313-1-git-send-email-dturner@twopensource.com>
From: Michael Haggerty <mhagger@alum.mit.edu>

Instead of creating scratch space in resolve_ref_unsafe() and passing it down through resolve_ref_1 to read_raw_ref(), teach read_raw_ref() to manage its own scratch space. This reduces coupling across the functions at the cost of some extra allocations. Also, when read_raw_ref() is implemented for different reference backends, the other implementations might have different scratch space requirements.

Note that we now preserve errno across the calls to strbuf_release(), which calls free() and can thus theoretically overwrite errno.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 76 ++++++++++++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 35 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index d51e778..f752568 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1421,17 +1421,20 @@ static int resolve_missing_loose_ref(const char *refname,
  *   refname will still be valid and unchanged.
  */
 static int read_raw_ref(const char *refname, unsigned char *sha1,
-			struct strbuf *symref, struct strbuf *sb_path,
-			struct strbuf *sb_contents, int *flags)
+			struct strbuf *symref, int *flags)
 {
+	struct strbuf sb_contents = STRBUF_INIT;
+	struct strbuf sb_path = STRBUF_INIT;
 	const char *path;
 	const char *buf;
 	struct stat st;
 	int fd;
+	int ret = -1;
+	int save_errno;
 
-	strbuf_reset(sb_path);
-	strbuf_git_path(sb_path, "%s", refname);
-	path = sb_path->buf;
+	strbuf_reset(&sb_path);
+	strbuf_git_path(&sb_path, "%s", refname);
+	path = sb_path.buf;
 
 stat_ref:
 	/*
@@ -1446,36 +1449,38 @@ stat_ref:
 
 	if (lstat(path, &st) < 0) {
 		if (errno != ENOENT)
-			return -1;
+			goto out;
 		if (resolve_missing_loose_ref(refname, sha1, flags)) {
 			errno = ENOENT;
-			return -1;
+			goto out;
 		}
-		return 0;
+		ret = 0;
+		goto out;
 	}
 
 	/* 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;
 			else
-				return -1;
+				goto out;
 		}
-		if (starts_with(sb_contents->buf, "refs/") &&
-		    !check_refname_format(sb_contents->buf, 0)) {
-			strbuf_swap(sb_contents, symref);
+		if (starts_with(sb_contents.buf, "refs/") &&
+		    !check_refname_format(sb_contents.buf, 0)) {
+			strbuf_swap(&sb_contents, symref);
 			*flags |= REF_ISSYMREF;
-			return 0;
+			ret = 0;
+			goto out;
 		}
 	}
 
 	/* Is it a directory? */
 	if (S_ISDIR(st.st_mode)) {
 		errno = EISDIR;
-		return -1;
+		goto out;
 	}
 
 	/*
@@ -1488,18 +1493,18 @@ stat_ref:
 			/* inconsistent with lstat; retry */
 			goto stat_ref;
 		else
-			return -1;
+			goto out;
 	}
-	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 -1;
+		goto out;
 	}
 	close(fd);
-	strbuf_rtrim(sb_contents);
-	buf = sb_contents->buf;
+	strbuf_rtrim(&sb_contents);
+	buf = sb_contents.buf;
 	if (starts_with(buf, "ref:")) {
 		buf += 4;
 		while (isspace(*buf))
@@ -1508,7 +1513,8 @@ stat_ref:
 		strbuf_reset(symref);
 		strbuf_addstr(symref, buf);
 		*flags |= REF_ISSYMREF;
-		return 0;
+		ret = 0;
+		goto out;
 	}
 
 	/*
@@ -1519,10 +1525,17 @@ stat_ref:
 	    (buf[40] != '\0' && !isspace(buf[40]))) {
 		*flags |= REF_ISBROKEN;
 		errno = EINVAL;
-		return -1;
+		goto out;
 	}
 
-	return 0;
+	ret = 0;
+
+out:
+	save_errno = errno;
+	strbuf_release(&sb_path);
+	strbuf_release(&sb_contents);
+	errno = save_errno;
+	return ret;
 }
 
 /* This function needs to return a meaningful errno on failure */
@@ -1530,9 +1543,7 @@ 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)
+				 struct strbuf *sb_refname)
 {
 	int symref_count;
 
@@ -1559,8 +1570,7 @@ static const char *resolve_ref_1(const char *refname,
 	for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
 		int read_flags = 0;
 
-		if (read_raw_ref(refname, sha1, sb_refname,
-				 sb_path, sb_contents, &read_flags)) {
+		if (read_raw_ref(refname, sha1, sb_refname, &read_flags)) {
 			*flags |= read_flags;
 			if (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))
 				return NULL;
@@ -1604,8 +1614,6 @@ 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;
 	int unused_flags;
 	const char *ret;
 
@@ -1613,9 +1621,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
 		flags = &unused_flags;
 
 	ret = resolve_ref_1(refname, resolve_flags, sha1, flags,
-			    &sb_refname, &sb_path, &sb_contents);
-	strbuf_release(&sb_path);
-	strbuf_release(&sb_contents);
+			    &sb_refname);
 	return ret;
 }
 

-- 
2.4.2.767.g62658d5-twtrsrc
Previous: David TurnerNext: David Turner
Message 20 of 32 in “Yet another pre-refs-backend series”
  1. 00/24 Yet another pre-refs-backend seriesDavid Turner, Apr 7, 2016
  2. 01/24 refs: move head_ref{,_submodule} to the common codeDavid Turner, Apr 7, 2016
  3. 02/24 refs: move for_each_*ref* functions into common codeDavid Turner, Apr 7, 2016
  4. 03/24 t1430: test the output and error of some commands more carefullyDavid Turner, Apr 7, 2016
  5. Michael HaggertyApr 8, 2016
  6. David TurnerApr 8, 2016
  7. Junio C HamanoApr 8, 2016
  8. David TurnerApr 8, 2016
  9. 04/24 t1430: clean up broken refs/tags/shadowDavid Turner, Apr 7, 2016
  10. 05/24 t1430: don't rely on symbolic-ref for creating broken symrefsDavid Turner, Apr 7, 2016
  11. 06/24 t1430: test for-each-ref in the presence of badly-named refsDavid Turner, Apr 7, 2016
  12. 07/24 t1430: improve test coverage of deletion of badly-named refsDavid Turner, Apr 7, 2016
  13. 08/24 resolve_missing_loose_ref(): simplify semanticsDavid Turner, Apr 7, 2016
  14. 09/24 resolve_ref_unsafe(): use for loop to count up to MAXDEPTHDavid Turner, Apr 7, 2016
  15. 10/24 resolve_ref_unsafe(): ensure flags is always setDavid Turner, Apr 7, 2016
  16. 11/24 resolve_ref_1(): eliminate local variableDavid Turner, Apr 7, 2016
  17. 12/24 resolve_ref_1(): reorder codeDavid Turner, Apr 7, 2016
  18. 13/24 resolve_ref_1(): eliminate local variable "bad_name"David Turner, Apr 7, 2016
  19. 14/24 files-backend: break out ref readingDavid Turner, Apr 7, 2016
  20. 15/24 read_raw_ref(): manage own scratch spaceDavid Turner, Apr 7, 2016
  21. 16/24 Inline resolve_ref_1() into resolve_ref_unsafe()David Turner, Apr 7, 2016
  22. 17/24 read_raw_ref(): change flags parameter to unsigned intDavid Turner, Apr 7, 2016
  23. 18/24 fsck_head_link(): remove unneeded flag variableDavid Turner, Apr 7, 2016
  24. 19/24 cmd_merge(): remove unneeded flag variableDavid Turner, Apr 7, 2016
  25. 20/24 checkout_paths(): remove unneeded flag variableDavid Turner, Apr 7, 2016
  26. 21/24 check_aliased_update(): check that dst_name is non-NULLDavid Turner, Apr 7, 2016
  27. 22/24 show_head_ref(): check the result of resolve_ref_namespace()David Turner, Apr 7, 2016
  28. 23/24 refs: move resolve_ref_unsafe into common codeDavid Turner, Apr 7, 2016
  29. 24/24 refs: on symref reflog expire, lock symref not referrentDavid Turner, Apr 7, 2016
  30. Junio C HamanoApr 7, 2016
  31. Michael HaggertyApr 9, 2016
  32. Junio C HamanoApr 10, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.