Fix saving remotes with several fetch/push ref specs.

At some moment git_config_delete_entry lost the ability to delete one entry of
a multivar configuration. The moment you had more than one fetch or push
ref spec for a remote you will not be able to save that remote anymore. The
changes in network::remote::remotes::save show that problem.

I needed to create a new git_config_delete_multivar because I was not able to
remove one or several entries of a multivar config with the current API.
Several tries modifying how git_config_set_multivar(..., NULL) behaved were
not successful.

git_config_delete_multivar is very similar to git_config_set_multivar, and
delegates into config_delete_multivar of config_file. This function search
for the cvar_t that will be deleted, storing them in a temporal array, and
rebuilding the linked list. After calling config_write to delete the entries,
the cvar_t stored in the temporal array are freed.

There is a little fix in config_write, it avoids an infinite loop when using
a regular expression (case for the multivars). This error was found by the
test network::remote::remotes::tagopt.

include/git2/config.h

@@ -435,6 +435,17 @@ GIT_EXTERN(int) git_config_set_multivar(git_config *cfg, const char *name, const
 GIT_EXTERN(int) git_config_delete_entry(git_config *cfg, const char *name);
 
 /**
+ * Deletes one or several entries from a multivar in the local config file.
+ *
+ * @param cfg where to look for the variables
+ * @param name the variable's name
+ * @param regexp a regular expression to indicate which values to delete
+ *
+ * @return 0 or an error code
+ */
+GIT_EXTERN(int) git_config_delete_multivar(git_config *cfg, const char *name, const char *regexp);
+
+/**
  * Perform an operation on each config variable.
  *
  * The callback receives the normalized name and value of each variable

include/git2/sys/config.h

@@ -61,6 +61,7 @@ struct git_config_backend {
 	int (*set)(struct git_config_backend *, const char *key, const char *value);
 	int (*set_multivar)(git_config_backend *cfg, const char *name, const char *regexp, const char *value);
 	int (*del)(struct git_config_backend *, const char *key);
+	int (*del_multivar)(struct git_config_backend *, const char *key, const char *regexp);
 	int (*iterator)(git_config_iterator **, struct git_config_backend *);
 	int (*refresh)(struct git_config_backend *);
 	void (*free)(struct git_config_backend *);

src/config.c

@@ -862,6 +862,19 @@ int git_config_set_multivar(git_config *cfg, const char *name, const char *regex
 	return file->set_multivar(file, name, regexp, value);
 }
 
+int git_config_delete_multivar(git_config *cfg, const char *name, const char *regexp)
+{
+	git_config_backend *file;
+	file_internal *internal;
+
+	internal = git_vector_get(&cfg->files, 0);
+	if (!internal || !internal->file)
+		return config_error_nofiles(name);
+	file = internal->file;
+
+	return file->del_multivar(file, name, regexp);
+}
+
 int git_config_next(git_config_entry **entry, git_config_iterator *iter)
 {
 	return iter->next(entry, iter);

src/config_file.c

@@ -120,6 +120,18 @@ static void cvar_free(cvar_t *var)
 	git__free(var);
 }
 
+static int cvar_length(cvar_t *var)
+{
+	int length = 0;
+
+	while (var) {
+		length += 1;
+		var = var->next;
+	}
+
+	return length;
+}
+
 int git_config_file_normalize_section(char *start, char *end)
 {
 	char *scan;
@@ -531,6 +543,81 @@ static int config_delete(git_config_backend *cfg, const char *name)
 	return result;
 }
 
+static int config_delete_multivar(git_config_backend *cfg, const char *name, const char *regexp)
+{
+	cvar_t *var, *prev = NULL, *new_head = NULL;
+	cvar_t **to_delete;
+	int to_delete_idx;
+	diskfile_backend *b = (diskfile_backend *)cfg;
+	char *key;
+	regex_t preg;
+	int result;
+	khiter_t pos;
+
+	if ((result = git_config__normalize_name(name, &key)) < 0)
+		return result;
+
+	pos = git_strmap_lookup_index(b->values, key);
+
+	if (!git_strmap_valid_index(b->values, pos)) {
+		giterr_set(GITERR_CONFIG, "Could not find key '%s' to delete", name);
+		git__free(key);
+		return GIT_ENOTFOUND;
+	}
+
+	var = git_strmap_value_at(b->values, pos);
+
+	result = regcomp(&preg, regexp, REG_EXTENDED);
+	if (result < 0) {
+		git__free(key);
+		giterr_set_regex(&preg, result);
+		regfree(&preg);
+		return -1;
+	}
+
+	to_delete = git__calloc(cvar_length(var), sizeof(cvar_t *));
+	GITERR_CHECK_ALLOC(to_delete);
+	to_delete_idx = 0;
+
+	for (;;) {
+		cvar_t *var_next = var->next;
+
+		if (regexec(&preg, var->entry->value, 0, NULL, 0) == 0) {
+			// If we are past the head, reattach previous node to next one,
+			// otherwise set the new head for the strmap.
+			if (prev != NULL) {
+				prev->next = var_next;
+			} else {
+				new_head = var_next;
+			}
+
+			to_delete[to_delete_idx++] = var;
+		} else {
+			prev = var;
+		}
+
+		if (var_next == NULL)
+			break;
+
+		var = var_next;
+	}
+
+	if (new_head != NULL) {
+		git_strmap_set_value_at(b->values, pos, new_head);
+	} else {
+		git_strmap_delete_at(b->values, pos);
+	}
+
+	if (to_delete_idx > 0)
+		result = config_write(b, key, &preg, NULL);
+
+	while (to_delete_idx-- > 0)
+		cvar_free(to_delete[to_delete_idx]);
+
+	git__free(key);
+	return result;
+}
+
 int git_config_file__ondisk(git_config_backend **out, const char *path)
 {
 	diskfile_backend *backend;
@@ -548,6 +635,7 @@ int git_config_file__ondisk(git_config_backend **out, const char *path)
 	backend->parent.set = config_set;
 	backend->parent.set_multivar = config_set_multivar;
 	backend->parent.del = config_delete;
+	backend->parent.del_multivar = config_delete_multivar;
 	backend->parent.iterator = config_iterator_new;
 	backend->parent.refresh = config_refresh;
 	backend->parent.free = backend_free;
@@ -1214,7 +1302,7 @@ static int config_write(diskfile_backend *cfg, const char *key, const regex_t *p
 			}
 
 			/* multiline variable? we need to keep reading lines to match */
-			if (preg != NULL) {
+			if (preg != NULL && section_matches) {
 				data_start = post_start;
 				continue;
 			}

src/remote.c

@@ -365,16 +365,18 @@ static int update_config_refspec(const git_remote *remote, git_config *config, i
 	const char *dir;
 	size_t i;
 	int error = 0;
+	const char *cname;
 
 	push = direction == GIT_DIRECTION_PUSH;
 	dir = push ? "push" : "fetch";
 
 	if (git_buf_printf(&name, "remote.%s.%s", remote->name, dir) < 0)
 		return -1;
+	cname = git_buf_cstr(&name);
 
 	/* Clear out the existing config */
 	while (!error)
-		error = git_config_delete_entry(config, git_buf_cstr(&name));
+		error = git_config_delete_multivar(config, cname, ".*");
 
 	if (error != GIT_ENOTFOUND)
 		return error;
@@ -386,7 +388,7 @@ static int update_config_refspec(const git_remote *remote, git_config *config, i
 			continue;
 
 		if ((error = git_config_set_multivar(
-				config, git_buf_cstr(&name), "", spec->string)) < 0) {
+				config, cname, "", spec->string)) < 0) {
 			goto cleanup;
 		}
 	}

tests-clar/config/multivar.c

@@ -221,3 +221,68 @@ void test_config_multivar__replace_multiple(void)
 
 	git_config_free(cfg);
 }
+
+void test_config_multivar__delete(void)
+{
+	git_config *cfg;
+	int n;
+
+	cl_git_pass(git_config_open_ondisk(&cfg, "config/config11"));
+
+	n = 0;
+	cl_git_pass(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n));
+	cl_assert(n == 2);
+
+	cl_git_pass(git_config_delete_multivar(cfg, _name, "github"));
+
+	n = 0;
+	cl_git_pass(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n));
+	cl_assert(n == 1);
+
+	git_config_free(cfg);
+
+	cl_git_pass(git_config_open_ondisk(&cfg, "config/config11"));
+
+	n = 0;
+	cl_git_pass(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n));
+	cl_assert(n == 1);
+
+	git_config_free(cfg);
+}
+
+void test_config_multivar__delete_multiple(void)
+{
+	git_config *cfg;
+	int n;
+
+	cl_git_pass(git_config_open_ondisk(&cfg, "config/config11"));
+
+	n = 0;
+	cl_git_pass(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n));
+	cl_assert(n == 2);
+
+	cl_git_pass(git_config_delete_multivar(cfg, _name, "git"));
+
+	n = 0;
+	cl_git_fail_with(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n), GIT_ENOTFOUND);
+
+	git_config_free(cfg);
+
+	cl_git_pass(git_config_open_ondisk(&cfg, "config/config11"));
+
+	n = 0;
+	cl_git_fail_with(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n), GIT_ENOTFOUND);
+
+	git_config_free(cfg);
+}
+
+void test_config_multivar__delete_notfound(void)
+{
+	git_config *cfg;
+
+	cl_git_pass(git_config_open_ondisk(&cfg, "config/config11"));
+
+	cl_git_fail_with(git_config_delete_multivar(cfg, "remote.ab.noturl", "git"), GIT_ENOTFOUND);
+
+	git_config_free(cfg);
+}

tests-clar/network/remote/remotes.c

@@ -150,8 +150,10 @@ void test_network_remote_remotes__add_pushspec(void)
 void test_network_remote_remotes__save(void)
 {
 	git_strarray array;
-	const char *fetch_refspec = "refs/heads/*:refs/remotes/upstream/*";
-	const char *push_refspec = "refs/heads/*:refs/heads/*";
+	const char *fetch_refspec1 = "refs/heads/ns1/*:refs/remotes/upstream/ns1/*";
+	const char *fetch_refspec2 = "refs/heads/ns2/*:refs/remotes/upstream/ns2/*";
+	const char *push_refspec1 = "refs/heads/ns1/*:refs/heads/ns1/*";
+	const char *push_refspec2 = "refs/heads/ns2/*:refs/heads/ns2/*";
 
 	git_remote_free(_remote);
 	_remote = NULL;
@@ -160,8 +162,10 @@ void test_network_remote_remotes__save(void)
 	cl_git_pass(git_remote_create(&_remote, _repo, "upstream", "git://github.com/libgit2/libgit2"));
 	git_remote_clear_refspecs(_remote);
 
-	cl_git_pass(git_remote_add_fetch(_remote, fetch_refspec));
-	cl_git_pass(git_remote_add_push(_remote, push_refspec));
+	cl_git_pass(git_remote_add_fetch(_remote, fetch_refspec1));
+	cl_git_pass(git_remote_add_fetch(_remote, fetch_refspec2));
+	cl_git_pass(git_remote_add_push(_remote, push_refspec1));
+	cl_git_pass(git_remote_add_push(_remote, push_refspec2));
 	cl_git_pass(git_remote_set_pushurl(_remote, "git://github.com/libgit2/libgit2_push"));
 	cl_git_pass(git_remote_save(_remote));
 	git_remote_free(_remote);
@@ -171,16 +175,19 @@ void test_network_remote_remotes__save(void)
 	cl_git_pass(git_remote_load(&_remote, _repo, "upstream"));
 
 	cl_git_pass(git_remote_get_fetch_refspecs(&array, _remote));
-	cl_assert_equal_i(1, (int)array.count);
-	cl_assert_equal_s(fetch_refspec, array.strings[0]);
+	cl_assert_equal_i(2, (int)array.count);
+	cl_assert_equal_s(fetch_refspec1, array.strings[0]);
+	cl_assert_equal_s(fetch_refspec2, array.strings[1]);
 	git_strarray_free(&array);
 
 	cl_git_pass(git_remote_get_push_refspecs(&array, _remote));
-	cl_assert_equal_i(1, (int)array.count);
-	cl_assert_equal_s(push_refspec, array.strings[0]);
+	cl_assert_equal_i(2, (int)array.count);
+	cl_assert_equal_s(push_refspec1, array.strings[0]);
+	cl_assert_equal_s(push_refspec2, array.strings[1]);
+	git_strarray_free(&array);
+
 	cl_assert_equal_s(git_remote_url(_remote), "git://github.com/libgit2/libgit2");
 	cl_assert_equal_s(git_remote_pushurl(_remote), "git://github.com/libgit2/libgit2_push");
-	git_strarray_free(&array);
 
 	/* remove the pushurl again and see if we can save that too */
 	cl_git_pass(git_remote_set_pushurl(_remote, NULL));