remote: deploy EINVALIDSPEC usage

include/git2/remote.h

@@ -39,30 +39,39 @@ typedef int (*git_remote_rename_problem_cb)(const char *problematic_refspec, voi
  * Create a remote with the default refspecs in memory. You can use
  * this when you have a URL instead of a remote's name.
  *
+ * The name, when provided, will be checked for validity.
+ * See `git_tag_create()` for rules about valid names.
+ *
  * @param out pointer to the new remote object
  * @param repo the associated repository
- * @param name the remote's name
+ * @param name the optional remote's name
  * @param url the remote repository's URL
  * @param fetch the fetch refspec to use for this remote
- * @return 0 or an error code
+ * @return 0, GIT_EINVALIDSPEC or an error code
  */
 GIT_EXTERN(int) git_remote_new(git_remote **out, git_repository *repo, const char *name, const char *url, const char *fetch);
 
 /**
  * Get the information for a particular remote
  *
+ * The name will be checked for validity.
+ * See `git_tag_create()` for rules about valid names.
+ *
  * @param out pointer to the new remote object
  * @param repo the associated repository
  * @param name the remote's name
- * @return 0 or an error code
+ * @return 0, GIT_ENOTFOUND, GIT_EINVALIDSPEC or an error code
  */
 GIT_EXTERN(int) git_remote_load(git_remote **out, git_repository *repo, const char *name);
 
 /**
  * Save a remote to its repository's configuration
  *
+ * One can't save a nameless inmemory remote. Doing so will
+ * result in a GIT_EINVALIDSPEC being returned.
+ *
  * @param remote the remote to save to config
- * @return 0 or an error code
+ * @return 0, GIT_EINVALIDSPEC or an error code
  */
 GIT_EXTERN(int) git_remote_save(const git_remote *remote);
 
@@ -391,12 +400,15 @@ GIT_EXTERN(void) git_remote_set_autotag(
  * All remote-tracking branches and configuration settings
  * for the remote are updated.
  *
+ * The new name will be checked for validity.
+ * See `git_tag_create()` for rules about valid names.
+ *
  * @param remote the remote to rename
  * @param new_name the new name the remote should bear
  * @param callback Optional callback to notify the consumer of fetch refspecs
  * that haven't been automatically updated and need potential manual tweaking.
  * @param payload Additional data to pass to the callback
- * @return 0 or an error code
+ * @return 0, GIT_EINVALIDSPEC or an error code
  */
 GIT_EXTERN(int) git_remote_rename(
 	git_remote *remote,

src/remote.c

@@ -57,6 +57,32 @@ static int download_tags_value(git_remote *remote, git_config *cfg)
 	return error;
 }
 
+static int ensure_remote_name_is_valid(const char *name)
+{
+	git_buf buf = GIT_BUF_INIT;
+	git_refspec refspec;
+	int error = -1;
+
+	if (!name || *name == '\0')
+		goto cleanup;
+
+	git_buf_printf(&buf, "refs/heads/test:refs/remotes/%s/test", name);
+	error = git_refspec__parse(&refspec, git_buf_cstr(&buf), true);
+
+	git_buf_free(&buf);
+	git_refspec__free(&refspec);
+
+cleanup:
+	if (error) {
+		giterr_set(
+			GITERR_CONFIG,
+			"'%s' is not a valid remote name.", name);
+		error = GIT_EINVALIDSPEC;
+	}
+
+	return error;
+}
+
 int git_remote_new(git_remote **out, git_repository *repo, const char *name, const char *url, const char *fetch)
 {
 	git_remote *remote;
@@ -79,6 +105,12 @@ int git_remote_new(git_remote **out, git_repository *repo, const char *name, con
 	GITERR_CHECK_ALLOC(remote->url);
 
 	if (name != NULL) {
+		int error;
+		if ((error = ensure_remote_name_is_valid(name)) < 0) {
+			git_remote_free(remote);
+			return error;
+		}
+
 		remote->name = git__strdup(name);
 		GITERR_CHECK_ALLOC(remote->name);
 	}
@@ -111,6 +143,9 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name)
 
 	assert(out && repo && name);
 
+	if ((error = ensure_remote_name_is_valid(name)) < 0)
+		return error;
+
 	if (git_repository_config__weakptr(&config, repo) < 0)
 		return -1;
 
@@ -212,30 +247,6 @@ cleanup:
 	return error;
 }
 
-static int ensure_remote_name_is_valid(const char *name)
-{
-	git_buf buf = GIT_BUF_INIT;
-	git_refspec refspec;
-	int error = -1;
-
-	if (!name || *name == '\0')
-		goto cleanup;
-
-	git_buf_printf(&buf, "refs/heads/test:refs/remotes/%s/test", name);
-	error = git_refspec__parse(&refspec, git_buf_cstr(&buf), true);
-
-	git_buf_free(&buf);
-	git_refspec__free(&refspec);
-
-cleanup:
-	if (error)
-		giterr_set(
-			GITERR_CONFIG,
-			"'%s' is not a valid remote name.", name);
-
-	return error;
-}
-
 static int update_config_refspec(
 	git_config *config,
 	const char *remote_name,
@@ -279,8 +290,8 @@ int git_remote_save(const git_remote *remote)
 
 	assert(remote);
 
-	if (ensure_remote_name_is_valid(remote->name) < 0)
-		return -1;
+	if ((error = ensure_remote_name_is_valid(remote->name)) < 0)
+		return error;
 
 	if (git_repository_config__weakptr(&config, remote->repo) < 0)
 		return -1;
@@ -958,6 +969,10 @@ int git_remote_list(git_strarray *remotes_list, git_repository *repo)
 int git_remote_add(git_remote **out, git_repository *repo, const char *name, const char *url)
 {
 	git_buf buf = GIT_BUF_INIT;
+	int error;
+
+	if ((error = ensure_remote_name_is_valid(name)) < 0)
+		return error;
 
 	if (git_buf_printf(&buf, "+refs/heads/*:refs/remotes/%s/*", name) < 0)
 		return -1;

tests-clar/network/remoterename.c

@@ -121,7 +121,9 @@ void test_network_remoterename__new_name_can_contain_dots(void)
 
 void test_network_remoterename__new_name_must_conform_to_reference_naming_conventions(void)
 {
-	cl_git_fail(git_remote_rename(_remote, "new@{name", dont_call_me_cb, NULL));
+	cl_assert_equal_i(
+		GIT_EINVALIDSPEC,
+		git_remote_rename(_remote, "new@{name", dont_call_me_cb, NULL));
 }
 
 void test_network_remoterename__renamed_name_is_persisted(void)

tests-clar/network/remotes.c

@@ -202,6 +202,11 @@ void test_network_remotes__loading_a_missing_remote_returns_ENOTFOUND(void)
 	cl_assert_equal_i(GIT_ENOTFOUND, git_remote_load(&_remote, _repo, "just-left-few-minutes-ago"));
 }
 
+void test_network_remotes__loading_with_an_invalid_name_returns_EINVALIDSPEC(void)
+{
+	cl_assert_equal_i(GIT_EINVALIDSPEC, git_remote_load(&_remote, _repo, "Inv@{id"));
+}
+
 /*
  * $ git remote add addtest http://github.com/libgit2/libgit2
  *
@@ -229,8 +234,9 @@ void test_network_remotes__cannot_add_a_nameless_remote(void)
 {
 	git_remote *remote;
 
-	cl_git_fail(git_remote_add(&remote, _repo, NULL, "git://github.com/libgit2/libgit2"));
-	cl_git_fail(git_remote_add(&remote, _repo, "", "git://github.com/libgit2/libgit2"));
+	cl_assert_equal_i(
+		GIT_EINVALIDSPEC,
+		git_remote_add(&remote, _repo, NULL, "git://github.com/libgit2/libgit2"));
 }
 
 void test_network_remotes__cannot_save_a_nameless_remote(void)
@@ -239,13 +245,34 @@ void test_network_remotes__cannot_save_a_nameless_remote(void)
 
 	cl_git_pass(git_remote_new(&remote, _repo, NULL, "git://github.com/libgit2/libgit2", NULL));
 
-	cl_git_fail(git_remote_save(remote));
+	cl_assert_equal_i(GIT_EINVALIDSPEC, git_remote_save(remote));
 	git_remote_free(remote);
+}
 
-	cl_git_pass(git_remote_new(&remote, _repo, "", "git://github.com/libgit2/libgit2", NULL));
+void test_network_remotes__cannot_add_a_remote_with_an_invalid_name(void)
+{
+	git_remote *remote;
 
-	cl_git_fail(git_remote_save(remote));
-	git_remote_free(remote);
+	cl_assert_equal_i(
+		GIT_EINVALIDSPEC,
+		git_remote_add(&remote, _repo, "Inv@{id", "git://github.com/libgit2/libgit2"));
+
+	cl_assert_equal_i(
+		GIT_EINVALIDSPEC,
+		git_remote_add(&remote, _repo, "", "git://github.com/libgit2/libgit2"));
+}
+
+void test_network_remotes__cannot_initialize_a_remote_with_an_invalid_name(void)
+{
+	git_remote *remote;
+
+	cl_assert_equal_i(
+		GIT_EINVALIDSPEC,
+		git_remote_new(&remote, _repo, "Inv@{id", "git://github.com/libgit2/libgit2", NULL));
+
+	cl_assert_equal_i(
+		GIT_EINVALIDSPEC,
+		git_remote_new(&remote, _repo, "", "git://github.com/libgit2/libgit2", NULL));
 }
 
 void test_network_remotes__tagopt(void)