Merge pull request #1627 from arrbee/iterator-api-improvementsvmg/full-ref-iterator

Make iterators use GIT_ITEROVER & smart advance

11 files changed, 329 insertions, 187 deletions

diff --git a/src/checkout.c b/src/checkout.c
index c28fcdee0..7a2e68300 100644
--- a/src/checkout.c
+++ b/src/checkout.c
@@ -495,12 +495,9 @@ static int checkout_action(
 			if (cmp == 0) {
 				if (wd->mode == GIT_FILEMODE_TREE) {
 					/* case 2 - entry prefixed by workdir tree */
-					if ((error = git_iterator_advance_into(&wd, workdir)) < 0) {
-						if (error != GIT_ENOTFOUND ||
-							git_iterator_advance(&wd, workdir) < 0)
-							goto fail;
-					}
-
+					error = git_iterator_advance_into_or_over(&wd, workdir);
+					if (error && error != GIT_ITEROVER)
+						goto fail;
 					*wditem_ptr = wd;
 					continue;
 				}
@@ -515,8 +512,10 @@ static int checkout_action(
 			}
 
 			/* case 1 - handle wd item (if it matches pathspec) */
-			if (checkout_action_wd_only(data, workdir, wd, pathspec) < 0 ||
-				git_iterator_advance(&wd, workdir) < 0)
+			if (checkout_action_wd_only(data, workdir, wd, pathspec) < 0)
+				goto fail;
+			if ((error = git_iterator_advance(&wd, workdir)) < 0 &&
+				error != GIT_ITEROVER)
 				goto fail;
 
 			*wditem_ptr = wd;
@@ -539,8 +538,9 @@ static int checkout_action(
 			if (delta->status == GIT_DELTA_TYPECHANGE) {
 				if (delta->old_file.mode == GIT_FILEMODE_TREE) {
 					act = checkout_action_with_wd(data, delta, wd);
-					if (git_iterator_advance_into(&wd, workdir) < 0)
-						wd = NULL;
+					if ((error = git_iterator_advance_into(&wd, workdir)) < 0 &&
+						error != GIT_ENOTFOUND)
+						goto fail;
 					*wditem_ptr = wd;
 					return act;
 				}
@@ -550,8 +550,9 @@ static int checkout_action(
 					delta->old_file.mode == GIT_FILEMODE_COMMIT)
 				{
 					act = checkout_action_with_wd(data, delta, wd);
-					if (git_iterator_advance(&wd, workdir) < 0)
-						wd = NULL;
+					if ((error = git_iterator_advance(&wd, workdir)) < 0 &&
+						error != GIT_ITEROVER)
+						goto fail;
 					*wditem_ptr = wd;
 					return act;
 				}
@@ -582,6 +583,9 @@ static int checkout_remaining_wd_items(
 			error = git_iterator_advance(&wd, workdir);
 	}
 
+	if (error == GIT_ITEROVER)
+		error = 0;
+
 	return error;
 }
 
@@ -603,7 +607,8 @@ static int checkout_get_actions(
 		git_pathspec_init(&pathspec, &data->opts.paths, &pathpool) < 0)
 		return -1;
 
-	if ((error = git_iterator_current(&wditem, workdir)) < 0)
+	if ((error = git_iterator_current(&wditem, workdir)) < 0 &&
+		error != GIT_ITEROVER)
 		goto fail;
 
 	deltas = &data->diff->deltas;
diff --git a/src/diff.c b/src/diff.c
index b96ff4705..05ef4f16b 100644
--- a/src/diff.c
+++ b/src/diff.c
@@ -796,6 +796,9 @@ static int diff_scan_inside_untracked_dir(
 done:
 	git_buf_free(&base);
 
+	if (error == GIT_ITEROVER)
+		error = 0;
+
 	return error;
 }
 
@@ -981,8 +984,11 @@ static int handle_matched_item(
 {
 	int error = 0;
 
-	if (!(error = maybe_modified(diff, info)) &&
-		!(error = git_iterator_advance(&info->oitem, info->old_iter)))
+	if ((error = maybe_modified(diff, info)) < 0)
+		return error;
+
+	if (!(error = git_iterator_advance(&info->oitem, info->old_iter)) ||
+		error == GIT_ITEROVER)
 		error = git_iterator_advance(&info->nitem, info->new_iter);
 
 	return error;
@@ -1011,15 +1017,22 @@ int git_diff__from_iterators(
 
 	/* make iterators have matching icase behavior */
 	if (DIFF_FLAG_IS_SET(diff, GIT_DIFF_DELTAS_ARE_ICASE)) {
-		if (!(error = git_iterator_set_ignore_case(old_iter, true)))
-			error = git_iterator_set_ignore_case(new_iter, true);
+		if ((error = git_iterator_set_ignore_case(old_iter, true)) < 0 ||
+			(error = git_iterator_set_ignore_case(new_iter, true)) < 0)
+			goto cleanup;
 	}
 
 	/* finish initialization */
-	if (!error &&
-		!(error = diff_list_apply_options(diff, opts)) &&
-		!(error = git_iterator_current(&info.oitem, old_iter)))
-		error = git_iterator_current(&info.nitem, new_iter);
+	if ((error = diff_list_apply_options(diff, opts)) < 0)
+		goto cleanup;
+
+	if ((error = git_iterator_current(&info.oitem, old_iter)) < 0 &&
+		error != GIT_ITEROVER)
+		goto cleanup;
+	if ((error = git_iterator_current(&info.nitem, new_iter)) < 0 &&
+		error != GIT_ITEROVER)
+		goto cleanup;
+	error = 0;
 
 	/* run iterators building diffs */
 	while (!error && (info.oitem || info.nitem)) {
@@ -1041,8 +1054,13 @@ int git_diff__from_iterators(
 		 */
 		else
 			error = handle_matched_item(diff, &info);
+
+		/* because we are iterating over two lists, ignore ITEROVER */
+		if (error == GIT_ITEROVER)
+			error = 0;
 	}
 
+cleanup:
 	if (!error)
 		*diff_ptr = diff;
 	else
diff --git a/src/iterator.c b/src/iterator.c
index ff08c1ce0..4360b99ad 100644
--- a/src/iterator.c
+++ b/src/iterator.c
@@ -46,6 +46,9 @@
 #define iterator__dont_autoexpand(I) iterator__flag(I,DONT_AUTOEXPAND)
 #define iterator__do_autoexpand(I)   !iterator__flag(I,DONT_AUTOEXPAND)
 
+#define GIT_ITERATOR_FIRST_ACCESS (1 << 15)
+#define iterator__has_been_accessed(I) iterator__flag(I,FIRST_ACCESS)
+
 #define iterator__end(I) ((git_iterator *)(I))->end
 #define iterator__past_end(I,PATH) \
 	(iterator__end(I) && ((git_iterator *)(I))->prefixcomp((PATH),iterator__end(I)) > 0)
@@ -68,6 +71,8 @@ static int iterator__reset_range(
 		GITERR_CHECK_ALLOC(iter->end);
 	}
 
+	iter->flags &= ~GIT_ITERATOR_FIRST_ACCESS;
+
 	return 0;
 }
 
@@ -109,7 +114,7 @@ static int empty_iterator__noop(const git_index_entry **e, git_iterator *i)
 {
 	GIT_UNUSED(i);
 	iterator__clear_entry(e);
-	return 0;
+	return GIT_ITEROVER;
 }
 
 static int empty_iterator__seek(git_iterator *i, const char *p)
@@ -193,6 +198,7 @@ typedef struct {
 	git_buf path;
 	int path_ambiguities;
 	bool path_has_filename;
+	bool entry_is_current;
 	int (*strncomp)(const char *a, const char *b, size_t sz);
 } tree_iterator;
 
@@ -266,9 +272,28 @@ static int tree_iterator__search_cmp(const void *key, const void *val, void *p)
 		((tree_iterator *)p)->strncomp);
 }
 
+static bool tree_iterator__move_to_next(
+	tree_iterator *ti, tree_iterator_frame *tf)
+{
+	if (tf->next > tf->current + 1)
+		ti->path_ambiguities--;
+
+	if (!tf->up) { /* at root */
+		tf->current = tf->next;
+		return false;
+	}
+
+	for (; tf->current < tf->next; tf->current++) {
+		git_tree_free(tf->entries[tf->current]->tree);
+		tf->entries[tf->current]->tree = NULL;
+	}
+
+	return (tf->current < tf->n_entries);
+}
+
 static int tree_iterator__set_next(tree_iterator *ti, tree_iterator_frame *tf)
 {
-	int error;
+	int error = 0;
 	const git_tree_entry *te, *last = NULL;
 
 	tf->next = tf->current;
@@ -279,18 +304,23 @@ static int tree_iterator__set_next(tree_iterator *ti, tree_iterator_frame *tf)
 		if (last && tree_iterator__te_cmp(last, te, ti->strncomp))
 			break;
 
-		/* load trees for items in [current,next) range */
-		if (git_tree_entry__is_tree(te) &&
-			(error = git_tree_lookup(
-				&tf->entries[tf->next]->tree, ti->base.repo, &te->oid)) < 0)
-			return error;
+		/* try to load trees for items in [current,next) range */
+		if (!error && git_tree_entry__is_tree(te))
+			error = git_tree_lookup(
+				&tf->entries[tf->next]->tree, ti->base.repo, &te->oid);
 	}
 
 	if (tf->next > tf->current + 1)
 		ti->path_ambiguities++;
 
+	/* if a tree lookup failed, advance over this span and return failure */
+	if (error < 0) {
+		tree_iterator__move_to_next(ti, tf);
+		return error;
+	}
+
 	if (last && !tree_iterator__current_filename(ti, last))
-		return -1;
+		return -1; /* must have been allocation failure */
 
 	return 0;
 }
@@ -308,7 +338,7 @@ static int tree_iterator__push_frame(tree_iterator *ti)
 	size_t i, n_entries = 0;
 
 	if (head->current >= head->n_entries || !head->entries[head->current]->tree)
-		return 0;
+		return GIT_ITEROVER;
 
 	for (i = head->current; i < head->next; ++i)
 		n_entries += git_tree_entrycount(head->entries[i]->tree);
@@ -359,7 +389,7 @@ static int tree_iterator__push_frame(tree_iterator *ti)
 		}
 	}
 
-	ti->path_has_filename = false;
+	ti->path_has_filename = ti->entry_is_current = false;
 
 	if ((error = tree_iterator__set_next(ti, tf)) < 0)
 		return error;
@@ -371,25 +401,6 @@ static int tree_iterator__push_frame(tree_iterator *ti)
 	return 0;
 }
 
-static bool tree_iterator__move_to_next(
-	tree_iterator *ti, tree_iterator_frame *tf)
-{
-	if (tf->next > tf->current + 1)
-		ti->path_ambiguities--;
-
-	if (!tf->up) { /* at root */
-		tf->current = tf->next;
-		return false;
-	}
-
-	for (; tf->current < tf->next; tf->current++) {
-		git_tree_free(tf->entries[tf->current]->tree);
-		tf->entries[tf->current]->tree = NULL;
-	}
-
-	return (tf->current < tf->n_entries);
-}
-
 static bool tree_iterator__pop_frame(tree_iterator *ti, bool final)
 {
 	tree_iterator_frame *tf = ti->head;
@@ -412,7 +423,7 @@ static bool tree_iterator__pop_frame(tree_iterator *ti, bool final)
 	return true;
 }
 
-static int tree_iterator__pop_all(tree_iterator *ti, bool to_end, bool final)
+static void tree_iterator__pop_all(tree_iterator *ti, bool to_end, bool final)
 {
 	while (tree_iterator__pop_frame(ti, final)) /* pop to root */;
 
@@ -421,22 +432,18 @@ static int tree_iterator__pop_all(tree_iterator *ti, bool to_end, bool final)
 		ti->path_ambiguities = 0;
 		git_buf_clear(&ti->path);
 	}
-
-	return 0;
 }
 
-static int tree_iterator__current(
-	const git_index_entry **entry, git_iterator *self)
+static int tree_iterator__update_entry(tree_iterator *ti)
 {
-	tree_iterator *ti = (tree_iterator *)self;
-	tree_iterator_frame *tf = ti->head;
-	const git_tree_entry *te;
+	tree_iterator_frame *tf;
+    const git_tree_entry *te;
 
-	iterator__clear_entry(entry);
+	if (ti->entry_is_current)
+        return 0;
 
-	if (tf->current >= tf->n_entries)
-		return 0;
-	te = tf->entries[tf->current]->te;
+	tf = ti->head;
+    te = tf->entries[tf->current]->te;
 
 	ti->entry.mode = te->attr;
 	git_oid_cpy(&ti->entry.oid, &te->oid);
@@ -447,12 +454,36 @@ static int tree_iterator__current(
 	if (ti->path_ambiguities > 0)
 		tree_iterator__rewrite_filename(ti);
 
-	if (iterator__past_end(ti, ti->entry.path))
-		return tree_iterator__pop_all(ti, true, false);
+	if (iterator__past_end(ti, ti->entry.path)) {
+		tree_iterator__pop_all(ti, true, false);
+		return GIT_ITEROVER;
+	}
+
+	ti->entry_is_current = true;
+
+	return 0;
+}
+
+static int tree_iterator__current(
+	const git_index_entry **entry, git_iterator *self)
+{
+	int error;
+	tree_iterator *ti = (tree_iterator *)self;
+	tree_iterator_frame *tf = ti->head;
+
+	iterator__clear_entry(entry);
+
+	if (tf->current >= tf->n_entries)
+		return GIT_ITEROVER;
+
+	if ((error = tree_iterator__update_entry(ti)) < 0)
+		return error;
 
 	if (entry)
 		*entry = &ti->entry;
 
+	ti->base.flags |= GIT_ITERATOR_FIRST_ACCESS;
+
 	return 0;
 }
 
@@ -464,8 +495,10 @@ static int tree_iterator__advance_into(
 
 	iterator__clear_entry(entry);
 
-	if (tree_iterator__at_tree(ti) &&
-		!(error = tree_iterator__push_frame(ti)))
+	if (tree_iterator__at_tree(ti))
+		error = tree_iterator__push_frame(ti);
+
+	if (!error && entry)
 		error = tree_iterator__current(entry, self);
 
 	return error;
@@ -480,8 +513,11 @@ static int tree_iterator__advance(
 
 	iterator__clear_entry(entry);
 
-	if (tf->current > tf->n_entries)
-		return 0;
+	if (tf->current >= tf->n_entries)
+		return GIT_ITEROVER;
+
+	if (!iterator__has_been_accessed(ti))
+		return tree_iterator__current(entry, self);
 
 	if (iterator__do_autoexpand(ti) && iterator__include_trees(ti) &&
 		tree_iterator__at_tree(ti))
@@ -489,7 +525,7 @@ static int tree_iterator__advance(
 
 	if (ti->path_has_filename) {
 		git_buf_rtruncate_at_char(&ti->path, '/');
-		ti->path_has_filename = false;
+		ti->path_has_filename = ti->entry_is_current = false;
 	}
 
 	/* scan forward and up, advancing in frame or popping frame when done */
@@ -699,7 +735,9 @@ static int index_iterator__current(
 	if (entry)
 		*entry = ie;
 
-	return 0;
+	ii->base.flags |= GIT_ITERATOR_FIRST_ACCESS;
+
+	return (ie != NULL) ? 0 : GIT_ITEROVER;
 }
 
 static int index_iterator__at_end(git_iterator *self)
@@ -715,6 +753,9 @@ static int index_iterator__advance(
 	size_t entrycount = git_index_entrycount(ii->index);
 	const git_index_entry *ie;
 
+	if (!iterator__has_been_accessed(ii))
+		return index_iterator__current(entry, self);
+
 	if (index_iterator__at_tree(ii)) {
 		if (iterator__do_autoexpand(ii)) {
 			ii->partial.ptr[ii->partial_pos] = ii->restore_terminator;
@@ -906,6 +947,8 @@ static void fs_iterator__pop_frame(
 }
 
 static int fs_iterator__update_entry(fs_iterator *fi);
+static int fs_iterator__advance_over(
+	const git_index_entry **entry, git_iterator *self);
 
 static int fs_iterator__entry_cmp(const void *i, const void *item)
 {
@@ -945,7 +988,13 @@ static int fs_iterator__expand_dir(fs_iterator *fi)
 		fi->path.ptr, fi->root_len, iterator__ignore_case(fi),
 		fi->base.start, fi->base.end, &ff->entries);
 
-	if (error < 0 || ff->entries.length == 0) {
+	if (error < 0) {
+		fs_iterator__free_frame(ff);
+		fs_iterator__advance_over(NULL, (git_iterator *)fi);
+		return error;
+	}
+
+	if (ff->entries.length == 0) {
 		fs_iterator__free_frame(ff);
 		return GIT_ENOTFOUND;
 	}
@@ -966,9 +1015,14 @@ static int fs_iterator__current(
 	const git_index_entry **entry, git_iterator *self)
 {
 	fs_iterator *fi = (fs_iterator *)self;
+	const git_index_entry *fe = (fi->entry.path == NULL) ? NULL : &fi->entry;
+
 	if (entry)
-		*entry = (fi->entry.path == NULL) ? NULL : &fi->entry;
-	return 0;
+		*entry = fe;
+
+	fi->base.flags |= GIT_ITERATOR_FIRST_ACCESS;
+
+	return (fe != NULL) ? 0 : GIT_ITEROVER;
 }
 
 static int fs_iterator__at_end(git_iterator *self)
@@ -998,6 +1052,9 @@ static int fs_iterator__advance_into(
 	if (!error && entry)
 		error = fs_iterator__current(entry, iter);
 
+	if (!error && !fi->entry.path)
+		error = GIT_ITEROVER;
+
 	return error;
 }
 
@@ -1035,6 +1092,9 @@ static int fs_iterator__advance(
 {
 	fs_iterator *fi = (fs_iterator *)self;
 
+	if (!iterator__has_been_accessed(fi))
+		return fs_iterator__current(entry, self);
+
 	/* given include_trees & autoexpand, we might have to go into a tree */
 	if (iterator__do_autoexpand(fi) &&
 		fi->entry.path != NULL &&
@@ -1063,18 +1123,23 @@ static int fs_iterator__seek(git_iterator *self, const char *prefix)
 static int fs_iterator__reset(
 	git_iterator *self, const char *start, const char *end)
 {
+	int error;
 	fs_iterator *fi = (fs_iterator *)self;
 
 	while (fi->stack != NULL && fi->stack->next != NULL)
 		fs_iterator__pop_frame(fi, fi->stack, false);
 	fi->depth = 0;
 
-	if (iterator__reset_range(self, start, end) < 0)
-		return -1;
+	if ((error = iterator__reset_range(self, start, end)) < 0)
+		return error;
 
 	fs_iterator__seek_frame_start(fi, fi->stack);
 
-	return fs_iterator__update_entry(fi);
+	error = fs_iterator__update_entry(fi);
+	if (error == GIT_ITEROVER)
+		error = 0;
+
+	return error;
 }
 
 static void fs_iterator__free(git_iterator *self)
@@ -1089,18 +1154,23 @@ static void fs_iterator__free(git_iterator *self)
 
 static int fs_iterator__update_entry(fs_iterator *fi)
 {
-	git_path_with_stat *ps =
-		git_vector_get(&fi->stack->entries, fi->stack->index);
+	git_path_with_stat *ps;
 
-	git_buf_truncate(&fi->path, fi->root_len);
 	memset(&fi->entry, 0, sizeof(fi->entry));
 
+	if (!fi->stack)
+		return GIT_ITEROVER;
+
+	ps = git_vector_get(&fi->stack->entries, fi->stack->index);
 	if (!ps)
-		return 0;
+		return GIT_ITEROVER;
+
+	git_buf_truncate(&fi->path, fi->root_len);
 	if (git_buf_put(&fi->path, ps->path, ps->path_len) < 0)
 		return -1;
+
 	if (iterator__past_end(fi, fi->path.ptr + fi->root_len))
-		return 0;
+		return GIT_ITEROVER;
 
 	fi->entry.path = ps->path;
 	git_index_entry__init_from_stat(&fi->entry, &ps->st);
@@ -1114,8 +1184,13 @@ static int fs_iterator__update_entry(fs_iterator *fi)
 		return fs_iterator__advance_over(NULL, (git_iterator *)fi);
 
 	/* if this is a tree and trees aren't included, then skip */
-	if (fi->entry.mode == GIT_FILEMODE_TREE && !iterator__include_trees(fi))
-		return git_iterator_advance(NULL, (git_iterator *)fi);
+	if (fi->entry.mode == GIT_FILEMODE_TREE && !iterator__include_trees(fi)) {
+		int error = fs_iterator__advance_into(NULL, (git_iterator *)fi);
+		if (error != GIT_ENOTFOUND)
+			return error;
+		giterr_clear();
+		return fs_iterator__advance_over(NULL, (git_iterator *)fi);
+	}
 
 	return 0;
 }
@@ -1131,13 +1206,14 @@ static int fs_iterator__initialize(
 	}
 	fi->root_len = fi->path.size;
 
-	if ((error = fs_iterator__expand_dir(fi)) == GIT_ENOTFOUND) {
-		giterr_clear();
-		error = 0;
-	}
-	if (error) {
-		git_iterator_free((git_iterator *)fi);
-		fi = NULL;
+	if ((error = fs_iterator__expand_dir(fi)) < 0) {
+		if (error == GIT_ENOTFOUND || error == GIT_ITEROVER) {
+			giterr_clear();
+			error = 0;
+		} else {
+			git_iterator_free((git_iterator *)fi);
+			fi = NULL;
+		}
 	}
 
 	*out = (git_iterator *)fi;
diff --git a/src/iterator.h b/src/iterator.h
index 7998f7c6b..493ff4b2a 100644
--- a/src/iterator.h
+++ b/src/iterator.h
@@ -142,9 +142,9 @@ GIT_INLINE(int) git_iterator_advance(
  *
  * If the current item is not a tree, this is a no-op.
  *
- * For working directory iterators only, a tree (i.e. directory) can be
- * empty.  In that case, this function returns GIT_ENOTFOUND and does not
- * advance.  That can't happen for tree and index iterators.
+ * For filesystem and working directory iterators, a tree (i.e. directory)
+ * can be empty.  In that case, this function returns GIT_ENOTFOUND and
+ * does not advance.  That can't happen for tree and index iterators.
  */
 GIT_INLINE(int) git_iterator_advance_into(
 	const git_index_entry **entry, git_iterator *iter)
@@ -152,18 +152,50 @@ GIT_INLINE(int) git_iterator_advance_into(
 	return iter->cb->advance_into(entry, iter);
 }
 
+/**
+ * Advance into a tree or skip over it if it is empty.
+ *
+ * Because `git_iterator_advance_into` may return GIT_ENOTFOUND if the
+ * directory is empty (only with filesystem and working directory
+ * iterators) and a common response is to just call `git_iterator_advance`
+ * when that happens, this bundles the two into a single simple call.
+ */
+GIT_INLINE(int) git_iterator_advance_into_or_over(
+	const git_index_entry **entry, git_iterator *iter)
+{
+	int error = iter->cb->advance_into(entry, iter);
+	if (error == GIT_ENOTFOUND) {
+		giterr_clear();
+		error = iter->cb->advance(entry, iter);
+	}
+	return error;
+}
+
+/* Seek is currently unimplemented */
 GIT_INLINE(int) git_iterator_seek(
 	git_iterator *iter, const char *prefix)
 {
 	return iter->cb->seek(iter, prefix);
 }
 
+/**
+ * Go back to the start of the iteration.
+ *
+ * This resets the iterator to the start of the iteration.  It also allows
+ * you to reset the `start` and `end` pathname boundaries of the iteration
+ * when doing so.
+ */
 GIT_INLINE(int) git_iterator_reset(
 	git_iterator *iter, const char *start, const char *end)
 {
 	return iter->cb->reset(iter, start, end);
 }
 
+/**
+ * Check if the iterator is at the end
+ *
+ * @return 0 if not at end, >0 if at end
+ */
 GIT_INLINE(int) git_iterator_at_end(git_iterator *iter)
 {
 	return iter->cb->at_end(iter);
diff --git a/src/merge.c b/src/merge.c
index 11345587c..047d96013 100644
--- a/src/merge.c
+++ b/src/merge.c
@@ -1259,7 +1259,8 @@ int git_merge_diff_list__find_differences(
 
 	/* Set up the iterators */
 	for (i = 0; i < 3; i++) {
-		if ((error = git_iterator_current(&items[i], iterators[i])) < 0)
+		error = git_iterator_current(&items[i], iterators[i]);
+		if (error < 0 && error != GIT_ITEROVER)
 			goto done;
 	}
 
@@ -1313,11 +1314,16 @@ int git_merge_diff_list__find_differences(
 			error = merge_index_insert_conflict(diff_list, &df_data, cur_items);
 		else
 			error = merge_index_insert_unmodified(diff_list, cur_items);
+		if (error < 0)
+			goto done;
 
 		/* Advance each iterator that participated */
 		for (i = 0; i < 3; i++) {
-			if (cur_items[i] != NULL &&
-				(error = git_iterator_advance(&items[i], iterators[i])) < 0)
+			if (cur_items[i] == NULL)
+				continue;
+
+			error = git_iterator_advance(&items[i], iterators[i]);
+			if (error < 0 && error != GIT_ITEROVER)
 				goto done;
 		}
 	}
@@ -1326,6 +1332,9 @@ done:
 	for (i = 0; i < 3; i++)
 		git_iterator_free(iterators[i]);
 
+	if (error == GIT_ITEROVER)
+		error = 0;
+
 	return error;
 }
 
diff --git a/src/notes.c b/src/notes.c
index 3e3db58db..beace1b50 100644
--- a/src/notes.c
+++ b/src/notes.c
@@ -647,18 +647,12 @@ int git_note_next(
 	const git_index_entry *item;
 
 	if ((error = git_iterator_current(&item, it)) < 0)
-		goto exit;
+		return error;
 
-	if (item != NULL) {
-		git_oid_cpy(note_id, &item->oid);
-		error = process_entry_path(item->path, annotated_id);
+	git_oid_cpy(note_id, &item->oid);
 
-		if (error >= 0)
-			error = git_iterator_advance(NULL, it);
-	} else {
-		error = GIT_ITEROVER;
-	}
+	if (!(error = process_entry_path(item->path, annotated_id)))
+		git_iterator_advance(NULL, it);
 
-exit:
 	return error;
 }
diff --git a/src/refdb_fs.c b/src/refdb_fs.c
index e7ffcd4a1..4083ba9e5 100644
--- a/src/refdb_fs.c
+++ b/src/refdb_fs.c
@@ -582,6 +582,9 @@ static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter)
 	git_iterator *fsit;
 	const git_index_entry *entry = NULL;
 
+	if (!backend->path) /* do nothing if no path for loose refs */
+		return 0;
+
 	if (git_buf_printf(&path, "%s/refs", backend->path) < 0)
 		return -1;
 
@@ -591,7 +594,7 @@ static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter)
 	git_vector_init(&iter->loose, 8, NULL);
 	git_buf_sets(&path, GIT_REFS_DIR);
 
-	while (!git_iterator_current(&entry, fsit) && entry) {
+	while (!git_iterator_advance(&entry, fsit)) {
 		const char *ref_name;
 		khiter_t pos;
 
@@ -600,10 +603,8 @@ static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter)
 		ref_name = git_buf_cstr(&path);
 
 		if (git__suffixcmp(ref_name, ".lock") == 0 ||
-			(iter->glob && p_fnmatch(iter->glob, ref_name, 0) != 0)) {
-			git_iterator_advance(NULL, fsit);
+			(iter->glob && p_fnmatch(iter->glob, ref_name, 0) != 0))
 			continue;
-		}
 
 		pos = git_strmap_lookup_index(packfile, ref_name);
 		if (git_strmap_valid_index(packfile, pos)) {
@@ -612,7 +613,6 @@ static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter)
 		}
 
 		git_vector_insert(&iter->loose, git__strdup(ref_name));
-		git_iterator_advance(NULL, fsit);
 	}
 
 	git_iterator_free(fsit);
diff --git a/src/submodule.c b/src/submodule.c
index f4fbcd35a..16114d8ac 100644
--- a/src/submodule.c
+++ b/src/submodule.c
@@ -1143,9 +1143,7 @@ static int load_submodule_config_from_index(
 		(error = git_iterator_for_index(&i, index, 0, NULL, NULL)) < 0)
 		return error;
 
-	error = git_iterator_current(&entry, i);
-
-	while (!error && entry != NULL) {
+	while (!(error = git_iterator_advance(&entry, i))) {
 
 		if (S_ISGITLINK(entry->mode)) {
 			error = submodule_load_from_index(repo, entry);
@@ -1158,10 +1156,11 @@ static int load_submodule_config_from_index(
 			if (strcmp(entry->path, GIT_MODULES_FILE) == 0)
 				git_oid_cpy(gitmodules_oid, &entry->oid);
 		}
-
-		error = git_iterator_advance(&entry, i);
 	}
 
+	if (error == GIT_ITEROVER)
+		error = 0;
+
 	git_iterator_free(i);
 
 	return error;
@@ -1183,9 +1182,7 @@ static int load_submodule_config_from_head(
 		return error;
 	}
 
-	error = git_iterator_current(&entry, i);
-
-	while (!error && entry != NULL) {
+	while (!(error = git_iterator_advance(&entry, i))) {
 
 		if (S_ISGITLINK(entry->mode)) {
 			error = submodule_load_from_head(repo, entry->path, &entry->oid);
@@ -1199,10 +1196,11 @@ static int load_submodule_config_from_head(
 				git_oid_iszero(gitmodules_oid))
 				git_oid_cpy(gitmodules_oid, &entry->oid);
 		}
-
-		error = git_iterator_advance(&entry, i);
 	}
 
+	if (error == GIT_ITEROVER)
+		error = 0;
+
 	git_iterator_free(i);
 	git_tree_free(head);
 
diff --git a/tests-clar/diff/iterator.c b/tests-clar/diff/iterator.c
index 15b10465a..bbdae8ad1 100644
--- a/tests-clar/diff/iterator.c
+++ b/tests-clar/diff/iterator.c
@@ -31,7 +31,7 @@ static void tree_iterator_test(
 	git_tree *t;
 	git_iterator *i;
 	const git_index_entry *entry;
-	int count = 0, count_post_reset = 0;
+	int error, count = 0, count_post_reset = 0;
 	git_repository *repo = cl_git_sandbox_init(sandbox);
 
 	cl_assert(t = resolve_commit_oid_to_tree(repo, treeish));
@@ -39,29 +39,30 @@ static void tree_iterator_test(
 		&i, t, GIT_ITERATOR_DONT_IGNORE_CASE, start, end));
 
 	/* test loop */
-	cl_git_pass(git_iterator_current(&entry, i));
-	while (entry != NULL) {
+	while (!(error = git_iterator_advance(&entry, i))) {
+		cl_assert(entry);
 		if (expected_values != NULL)
 			cl_assert_equal_s(expected_values[count], entry->path);
 		count++;
-		cl_git_pass(git_iterator_advance(&entry, i));
 	}
+	cl_assert_equal_i(GIT_ITEROVER, error);
+	cl_assert(!entry);
+	cl_assert_equal_i(expected_count, count);
 
 	/* test reset */
 	cl_git_pass(git_iterator_reset(i, NULL, NULL));
-	cl_git_pass(git_iterator_current(&entry, i));
-	while (entry != NULL) {
+
+	while (!(error = git_iterator_advance(&entry, i))) {
+		cl_assert(entry);
 		if (expected_values != NULL)
 			cl_assert_equal_s(expected_values[count_post_reset], entry->path);
 		count_post_reset++;
-		cl_git_pass(git_iterator_advance(&entry, i));
 	}
-
-	git_iterator_free(i);
-
-	cl_assert_equal_i(expected_count, count);
+	cl_assert_equal_i(GIT_ITEROVER, error);
+	cl_assert(!entry);
 	cl_assert_equal_i(count, count_post_reset);
 
+	git_iterator_free(i);
 	git_tree_free(t);
 }
 
@@ -298,7 +299,7 @@ void test_diff_iterator__tree_special_functions(void)
 	git_iterator *i;
 	const git_index_entry *entry;
 	git_repository *repo = cl_git_sandbox_init("attr");
-	int cases = 0;
+	int error, cases = 0;
 	const char *rootoid = "ce39a97a7fb1fa90bcf5e711249c1e507476ae0e";
 
 	t = resolve_commit_oid_to_tree(
@@ -307,9 +308,10 @@ void test_diff_iterator__tree_special_functions(void)
 
 	cl_git_pass(git_iterator_for_tree(
 		&i, t, GIT_ITERATOR_DONT_IGNORE_CASE, NULL, NULL));
-	cl_git_pass(git_iterator_current(&entry, i));
 
-	while (entry != NULL) {
+	while (!(error = git_iterator_advance(&entry, i))) {
+		cl_assert(entry);
+
 		if (strcmp(entry->path, "sub/file") == 0) {
 			cases++;
 			check_tree_entry(
@@ -338,11 +340,11 @@ void test_diff_iterator__tree_special_functions(void)
 				"2929de282ce999e95183aedac6451d3384559c4b",
 				rootoid, NULL);
 		}
-
-		cl_git_pass(git_iterator_advance(&entry, i));
 	}
-
+	cl_assert_equal_i(GIT_ITEROVER, error);
+	cl_assert(!entry);
 	cl_assert_equal_i(4, cases);
+
 	git_iterator_free(i);
 	git_tree_free(t);
 }
@@ -360,14 +362,15 @@ static void index_iterator_test(
 	git_index *index;
 	git_iterator *i;
 	const git_index_entry *entry;
-	int count = 0;
+	int error, count = 0;
 	git_repository *repo = cl_git_sandbox_init(sandbox);
 
 	cl_git_pass(git_repository_index(&index, repo));
 	cl_git_pass(git_iterator_for_index(&i, index, 0, start, end));
-	cl_git_pass(git_iterator_current(&entry, i));
 
-	while (entry != NULL) {
+	while (!(error = git_iterator_advance(&entry, i))) {
+		cl_assert(entry);
+
 		if (expected_names != NULL)
 			cl_assert_equal_s(expected_names[count], entry->path);
 
@@ -378,13 +381,14 @@ static void index_iterator_test(
 		}
 
 		count++;
-		cl_git_pass(git_iterator_advance(&entry, i));
 	}
 
+	cl_assert_equal_i(GIT_ITEROVER, error);
+	cl_assert(!entry);
+	cl_assert_equal_i(expected_count, count);
+
 	git_iterator_free(i);
 	git_index_free(index);
-
-	cl_assert_equal_i(expected_count, count);
 }
 
 static const char *expected_index_0[] = {
@@ -535,12 +539,15 @@ static void workdir_iterator_test(
 {
 	git_iterator *i;
 	const git_index_entry *entry;
-	int count = 0, count_all = 0, count_all_post_reset = 0;
+	int error, count = 0, count_all = 0, count_all_post_reset = 0;
 	git_repository *repo = cl_git_sandbox_init(sandbox);
 
 	cl_git_pass(git_iterator_for_workdir(
 		&i, repo, GIT_ITERATOR_DONT_AUTOEXPAND, start, end));
-	cl_git_pass(git_iterator_current(&entry, i));
+
+	error = git_iterator_current(&entry, i);
+	cl_assert((error == 0 && entry != NULL) ||
+			  (error == GIT_ITEROVER && entry == NULL));
 
 	while (entry != NULL) {
 		int ignored = git_iterator_current_is_ignored(i);
@@ -560,29 +567,39 @@ static void workdir_iterator_test(
 			count++;
 		count_all++;
 
-		cl_git_pass(git_iterator_advance(&entry, i));
+		error = git_iterator_advance(&entry, i);
+
+		cl_assert((error == 0 && entry != NULL) ||
+				  (error == GIT_ITEROVER && entry == NULL));
 	}
 
+	cl_assert_equal_i(expected_count, count);
+	cl_assert_equal_i(expected_count + expected_ignores, count_all);
+
 	cl_git_pass(git_iterator_reset(i, NULL, NULL));
-	cl_git_pass(git_iterator_current(&entry, i));
+
+	error = git_iterator_current(&entry, i);
+	cl_assert((error == 0 && entry != NULL) ||
+			  (error == GIT_ITEROVER && entry == NULL));
 
 	while (entry != NULL) {
 		if (S_ISDIR(entry->mode)) {
 			cl_git_pass(git_iterator_advance_into(&entry, i));
 			continue;
 		}
+
 		if (expected_names != NULL)
 			cl_assert_equal_s(
 				expected_names[count_all_post_reset], entry->path);
 		count_all_post_reset++;
-		cl_git_pass(git_iterator_advance(&entry, i));
-	}
 
-	git_iterator_free(i);
+		error = git_iterator_advance(&entry, i);
+		cl_assert(error == 0 || error == GIT_ITEROVER);
+	}
 
-	cl_assert_equal_i(expected_count, count);
-	cl_assert_equal_i(expected_count + expected_ignores, count_all);
 	cl_assert_equal_i(count_all, count_all_post_reset);
+
+	git_iterator_free(i);
 }
 
 void test_diff_iterator__workdir_0(void)
@@ -752,8 +769,10 @@ void test_diff_iterator__workdir_builtin_ignores(void)
 		{
 			/* it is possible to advance "into" a submodule */
 			cl_git_pass(git_iterator_advance_into(&entry, i));
-		} else
-			cl_git_pass(git_iterator_advance(&entry, i));
+		} else {
+			int error = git_iterator_advance(&entry, i);
+			cl_assert(!error || error == GIT_ITEROVER);
+		}
 	}
 
 	cl_assert(expected[idx].path == NULL);
@@ -766,7 +785,7 @@ static void check_wd_first_through_third_range(
 {
 	git_iterator *i;
 	const git_index_entry *entry;
-	int idx;
+	int error, idx;
 	static const char *expected[] = { "FIRST", "second", "THIRD", NULL };
 
 	cl_git_pass(git_iterator_for_workdir(
@@ -776,7 +795,8 @@ static void check_wd_first_through_third_range(
 	for (idx = 0; entry != NULL; ++idx) {
 		cl_assert_equal_s(expected[idx], entry->path);
 
-		cl_git_pass(git_iterator_advance(&entry, i));
+		error = git_iterator_advance(&entry, i);
+		cl_assert(!error || error == GIT_ITEROVER);
 	}
 
 	cl_assert(expected[idx] == NULL);
@@ -814,8 +834,7 @@ static void check_tree_range(
 {
 	git_tree *head;
 	git_iterator *i;
-	const git_index_entry *entry;
-	int count;
+	int error, count;
 
 	cl_git_pass(git_repository_head_tree(&head, repo));
 
@@ -824,13 +843,10 @@ static void check_tree_range(
 		ignore_case ? GIT_ITERATOR_IGNORE_CASE : GIT_ITERATOR_DONT_IGNORE_CASE,
 		start, end));
 
-	cl_git_pass(git_iterator_current(&entry, i));
-
-	for (count = 0; entry != NULL; ) {
-		++count;
-		cl_git_pass(git_iterator_advance(&entry, i));
-	}
+	for (count = 0; !(error = git_iterator_advance(NULL, i)); ++count)
+		/* count em up */;
 
+	cl_assert_equal_i(GIT_ITEROVER, error);
 	cl_assert_equal_i(expected_count, count);
 
 	git_iterator_free(i);
@@ -872,8 +888,7 @@ static void check_index_range(
 {
 	git_index *index;
 	git_iterator *i;
-	const git_index_entry *entry;
-	int count, caps;
+	int error, count, caps;
 	bool is_ignoring_case;
 
 	cl_git_pass(git_repository_index(&index, repo));
@@ -888,13 +903,10 @@ static void check_index_range(
 
 	cl_assert(git_iterator_ignore_case(i) == ignore_case);
 
-	cl_git_pass(git_iterator_current(&entry, i));
-
-	for (count = 0; entry != NULL; ) {
-		++count;
-		cl_git_pass(git_iterator_advance(&entry, i));
-	}
+	for (count = 0; !(error = git_iterator_advance(NULL, i)); ++count)
+		/* count em up */;
 
+	cl_assert_equal_i(GIT_ITEROVER, error);
 	cl_assert_equal_i(expected_count, count);
 
 	git_iterator_free(i);
diff --git a/tests-clar/repo/iterator.c b/tests-clar/repo/iterator.c
index ab460735c..11a7d2a23 100644
--- a/tests-clar/repo/iterator.c
+++ b/tests-clar/repo/iterator.c
@@ -31,12 +31,11 @@ static void expect_iterator_items(
 	if (expected_flat < 0) { v = true; expected_flat = -expected_flat; }
 	if (expected_total < 0) { v = true; expected_total = -expected_total; }
 
-	count = 0;
-	cl_git_pass(git_iterator_current(&entry, i));
-
 	if (v) fprintf(stderr, "== %s ==\n", no_trees ? "notrees" : "trees");
 
-	while (entry != NULL) {
+	count = 0;
+
+	while (!git_iterator_advance(&entry, i)) {
 		if (v) fprintf(stderr, "  %s %07o\n", entry->path, (int)entry->mode);
 
 		if (no_trees)
@@ -54,8 +53,6 @@ static void expect_iterator_items(
 				cl_assert(entry->mode != GIT_FILEMODE_TREE);
 		}
 
-		cl_git_pass(git_iterator_advance(&entry, i));
-
 		if (++count > expected_flat)
 			break;
 	}
@@ -93,10 +90,14 @@ static void expect_iterator_items(
 			/* could return NOTFOUND if directory is empty */
 			cl_assert(!error || error == GIT_ENOTFOUND);
 
-			if (error == GIT_ENOTFOUND)
-				cl_git_pass(git_iterator_advance(&entry, i));
-		} else
-			cl_git_pass(git_iterator_advance(&entry, i));
+			if (error == GIT_ENOTFOUND) {
+				error = git_iterator_advance(&entry, i);
+				cl_assert(!error || error == GIT_ITEROVER);
+			}
+		} else {
+			error = git_iterator_advance(&entry, i);
+			cl_assert(!error || error == GIT_ITEROVER);
+		}
 
 		if (++count > expected_total)
 			break;
diff --git a/tests-clar/submodule/status.c b/tests-clar/submodule/status.c
index 88f388052..39c83a0b7 100644
--- a/tests-clar/submodule/status.c
+++ b/tests-clar/submodule/status.c
@@ -370,12 +370,9 @@ void test_submodule_status__iterator(void)
 
 	cl_git_pass(git_iterator_for_workdir(&iter, g_repo,
 		GIT_ITERATOR_IGNORE_CASE | GIT_ITERATOR_INCLUDE_TREES, NULL, NULL));
-	cl_git_pass(git_iterator_current(&entry, iter));
 
-	for (i = 0; entry; ++i) {
+	for (i = 0; !git_iterator_advance(&entry, iter); ++i)
 		cl_assert_equal_s(expected[i], entry->path);
-		cl_git_pass(git_iterator_advance(&entry, iter));
-	}
 
 	git_iterator_free(iter);