API review / update for tree.h

8 files changed, 154 insertions, 121 deletions

diff --git a/examples/general.c b/examples/general.c
index 70152d7e0..c4ff21d8b 100644
--- a/examples/general.c
+++ b/examples/general.c
@@ -261,8 +261,8 @@ int main (int argc, char** argv)
   git_tree_lookup(&tree, repo, &oid);
 
   // Getting the count of entries in the tree so you can iterate over them if you want to.
-  int cnt = git_tree_entrycount(tree); // 3
-  printf("tree entries: %d\n", cnt);
+  size_t cnt = git_tree_entrycount(tree); // 3
+  printf("tree entries: %d\n", (int)cnt);
 
   entry = git_tree_entry_byindex(tree, 0);
   printf("Entry name: %s\n", git_tree_entry_name(entry)); // "hello.c"
diff --git a/include/git2/tree.h b/include/git2/tree.h
index 527f81819..d20141052 100644
--- a/include/git2/tree.h
+++ b/include/git2/tree.h
@@ -24,14 +24,15 @@ GIT_BEGIN_DECL
 /**
  * Lookup a tree object from the repository.
  *
- * @param tree pointer to the looked up tree
- * @param repo the repo to use when locating the tree.
- * @param id identity of the tree to locate.
+ * @param out Pointer to the looked up tree
+ * @param repo The repo to use when locating the tree.
+ * @param id Identity of the tree to locate.
  * @return 0 or an error code
  */
-GIT_INLINE(int) git_tree_lookup(git_tree **tree, git_repository *repo, const git_oid *id)
+GIT_INLINE(int) git_tree_lookup(
+	git_tree **out, git_repository *repo, const git_oid *id)
 {
-	return git_object_lookup((git_object **)tree, repo, id, GIT_OBJ_TREE);
+	return git_object_lookup((git_object **)out, repo, id, GIT_OBJ_TREE);
 }
 
 /**
@@ -47,54 +48,31 @@ GIT_INLINE(int) git_tree_lookup(git_tree **tree, git_repository *repo, const git
  * @return 0 or an error code
  */
 GIT_INLINE(int) git_tree_lookup_prefix(
-	git_tree **tree,
+	git_tree **out,
 	git_repository *repo,
 	const git_oid *id,
 	size_t len)
 {
-	return git_object_lookup_prefix((git_object **)tree, repo, id, len, GIT_OBJ_TREE);
+	return git_object_lookup_prefix(
+		(git_object **)out, repo, id, len, GIT_OBJ_TREE);
 }
 
 /**
  * Close an open tree
  *
- * This is a wrapper around git_object_free()
+ * You can no longer use the git_tree pointer after this call.
  *
- * IMPORTANT:
- * It *is* necessary to call this method when you stop
- * using a tree. Failure to do so will cause a memory leak.
+ * IMPORTANT: You MUST call this method when you stop using a tree to
+ * release memory. Failure to do so will cause a memory leak.
  *
- * @param tree the tree to close
+ * @param tree The tree to close
  */
 GIT_INLINE(void) git_tree_free(git_tree *tree)
 {
-	git_object_free((git_object *) tree);
+	git_object_free((git_object *)tree);
 }
 
 /**
- * Free a tree entry
- *
- * IMPORTANT: This function is only needed for tree
- * entries owned by the user, such as the ones returned
- * by `git_tree_entry_dup`.
- *
- * @param entry The entry to free
- */
-GIT_EXTERN(void) git_tree_entry_free(git_tree_entry *entry);
-
-/**
- * Duplicate a tree entry
- *
- * Create a copy of a tree entry. The returned copy is owned
- * by the user, and must be freed manually with
- * `git_tree_entry_free`.
- *
- * @param entry A tree entry to duplicate
- * @return a copy of the original entry
- */
-GIT_EXTERN(git_tree_entry *) git_tree_entry_dup(const git_tree_entry *entry);
-
-/**
  * Get the id of a tree.
  *
  * @param tree a previously loaded tree.
@@ -108,44 +86,87 @@ GIT_EXTERN(const git_oid *) git_tree_id(const git_tree *tree);
  * @param tree a previously loaded tree.
  * @return the number of entries in the tree
  */
-GIT_EXTERN(unsigned int) git_tree_entrycount(const git_tree *tree);
+GIT_EXTERN(size_t) git_tree_entrycount(const git_tree *tree);
 
 /**
  * Lookup a tree entry by its filename
  *
+ * This returns a git_tree_entry that is owned by the git_tree.  You don't
+ * have to free it, but you must not use it after the git_tree is released.
+ *
  * @param tree a previously loaded tree.
  * @param filename the filename of the desired entry
  * @return the tree entry; NULL if not found
  */
-GIT_EXTERN(const git_tree_entry *) git_tree_entry_byname(git_tree *tree, const char *filename);
+GIT_EXTERN(const git_tree_entry *) git_tree_entry_byname(
+	git_tree *tree, const char *filename);
 
 /**
  * Lookup a tree entry by its position in the tree
  *
+ * This returns a git_tree_entry that is owned by the git_tree.  You don't
+ * have to free it, but you must not use it after the git_tree is released.
+ *
  * @param tree a previously loaded tree.
  * @param idx the position in the entry list
  * @return the tree entry; NULL if not found
  */
-GIT_EXTERN(const git_tree_entry *) git_tree_entry_byindex(git_tree *tree, size_t idx);
+GIT_EXTERN(const git_tree_entry *) git_tree_entry_byindex(
+	git_tree *tree, size_t idx);
 
 /**
  * Lookup a tree entry by SHA value.
  *
+ * This returns a git_tree_entry that is owned by the git_tree.  You don't
+ * have to free it, but you must not use it after the git_tree is released.
+ *
  * Warning: this must examine every entry in the tree, so it is not fast.
  *
  * @param tree a previously loaded tree.
  * @param oid the sha being looked for
  * @return the tree entry; NULL if not found
  */
-GIT_EXTERN(const git_tree_entry *) git_tree_entry_byoid(git_tree *tree, const git_oid *oid);
+GIT_EXTERN(const git_tree_entry *) git_tree_entry_byoid(
+	git_tree *tree, const git_oid *oid);
 
 /**
- * Get the UNIX file attributes of a tree entry
+ * Retrieve a tree entry contained in a tree or in any of its subtrees,
+ * given its relative path.
  *
- * @param entry a tree entry
- * @return filemode as an integer
+ * Unlike the other lookup functions, the returned tree entry is owned by
+ * the user and must be freed explicitly with `git_tree_entry_free()`.
+ *
+ * @param out Pointer where to store the tree entry
+ * @param root Previously loaded tree which is the root of the relative path
+ * @param subtree_path Path to the contained entry
+ * @return 0 on success; GIT_ENOTFOUND if the path does not exist
  */
-GIT_EXTERN(git_filemode_t) git_tree_entry_filemode(const git_tree_entry *entry);
+GIT_EXTERN(int) git_tree_entry_bypath(
+	git_tree_entry **out,
+	git_tree *root,
+	const char *path);
+
+/**
+ * Duplicate a tree entry
+ *
+ * Create a copy of a tree entry. The returned copy is owned by the user,
+ * and must be freed explicitly with `git_tree_entry_free()`.
+ *
+ * @param entry A tree entry to duplicate
+ * @return a copy of the original entry or NULL on error (alloc failure)
+ */
+GIT_EXTERN(git_tree_entry *) git_tree_entry_dup(const git_tree_entry *entry);
+
+/**
+ * Free a user-owned tree entry
+ *
+ * IMPORTANT: This function is only needed for tree entries owned by the
+ * user, such as the ones returned by `git_tree_entry_dup()` or
+ * `git_tree_entry_bypath()`.
+ *
+ * @param entry The entry to free
+ */
+GIT_EXTERN(void) git_tree_entry_free(git_tree_entry *entry);
 
 /**
  * Get the filename of a tree entry
@@ -172,8 +193,18 @@ GIT_EXTERN(const git_oid *) git_tree_entry_id(const git_tree_entry *entry);
 GIT_EXTERN(git_otype) git_tree_entry_type(const git_tree_entry *entry);
 
 /**
+ * Get the UNIX file attributes of a tree entry
+ *
+ * @param entry a tree entry
+ * @return filemode as an integer
+ */
+GIT_EXTERN(git_filemode_t) git_tree_entry_filemode(const git_tree_entry *entry);
+
+/**
  * Convert a tree entry to the git_object it points too.
  *
+ * You must call `git_object_free()` on the object when you are done with it.
+ *
  * @param object pointer to the converted object
  * @param repo repository where to lookup the pointed object
  * @param entry a tree entry
@@ -187,21 +218,21 @@ GIT_EXTERN(int) git_tree_entry_to_object(
 /**
  * Create a new tree builder.
  *
- * The tree builder can be used to create or modify
- * trees in memory and write them as tree objects to the
- * database.
+ * The tree builder can be used to create or modify trees in memory and
+ * write them as tree objects to the database.
  *
- * If the `source` parameter is not NULL, the tree builder
- * will be initialized with the entries of the given tree.
+ * If the `source` parameter is not NULL, the tree builder will be
+ * initialized with the entries of the given tree.
  *
- * If the `source` parameter is NULL, the tree builder will
- * have no entries and will have to be filled manually.
+ * If the `source` parameter is NULL, the tree builder will start with no
+ * entries and will have to be filled manually.
  *
- * @param builder_p Pointer where to store the tree builder
+ * @param out Pointer where to store the tree builder
  * @param source Source tree to initialize the builder (optional)
  * @return 0 on success; error code otherwise
  */
-GIT_EXTERN(int) git_treebuilder_create(git_treebuilder **builder_p, const git_tree *source);
+GIT_EXTERN(int) git_treebuilder_create(
+	git_treebuilder **out, const git_tree *source);
 
 /**
  * Clear all the entires in the builder
@@ -231,7 +262,8 @@ GIT_EXTERN(void) git_treebuilder_free(git_treebuilder *bld);
  * @param filename Name of the entry
  * @return pointer to the entry; NULL if not found
  */
-GIT_EXTERN(const git_tree_entry *) git_treebuilder_get(git_treebuilder *bld, const char *filename);
+GIT_EXTERN(const git_tree_entry *) git_treebuilder_get(
+	git_treebuilder *bld, const char *filename);
 
 /**
  * Add or update an entry to the builder
@@ -239,17 +271,17 @@ GIT_EXTERN(const git_tree_entry *) git_treebuilder_get(git_treebuilder *bld, con
  * Insert a new entry for `filename` in the builder with the
  * given attributes.
  *
- * if an entry named `filename` already exists, its attributes
+ * If an entry named `filename` already exists, its attributes
  * will be updated with the given ones.
  *
- * The optional pointer `entry_out` can be used to retrieve a
- * pointer to the newly created/updated entry.
+ * The optional pointer `out` can be used to retrieve a pointer to
+ * the newly created/updated entry.  Pass NULL if you do not need it.
  *
  * No attempt is being made to ensure that the provided oid points
  * to an existing git object in the object database, nor that the
  * attributes make sense regarding the type of the pointed at object.
  *
- * @param entry_out Pointer to store the entry (optional)
+ * @param out Pointer to store the entry (optional)
  * @param bld Tree builder
  * @param filename Filename of the entry
  * @param id SHA1 oid of the entry
@@ -259,7 +291,7 @@ GIT_EXTERN(const git_tree_entry *) git_treebuilder_get(git_treebuilder *bld, con
  * @return 0 or an error code
  */
 GIT_EXTERN(int) git_treebuilder_insert(
-	const git_tree_entry **entry_out,
+	const git_tree_entry **out,
 	git_treebuilder *bld,
 	const char *filename,
 	const git_oid *id,
@@ -271,63 +303,52 @@ GIT_EXTERN(int) git_treebuilder_insert(
  * @param bld Tree builder
  * @param filename Filename of the entry to remove
  */
-GIT_EXTERN(int) git_treebuilder_remove(git_treebuilder *bld, const char *filename);
+GIT_EXTERN(int) git_treebuilder_remove(
+	git_treebuilder *bld, const char *filename);
+
+typedef int (*git_treebuilder_filter_cb)(
+	const git_tree_entry *entry, void *payload);
 
 /**
  * Filter the entries in the tree
  *
- * The `filter` callback will be called for each entry
- * in the tree with a pointer to the entry and the
- * provided `payload`: if the callback returns 1, the
- * entry will be filtered (removed from the builder).
+ * The `filter` callback will be called for each entry in the tree with a
+ * pointer to the entry and the provided `payload`; if the callback returns
+ * non-zero, the entry will be filtered (removed from the builder).
  *
  * @param bld Tree builder
  * @param filter Callback to filter entries
+ * @param payload Extra data to pass to filter
  */
 GIT_EXTERN(void) git_treebuilder_filter(
 	git_treebuilder *bld,
-	int (*filter)(const git_tree_entry *, void *),
+	git_treebuilder_filter_cb filter,
 	void *payload);
 
 /**
  * Write the contents of the tree builder as a tree object
  *
- * The tree builder will be written to the given `repo`, and
- * it's identifying SHA1 hash will be stored in the `oid`
- * pointer.
+ * The tree builder will be written to the given `repo`, and its
+ * identifying SHA1 hash will be stored in the `id` pointer.
  *
- * @param oid Pointer where to store the written OID
- * @param repo Repository where to store the object
+ * @param id Pointer to store the OID of the newly written tree
+ * @param repo Repository in which to store the object
  * @param bld Tree builder to write
  * @return 0 or an error code
  */
-GIT_EXTERN(int) git_treebuilder_write(git_oid *oid, git_repository *repo, git_treebuilder *bld);
+GIT_EXTERN(int) git_treebuilder_write(
+	git_oid *id, git_repository *repo, git_treebuilder *bld);
 
-/**
- * Retrieve a tree entry contained in a tree or in any
- * of its subtrees, given its relative path.
- *
- * The returned tree entry is owned by the user and must
- * be freed manually with `git_tree_entry_free`.
- *
- * @param entry Pointer where to store the tree entry
- * @param root A previously loaded tree which will be the root of the relative path
- * @param subtree_path Path to the contained entry
- * @return 0 on success; GIT_ENOTFOUND if the path does not exist
- */
-GIT_EXTERN(int) git_tree_entry_bypath(
-	git_tree_entry **entry,
-	git_tree *root,
-	const char *path);
 
 /** Callback for the tree traversal method */
-typedef int (*git_treewalk_cb)(const char *root, const git_tree_entry *entry, void *payload);
+typedef int (*git_treewalk_cb)(
+	const char *root, const git_tree_entry *entry, void *payload);
 
 /** Tree traversal modes */
-enum git_treewalk_mode {
+typedef enum {
 	GIT_TREEWALK_PRE = 0, /* Pre-order */
 	GIT_TREEWALK_POST = 1, /* Post-order */
-};
+} git_treewalk_mode;
 
 /**
  * Traverse the entries in a tree and its subtrees in
@@ -344,12 +365,16 @@ enum git_treewalk_mode {
  * walk.
  *
  * @param tree The tree to walk
- * @param callback Function to call on each tree entry
  * @param mode Traversal mode (pre or post-order)
+ * @param callback Function to call on each tree entry
  * @param payload Opaque pointer to be passed on each callback
  * @return 0 or an error code
  */
-GIT_EXTERN(int) git_tree_walk(git_tree *tree, git_treewalk_cb callback, int mode, void *payload);
+GIT_EXTERN(int) git_tree_walk(
+	git_tree *tree,
+	git_treewalk_mode mode,
+	git_treewalk_cb callback,
+	void *payload);
 
 /** @} */
 
diff --git a/src/index.c b/src/index.c
index 350d3632b..971fec9fa 100644
--- a/src/index.c
+++ b/src/index.c
@@ -1603,7 +1603,7 @@ int git_index_read_tree(git_index *index, git_tree *tree)
 {
 	git_index_clear(index);
 
-	return git_tree_walk(tree, read_tree_cb, GIT_TREEWALK_POST, index);
+	return git_tree_walk(tree, GIT_TREEWALK_POST, read_tree_cb, index);
 }
 
 git_repository *git_index_owner(const git_index *index)
diff --git a/src/iterator.c b/src/iterator.c
index ee83a4fda..ec3c08ee7 100644
--- a/src/iterator.c
+++ b/src/iterator.c
@@ -85,7 +85,7 @@ struct tree_iterator_frame {
 	tree_iterator_frame *next, *prev;
 	git_tree *tree;
 	char *start;
-	unsigned int index;
+	size_t index;
 };
 
 typedef struct {
diff --git a/src/notes.c b/src/notes.c
index 75848465e..95ff0170a 100644
--- a/src/notes.c
+++ b/src/notes.c
@@ -19,11 +19,11 @@ static int find_subtree_in_current_level(
 	const char *annotated_object_sha,
 	int fanout)
 {
-	unsigned int i;
+	size_t i;
 	const git_tree_entry *entry;
 
 	*out = NULL;
-	
+
 	if (parent == NULL)
 		return GIT_ENOTFOUND;
 
@@ -34,12 +34,12 @@ static int find_subtree_in_current_level(
 			continue;
 
 		if (S_ISDIR(git_tree_entry_filemode(entry))
-			&& strlen(git_tree_entry_name(entry)) == 2 
+			&& strlen(git_tree_entry_name(entry)) == 2
 			&& !strncmp(git_tree_entry_name(entry), annotated_object_sha + fanout, 2))
 			return git_tree_lookup(out, repo, git_tree_entry_id(entry));
 
 		/* Not a DIR, so do we have an already existing blob? */
-		if (!strcmp(git_tree_entry_name(entry), annotated_object_sha + fanout))				
+		if (!strcmp(git_tree_entry_name(entry), annotated_object_sha + fanout))
 			return GIT_EEXISTS;
 	}
 
@@ -71,7 +71,7 @@ static int find_subtree_r(git_tree **out, git_tree *root,
 
 static int find_blob(git_oid *blob, git_tree *tree, const char *target)
 {
-	unsigned int i;
+	size_t i;
 	const git_tree_entry *entry;
 
 	for (i=0; i<git_tree_entrycount(tree); i++) {
diff --git a/src/pack-objects.c b/src/pack-objects.c
index 5db4bc9ae..e2840a745 100644
--- a/src/pack-objects.c
+++ b/src/pack-objects.c
@@ -1284,7 +1284,7 @@ int git_packbuilder_insert_tree(git_packbuilder *pb, const git_oid *oid)
 	    git_packbuilder_insert(pb, oid, NULL) < 0)
 		return -1;
 
-	if (git_tree_walk(tree, cb_tree_walk, GIT_TREEWALK_PRE, pb) < 0) {
+	if (git_tree_walk(tree, GIT_TREEWALK_PRE, cb_tree_walk, pb) < 0) {
 		git_tree_free(tree);
 		return -1;
 	}
diff --git a/src/tree.c b/src/tree.c
index 6f9838880..252980182 100644
--- a/src/tree.c
+++ b/src/tree.c
@@ -166,6 +166,7 @@ git_tree_entry *git_tree_entry_dup(const git_tree_entry *entry)
 		return NULL;
 
 	memcpy(copy, entry, total_size);
+
 	return copy;
 }
 
@@ -288,10 +289,10 @@ int git_tree__prefix_position(git_tree *tree, const char *path)
 	return at_pos;
 }
 
-unsigned int git_tree_entrycount(const git_tree *tree)
+size_t git_tree_entrycount(const git_tree *tree)
 {
 	assert(tree);
-	return (unsigned int)tree->entries.length;
+	return tree->entries.length;
 }
 
 static int tree_error(const char *str)
@@ -694,7 +695,10 @@ on_error:
 	return -1;
 }
 
-void git_treebuilder_filter(git_treebuilder *bld, int (*filter)(const git_tree_entry *, void *), void *payload)
+void git_treebuilder_filter(
+	git_treebuilder *bld,
+	git_treebuilder_filter_cb filter,
+	void *payload)
 {
 	unsigned int i;
 
@@ -855,23 +859,27 @@ static int tree_walk(
 	return error;
 }
 
-int git_tree_walk(git_tree *tree, git_treewalk_cb callback, int mode, void *payload)
+int git_tree_walk(
+	git_tree *tree,
+	git_treewalk_mode mode,
+	git_treewalk_cb callback,
+	void *payload)
 {
 	int error = 0;
 	git_buf root_path = GIT_BUF_INIT;
 
 	switch (mode) {
-		case GIT_TREEWALK_POST:
-			error = tree_walk(tree, callback, &root_path, payload, false);
-			break;
+	case GIT_TREEWALK_POST:
+		error = tree_walk(tree, callback, &root_path, payload, false);
+		break;
 
-		case GIT_TREEWALK_PRE:
-			error = tree_walk(tree, callback, &root_path, payload, true);
-			break;
+	case GIT_TREEWALK_PRE:
+		error = tree_walk(tree, callback, &root_path, payload, true);
+		break;
 
-		default:
-			giterr_set(GITERR_INVALID, "Invalid walking mode for tree walk");
-			return -1;
+	default:
+		giterr_set(GITERR_INVALID, "Invalid walking mode for tree walk");
+		return -1;
 	}
 
 	git_buf_free(&root_path);
diff --git a/tests-clar/object/tree/walk.c b/tests-clar/object/tree/walk.c
index 58b0bca4c..b7af4924d 100644
--- a/tests-clar/object/tree/walk.c
+++ b/tests-clar/object/tree/walk.c
@@ -38,11 +38,11 @@ void test_object_tree_walk__0(void)
 	cl_git_pass(git_tree_lookup(&tree, g_repo, &id));
 
 	ct = 0;
-	cl_git_pass(git_tree_walk(tree, treewalk_count_cb, GIT_TREEWALK_PRE, &ct));
+	cl_git_pass(git_tree_walk(tree, GIT_TREEWALK_PRE, treewalk_count_cb, &ct));
 	cl_assert_equal_i(3, ct);
 
 	ct = 0;
-	cl_git_pass(git_tree_walk(tree, treewalk_count_cb, GIT_TREEWALK_POST, &ct));
+	cl_git_pass(git_tree_walk(tree, GIT_TREEWALK_POST, treewalk_count_cb, &ct));
 	cl_assert_equal_i(3, ct);
 
 	git_tree_free(tree);
@@ -83,21 +83,21 @@ void test_object_tree_walk__1(void)
 
 	ct = 0;
 	cl_assert_equal_i(
-		GIT_EUSER, git_tree_walk(tree, treewalk_stop_cb, GIT_TREEWALK_PRE, &ct));
+		GIT_EUSER, git_tree_walk(tree, GIT_TREEWALK_PRE, treewalk_stop_cb, &ct));
 	cl_assert_equal_i(2, ct);
 
 	ct = 0;
 	cl_assert_equal_i(
-		GIT_EUSER, git_tree_walk(tree, treewalk_stop_cb, GIT_TREEWALK_POST, &ct));
+		GIT_EUSER, git_tree_walk(tree, GIT_TREEWALK_POST, treewalk_stop_cb, &ct));
 	cl_assert_equal_i(2, ct);
 
 	cl_assert_equal_i(
 		GIT_EUSER, git_tree_walk(
-			tree, treewalk_stop_immediately_cb, GIT_TREEWALK_PRE, NULL));
+			tree, GIT_TREEWALK_PRE, treewalk_stop_immediately_cb, NULL));
 
 	cl_assert_equal_i(
 		GIT_EUSER, git_tree_walk(
-			tree, treewalk_stop_immediately_cb, GIT_TREEWALK_POST, NULL));
+			tree, GIT_TREEWALK_POST, treewalk_stop_immediately_cb, NULL));
 
 	git_tree_free(tree);
 }