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

[PATCH v6 7/7] RFC: Change error handling scheme in read_gitfile_gently

From
Erik Elfström <erik.elfstrom@gmail.com>
Date
May 10, 2015, 20:00 UTC
Message-ID
<1431288041-21077-8-git-send-email-erik.elfstrom@gmail.com>
In-Reply-To
<1431288041-21077-1-git-send-email-erik.elfstrom@gmail.com>
Signed-off-by: Erik Elfström <erik.elfstrom@gmail.com>
---

Since there was a lot of discussion on error reporting strategy on the previous patch I have done a quick prototype of the theme proposed by Jonathan Nieder.

I believe the conclusion was to NOT go this route but this way people get to see an example of what it could look like to make the discussion and decision a bit easier.

I will either drop this patch or split it up and squash it into the appropriate commits (along with change requests if any) depending on the outcome of the review discussion.

 builtin/clean.c |   3 +-
 cache.h         |   5 +--
 setup.c         | 106 +++++++++++++++++++++++++++++++-------------------------
 3 files changed, 63 insertions(+), 51 deletions(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index d739dcf..7047d6e 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -162,7 +162,8 @@ static int is_git_repository(struct strbuf *path)
 	if (path->buf[orig_path_len - 1] != '/')
 		strbuf_addch(path, '/');
 	strbuf_addstr(path, ".git");
-	if (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))
+	if (read_gitfile_gently(path->buf, &gitfile_error, NULL) ||
+	    is_git_directory(path->buf))
 		ret = 1;
 	if (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||
 	    gitfile_error == READ_GITFILE_ERR_READ_FAILED ||
diff --git a/cache.h b/cache.h
index 7c8abcb..76d311a 100644
--- a/cache.h
+++ b/cache.h
@@ -453,8 +453,9 @@ extern const char *get_git_work_tree(void);
 #define READ_GITFILE_ERR_CANT_VERIFY_PATH 7
 #define READ_GITFILE_ERR_NOT_A_REPO 8
 #define READ_GITFILE_ERR_TOO_LARGE 9
-extern const char *read_gitfile_gently(const char *path, int *return_error_code);
-#define read_gitfile(path) read_gitfile_gently((path), NULL)
+extern const char *read_gitfile_gently(const char *path, int *return_err, struct strbuf *err_msg);
+extern const char *read_gitfile(const char *path);
+
 extern const char *resolve_gitdir(const char *suspect);
 extern void set_git_work_tree(const char *tree);
 
diff --git a/setup.c b/setup.c
index bfaf4a6..49274b3 100644
--- a/setup.c
+++ b/setup.c
@@ -374,15 +374,16 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
  * Try to read the location of the git directory from the .git file,
  * return path to git directory if found.
  *
- * On failure, if return_error_code is not NULL, return_error_code
- * will be set to an error code and NULL will be returned. If
- * return_error_code is NULL the function will die instead (for most
- * cases).
+ * In the event of an error, return_err will be set to an error code
+ * and err_msg will be set to an error message describing the error
+ * and NULL will be returned. If no error reporting is required, pass
+ * NULL for return_err and/or err_msg.
  */
-const char *read_gitfile_gently(const char *path, int *return_error_code)
+const char *read_gitfile_gently(const char *path, int *return_err,
+				struct strbuf *err_msg)
 {
 	static const int one_MB = 1 << 20;
-	int error_code = 0;
+	const char *ret = NULL;
 	char *buf = NULL;
 	char *dir = NULL;
 	const char *slash;
@@ -390,42 +391,59 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 	int fd;
 	ssize_t len;
 	int is_git_dir;
-	struct strbuf err_msg = STRBUF_INIT;
+	int is_git_dir_err;
+
+	if (return_err)
+		*return_err = 0;
 
 	if (stat(path, &st)) {
-		error_code = READ_GITFILE_ERR_STAT_FAILED;
+		set_error(return_err, err_msg,
+			  READ_GITFILE_ERR_STAT_FAILED,
+			  "Could not stat: '%s'", path);
 		goto cleanup_return;
 	}
 	if (!S_ISREG(st.st_mode)) {
-		error_code = READ_GITFILE_ERR_NOT_A_FILE;
+		set_error(return_err, err_msg,
+			  READ_GITFILE_ERR_NOT_A_FILE,
+			  "Not a file: '%s'", path);
 		goto cleanup_return;
 	}
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
-		error_code = READ_GITFILE_ERR_OPEN_FAILED;
+		set_error(return_err, err_msg,
+			  READ_GITFILE_ERR_OPEN_FAILED,
+			  "Error opening '%s'", path);
 		goto cleanup_return;
 	}
 	if (st.st_size > one_MB) {
 		close(fd);
-		error_code = READ_GITFILE_ERR_TOO_LARGE;
+		set_error(return_err, err_msg,
+			  READ_GITFILE_ERR_TOO_LARGE,
+			  "Too large to be a .git file: '%s'", path);
 		goto cleanup_return;
 	}
 	buf = xmalloc(st.st_size + 1);
 	len = read_in_full(fd, buf, st.st_size);
 	close(fd);
 	if (len != st.st_size) {
-		error_code = READ_GITFILE_ERR_READ_FAILED;
+		set_error(return_err, err_msg,
+			  READ_GITFILE_ERR_READ_FAILED,
+			  "Error reading %s", path);
 		goto cleanup_return;
 	}
 	buf[len] = '\0';
 	if (!starts_with(buf, "gitdir: ")) {
-		error_code = READ_GITFILE_ERR_INVALID_FORMAT;
+		set_error(return_err, err_msg,
+			  READ_GITFILE_ERR_INVALID_FORMAT,
+			  "Invalid gitfile format: %s", path);
 		goto cleanup_return;
 	}
 	while (buf[len - 1] == '\n' || buf[len - 1] == '\r')
 		len--;
 	if (len < 9) {
-		error_code = READ_GITFILE_ERR_NO_PATH;
+		set_error(return_err, err_msg,
+			  READ_GITFILE_ERR_NO_PATH,
+			  "No path in gitfile: %s", path);
 		goto cleanup_return;
 	}
 	buf[len] = '\0';
@@ -442,52 +460,44 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 		buf = dir;
 	}
 
-	is_git_dir = is_git_directory_gently(dir, &error_code, &err_msg);
-	if (error_code) {
-		error_code = READ_GITFILE_ERR_CANT_VERIFY_PATH;
+	is_git_dir = is_git_directory_gently(dir, &is_git_dir_err, err_msg);
+	if (is_git_dir_err) {
+		if (return_err)
+			*return_err = READ_GITFILE_ERR_CANT_VERIFY_PATH;
 		goto cleanup_return;
 	}
 	if (!is_git_dir) {
-		error_code = READ_GITFILE_ERR_NOT_A_REPO;
+		set_error(return_err, err_msg,
+			  READ_GITFILE_ERR_NOT_A_REPO,
+			  "Not a git repository: %s", dir);
 		goto cleanup_return;
 	}
-	path = real_path(dir);
+	ret = real_path(dir);
 
 cleanup_return:
 	free(buf);
+	return ret;
+}
 
-	if (return_error_code)
-		*return_error_code = error_code;
+const char *read_gitfile(const char *path)
+{
+	int err;
+	const char *ret;
+	struct strbuf err_msg = STRBUF_INIT;
 
-	if (error_code) {
-		if (return_error_code)
-			return NULL;
+	ret = read_gitfile_gently(path, &err, &err_msg);
 
-		switch (error_code) {
-		case READ_GITFILE_ERR_STAT_FAILED:
-		case READ_GITFILE_ERR_NOT_A_FILE:
-			return NULL;
-		case READ_GITFILE_ERR_OPEN_FAILED:
-			die_errno("Error opening '%s'", path);
-		case READ_GITFILE_ERR_TOO_LARGE:
-			die("Too large to be a .git file: '%s'", path);
-		case READ_GITFILE_ERR_READ_FAILED:
-			die("Error reading %s", path);
-		case READ_GITFILE_ERR_INVALID_FORMAT:
-			die("Invalid gitfile format: %s", path);
-		case READ_GITFILE_ERR_NO_PATH:
-			die("No path in gitfile: %s", path);
-		case READ_GITFILE_ERR_CANT_VERIFY_PATH:
-			die("%s", err_msg.buf);
-		case READ_GITFILE_ERR_NOT_A_REPO:
-			die("Not a git repository: %s", dir);
-		default:
-			assert(0);
-		}
+	switch (err) {
+	case 0: /* No need to free err_msg, will only be
+		 * touched in case of error */
+		return ret;
+	case READ_GITFILE_ERR_STAT_FAILED:
+	case READ_GITFILE_ERR_NOT_A_FILE:
+		strbuf_release(&err_msg);
+		return NULL;
+	default:
+		die("%s", err_msg.buf);
 	}
-
-	strbuf_release(&err_msg);
-	return path;
 }
 
 static const char *setup_explicit_git_dir(const char *gitdirenv,

-- 
2.4.0.60.gf7143f7
Previous: Erik Elfström
Message 9 of 9 in “Improving performance of git clean”
  1. 0/7 Improving performance of git cleanErik Elfström, May 10, 2015
  2. 1/7 setup: add gentle version of is_git_directoryErik Elfström, May 10, 2015
  3. 2/7 setup: add gentle version of read_gitfileErik Elfström, May 10, 2015
  4. 3/7 setup: sanity check file size in read_gitfile_gentlyErik Elfström, May 10, 2015
  5. erik elfströmMay 12, 2015
  6. 4/7 t7300: add tests to document behavior of clean and nested gitErik Elfström, May 10, 2015
  7. 5/7 p7300: add performance tests for cleanErik Elfström, May 10, 2015
  8. 6/7 clean: improve performance when removing lots of directoriesErik Elfström, May 10, 2015
  9. 7/7 RFC: Change error handling scheme in read_gitfile_gentlyErik Elfström, May 10, 2015

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.