refs: Deploy EINVALIDSPEC usage

7 files changed, 115 insertions, 37 deletions

diff --git a/include/git2/refs.h b/include/git2/refs.h
index c92646115..7d047ca49 100644
--- a/include/git2/refs.h
+++ b/include/git2/refs.h
@@ -26,13 +26,13 @@ GIT_BEGIN_DECL
  *
  * The returned reference must be freed by the user.
  *
- * See `git_reference_create_symbolic()` for documentation about valid
- * reference names.
+ * The name will be checked for validity.
+ * See `git_reference_create_symbolic()` for rules about valid names.
  *
  * @param out pointer to the looked-up reference
  * @param repo the repository to look up the reference
  * @param name the long name for the reference (e.g. HEAD, refs/heads/master, refs/tags/v0.1.0, ...)
- * @return 0 or an error code (ENOTFOUND, EINVALIDSPEC)
+ * @return 0 on success, ENOTFOUND, EINVALIDSPEC or an error code.
  */
 GIT_EXTERN(int) git_reference_lookup(git_reference **out, git_repository *repo, const char *name);
 
@@ -43,11 +43,13 @@ GIT_EXTERN(int) git_reference_lookup(git_reference **out, git_repository *repo,
  * through to the object id that it refers to.  This avoids having to
  * allocate or free any `git_reference` objects for simple situations.
  *
+ * The name will be checked for validity.
+ * See `git_reference_create_symbolic()` for rules about valid names.
+ *
  * @param out Pointer to oid to be filled in
  * @param repo The repository in which to look up the reference
  * @param name The long name for the reference
- * @return 0 on success, -1 if name could not be resolved (EINVALIDSPEC,
- * ENOTFOUND, etc)
+ * @return 0 on success, ENOTFOUND, EINVALIDSPEC or an error code.
  */
 GIT_EXTERN(int) git_reference_name_to_id(
 	git_oid *out, git_repository *repo, const char *name);
@@ -79,7 +81,7 @@ GIT_EXTERN(int) git_reference_name_to_id(
  * @param name The name of the reference
  * @param target The target of the reference
  * @param force Overwrite existing references
- * @return 0 or an error code (EEXISTS, EINVALIDSPEC)
+ * @return 0 on success, EEXISTS, EINVALIDSPEC or an error code
  */
 GIT_EXTERN(int) git_reference_symbolic_create(git_reference **out, git_repository *repo, const char *name, const char *target, int force);
 
@@ -111,7 +113,7 @@ GIT_EXTERN(int) git_reference_symbolic_create(git_reference **out, git_repositor
  * @param name The name of the reference
  * @param id The object id pointed to by the reference.
  * @param force Overwrite existing references
- * @return 0 or an error code (EINVALIDSPEC, EEXISTS)
+ * @return 0 on success, EEXISTS, EINVALIDSPEC or an error code
  */
 GIT_EXTERN(int) git_reference_create(git_reference **out, git_repository *repo, const char *name, const git_oid *id, int force);
 
@@ -193,9 +195,12 @@ GIT_EXTERN(git_repository *) git_reference_owner(const git_reference *ref);
  *
  * The reference will be automatically updated in memory and on disk.
  *
+ * The target name will be checked for validity.
+ * See `git_reference_create_symbolic()` for rules about valid names.
+ *
  * @param ref The reference
  * @param target The new target for the reference
- * @return 0 or an error code (EINVALIDSPEC)
+ * @return 0 on success, EINVALIDSPEC or an error code
  */
 GIT_EXTERN(int) git_reference_symbolic_set_target(git_reference *ref, const char *target);
 
@@ -216,8 +221,9 @@ GIT_EXTERN(int) git_reference_set_target(git_reference *ref, const git_oid *id);
  * Rename an existing reference.
  *
  * This method works for both direct and symbolic references.
- * The new name will be checked for validity and may be
- * modified into a normalized form.
+ *
+ * The new name will be checked for validity.
+ * See `git_reference_create_symbolic()` for rules about valid names.
  *
  * The given git_reference will be updated in place.
  *
@@ -234,7 +240,7 @@ GIT_EXTERN(int) git_reference_set_target(git_reference *ref, const git_oid *id);
  * @param ref The reference to rename
  * @param name The new name for the reference
  * @param force Overwrite an existing reference
- * @return 0 or an error code (EINVALIDSPEC, EEXISTS)
+ * @return 0 on success, EINVALIDSPEC, EEXISTS or an error code
  *
  */
 GIT_EXTERN(int) git_reference_rename(git_reference *ref, const char *name, int force);
@@ -446,13 +452,15 @@ typedef enum {
  * Once normalized, if the reference name is valid, it will be returned in
  * the user allocated buffer.
  *
+ * See `git_reference_create_symbolic()` for rules about valid names.
+ *
  * @param buffer_out User allocated buffer to store normalized name
  * @param buffer_size Size of buffer_out
  * @param name Reference name to be checked.
  * @param flags Flags to constrain name validation rules - see the
  *              GIT_REF_FORMAT constants above.
- * @return 0 on success or error code (GIT_EBUFS if buffer is too small, -1
- *         if reference is invalid)
+ * @return 0 on success, GIT_EBUFS if buffer is too small, EINVALIDSPEC
+ * or an error code.
  */
 GIT_EXTERN(int) git_reference_normalize_name(
 	char *buffer_out,
diff --git a/src/refs.c b/src/refs.c
index 61a515c94..85813096b 100644
--- a/src/refs.c
+++ b/src/refs.c
@@ -1215,11 +1215,11 @@ int git_reference_symbolic_create(
 	git_reference *ref = NULL;
 	int error;
 
-	if (git_reference__normalize_name_lax(
+	if ((error = git_reference__normalize_name_lax(
 		normalized,
 		sizeof(normalized),
-		name) < 0)
-			return -1;
+		name)) < 0)
+			return error;
 
 	if ((error = reference_can_write(repo, normalized, NULL, force)) < 0)
 		return error;
@@ -1255,11 +1255,11 @@ int git_reference_create(
 	git_reference *ref = NULL;
 	char normalized[GIT_REFNAME_MAX];
 
-	if (git_reference__normalize_name_lax(
+	if ((error = git_reference__normalize_name_lax(
 		normalized,
 		sizeof(normalized),
-		name) < 0)
-			return -1;
+		name)) < 0)
+			return error;
 
 	if ((error = reference_can_write(repo, normalized, NULL, force)) < 0)
 		return error;
@@ -1330,6 +1330,7 @@ int git_reference_set_target(git_reference *ref, const git_oid *id)
  */
 int git_reference_symbolic_set_target(git_reference *ref, const char *target)
 {
+	int error;
 	char normalized[GIT_REFNAME_MAX];
 
 	if ((ref->flags & GIT_REF_SYMBOLIC) == 0) {
@@ -1338,11 +1339,11 @@ int git_reference_symbolic_set_target(git_reference *ref, const char *target)
 		return -1;
 	}
 
-	if (git_reference__normalize_name_lax(
+	if ((error = git_reference__normalize_name_lax(
 		normalized,
 		sizeof(normalized),
-		target))
-			return -1;
+		target)) < 0)
+			return error;
 
 	git__free(ref->target.symbolic);
 	ref->target.symbolic = git__strdup(normalized);
@@ -1363,12 +1364,12 @@ int git_reference_rename(git_reference *ref, const char *new_name, int force)
 		GIT_REF_FORMAT_ALLOW_ONELEVEL
 		: GIT_REF_FORMAT_NORMAL;
 
-	if (git_reference_normalize_name(
+	if ((result = git_reference_normalize_name(
 		normalized,
 		sizeof(normalized),
 		new_name,
-		normalization_flags) < 0)
-			return -1;
+		normalization_flags)) < 0)
+			return result;
 
 	if ((result = reference_can_write(ref->owner, normalized, ref->name, force)) < 0)
 		return result;
@@ -1645,7 +1646,7 @@ int git_reference__normalize_name(
 	// Inspired from https://github.com/git/git/blob/f06d47e7e0d9db709ee204ed13a8a7486149f494/refs.c#L36-100
 
 	char *current;
-	int segment_len, segments_count = 0, error = -1;
+	int segment_len, segments_count = 0, error = GIT_EINVALIDSPEC;
 	unsigned int process_flags;
 	bool normalize = (buf != NULL);
 	assert(name);
@@ -1677,8 +1678,10 @@ int git_reference__normalize_name(
 				git_buf_truncate(buf,
 					cur_len + segment_len + (segments_count ? 1 : 0));
 
-				if (git_buf_oom(buf))
+				if (git_buf_oom(buf)) {
+					error = -1;
 					goto cleanup;
+				}
 			}
 
 			segments_count++;
@@ -1721,7 +1724,7 @@ int git_reference__normalize_name(
 	error = 0;
 
 cleanup:
-	if (error)
+	if (error == GIT_EINVALIDSPEC)
 		giterr_set(
 			GITERR_REFERENCE,
 			"The given reference name '%s' is not valid", name);
diff --git a/tests-clar/refs/create.c b/tests-clar/refs/create.c
index d22f04939..56c323d8a 100644
--- a/tests-clar/refs/create.c
+++ b/tests-clar/refs/create.c
@@ -149,3 +149,19 @@ void test_refs_create__propagate_eexists(void)
 	error = git_reference_symbolic_create(&ref, g_repo, "HEAD", current_head_target, false);
 	cl_assert(error == GIT_EEXISTS);
 }
+
+void test_refs_create__creating_a_reference_with_an_invalid_name_returns_EINVALIDSPEC(void)
+{
+	git_reference *new_reference;
+	git_oid id;
+
+	const char *name = "refs/heads/inv@{id";
+
+	git_oid_fromstr(&id, current_master_tip);
+
+	cl_assert_equal_i(GIT_EINVALIDSPEC, git_reference_create(
+		&new_reference, g_repo, name, &id, 0));
+
+	cl_assert_equal_i(GIT_EINVALIDSPEC, git_reference_symbolic_create(
+		&new_reference, g_repo, name, current_head_target, 0));
+}
diff --git a/tests-clar/refs/normalize.c b/tests-clar/refs/normalize.c
index a144ef5c0..870a533ca 100644
--- a/tests-clar/refs/normalize.c
+++ b/tests-clar/refs/normalize.c
@@ -21,7 +21,9 @@ static void ensure_refname_invalid(unsigned int flags, const char *input_refname
 {
 	char buffer_out[GIT_REFNAME_MAX];
 
-	cl_git_fail(git_reference_normalize_name(buffer_out, sizeof(buffer_out), input_refname, flags));
+	cl_assert_equal_i(
+		GIT_EINVALIDSPEC,
+		git_reference_normalize_name(buffer_out, sizeof(buffer_out), input_refname, flags));
 }
 
 void test_refs_normalize__can_normalize_a_direct_reference_name(void)
diff --git a/tests-clar/refs/read.c b/tests-clar/refs/read.c
index c10a540c0..aa7f01d57 100644
--- a/tests-clar/refs/read.c
+++ b/tests-clar/refs/read.c
@@ -224,10 +224,14 @@ void test_refs_read__unfound_return_ENOTFOUND(void)
 {
 	git_reference *reference;
 
-	cl_assert_equal_i(GIT_ENOTFOUND, git_reference_lookup(&reference, g_repo, "TEST_MASTER"));
-	cl_assert_equal_i(GIT_ENOTFOUND, git_reference_lookup(&reference, g_repo, "refs/test/master"));
-	cl_assert_equal_i(GIT_ENOTFOUND, git_reference_lookup(&reference, g_repo, "refs/tags/test/master"));
-	cl_assert_equal_i(GIT_ENOTFOUND, git_reference_lookup(&reference, g_repo, "refs/tags/test/farther/master"));
+	cl_assert_equal_i(GIT_ENOTFOUND,
+		git_reference_lookup(&reference, g_repo, "TEST_MASTER"));
+	cl_assert_equal_i(GIT_ENOTFOUND,
+		git_reference_lookup(&reference, g_repo, "refs/test/master"));
+	cl_assert_equal_i(GIT_ENOTFOUND,
+		git_reference_lookup(&reference, g_repo, "refs/tags/test/master"));
+	cl_assert_equal_i(GIT_ENOTFOUND,
+		git_reference_lookup(&reference, g_repo, "refs/tags/test/farther/master"));
 }
 
 static void assert_is_branch(const char *name, bool expected_branchness)
@@ -245,3 +249,15 @@ void test_refs_read__can_determine_if_a_reference_is_a_local_branch(void)
 	assert_is_branch("refs/remotes/test/master", false);
 	assert_is_branch("refs/tags/e90810b", false);
 }
+
+void test_refs_read__invalid_name_returns_EINVALIDSPEC(void)
+{
+	git_reference *reference;
+	git_oid id;
+
+	cl_assert_equal_i(GIT_EINVALIDSPEC,
+		git_reference_lookup(&reference, g_repo, "refs/heads/Inv@{id"));
+
+	cl_assert_equal_i(GIT_EINVALIDSPEC,
+		git_reference_name_to_id(&id, g_repo, "refs/heads/Inv@{id"));
+}
diff --git a/tests-clar/refs/rename.c b/tests-clar/refs/rename.c
index ec5c12507..bfdef15fa 100644
--- a/tests-clar/refs/rename.c
+++ b/tests-clar/refs/rename.c
@@ -180,10 +180,14 @@ void test_refs_rename__invalid_name(void)
 	cl_git_pass(git_reference_lookup(&looked_up_ref, g_repo, packed_test_head_name));
 
 	/* Can not be renamed with an invalid name. */
-	cl_git_fail(git_reference_rename(looked_up_ref, "Hello! I'm a very invalid name.", 0));
-
-	/* Can not be renamed outside of the refs hierarchy. */
-	cl_git_fail(git_reference_rename(looked_up_ref, "i-will-sudo-you", 0));
+	cl_assert_equal_i(
+		GIT_EINVALIDSPEC,
+		git_reference_rename(looked_up_ref, "Hello! I'm a very invalid name.", 0));
+
+	/* Can not be renamed outside of the refs hierarchy
+	 * unless it's ALL_CAPS_AND_UNDERSCORES.
+	 */
+	cl_assert_equal_i(GIT_EINVALIDSPEC, git_reference_rename(looked_up_ref, "i-will-sudo-you", 0));
 
 	/* Failure to rename it hasn't corrupted its state */
 	git_reference_free(looked_up_ref);
diff --git a/tests-clar/refs/update.c b/tests-clar/refs/update.c
new file mode 100644
index 000000000..6c2107ee2
--- /dev/null
+++ b/tests-clar/refs/update.c
@@ -0,0 +1,29 @@
+#include "clar_libgit2.h"
+
+#include "refs.h"
+
+static git_repository *g_repo;
+
+void test_refs_update__initialize(void)
+{
+   g_repo = cl_git_sandbox_init("testrepo.git");
+}
+
+void test_refs_update__cleanup(void)
+{
+   cl_git_sandbox_cleanup();
+}
+
+void test_refs_update__updating_the_target_of_a_symref_with_an_invalid_name_returns_EINVALIDSPEC(void)
+{
+	git_reference *head;
+
+	cl_git_pass(git_reference_lookup(&head, g_repo, GIT_HEAD_FILE));
+
+	cl_assert_equal_i(GIT_REF_SYMBOLIC, git_reference_type(head));
+
+	cl_assert_equal_i(GIT_EINVALIDSPEC, git_reference_symbolic_set_target(
+		head, "refs/heads/inv@{id"));
+
+	git_reference_free(head);
+}