Merge pull request #2718 from libgit2/cmn/peeling-errors

peel: reject bad queries with EPEEL

7 files changed, 80 insertions, 32 deletions

diff --git a/include/git2/errors.h b/include/git2/errors.h
index b33118e02..9b4cc7c00 100644
--- a/include/git2/errors.h
+++ b/include/git2/errors.h
@@ -44,6 +44,7 @@ typedef enum {
 	GIT_EAUTH           = -16,      /**< Authentication error */
 	GIT_ECERTIFICATE    = -17,      /**< Server certificate is invalid */
 	GIT_EAPPLIED        = -18,	/**< Patch/merge has already been applied */
+	GIT_EPEEL           = -19,      /**< The requested peel operation is not possible */
 
 	GIT_PASSTHROUGH     = -30,	/**< Internal only */
 	GIT_ITEROVER        = -31,	/**< Signals end of iteration with iterator */
diff --git a/include/git2/object.h b/include/git2/object.h
index 9b13d824e..a798c9dc3 100644
--- a/include/git2/object.h
+++ b/include/git2/object.h
@@ -202,18 +202,25 @@ GIT_EXTERN(size_t) git_object__size(git_otype type);
 /**
  * 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.
+ * If the query cannot be satisfied due to the object model,
+ * GIT_EINVALIDSPEC will be returned (e.g. trying to peel a blob to a
+ * tree).
  *
- * 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).
+ * If you pass `GIT_OBJ_ANY` as the target type, then the object will
+ * be peeled until the type changes. A tag will be peeled until the
+ * referenced object is no longer a tag, and a commit will be peeled
+ * to a tree. Any other object type will return GIT_EINVALIDSPEC.
+ *
+ * If peeling a tag we discover an object which cannot be peeled to
+ * the target type due to the object model, GIT_EPEEL will be
+ * returned.
+ *
+ * You must free the returned object.
  *
  * @param peeled Pointer to the peeled git_object
  * @param object The object to be processed
- * @param target_type The type of the requested object (GIT_OBJ_COMMIT,
- * GIT_OBJ_TAG, GIT_OBJ_TREE, GIT_OBJ_BLOB or GIT_OBJ_ANY).
- * @return 0 on success, GIT_EAMBIGUOUS, GIT_ENOTFOUND or an error code
+ * @param target_type The type of the requested object (a GIT_OBJ_ value)
+ * @return 0 on success, GIT_EINVALIDSPEC, GIT_EPEEL, or an error code
  */
 GIT_EXTERN(int) git_object_peel(
 	git_object **peeled,
diff --git a/src/object.c b/src/object.c
index 93068b85f..1073559fd 100644
--- a/src/object.c
+++ b/src/object.c
@@ -277,10 +277,8 @@ static int dereference_object(git_object **dereferenced, git_object *obj)
 		return git_tag_target(dereferenced, (git_tag*)obj);
 
 	case GIT_OBJ_BLOB:
-		return GIT_ENOTFOUND;
-
 	case GIT_OBJ_TREE:
-		return GIT_EAMBIGUOUS;
+		return GIT_EPEEL;
 
 	default:
 		return GIT_EINVALIDSPEC;
@@ -303,6 +301,32 @@ static int peel_error(int error, const git_oid *oid, git_otype type)
 	return error;
 }
 
+static int check_type_combination(git_otype type, git_otype target)
+{
+	if (type == target)
+		return 0;
+
+	switch (type) {
+	case GIT_OBJ_BLOB:
+	case GIT_OBJ_TREE:
+		/* a blob or tree can never be peeled to anything but themselves */
+		return GIT_EINVALIDSPEC;
+		break;
+	case GIT_OBJ_COMMIT:
+		/* a commit can only be peeled to a tree */
+		if (target != GIT_OBJ_TREE && target != GIT_OBJ_ANY)
+			return GIT_EINVALIDSPEC;
+		break;
+	case GIT_OBJ_TAG:
+		/* a tag may point to anything, so we let anything through */
+		break;
+	default:
+		return GIT_EINVALIDSPEC;
+	}
+
+	return 0;
+}
+
 int git_object_peel(
 	git_object **peeled,
 	const git_object *object,
@@ -313,15 +337,18 @@ int git_object_peel(
 
 	assert(object && peeled);
 
-	if (git_object_type(object) == target_type)
-		return git_object_dup(peeled, (git_object *)object);
-
 	assert(target_type == GIT_OBJ_TAG ||
 		target_type == GIT_OBJ_COMMIT ||
 		target_type == GIT_OBJ_TREE ||
 		target_type == GIT_OBJ_BLOB ||
 		target_type == GIT_OBJ_ANY);
 
+	if ((error = check_type_combination(git_object_type(object), target_type)) < 0)
+		return peel_error(error, git_object_id(object), target_type);
+
+	if (git_object_type(object) == target_type)
+		return git_object_dup(peeled, (git_object *)object);
+
 	source = (git_object *)object;
 
 	while (!(error = dereference_object(&deref, source))) {
diff --git a/src/revwalk.c b/src/revwalk.c
index 4dca7599a..e44385d48 100644
--- a/src/revwalk.c
+++ b/src/revwalk.c
@@ -124,7 +124,7 @@ static int push_commit(git_revwalk *walk, const git_oid *oid, int uninteresting,
 	error = git_object_peel(&obj, oobj, GIT_OBJ_COMMIT);
 	git_object_free(oobj);
 
-	if (error == GIT_ENOTFOUND) {
+	if (error == GIT_ENOTFOUND || error == GIT_EINVALIDSPEC || error == GIT_EPEEL) {
 		/* If this comes from e.g. push_glob("tags"), ignore this */
 		if (from_glob)
 			return 0;
diff --git a/tests/object/peel.c b/tests/object/peel.c
index 6310388c4..344885f1d 100644
--- a/tests/object/peel.c
+++ b/tests/object/peel.c
@@ -63,28 +63,47 @@ void test_object_peel__peeling_an_object_into_its_own_type_returns_another_insta
 		"0266163a49e280c4f5ed1e08facd36a2bd716bcf", GIT_OBJ_BLOB);
 }
 
-void test_object_peel__can_peel_a_tag(void)
+void test_object_peel__tag(void)
 {
 	assert_peel("7b4384978d2493e851f9cca7858815fac9b10980", GIT_OBJ_COMMIT,
 		"e90810b8df3e80c413d903f631643c716887138d", GIT_OBJ_COMMIT);
 	assert_peel("7b4384978d2493e851f9cca7858815fac9b10980", GIT_OBJ_TREE,
 		"53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_TREE);
+	assert_peel_error(GIT_EPEEL, "7b4384978d2493e851f9cca7858815fac9b10980", GIT_OBJ_BLOB);
+	assert_peel("7b4384978d2493e851f9cca7858815fac9b10980", GIT_OBJ_ANY,
+		    "e90810b8df3e80c413d903f631643c716887138d", GIT_OBJ_COMMIT);
 }
 
-void test_object_peel__can_peel_a_commit(void)
+void test_object_peel__commit(void)
 {
+	assert_peel_error(GIT_EINVALIDSPEC, "e90810b8df3e80c413d903f631643c716887138d", GIT_OBJ_BLOB);
 	assert_peel("e90810b8df3e80c413d903f631643c716887138d", GIT_OBJ_TREE,
-		"53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_TREE);
+		    "53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_TREE);
+	assert_peel("e90810b8df3e80c413d903f631643c716887138d", GIT_OBJ_COMMIT,
+		    "e90810b8df3e80c413d903f631643c716887138d", GIT_OBJ_COMMIT);
+	assert_peel_error(GIT_EINVALIDSPEC, "e90810b8df3e80c413d903f631643c716887138d", GIT_OBJ_TAG);
+	assert_peel("e90810b8df3e80c413d903f631643c716887138d", GIT_OBJ_ANY,
+		    "53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_TREE);
 }
 
-void test_object_peel__cannot_peel_a_tree(void)
+void test_object_peel__tree(void)
 {
-	assert_peel_error(GIT_EAMBIGUOUS, "53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_BLOB);
+	assert_peel_error(GIT_EINVALIDSPEC, "53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_BLOB);
+	assert_peel("53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_TREE,
+		    "53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_TREE);
+	assert_peel_error(GIT_EINVALIDSPEC, "53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_COMMIT);
+	assert_peel_error(GIT_EINVALIDSPEC, "53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_TAG);
+	assert_peel_error(GIT_EINVALIDSPEC, "53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_ANY);
 }
 
-void test_object_peel__cannot_peel_a_blob(void)
+void test_object_peel__blob(void)
 {
-	assert_peel_error(GIT_ENOTFOUND, "0266163a49e280c4f5ed1e08facd36a2bd716bcf", GIT_OBJ_COMMIT);
+	assert_peel("0266163a49e280c4f5ed1e08facd36a2bd716bcf", GIT_OBJ_BLOB,
+		    "0266163a49e280c4f5ed1e08facd36a2bd716bcf", GIT_OBJ_BLOB);
+	assert_peel_error(GIT_EINVALIDSPEC, "0266163a49e280c4f5ed1e08facd36a2bd716bcf", GIT_OBJ_TREE);
+	assert_peel_error(GIT_EINVALIDSPEC, "0266163a49e280c4f5ed1e08facd36a2bd716bcf", GIT_OBJ_COMMIT);
+	assert_peel_error(GIT_EINVALIDSPEC, "0266163a49e280c4f5ed1e08facd36a2bd716bcf", GIT_OBJ_TAG);
+	assert_peel_error(GIT_EINVALIDSPEC, "0266163a49e280c4f5ed1e08facd36a2bd716bcf", GIT_OBJ_ANY);
 }
 
 void test_object_peel__target_any_object_for_type_change(void)
@@ -96,10 +115,4 @@ void test_object_peel__target_any_object_for_type_change(void)
 	/* commit to tree */
 	assert_peel("e90810b8df3e80c413d903f631643c716887138d", GIT_OBJ_ANY,
 		"53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_TREE);
-
-	/* fail to peel tree */
-	assert_peel_error(GIT_EAMBIGUOUS, "53fc32d17276939fc79ed05badaef2db09990016", GIT_OBJ_ANY);
-
-	/* fail to peel blob */
-	assert_peel_error(GIT_ENOTFOUND, "0266163a49e280c4f5ed1e08facd36a2bd716bcf", GIT_OBJ_ANY);
 }
diff --git a/tests/refs/peel.c b/tests/refs/peel.c
index 542694c47..83f6109c0 100644
--- a/tests/refs/peel.c
+++ b/tests/refs/peel.c
@@ -93,7 +93,7 @@ void test_refs_peel__can_peel_a_symbolic_reference(void)
 
 void test_refs_peel__cannot_peel_into_a_non_existing_target(void)
 {
-	assert_peel_error(GIT_ENOTFOUND, "refs/tags/point_to_blob", GIT_OBJ_TAG);
+	assert_peel_error(GIT_EINVALIDSPEC, "refs/tags/point_to_blob", GIT_OBJ_TAG);
 }
 
 void test_refs_peel__can_peel_into_any_non_tag_object(void)
diff --git a/tests/refs/revparse.c b/tests/refs/revparse.c
index a540f389d..94e55bda4 100644
--- a/tests/refs/revparse.c
+++ b/tests/refs/revparse.c
@@ -24,11 +24,11 @@ static void test_object_and_ref_inrepo(
 	error = git_revparse_ext(&obj, &ref, repo, spec);
 
 	if (expected_oid != NULL) {
-		cl_assert_equal_i(0, error);
+		cl_git_pass(error);
 		git_oid_fmt(objstr, git_object_id(obj));
 		cl_assert_equal_s(objstr, expected_oid);
 	} else
-		cl_assert_equal_i(GIT_ENOTFOUND, error);
+		cl_git_fail(error);
 
 	if (assert_reference_retrieval) {
 		if (expected_refname == NULL)
@@ -222,7 +222,7 @@ void test_refs_revparse__to_type(void)
 	assert_invalid_single_spec("wrapped_tag^{trip}");
 	test_object("point_to_blob^{commit}", NULL);
 	cl_assert_equal_i(
-		GIT_EAMBIGUOUS, git_revparse_single(&g_obj, g_repo, "wrapped_tag^{blob}"));
+		GIT_EPEEL, git_revparse_single(&g_obj, g_repo, "wrapped_tag^{blob}"));
 
 	test_object("wrapped_tag^{commit}", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750");
 	test_object("wrapped_tag^{tree}", "944c0f6e4dfa41595e6eb3ceecdb14f50fe18162");