Minor code cleanup

2 files changed, 79 insertions, 108 deletions

diff --git a/src/submodule.c b/src/submodule.c
index 24f250aa1..e49f09699 100644
--- a/src/submodule.c
+++ b/src/submodule.c
@@ -92,9 +92,7 @@ static void submodule_cache_free(git_submodule_cache *cache);
 
 static git_config_backend *open_gitmodules(git_submodule_cache *, int gitmod);
 static int get_url_base(git_buf *url, git_repository *repo);
-static int lookup_default_remote(git_remote **remote, git_repository *repo);
 static int lookup_head_remote_key(git_buf *remote_key, git_repository *repo);
-static int lookup_head_remote(git_remote **remote, git_repository *repo);
 static int submodule_get(git_submodule **, git_submodule_cache *, const char *, const char *);
 static int submodule_load_from_config(const git_config_entry *, void *);
 static int submodule_load_from_wd_lite(git_submodule *);
@@ -1797,108 +1795,79 @@ static int submodule_cache_init(git_repository *repo, int cache_refresh)
 	return error;
 }
 
-static int get_url_base(git_buf *url, git_repository *repo)
+/* Lookup name of remote of the local tracking branch HEAD points to */
+static int lookup_head_remote_key(git_buf *remote_name, git_repository *repo)
 {
 	int error;
-	git_remote *remote;
-	error = lookup_default_remote(&remote, repo);
-	const char *url_ptr;
-
-	assert(url && repo);
-	
-	if (!error)	{
-		url_ptr = git_remote_url(remote);
-	} else if (error == GIT_ENOTFOUND) {
-		/* if repository does not have a default remote, use workdir instead */
-		giterr_clear();
-		error = 0;
-		url_ptr = git_repository_workdir(repo);
+	git_reference *head = NULL;
+	git_buf upstream_name = GIT_BUF_INIT;
+
+	/* lookup and dereference HEAD */
+	if ((error = git_repository_head(&head, repo)) < 0)
+		return error;
+
+	/* lookup remote tracking branch of HEAD */
+	if (!(error = git_branch_upstream_name(
+			&upstream_name, repo, git_reference_name(head))))
+	{
+		/* lookup remote of remote tracking branch */
+		error = git_branch_remote_name(remote_name, repo, upstream_name.ptr);
+
+		git_buf_free(&upstream_name);
 	}
-	
-	if (error < 0)
-		goto cleanup;
-		
-	error = git_buf_sets(url, url_ptr);
 
-cleanup:
-	git_remote_free(remote);
+	git_reference_free(head);
 
 	return error;
 }
 
-/**
- * Lookup the remote that is considered the default remote in the current state
- */
-static int lookup_default_remote(git_remote **remote, git_repository *repo)
+/* Lookup the remote of the local tracking branch HEAD points to */
+static int lookup_head_remote(git_remote **remote, git_repository *repo)
 {
 	int error;
-	error = lookup_head_remote(remote, repo);
-
-	assert(remote && repo);
+	git_buf remote_name = GIT_BUF_INIT;
 
-	// if that failed, use 'origin' instead
-	if (error == GIT_ENOTFOUND) {
-		error = git_remote_load(remote, repo, "origin");
-	}
+	/* lookup remote of remote tracking branch name */
+	if (!(error = lookup_head_remote_key(&remote_name, repo)))
+		error = git_remote_load(remote, repo, remote_name.ptr);
 
-	if (error == GIT_ENOTFOUND) {
-		giterr_set(GITERR_SUBMODULE,
-			"Neither HEAD points to a local tracking branch, nor does origin exist");
-	}
+	git_buf_free(&remote_name);
 
 	return error;
 }
 
-/**
- * Lookup name of remote of the local tracking branch HEAD points to
- */
-static int lookup_head_remote_key(git_buf *remote_name, git_repository *repo)
+/* Lookup remote, either from HEAD or fall back on origin */
+static int lookup_default_remote(git_remote **remote, git_repository *repo)
 {
-	int error;
-	git_reference *head = NULL;
-	git_buf upstream_name = GIT_BUF_INIT;
-
-	/* lookup and dereference HEAD */
-	if ((error = git_repository_head(&head, repo) < 0))
-	   goto cleanup;
+	int error = lookup_head_remote(remote, repo);
 
-	/* lookup remote tracking branch of HEAD */
-	if ((error = git_branch_upstream_name(&upstream_name, repo, git_reference_name(head))) < 0)
-		goto cleanup;
+	/* if that failed, use 'origin' instead */
+	if (error == GIT_ENOTFOUND)
+		error = git_remote_load(remote, repo, "origin");
 
-	/* lookup remote of remote tracking branch */
-	if ((error = git_branch_remote_name(remote_name, repo, upstream_name.ptr)) < 0)
-		goto cleanup;
-
-cleanup:
-	git_buf_free(&upstream_name);
-	if (head)
-		git_reference_free(head);
+	if (error == GIT_ENOTFOUND)
+		giterr_set(
+			GITERR_SUBMODULE,
+			"Cannot get default remote for submodule - no local tracking "
+			"branch for HEAD and origin does not exist");
 
 	return error;
 }
 
-/**
- * Lookup the remote of the local tracking branch HEAD points to
- */
-static int lookup_head_remote(git_remote **remote, git_repository *repo)
+static int get_url_base(git_buf *url, git_repository *repo)
 {
 	int error;
-	git_buf remote_name = GIT_BUF_INIT;
-
-	assert(remote && repo);
-
-	/* should be NULL in case of error */
-	*remote = NULL;
-
-	/* lookup remote of remote tracking branch name */
-	if ((error = lookup_head_remote_key(&remote_name, repo)) < 0)
-		goto cleanup;
+	git_remote *remote = NULL;
 
-	error = git_remote_load(remote, repo, remote_name.ptr);
-
-cleanup:
-	git_buf_free(&remote_name);
+	if (!(error = lookup_default_remote(&remote, repo))) {
+		error = git_buf_sets(url, git_remote_url(remote));
+		git_remote_free(remote);
+	}
+	else if (error == GIT_ENOTFOUND) {
+		/* if repository does not have a default remote, use workdir instead */
+		giterr_clear();
+		error = git_buf_sets(url, git_repository_workdir(repo));
+	}
 
 	return error;
 }
diff --git a/tests/submodule/add.c b/tests/submodule/add.c
index 2d51b3d7e..af81713f1 100644
--- a/tests/submodule/add.c
+++ b/tests/submodule/add.c
@@ -5,20 +5,35 @@
 
 static git_repository *g_repo = NULL;
 
-static void assert_submodule_url(const char* name, const char *url);
-
 void test_submodule_add__cleanup(void)
 {
 	cl_git_sandbox_cleanup();
 }
 
+static void assert_submodule_url(const char* name, const char *url)
+{
+	git_config *cfg;
+	const char *s;
+	git_buf key = GIT_BUF_INIT;
+
+	cl_git_pass(git_repository_config(&cfg, g_repo));
+
+	cl_git_pass(git_buf_printf(&key, "submodule.%s.url", name));
+	cl_git_pass(git_config_get_string(&s, cfg, git_buf_cstr(&key)));
+	cl_assert_equal_s(s, url);
+
+	git_config_free(cfg);
+	git_buf_free(&key);
+}
+
 void test_submodule_add__url_absolute(void)
 {
-	g_repo = setup_fixture_submod2();
 	git_submodule *sm;
 
+	g_repo = setup_fixture_submod2();
+
 	/* re-add existing submodule */
-	cl_assert_equal_i(
+	cl_git_fail_with(
 		GIT_EEXISTS,
 		git_submodule_add_setup(NULL, g_repo, "whatever", "sm_unchanged", 1));
 
@@ -49,14 +64,15 @@ void test_submodule_add__url_absolute(void)
 	assert_submodule_url("sm_libgit2b", "https://github.com/libgit2/libgit2.git");
 }
 
-void test_submodule_add__url_relative(void) {
+void test_submodule_add__url_relative(void)
+{
 	git_submodule *sm;
 	git_remote *remote;
-	
+
 	/* default remote url is https://github.com/libgit2/false.git */
 	g_repo = cl_git_sandbox_init("testrepo2");
-	
-	/* make sure we're not defaulting to origin - rename origin -> test_remote */
+
+	/* make sure we don't default to origin - rename origin -> test_remote */
 	cl_git_pass(git_remote_load(&remote, g_repo, "origin"));
 	cl_git_pass(git_remote_rename(remote, "test_remote", NULL, NULL));
 	cl_git_fail(git_remote_load(&remote, g_repo, "origin"));
@@ -66,13 +82,14 @@ void test_submodule_add__url_relative(void) {
 		git_submodule_add_setup(&sm, g_repo, "../TestGitRepository", "TestGitRepository", 1)
 		);
 	git_submodule_free(sm);
-		
+
 	assert_submodule_url("TestGitRepository", "https://github.com/libgit2/TestGitRepository");
 }
 
-void test_submodule_add__url_relative_to_origin(void) {
+void test_submodule_add__url_relative_to_origin(void)
+{
 	git_submodule *sm;
-	
+
 	/* default remote url is https://github.com/libgit2/false.git */
 	g_repo = cl_git_sandbox_init("testrepo2");
 
@@ -80,11 +97,12 @@ void test_submodule_add__url_relative_to_origin(void) {
 		git_submodule_add_setup(&sm, g_repo, "../TestGitRepository", "TestGitRepository", 1)
 		);
 	git_submodule_free(sm);
-		
+
 	assert_submodule_url("TestGitRepository", "https://github.com/libgit2/TestGitRepository");
 }
 
-void test_submodule_add__url_relative_to_workdir(void) {
+void test_submodule_add__url_relative_to_workdir(void)
+{
 	git_submodule *sm;
 
 	/* In this repo, HEAD (master) has no remote tracking branc h*/
@@ -94,22 +112,6 @@ void test_submodule_add__url_relative_to_workdir(void) {
 		git_submodule_add_setup(&sm, g_repo, "./", "TestGitRepository", 1)
 		);
 	git_submodule_free(sm);
-		
-	assert_submodule_url("TestGitRepository", git_repository_workdir(g_repo));
-}
-
-static void assert_submodule_url(const char* name, const char *url)
-{
-	git_config *cfg;
-	const char *s;
-	git_buf key = GIT_BUF_INIT;
-
-	cl_git_pass(git_repository_config(&cfg, g_repo));
 
-	cl_git_pass(git_buf_printf(&key, "submodule.%s.url", name));
-	cl_git_pass(git_config_get_string(&s, cfg, git_buf_cstr(&key)));
-	cl_assert_equal_s(s, url);
-
-	git_config_free(cfg);
-	git_buf_free(&key);
+	assert_submodule_url("TestGitRepository", git_repository_workdir(g_repo));
 }