Merge pull request #904 from arrbee/better-object-peel

Make git_object_peel a bit smarter

6 files changed, 53 insertions, 74 deletions

diff --git a/include/git2/object.h b/include/git2/object.h
index 722434de..fd6ae95c 100644
--- a/include/git2/object.h
+++ b/include/git2/object.h
@@ -168,11 +168,14 @@ GIT_EXTERN(int) git_object_typeisloose(git_otype type);
 GIT_EXTERN(size_t) git_object__size(git_otype type);
 
 /**
- * Recursively peel an object until an object of the specified
- * type is met
+ * Recursively peel an object until an object of the specified type is met.
  *
- * The retrieved `peeled` object is owned by the repository
- * and should be closed with the `git_object_free` method.
+ * The retrieved `peeled` object is owned by the repository and should be
+ * closed with the `git_object_free` method.
+ *
+ * If you pass `GIT_OBJ_ANY` as the target type, then the object will be
+ * peeled until the type changes (e.g. a tag will be chased until the
+ * referenced object is no longer a tag).
  *
  * @param peeled Pointer to the peeled git_object
  * @param object The object to be processed
diff --git a/include/git2/reset.h b/include/git2/reset.h
index 12517874..cd263fa9 100644
--- a/include/git2/reset.h
+++ b/include/git2/reset.h
@@ -37,7 +37,7 @@ GIT_BEGIN_DECL
  *
  * @return GIT_SUCCESS or an error code
  */
-GIT_EXTERN(int) git_reset(git_repository *repo, const git_object *target, git_reset_type reset_type);
+GIT_EXTERN(int) git_reset(git_repository *repo, git_object *target, git_reset_type reset_type);
 
 /** @} */
 GIT_END_DECL
diff --git a/src/object.c b/src/object.c
index 22777404..5130d97a 100644
--- a/src/object.c
+++ b/src/object.c
@@ -334,6 +334,12 @@ int git_object__resolve_to_type(git_object **obj, git_otype type)
 	return error;
 }
 
+static int peel_error(int error, const char* msg)
+{
+	giterr_set(GITERR_INVALID, "The given object cannot be peeled - %s", msg);
+	return error;
+}
+
 static int dereference_object(git_object **dereferenced, git_object *obj)
 {
 	git_otype type = git_object_type(obj);
@@ -341,48 +347,36 @@ static int dereference_object(git_object **dereferenced, git_object *obj)
 	switch (type) {
 	case GIT_OBJ_COMMIT:
 		return git_commit_tree((git_tree **)dereferenced, (git_commit*)obj);
-		break;
 
 	case GIT_OBJ_TAG:
 		return git_tag_target(dereferenced, (git_tag*)obj);
-		break;
+
+	case GIT_OBJ_BLOB:
+		return peel_error(GIT_ERROR, "cannot dereference blob");
+
+	case GIT_OBJ_TREE:
+		return peel_error(GIT_ERROR, "cannot dereference tree");
 
 	default:
-		return GIT_ENOTFOUND;
-		break;
+		return peel_error(GIT_ENOTFOUND, "unexpected object type encountered");
 	}
 }
 
-static int peel_error(int error, const char* msg)
-{
-	giterr_set(GITERR_INVALID, "The given object cannot be peeled - %s", msg);
-	return error;
-}
-
 int git_object_peel(
-		git_object **peeled,
-		git_object *object,
-		git_otype target_type)
+	git_object **peeled,
+	git_object *object,
+	git_otype target_type)
 {
 	git_object *source, *deref = NULL;
 
-	assert(object);
+	assert(object && peeled);
 
 	if (git_object_type(object) == target_type)
 		return git_object__dup(peeled, object);
 
-	if (target_type == GIT_OBJ_BLOB
-		|| target_type == GIT_OBJ_ANY)
-		return peel_error(GIT_EAMBIGUOUS, "Ambiguous target type");
-
-	if (git_object_type(object) == GIT_OBJ_BLOB)
-		return peel_error(GIT_ERROR, "A blob cannot be dereferenced");
-
 	source = object;
 
-	while (true) {
-		if (dereference_object(&deref, source) < 0)
-			goto cleanup;
+	while (!dereference_object(&deref, source)) {
 
 		if (source != object)
 			git_object_free(source);
@@ -392,13 +386,20 @@ int git_object_peel(
 			return 0;
 		}
 
+		if (target_type == GIT_OBJ_ANY &&
+			git_object_type(deref) != git_object_type(object))
+		{
+			*peeled = deref;
+			return 0;
+		}
+
 		source = deref;
 		deref = NULL;
 	}
 
-cleanup:
 	if (source != object)
 		git_object_free(source);
+
 	git_object_free(deref);
 	return -1;
 }
diff --git a/src/reset.c b/src/reset.c
index 1379f644..f9e16f7c 100644
--- a/src/reset.c
+++ b/src/reset.c
@@ -20,10 +20,9 @@ static int reset_error_invalid(const char *msg)
 
 int git_reset(
 	git_repository *repo,
-	const git_object *target,
+	git_object *target,
 	git_reset_type reset_type)
 {
-	git_otype target_type = GIT_OBJ_BAD;
 	git_object *commit = NULL;
 	git_index *index = NULL;
 	git_tree *tree = NULL;
@@ -38,26 +37,9 @@ int git_reset(
 	if (reset_type == GIT_RESET_MIXED && git_repository_is_bare(repo))
 		return reset_error_invalid("Mixed reset is not allowed in a bare repository.");
 
-	target_type = git_object_type(target);
-
-	switch (target_type)
-	{
-	case GIT_OBJ_TAG:
-		if (git_tag_peel(&commit, (git_tag *)target) < 0)
-			goto cleanup;
-
-		if (git_object_type(commit) != GIT_OBJ_COMMIT) {
-			reset_error_invalid("The given target does not resolve to a commit.");
-			goto cleanup;
-		}
-		break;
-
-	case GIT_OBJ_COMMIT:
-		commit = (git_object *)target;
-		break;
-
-	default:
-		return reset_error_invalid("Only git_tag and git_commit objects are valid targets.");
+	if (git_object_peel(&commit, target, GIT_OBJ_COMMIT) < 0) {
+		reset_error_invalid("The given target does not resolve to a commit");
+		goto cleanup;
 	}
 
 	//TODO: Check for unmerged entries
@@ -93,9 +75,7 @@ int git_reset(
 	error = 0;
 
 cleanup:
-	if (target_type == GIT_OBJ_TAG)
-		git_object_free(commit);
-
+	git_object_free(commit);
 	git_index_free(index);
 	git_tree_free(tree);
 
diff --git a/src/tag.c b/src/tag.c
index 463619f6..6495d470 100644
--- a/src/tag.c
+++ b/src/tag.c
@@ -445,20 +445,5 @@ int git_tag_list(git_strarray *tag_names, git_repository *repo)
 
 int git_tag_peel(git_object **tag_target, git_tag *tag)
 {
-	int error;
-	git_object *target;
-
-	assert(tag_target && tag);
-
-	if (git_tag_target(&target, tag) < 0)
-		return -1;
-
-	if (git_object_type(target) == GIT_OBJ_TAG) {
-		error = git_tag_peel(tag_target, (git_tag *)target);
-		git_object_free(target);
-		return error;
-	}
-
-	*tag_target = target;
-	return 0;
+	return git_object_peel(tag_target, (git_object *)tag, GIT_OBJ_ANY);
 }
diff --git a/tests-clar/object/peel.c b/tests-clar/object/peel.c
index f6d2a776..f4ea1eb0 100644
--- a/tests-clar/object/peel.c
+++ b/tests-clar/object/peel.c
@@ -65,7 +65,7 @@ void test_object_peel__can_peel_a_commit(void)
 
 void test_object_peel__cannot_peel_a_tree(void)
 {
-	assert_peel_error(GIT_EAMBIGUOUS, "53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_BLOB);
+	assert_peel_error(GIT_ERROR, "53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_BLOB);
 }
 
 void test_object_peel__cannot_peel_a_blob(void)
@@ -73,7 +73,17 @@ void test_object_peel__cannot_peel_a_blob(void)
 	assert_peel_error(GIT_ERROR, "0266163a49e280c4f5ed1e08facd36a2bd716bcf", GIT_OBJ_COMMIT);
 }
 
-void test_object_peel__cannot_target_any_object(void)
+void test_object_peel__target_any_object_for_type_change(void)
 {
-	assert_peel_error(GIT_EAMBIGUOUS, "e90810b8df3e80c413d903f631643c716887138d", GIT_OBJ_ANY);
+	/* tag to commit */
+	assert_peel("e90810b8df3e80c413d903f631643c716887138d", "7b4384978d2493e851f9cca7858815fac9b10980", GIT_OBJ_ANY);
+
+	/* commit to tree */
+	assert_peel("53fc32d17276939fc79ed05badaef2db09990016", "e90810b8df3e80c413d903f631643c716887138d", GIT_OBJ_ANY);
+
+	/* fail to peel tree */
+	assert_peel_error(GIT_ERROR, "53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_ANY);
+
+	/* fail to peel blob */
+	assert_peel_error(GIT_ERROR, "0266163a49e280c4f5ed1e08facd36a2bd716bcf", GIT_OBJ_ANY);
 }