submodule: make `_set_branch()` affect the configuration

3 files changed, 53 insertions, 70 deletions

diff --git a/include/git2/submodule.h b/include/git2/submodule.h
index 63572150d..03b9efe96 100644
--- a/include/git2/submodule.h
+++ b/include/git2/submodule.h
@@ -373,20 +373,17 @@ GIT_EXTERN(int) git_submodule_resolve_url(git_buf *out, git_repository *repo, co
 GIT_EXTERN(const char *) git_submodule_branch(git_submodule *submodule);
 
 /**
- * Set the branch for the submodule.
+ * Set the branch for the submodule in the configuration
  *
- * This sets the branch in memory for the submodule. This will be used for
- * any following submodule actions while this submodule data is in memory.
- *
- * After calling this, you may wish to call `git_submodule_save()` to write
- * the changes back to the ".gitmodules" file and `git_submodule_sync()` to
+ * After calling this, you may wish to call `git_submodule_sync()` to
  * write the changes to the checked out submodule repository.
  *
- * @param submodule Pointer to the submodule object
+ * @param repo the repository to affect
+ * @param name the name of the submodule to configure
  * @param branch Branch that should be used for the submodule
  * @return 0 on success, <0 on failure
  */
-GIT_EXTERN(int) git_submodule_set_branch(git_submodule *submodule, const char *branch);
+GIT_EXTERN(int) git_submodule_set_branch(git_repository *repo, const char *name, const char *branch);
 
 /**
  * Set the URL for the submodule.
diff --git a/src/submodule.c b/src/submodule.c
index 7576ead33..3ad5d3eb3 100644
--- a/src/submodule.c
+++ b/src/submodule.c
@@ -792,19 +792,6 @@ int git_submodule_save(git_submodule *submodule)
 		(error = git_config_file_set_string(mods, key.ptr, submodule->url)) < 0)
 		goto cleanup;
 
-	if ((error = submodule_config_key_trunc_puts(&key, "branch")) < 0)
-		goto cleanup;
-	if (submodule->branch == NULL)
-		error = git_config_file_delete(mods, key.ptr);
-	else
-		error = git_config_file_set_string(mods, key.ptr, submodule->branch);
-	if (error == GIT_ENOTFOUND) {
-		error = 0;
-		giterr_clear();
-	}
-	if (error < 0)
-		goto cleanup;
-
 	/* update internal defaults */
 
 	submodule->ignore_default = submodule->ignore;
@@ -864,25 +851,43 @@ int git_submodule_resolve_url(git_buf *out, git_repository *repo, const char *ur
 	return error;
 }
 
+static int write_var(git_repository *repo, const char *name, const char *var, const char *val)
+{
+	git_buf key = GIT_BUF_INIT;
+	git_config_backend *mods;
+	int error;
+
+	mods = open_gitmodules(repo, GITMODULES_CREATE);
+	if (!mods)
+		return -1;
+
+	if ((error = git_buf_printf(&key, "submodule.%s.%s", name, var)) < 0)
+		goto cleanup;
+
+	if (val)
+		error = git_config_file_set_string(mods, key.ptr, val);
+	else
+		error = git_config_file_delete(mods, key.ptr);
+
+	git_buf_free(&key);
+
+cleanup:
+	git_config_file_free(mods);
+	return error;
+}
+
 const char *git_submodule_branch(git_submodule *submodule)
 {
 	assert(submodule);
 	return submodule->branch;
 }
 
-int git_submodule_set_branch(git_submodule *submodule, const char *branch)
+int git_submodule_set_branch(git_repository *repo, const char *name, const char *branch)
 {
-	assert(submodule);
 
-	git__free(submodule->branch);
-	submodule->branch = NULL;
-
-	if (branch != NULL) {
-		submodule->branch = git__strdup(branch);
-		GITERR_CHECK_ALLOC(submodule->branch);
-	}
+	assert(repo && name);
 
-	return 0;
+	return write_var(repo, name, "branch", branch);
 }
 
 int git_submodule_set_url(git_submodule *submodule, const char *url)
@@ -945,31 +950,6 @@ git_submodule_ignore_t git_submodule_ignore(git_submodule *submodule)
 		GIT_SUBMODULE_IGNORE_NONE : submodule->ignore;
 }
 
-static int write_var(git_repository *repo, const char *name, const char *var, const char *val)
-{
-	git_buf key = GIT_BUF_INIT;
-	git_config_backend *mods;
-	int error;
-
-	mods = open_gitmodules(repo, GITMODULES_CREATE);
-	if (!mods)
-		return -1;
-
-	if ((error = git_buf_printf(&key, "submodule.%s.%s", name, var)) < 0)
-		goto cleanup;
-
-	if (val)
-		error = git_config_file_set_string(mods, key.ptr, val);
-	else
-		error = git_config_file_delete(mods, key.ptr);
-
-	git_buf_free(&key);
-
-cleanup:
-	git_config_file_free(mods);
-	return error;
-}
-
 int git_submodule_set_ignore(git_repository *repo, const char *name, git_submodule_ignore_t ignore)
 {
 	const char *val;
diff --git a/tests/submodule/modify.c b/tests/submodule/modify.c
index e26114783..d0902b3e1 100644
--- a/tests/submodule/modify.c
+++ b/tests/submodule/modify.c
@@ -166,34 +166,47 @@ void test_submodule_modify__set_fetch_recurse_submodules(void)
 	git_submodule_free(sm);
 }
 
+void test_submodule_modify__set_branch(void)
+{
+	git_submodule *sm;
+
+	cl_git_pass(git_submodule_lookup(&sm, g_repo, "sm_changed_head"));
+	cl_assert(git_submodule_branch(sm) == NULL);
+	git_submodule_free(sm);
+
+	cl_git_pass(git_submodule_set_branch(g_repo, "sm_changed_head", SM_LIBGIT2_BRANCH));
+	cl_git_pass(git_submodule_lookup(&sm, g_repo, "sm_changed_head"));
+	cl_assert_equal_s(SM_LIBGIT2_BRANCH, git_submodule_branch(sm));
+	git_submodule_free(sm);
+
+	cl_git_pass(git_submodule_set_branch(g_repo, "sm_changed_head", NULL));
+	cl_git_pass(git_submodule_lookup(&sm, g_repo, "sm_changed_head"));
+	cl_assert(git_submodule_branch(sm) == NULL);
+	git_submodule_free(sm);
+}
+
 void test_submodule_modify__edit_and_save(void)
 {
 	git_submodule *sm1, *sm2;
-	char *old_url, *old_branch;
+	char *old_url;
 	git_repository *r2;
 
 	cl_git_pass(git_submodule_lookup(&sm1, g_repo, "sm_changed_head"));
 
 	old_url = git__strdup(git_submodule_url(sm1));
-	old_branch = NULL;
 
 	/* modify properties of submodule */
 	cl_git_pass(git_submodule_set_url(sm1, SM_LIBGIT2_URL));
-	cl_git_pass(git_submodule_set_branch(sm1, SM_LIBGIT2_BRANCH));
 	cl_assert_equal_s(SM_LIBGIT2_URL, git_submodule_url(sm1));
-	cl_assert_equal_s(SM_LIBGIT2_BRANCH, git_submodule_branch(sm1));
 
 	/* revert without saving (and confirm setters return old value) */
 	cl_git_pass(git_submodule_set_url(sm1, old_url));
-	cl_git_pass(git_submodule_set_branch(sm1, old_branch));
 
 	/* check that revert was successful */
 	cl_assert_equal_s(old_url, git_submodule_url(sm1));
-	cl_assert_equal_s(old_branch, git_submodule_branch(sm1));
 
 	/* modify properties of submodule (again) */
 	cl_git_pass(git_submodule_set_url(sm1, SM_LIBGIT2_URL));
-	cl_git_pass(git_submodule_set_branch(sm1, SM_LIBGIT2_BRANCH));
 
 	/* call save */
 	cl_git_pass(git_submodule_save(sm1));
@@ -202,13 +215,6 @@ void test_submodule_modify__edit_and_save(void)
 	cl_git_pass(git_submodule_reload(sm1, 0));
 
 	cl_assert_equal_s(SM_LIBGIT2_URL, git_submodule_url(sm1));
-	cl_assert_equal_s(SM_LIBGIT2_BRANCH, git_submodule_branch(sm1));
-
-	/* unset branch again and verify that the property is deleted in config */
-	cl_git_pass(git_submodule_set_branch(sm1, NULL));
-	cl_git_pass(git_submodule_save(sm1));
-	cl_git_pass(git_submodule_reload(sm1, 0));
-	cl_assert_equal_s(NULL, git_submodule_branch(sm1));
 
 	/* open a second copy of the repo and compare submodule */
 	cl_git_pass(git_repository_open(&r2, "submod2"));