Migrating diff to new error handling

Ended up migrating a bunch of upstream functions as well
including vector, attr_file, and odb in order to get this
to work right.

src/attr_file.c

@@ -11,24 +11,19 @@ static void git_attr_rule__clear(git_attr_rule *rule);
 
 int git_attr_file__new(git_attr_file **attrs_ptr)
 {
-	int error;
 	git_attr_file *attrs = NULL;
 
 	attrs = git__calloc(1, sizeof(git_attr_file));
-	if (attrs == NULL)
-		error = GIT_ENOMEM;
-	else
-		error = git_vector_init(&attrs->rules, 4, NULL);
+	GITERR_CHECK_ALLOC(attrs);
 
-	if (error != GIT_SUCCESS) {
-		git__rethrow(error, "Could not allocate attribute storage");
+	if (git_vector_init(&attrs->rules, 4, NULL) < 0) {
 		git__free(attrs);
 		attrs = NULL;
 	}
 
 	*attrs_ptr = attrs;
 
-	return error;
+	return attrs ? 0 : -1;
 }
 
 int git_attr_file__set_path(
@@ -50,13 +45,15 @@ int git_attr_file__set_path(
 			file->path = git__strdup(path);
 	}
 
-	return (file->path == NULL) ? GIT_ENOMEM : GIT_SUCCESS;
+	GITERR_CHECK_ALLOC(file->path);
+
+	return 0;
 }
 
 int git_attr_file__from_buffer(
 	git_repository *repo, const char *buffer, git_attr_file *attrs)
 {
-	int error = GIT_SUCCESS;
+	int error = 0;
 	const char *scan = NULL;
 	char *context = NULL;
 	git_attr_rule *rule = NULL;
@@ -68,13 +65,13 @@ int git_attr_file__from_buffer(
 	if (attrs->path && git__suffixcmp(attrs->path, GIT_ATTR_FILE) == 0) {
 		context = git__strndup(attrs->path,
 			strlen(attrs->path) - strlen(GIT_ATTR_FILE));
-		if (!context) error = GIT_ENOMEM;
+		GITERR_CHECK_ALLOC(context);
 	}
 
-	while (error == GIT_SUCCESS && *scan) {
+	while (!error && *scan) {
 		/* allocate rule if needed */
 		if (!rule && !(rule = git__calloc(1, sizeof(git_attr_rule)))) {
-			error = GIT_ENOMEM;
+			error = -1;
 			break;
 		}
 
@@ -92,10 +89,10 @@ int git_attr_file__from_buffer(
 		}
 
 		/* if the rule wasn't a pattern, on to the next */
-		if (error != GIT_SUCCESS) {
+		if (error < 0) {
 			git_attr_rule__clear(rule); /* reset rule contents */
 			if (error == GIT_ENOTFOUND)
-				error = GIT_SUCCESS;
+				error = 0;
 		} else {
 			rule = NULL; /* vector now "owns" the rule */
 		}
@@ -110,21 +107,20 @@ int git_attr_file__from_buffer(
 int git_attr_file__from_file(
 	git_repository *repo, const char *path, git_attr_file *file)
 {
-	int error = GIT_SUCCESS;
+	int error;
 	git_buf fbuf = GIT_BUF_INIT;
 
 	assert(path && file);
 
-	if (file->path == NULL)
-		error = git_attr_file__set_path(repo, path, file);
+	if (file->path == NULL && git_attr_file__set_path(repo, path, file) < 0)
+		return -1;
 
-	if (error == GIT_SUCCESS &&
-		(error = git_futils_readbuffer(&fbuf, path)) == GIT_SUCCESS)
-		error = git_attr_file__from_buffer(repo, fbuf.ptr, file);
+	if (git_futils_readbuffer(&fbuf, path) < 0)
+		return -1;
+
+	error = git_attr_file__from_buffer(repo, fbuf.ptr, file);
 
 	git_buf_free(&fbuf);
-	if (error != GIT_SUCCESS)
-		git__rethrow(error, "Could not open attribute file '%s'", path);
 
 	return error;
 }
@@ -176,7 +172,6 @@ int git_attr_file__lookup_one(
 
 	git_attr_file__foreach_matching_rule(file, path, i, rule) {
 		int pos = git_vector_bsearch(&rule->assigns, &name);
-		git_clearerror(); /* okay if search failed */
 
 		if (pos >= 0) {
 			*value = ((git_attr_assignment *)
@@ -230,7 +225,6 @@ git_attr_assignment *git_attr_rule__lookup_assignment(
 	key.name_hash = git_attr_file__name_hash(name);
 
 	pos = git_vector_bsearch(&rule->assigns, &key);
-	git_clearerror(); /* okay if search failed */
 
 	return (pos >= 0) ? git_vector_get(&rule->assigns, pos) : NULL;
 }
@@ -248,16 +242,15 @@ int git_attr_path__init(
 
 	if (base != NULL && git_path_root(path) < 0) {
 		git_buf full_path = GIT_BUF_INIT;
-		int error = git_buf_joinpath(&full_path, base, path);
-		if (error == GIT_SUCCESS)
-			info->is_dir = (int)git_path_isdir(full_path.ptr);
-
+		if (git_buf_joinpath(&full_path, base, path) < 0)
+			return -1;
+		info->is_dir = (int)git_path_isdir(full_path.ptr);
 		git_buf_free(&full_path);
-		return error;
+		return 0;
 	}
 	info->is_dir = (int)git_path_isdir(path);
 
-	return GIT_SUCCESS;
+	return 0;
 }
 
 
@@ -374,7 +367,7 @@ int git_attr_fnmatch__parse(
 
 	if (!spec->pattern) {
 		*base = git__next_line(pattern);
-		return GIT_ENOMEM;
+		return -1;
 	} else {
 		/* strip '\' that might have be used for internal whitespace */
 		char *to = spec->pattern;
@@ -390,7 +383,7 @@ int git_attr_fnmatch__parse(
 		}
 	}
 
-	return GIT_SUCCESS;
+	return 0;
 }
 
 static int sort_by_hash_and_name(const void *a_raw, const void *b_raw)
@@ -434,7 +427,7 @@ int git_attr_assignment__parse(
 	git_vector *assigns,
 	const char **base)
 {
-	int error = GIT_SUCCESS;
+	int error;
 	const char *scan = *base;
 	git_attr_assignment *assign = NULL;
 
@@ -442,7 +435,7 @@ int git_attr_assignment__parse(
 
 	assigns->_cmp = sort_by_hash_and_name;
 
-	while (*scan && *scan != '\n' && error == GIT_SUCCESS) {
+	while (*scan && *scan != '\n') {
 		const char *name_start, *value_start;
 
 		/* skip leading blanks */
@@ -451,10 +444,7 @@ int git_attr_assignment__parse(
 		/* allocate assign if needed */
 		if (!assign) {
 			assign = git__calloc(1, sizeof(git_attr_assignment));
-			if (!assign) {
-				error = GIT_ENOMEM;
-				break;
-			}
+			GITERR_CHECK_ALLOC(assign);
 			GIT_REFCOUNT_INC(assign);
 		}
 
@@ -489,10 +479,7 @@ int git_attr_assignment__parse(
 
 		/* allocate permanent storage for name */
 		assign->name = git__strndup(name_start, scan - name_start);
-		if (!assign->name) {
-			error = GIT_ENOMEM;
-			break;
-		}
+		GITERR_CHECK_ALLOC(assign->name);
 
 		/* if there is an equals sign, find the value */
 		if (*scan == '=') {
@@ -501,12 +488,8 @@ int git_attr_assignment__parse(
 			/* if we found a value, allocate permanent storage for it */
 			if (scan > value_start) {
 				assign->value = git__strndup(value_start, scan - value_start);
-				if (!assign->value) {
-					error = GIT_ENOMEM;
-					break;
-				} else {
-					assign->is_allocated = 1;
-				}
+				GITERR_CHECK_ALLOC(assign->value);
+				assign->is_allocated = 1;
 			}
 		}
 
@@ -524,35 +507,27 @@ int git_attr_assignment__parse(
 
 					error = git_vector_insert_sorted(
 						assigns, massign, &merge_assignments);
-
-					if (error == GIT_EEXISTS)
-						error = GIT_SUCCESS;
-					else if (error != GIT_SUCCESS)
-						break;
+					if (error < 0 && error != GIT_EEXISTS)
+						return error;
 				}
 			}
 		}
 
 		/* insert allocated assign into vector */
 		error = git_vector_insert_sorted(assigns, assign, &merge_assignments);
-		if (error == GIT_EEXISTS)
-			error = GIT_SUCCESS;
-		else if (error < GIT_SUCCESS)
-			break;
+		if (error < 0 && error != GIT_EEXISTS)
+			return error;
 
 		/* clear assign since it is now "owned" by the vector */
 		assign = NULL;
 	}
 
-	if (!assigns->length)
-		error = git__throw(GIT_ENOTFOUND, "No attribute assignments found for rule");
-
 	if (assign != NULL)
 		git_attr_assignment__free(assign);
 
 	*base = git__next_line(scan);
 
-	return error;
+	return (assigns->length == 0) ? GIT_ENOTFOUND : 0;
 }
 
 static void git_attr_rule__clear(git_attr_rule *rule)

src/errors.c

@@ -123,6 +123,8 @@ void giterr_set(int error_class, const char *string, ...)
 	char error_str[1024];
 	va_list arglist;
 	git_error *error;
+	const char *oserr =
+		(error_class == GITERR_OS && errno > 0) ? strerror(errno) : NULL;
 
 	error = &GIT_GLOBAL->error_t;
 	free(error->message);
@@ -131,6 +133,13 @@ void giterr_set(int error_class, const char *string, ...)
 	p_vsnprintf(error_str, sizeof(error_str), string, arglist);
 	va_end(arglist);
 
+	/* automatically suffix strerror(errno) for GITERR_OS errors */
+	if (oserr != NULL) {
+		strncat(error_str, ": ", sizeof(error_str));
+		strncat(error_str, oserr, sizeof(error_str));
+		errno = 0;
+	}
+
 	error->message = git__strdup(error_str);
 	error->klass = error_class;
 

src/filebuf.c

@@ -45,8 +45,8 @@ static int lock_file(git_filebuf *file, int flags)
 		source = p_open(file->path_original, O_RDONLY);
 		if (source < 0) {
 			giterr_set(GITERR_OS,
-				"Failed to open file '%s' for reading: %s",
-				file->path_original, strerror(errno));
+				"Failed to open file '%s' for reading",
+				file->path_original);
 			return -1;
 		}
 

src/fileops.c

@@ -37,7 +37,7 @@ int git_futils_mktmp(git_buf *path_out, const char *filename)
 
 	if ((fd = p_mkstemp(path_out->ptr)) < 0) {
 		giterr_set(GITERR_OS,
-			"Failed to create temporary file '%s': %s", path_out->ptr, strerror(errno));
+			"Failed to create temporary file '%s'", path_out->ptr);
 		return -1;
 	}
 
@@ -53,8 +53,7 @@ int git_futils_creat_withpath(const char *path, const mode_t dirmode, const mode
 
 	fd = p_creat(path, mode);
 	if (fd < 0) {
-		giterr_set(GITERR_OS,
-			"Failed to create file '%s': %s", path, strerror(errno));
+		giterr_set(GITERR_OS, "Failed to create file '%s'", path);
 		return -1;
 	}
 
@@ -65,8 +64,7 @@ int git_futils_creat_locked(const char *path, const mode_t mode)
 {
 	int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_EXCL, mode);
 	if (fd < 0) {
-		giterr_set(GITERR_OS,
-			"Failed to create locked file '%s': %s", path, strerror(errno));
+		giterr_set(GITERR_OS, "Failed to create locked file '%s'", path);
 		return -1;
 	}
 
@@ -115,10 +113,8 @@ int git_futils_readbuffer_updated(git_buf *buf, const char *path, time_t *mtime,
 	if (updated != NULL)
 		*updated = 0;
 
-	if ((fd = p_open(path, O_RDONLY)) < 0) {
-		giterr_set(GITERR_OS, "Failed to read file '%s': %s", path, strerror(errno));
-		return (errno == ENOENT) ? GIT_ENOTFOUND : -1;
-	}
+	if ((fd = git_futils_open_ro(path)) < 0)
+		return fd;
 
 	if (p_fstat(fd, &st) < 0 || S_ISDIR(st.st_mode) || !git__is_sizet(st.st_size+1)) {
 		close(fd);
@@ -154,8 +150,7 @@ int git_futils_readbuffer_updated(git_buf *buf, const char *path, time_t *mtime,
 
 		if (read_size < 0) {
 			close(fd);
-			giterr_set(GITERR_OS, "Failed to read descriptor for %s: %s",
-				path, strerror(errno));
+			giterr_set(GITERR_OS, "Failed to read descriptor for '%s'", path);
 			return -1;
 		}
 

src/fileops.h

@@ -77,6 +77,20 @@ extern int git_futils_mktmp(git_buf *path_out, const char *filename);
 extern int git_futils_mv_withpath(const char *from, const char *to, const mode_t dirmode);
 
 /**
+ * Open a file readonly and set error if needed
+ */
+GIT_INLINE(int) git_futils_open_ro(const char *path)
+{
+	int fd = p_open(path, O_RDONLY);
+	if (fd < 0) {
+		if (errno == ENOENT)
+			fd = GIT_ENOTFOUND;
+		giterr_set(GITERR_OS, "Failed to open '%s'", path);
+	}
+	return fd;
+}
+
+/**
  * Get the filesize in bytes of a file
  */
 extern git_off_t git_futils_filesize(git_file fd);

src/index.c

@@ -544,7 +544,7 @@ const git_index_entry_unmerged *git_index_get_unmerged_bypath(git_index *index,
 	if (!index->unmerged.length)
 		return NULL;
 
-	if ((pos = git_vector_bsearch2(&index->unmerged, unmerged_srch, path)) < GIT_SUCCESS)
+	if ((pos = git_vector_bsearch2(&index->unmerged, unmerged_srch, path)) < 0)
 		return NULL;
 
 	return git_vector_get(&index->unmerged, pos);

src/path.c

@@ -499,7 +499,7 @@ int git_path_direach(
 	wd_len = path->size;
 	dir = opendir(path->ptr);
 	if (!dir) {
-		giterr_set(GITERR_OS, "Failed to `opendir` %s: %s", path->ptr, strerror(errno));
+		giterr_set(GITERR_OS, "Failed to 'opendir' %s", path->ptr);
 		return -1;
 	}
 

src/refs.c

@@ -1021,8 +1021,7 @@ static int reference_delete(git_reference *ref)
 		git_buf_free(&full_path); /* done with path at this point */
 
 		if (result < 0) {
-			giterr_set(GITERR_OS,
-				"Failed to unlink '%s': %s", full_path.ptr, strerror(errno));
+			giterr_set(GITERR_OS, "Failed to unlink '%s'", full_path.ptr);
 			return -1;
 		}
 

src/util.c

@@ -355,23 +355,26 @@ int git__bsearch(
 	int (*compare)(const void *, const void *),
 	size_t *position)
 {
-	int lim, cmp;
+	int lim, cmp = -1;
 	void **part, **base = array;
 
 	for (lim = array_len; lim != 0; lim >>= 1) {
 		part = base + (lim >> 1);
 		cmp = (*compare)(key, *part);
 		if (cmp == 0) {
-			*position = (part - array);
-			return GIT_SUCCESS;
-		} else if (cmp > 0) { /* key > p; take right partition */
+			base = part;
+			break;
+		}
+		if (cmp > 0) { /* key > p; take right partition */
 			base = part + 1;
 			lim--;
 		} /* else take left partition */
 	}
 
-	*position = (base - array);
-	return GIT_ENOTFOUND;
+	if (position)
+		*position = (base - array);
+
+	return (cmp == 0) ? 0 : -1;
 }
 
 /**

src/util.h

@@ -115,6 +115,11 @@ extern int git__fnmatch(const char *pattern, const char *name, int flags);
 
 extern void git__tsort(void **dst, size_t size, int (*cmp)(const void *, const void *));
 
+/**
+ * @param position If non-NULL, this will be set to the position where the
+ * 		element is or would be inserted if not found.
+ * @return pos (>=0) if found or -1 if not found
+ */
 extern int git__bsearch(
 	void **array,
 	size_t array_len,

src/vector.c

@@ -19,10 +19,9 @@ static int resize_vector(git_vector *v)
 		v->_alloc_size = minimum_size;
 
 	v->contents = git__realloc(v->contents, v->_alloc_size * sizeof(void *));
-	if (v->contents == NULL)
-		return GIT_ENOMEM;
+	GITERR_CHECK_ALLOC(v->contents);
 
-	return GIT_SUCCESS;
+	return 0;
 }
 
 void git_vector_free(git_vector *v)
@@ -52,30 +51,29 @@ int git_vector_init(git_vector *v, unsigned int initial_size, git_vector_cmp cmp
 	v->sorted = 1;
 
 	v->contents = git__malloc(v->_alloc_size * sizeof(void *));
-	if (v->contents == NULL)
-		return GIT_ENOMEM;
+	GITERR_CHECK_ALLOC(v->contents);
 
-	return GIT_SUCCESS;
+	return 0;
 }
 
 int git_vector_insert(git_vector *v, void *element)
 {
 	assert(v);
 
-	if (v->length >= v->_alloc_size) {
-		if (resize_vector(v) < 0)
-			return GIT_ENOMEM;
-	}
+	if (v->length >= v->_alloc_size &&
+		resize_vector(v) < 0)
+		return -1;
 
 	v->contents[v->length++] = element;
 	v->sorted = 0;
 
-	return GIT_SUCCESS;
+	return 0;
 }
 
-int git_vector_insert_sorted(git_vector *v, void *element, int (*on_dup)(void **old, void *new))
+int git_vector_insert_sorted(
+	git_vector *v, void *element, int (*on_dup)(void **old, void *new))
 {
-	int error = GIT_SUCCESS;
+	int result;
 	size_t pos;
 
 	assert(v && v->_cmp);
@@ -83,22 +81,18 @@ int git_vector_insert_sorted(git_vector *v, void *element, int (*on_dup)(void **
 	if (!v->sorted)
 		git_vector_sort(v);
 
-	if (v->length >= v->_alloc_size) {
-		if (resize_vector(v) < 0)
-			return GIT_ENOMEM;
-	}
-
-	error = git__bsearch(v->contents, v->length, element, v->_cmp, &pos);
+	if (v->length >= v->_alloc_size &&
+		resize_vector(v) < 0)
+		return -1;
 
-	/* If we found the element and have a duplicate handler callback,
-	 * invoke it.  If it returns an error, then cancel insert, otherwise
+	/* If we find the element and have a duplicate handler callback,
+	 * invoke it.  If it returns non-zero, then cancel insert, otherwise
 	 * proceed with normal insert.
 	 */
-	if (error == GIT_SUCCESS && on_dup != NULL) {
-		error = on_dup(&v->contents[pos], element);
-		if (error != GIT_SUCCESS)
-			return error;
-	}
+	if (git__bsearch(v->contents, v->length, element, v->_cmp, &pos) >= 0 &&
+		on_dup != NULL &&
+		(result = on_dup(&v->contents[pos], element)) < 0)
+		return result;
 
 	/* shift elements to the right */
 	if (pos < v->length) {
@@ -108,8 +102,7 @@ int git_vector_insert_sorted(git_vector *v, void *element, int (*on_dup)(void **
 
 	v->contents[pos] = element;
 	v->length++;
-
-	return GIT_SUCCESS;
+	return 0;
 }
 
 void git_vector_sort(git_vector *v)
@@ -130,16 +123,14 @@ int git_vector_bsearch2(git_vector *v, git_vector_cmp key_lookup, const void *ke
 	assert(v && key && key_lookup);
 
 	/* need comparison function to sort the vector */
-	if (v->_cmp == NULL)
-		return git__throw(GIT_ENOTFOUND, "Can't sort vector. No comparison function set");
+	assert(v->_cmp != NULL);
 
 	git_vector_sort(v);
 
-	if (git__bsearch(v->contents, v->length, key, key_lookup,
-			&pos) == GIT_SUCCESS)
+	if (git__bsearch(v->contents, v->length, key, key_lookup, &pos) >= 0)
 		return (int)pos;
 
-	return git__throw(GIT_ENOTFOUND, "Can't find element");
+	return GIT_ENOTFOUND;
 }
 
 int git_vector_search2(git_vector *v, git_vector_cmp key_lookup, const void *key)
@@ -153,7 +144,7 @@ int git_vector_search2(git_vector *v, git_vector_cmp key_lookup, const void *key
 			return i;
 	}
 
-	return git__throw(GIT_ENOTFOUND, "Can't find element");
+	return GIT_ENOTFOUND;
 }
 
 static int strict_comparison(const void *a, const void *b)
@@ -178,13 +169,13 @@ int git_vector_remove(git_vector *v, unsigned int idx)
 	assert(v);
 
 	if (idx >= v->length || v->length == 0)
-		return git__throw(GIT_ENOTFOUND, "Can't remove element. Index out of bounds");
+		return GIT_ENOTFOUND;
 
 	for (i = idx; i < v->length - 1; ++i)
 		v->contents[i] = v->contents[i + 1];
 
 	v->length--;
-	return GIT_SUCCESS;
+	return 0;
 }
 
 void git_vector_pop(git_vector *v)