Re: [PATCH 02/18] config: Introduce functions to write non-standard file

From

Jeff King <peff@peff.net>

Date

Jul 19, 2011, 19:55 UTC

Message-ID

<20110719195504.GA3957@sigill.intra.peff.net>

In-Reply-To

<1311095876-3098-3-git-send-email-artagnon@gmail.com>

On Tue, Jul 19, 2011 at 10:47:40PM +0530, Ramkumar Ramachandra wrote:

> -int git_config_set_multivar(const char *key, const char *value,
> -	const char *value_regex, int multi_replace)
> +int git_config_set_multivar_in_file(const char *key, const char *value,
> +				const char *value_regex, int multi_replace,
> +				const char *filename)
>  {
>  	int fd = -1, in_fd;
>  	int ret;
>  	char *config_filename;
> +	const char *saved_config_filename;
>  	struct lock_file *lock = NULL;
>  
> +	saved_config_filename = config_exclusive_filename;
> +	config_exclusive_filename = filename;
> +
>  	if (config_exclusive_filename)
>  		config_filename = xstrdup(config_exclusive_filename);
>  	else
> @@ -1379,6 +1390,7 @@ out_free:
>  	if (lock)
>  		rollback_lock_file(lock);
>  	free(config_filename);
> +	config_exclusive_filename = saved_config_filename;
>  	return ret;
>  
>  write_err_out:

Is there a need for this "config_exclusive_filename" hackery at all?

I was thinking the result would look more like:

diff --git a/config.c b/config.c
index ee643eb..35be842 100644
--- a/config.c
+++ b/config.c
@@ -1193,19 +1193,14 @@ out_free_ret_1:
  * - the config file is removed and the lock file rename()d to it.
  *
  */
-int git_config_set_multivar(const char *key, const char *value,
+int git_config_set_multivar_in_file(const char *config_filename,
+	const char *key, const char *value,
 	const char *value_regex, int multi_replace)
 {
 	int fd = -1, in_fd;
 	int ret;
-	char *config_filename;
 	struct lock_file *lock = NULL;
 
-	if (config_exclusive_filename)
-		config_filename = xstrdup(config_exclusive_filename);
-	else
-		config_filename = git_pathdup("config");
-
 	/* parse-key returns negative; flip the sign to feed exit(3) */
 	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
 	if (ret)
@@ -1382,7 +1377,6 @@ int git_config_set_multivar(const char *key, const char *value,
 out_free:
 	if (lock)
 		rollback_lock_file(lock);
-	free(config_filename);
 	return ret;
 
 write_err_out:
@@ -1391,6 +1385,24 @@ write_err_out:
 
 }
 
+int git_config_set_multivar(const char *key, const char *value,
+			    const char *value_regex, int multi_replace)
+{
+	char *config_filename;
+	int r;
+
+	if (config_exclusive_filename)
+		config_filename = xstrdup(config_exclusive_filename);
+	else
+		config_filename = git_pathdup("config");
+
+	r = git_config_set_multivar_in_file(config_filename, key, value,
+					    value_regex, multi_replace);
+
+	free(config_filename);
+	return r;
+}
+
 static int section_name_match (const char *buf, const char *name)
 {
 	int i = 0, j = 0, dot = 0;

Which is a bit cleaner to read, IMHO.

BTW, I'm unclear why we bother duplicating the filename in the first
place. It seems like we could go even simpler with:

int git_config_set_multivar(const char *key, const char *value,
                            const char *value_regex, int multi_replace)
{
	char *config_filename;

	if (config_exclusive_filename)
		config_filename = config_exclusive_filename;
	else
		config_filename = git_path("config");

        return git_config_set_multivar_in_file(config_filename, key, value,
                                               value_regex, multi_replace);
}