branch: move from foreach to an iterator

Create a git_branch_iterator type which is equivalent to the foreach but
lets us write loops instead of callbacks.

Since the introduction of git_reference_shorthand(), the added value of
passing the name is reduced.

include/git2/branch.h

@@ -66,33 +66,41 @@ GIT_EXTERN(int) git_branch_create(
  */
 GIT_EXTERN(int) git_branch_delete(git_reference *branch);
 
-typedef int (*git_branch_foreach_cb)(
+/** Iterator type for branches */
-	const char *branch_name,
+typedef struct git_branch_iterator git_branch_iterator;
-	git_branch_t branch_type,
-	void *payload);
 
 /**
- * Loop over all the branches and issue a callback for each one.
+ * Create an iterator which loops over the requested branches.
- *
- * If the callback returns a non-zero value, this will stop looping.
  *
+ * @param out the iterator
  * @param repo Repository where to find the branches.
- *
  * @param list_flags Filtering flags for the branch
  * listing. Valid values are GIT_BRANCH_LOCAL, GIT_BRANCH_REMOTE
  * or a combination of the two.
  *
- * @param branch_cb Callback to invoke per found branch.
+ * @return 0 on success  or an error code
- *
- * @param payload Extra parameter to callback function.
- *
- * @return 0 on success, GIT_EUSER on non-zero callback, or error code
  */
-GIT_EXTERN(int) git_branch_foreach(
+GIT_EXTERN(int) git_branch_iterator_new(
+	git_branch_iterator **out,
 	git_repository *repo,
-	unsigned int list_flags,
+	unsigned int list_flags);
-	git_branch_foreach_cb branch_cb,
+
-	void *payload);
+/**
+ * Retrieve the next branch from the iterator
+ *
+ * @param out the reference
+ * @param out_type the type of branch (local or remote-tracking)
+ * @param iter the branch iterator
+ * @return 0 on success, GIT_ITEROVER if there are no more branches or an error code.
+ */
+GIT_EXTERN(int) git_branch_next(git_reference **out, unsigned int *out_type, git_branch_iterator *iter);
+
+/**
+ * Free a branch iterator
+ *
+ * @param iter the iterator to free
+ */
+GIT_EXTERN(void) git_branch_iterator_free(git_branch_iterator *iter);
 
 /**
  * Move/rename an existing local branch reference.

src/branch.c

@@ -124,50 +124,68 @@ on_error:
 	return error;
 }
 
-int git_branch_foreach(
+typedef struct {
-	git_repository *repo,
-	unsigned int list_flags,
-	git_branch_foreach_cb callback,
-	void *payload)
-{
 	git_reference_iterator *iter;
+	unsigned int flags;
+} branch_iter;
+
+int git_branch_next(git_reference **out, unsigned int *out_type, git_branch_iterator *_iter)
+{
+	branch_iter *iter = (branch_iter *) _iter;
 	git_reference *ref;
-	int error = 0;
+	int error;
 
-	if (git_reference_iterator_new(&iter, repo) < 0)
+	while ((error = git_reference_next(&ref, iter->iter)) == 0) {
-		return -1;
+		if ((iter->flags & GIT_BRANCH_LOCAL) &&
+		    !git__prefixcmp(ref->name, GIT_REFS_HEADS_DIR)) {
+			*out = ref;
+			*out_type = GIT_BRANCH_LOCAL;
 
-	while ((error = git_reference_next(&ref, iter)) == 0) {
+			return 0;
-		if (list_flags & GIT_BRANCH_LOCAL &&
+		} else  if ((iter->flags & GIT_BRANCH_REMOTE) &&
-		    git__prefixcmp(ref->name, GIT_REFS_HEADS_DIR) == 0) {
+			    !git__prefixcmp(ref->name, GIT_REFS_REMOTES_DIR)) {
-			if (callback(ref->name + strlen(GIT_REFS_HEADS_DIR),
+			*out = ref;
-					GIT_BRANCH_LOCAL, payload)) {
+			*out_type = GIT_BRANCH_REMOTE;
-				error = GIT_EUSER;
+
-			}
+			return 0;
+		} else {
+			git_reference_free(ref);
 		}
-
-		if (list_flags & GIT_BRANCH_REMOTE &&
-		    git__prefixcmp(ref->name, GIT_REFS_REMOTES_DIR) == 0) {
-			if (callback(ref->name + strlen(GIT_REFS_REMOTES_DIR),
-					GIT_BRANCH_REMOTE, payload)) {
-				error = GIT_EUSER;
-			}
-		}
-
-		git_reference_free(ref);
-
-		/* check if the callback has cancelled iteration */
-		if (error == GIT_EUSER)
-			break;
 	}
 
-	if (error == GIT_ITEROVER)
-		error = 0;
-
-	git_reference_iterator_free(iter);
 	return error;
 }
 
+int git_branch_iterator_new(
+	git_branch_iterator **out,
+	git_repository *repo,
+	unsigned int list_flags)
+{
+	branch_iter *iter;
+
+	iter = git__calloc(1, sizeof(branch_iter));
+	GITERR_CHECK_ALLOC(iter);
+
+	iter->flags = list_flags;
+
+	if (git_reference_iterator_new(&iter->iter, repo) < 0) {
+		git__free(iter);
+		return -1;
+	}
+
+	*out = (git_branch_iterator *) iter;
+
+	return 0;
+}
+
+void git_branch_iterator_free(git_branch_iterator *_iter)
+{
+	branch_iter *iter = (branch_iter *) _iter;
+
+	git_reference_iterator_free(iter->iter);
+	git__free(iter);
+}
+
 int git_branch_move(
 	git_reference **out,
 	git_reference *branch,

tests-clar/online/push.c

@@ -163,18 +163,6 @@ static void verify_refs(git_remote *remote, expected_ref expected_refs[], size_t
 	git_vector_free(&actual_refs);
 }
 
-static int tracking_branch_list_cb(const char *branch_name, git_branch_t branch_type, void *payload)
-{
-	git_vector *tracking = (git_vector *)payload;
-
-	if (branch_type == GIT_BRANCH_REMOTE)
-		git_vector_insert(tracking, git__strdup(branch_name));
-	else
-		GIT_UNUSED(branch_name);
-
-	return 0;
-}
-
 /**
  * Verifies that after git_push_update_tips(), remote tracking branches have the expected
  * names and oids.
@@ -189,14 +177,24 @@ static void verify_tracking_branches(git_remote *remote, expected_ref expected_r
 	size_t i, j;
 	git_buf msg = GIT_BUF_INIT;
 	git_buf ref_name = GIT_BUF_INIT;
-	git_buf canonical_ref_name = GIT_BUF_INIT;
 	git_vector actual_refs = GIT_VECTOR_INIT;
+	git_branch_iterator *iter;
 	char *actual_ref;
 	git_oid oid;
-	int failed = 0;
+	int failed = 0, error;
+	unsigned int branch_type;
+	git_reference *ref;
 
 	/* Get current remote branches */
-	cl_git_pass(git_branch_foreach(remote->repo, GIT_BRANCH_REMOTE, tracking_branch_list_cb, &actual_refs));
+	cl_git_pass(git_branch_iterator_new(&iter, remote->repo, GIT_BRANCH_REMOTE));
+
+	while ((error = git_branch_next(&ref, &branch_type, iter)) == 0) {
+		cl_assert_equal_i(branch_type, GIT_BRANCH_REMOTE);
+
+		cl_git_pass(git_vector_insert(&actual_refs, git__strdup(git_reference_name(ref))));
+	}
+
+	cl_assert_equal_i(error, GIT_ITEROVER);
 
 	/* Loop through expected refs, make sure they exist */
 	for (i = 0; i < expected_refs_len; i++) {
@@ -212,11 +210,7 @@ static void verify_tracking_branches(git_remote *remote, expected_ref expected_r
 
 		/* Find matching remote branch */
 		git_vector_foreach(&actual_refs, j, actual_ref) {
-
+			if (!strcmp(git_buf_cstr(&ref_name), actual_ref))
-			/* Construct canonical ref name from the actual_ref name */
-			git_buf_clear(&canonical_ref_name);
-			cl_git_pass(git_buf_printf(&canonical_ref_name, "refs/remotes/%s", actual_ref));
-			if (!strcmp(git_buf_cstr(&ref_name), git_buf_cstr(&canonical_ref_name)))
 				break;
 		}
 
@@ -227,7 +221,7 @@ static void verify_tracking_branches(git_remote *remote, expected_ref expected_r
 		}
 
 		/* Make sure tracking branch is at expected commit ID */
-		cl_git_pass(git_reference_name_to_id(&oid, remote->repo, git_buf_cstr(&canonical_ref_name)));
+		cl_git_pass(git_reference_name_to_id(&oid, remote->repo, actual_ref));
 
 		if (git_oid_cmp(expected_refs[i].oid, &oid) != 0) {
 			git_buf_puts(&msg, "Tracking branch commit does not match expected ID.");
@@ -256,7 +250,6 @@ failed:
 
 	git_vector_free(&actual_refs);
 	git_buf_free(&msg);
-	git_buf_free(&canonical_ref_name);
 	git_buf_free(&ref_name);
 	return;
 }

tests-clar/refs/branches/iterator.c

@@ -4,7 +4,7 @@
 static git_repository *repo;
 static git_reference *fake_remote;
 
-void test_refs_branches_foreach__initialize(void)
+void test_refs_branches_iterator__initialize(void)
 {
 	git_oid id;
 
@@ -15,7 +15,7 @@ void test_refs_branches_foreach__initialize(void)
 	cl_git_pass(git_reference_create(&fake_remote, repo, "refs/remotes/nulltoken/master", &id, 0));
 }
 
-void test_refs_branches_foreach__cleanup(void)
+void test_refs_branches_iterator__cleanup(void)
 {
 	git_reference_free(fake_remote);
 	fake_remote = NULL;
@@ -28,39 +28,35 @@ void test_refs_branches_foreach__cleanup(void)
 	cl_git_sandbox_cleanup();
 }
 
-static int count_branch_list_cb(const char *branch_name, git_branch_t branch_type, void *payload)
-{
-	int *count;
-
-	GIT_UNUSED(branch_type);
-	GIT_UNUSED(branch_name);
-
-	count = (int *)payload;
-	(*count)++;
-
-	return 0;
-}
-
 static void assert_retrieval(unsigned int flags, unsigned int expected_count)
 {
-	int count = 0;
+	git_branch_iterator *iter;
+	git_reference *ref;
+	int count = 0, error;
+	unsigned int type;
 
-	cl_git_pass(git_branch_foreach(repo, flags, count_branch_list_cb, &count));
+	cl_git_pass(git_branch_iterator_new(&iter, repo, flags));
+	while ((error = git_branch_next(&ref, &type, iter)) == 0) {
+		count++;
+		git_reference_free(ref);
+	}
 
+	git_branch_iterator_free(iter);
+	cl_assert_equal_i(error, GIT_ITEROVER);
 	cl_assert_equal_i(expected_count, count);
 }
 
-void test_refs_branches_foreach__retrieve_all_branches(void)
+void test_refs_branches_iterator__retrieve_all_branches(void)
 {
 	assert_retrieval(GIT_BRANCH_LOCAL | GIT_BRANCH_REMOTE, 14);
 }
 
-void test_refs_branches_foreach__retrieve_remote_branches(void)
+void test_refs_branches_iterator__retrieve_remote_branches(void)
 {
 	assert_retrieval(GIT_BRANCH_REMOTE, 2);
 }
 
-void test_refs_branches_foreach__retrieve_local_branches(void)
+void test_refs_branches_iterator__retrieve_local_branches(void)
 {
 	assert_retrieval(GIT_BRANCH_LOCAL, 12);
 }
@@ -84,21 +80,22 @@ static void assert_branch_has_been_found(struct expectations *findings, const ch
 	cl_fail("expected branch not found in list.");
 }
 
-static int contains_branch_list_cb(const char *branch_name, git_branch_t branch_type, void *payload)
+static void contains_branches(struct expectations exp[], git_branch_iterator *iter)
 {
-	int pos = 0;
+	git_reference *ref;
-	struct expectations *exp;
+	unsigned int type;
+	int error, pos = 0;
 
-	GIT_UNUSED(branch_type);
+	while ((error = git_branch_next(&ref, &type, iter)) == 0) {
+		for (pos = 0; exp[pos].branch_name; ++pos) {
+			if (strcmp(git_reference_shorthand(ref), exp[pos].branch_name) == 0)
+				exp[pos].encounters++;
+		}
 
-	exp = (struct expectations *)payload;
+		git_reference_free(ref);
-
-	for (pos = 0; exp[pos].branch_name; ++pos) {
-		if (strcmp(branch_name, exp[pos].branch_name) == 0)
-			exp[pos].encounters++;
 	}
 
-	return 0;
+	cl_assert_equal_i(error, GIT_ITEROVER);
 }
 
 /*
@@ -106,8 +103,9 @@ static int contains_branch_list_cb(const char *branch_name, git_branch_t branch_
  *  nulltoken/HEAD -> nulltoken/master
  *  nulltoken/master
  */
-void test_refs_branches_foreach__retrieve_remote_symbolic_HEAD_when_present(void)
+void test_refs_branches_iterator__retrieve_remote_symbolic_HEAD_when_present(void)
 {
+	git_branch_iterator *iter;
 	struct expectations exp[] = {
 		{ "nulltoken/HEAD", 0 },
 		{ "nulltoken/master", 0 },
@@ -119,39 +117,17 @@ void test_refs_branches_foreach__retrieve_remote_symbolic_HEAD_when_present(void
 
 	assert_retrieval(GIT_BRANCH_REMOTE, 3);
 
-	cl_git_pass(git_branch_foreach(repo, GIT_BRANCH_REMOTE, contains_branch_list_cb, &exp));
+	cl_git_pass(git_branch_iterator_new(&iter, repo, GIT_BRANCH_REMOTE));
+	contains_branches(exp, iter);
+	git_branch_iterator_free(iter);
 
 	assert_branch_has_been_found(exp, "nulltoken/HEAD");
 	assert_branch_has_been_found(exp, "nulltoken/master");
 }
 
-static int branch_list_interrupt_cb(
+void test_refs_branches_iterator__mix_of_packed_and_loose(void)
-	const char *branch_name, git_branch_t branch_type, void *payload)
-{
-	int *count;
-
-	GIT_UNUSED(branch_type);
-	GIT_UNUSED(branch_name);
-
-	count = (int *)payload;
-	(*count)++;
-
-	return (*count == 5);
-}
-
-void test_refs_branches_foreach__can_cancel(void)
-{
-	int count = 0;
-
-	cl_assert_equal_i(GIT_EUSER,
-		git_branch_foreach(repo, GIT_BRANCH_LOCAL | GIT_BRANCH_REMOTE,
-			branch_list_interrupt_cb, &count));
-
-	cl_assert_equal_i(5, count);
-}
-
-void test_refs_branches_foreach__mix_of_packed_and_loose(void)
 {
+	git_branch_iterator *iter;
 	struct expectations exp[] = {
 		{ "master", 0 },
 		{ "origin/HEAD", 0 },
@@ -163,8 +139,10 @@ void test_refs_branches_foreach__mix_of_packed_and_loose(void)
 
 	r2 = cl_git_sandbox_init("testrepo2");
 
-	cl_git_pass(git_branch_foreach(r2, GIT_BRANCH_LOCAL | GIT_BRANCH_REMOTE,
+	cl_git_pass(git_branch_iterator_new(&iter, r2, GIT_BRANCH_LOCAL | GIT_BRANCH_REMOTE));
-		contains_branch_list_cb, &exp));
+	contains_branches(exp, iter);
+
+	git_branch_iterator_free(iter);
 
 	assert_branch_has_been_found(exp, "master");
 	assert_branch_has_been_found(exp, "origin/HEAD");