Re: [PATCH v1 2/3] config: introduce repo_config_set* functions

[Junio C Hamano <gitster@pobox.com>]

Glen Choo <chooglen@google.com> writes:

> We have git_config_set() that sets a config value for the_repository's
> config file, and repo_config_get* that reads config values from a struct
> repository. Thus, it seems reasonable to have to have
> repo_git_config_set() that can set values for a config file of a struct
> repository.
>
> Implement repo_config_set() and repo_config_set_gently(), which
> take struct repository. However, unlike other instances where struct
> repository is added to a repo_* function, this patch does not change the
> implementations of git_config_set() or git_config_set_gently(); those
> functions use the_repository much deeper in their call chain through
> git_pathdup(). Mark this inconsistency as NEEDSWORK.

Being able to only affect "config" in the_repository->gitdir is less
flexible than being able to affect "config" in repo->gitdir for any
repository is good.  Do we need a similar thing for repo->commondir
as well?

> +/*
> + * Sets a config value in a repository.
> + */
> +int repo_config_set_gently(struct repository *r, const char *key,
> +			   const char *value)
> +{
> +	int ret;
> +	char *path;
> +
> +	path = repo_git_path(r, "config");
> +	ret = git_config_set_in_file_gently(path, key, value);
> +	free(path);
> +	return ret;
> +}
> +
> +void repo_config_set(struct repository *r, const char *key, const char *value)
> +{
> +	char *path;
> +
> +	path = repo_git_path(r, "config");
> +	git_config_set_in_file(path, key, value);
> +	free(path);
> +}

Many questions:

 - What do these do for an existing key?  Add another value?
   Replace existing one?  If the latter, what do we plan to do with
   multi-valued keys?

 - Don't we need an interface to remove, rename, etc.?

 - If we call repo_config_set(repo, "key", "value") and then call
   repo_git_config_string(repo, "key", &value) on the same
   repository, do we read the value back or do we give a stale
   value?

 - A change like this should make existing config_set() that only
   works on the_repository into a thin wrapper, e.g.

	void git_config_set(const char *keyu, const char **value)
	{
		repo_config_set(the_repository, key, value);
	}

   But that is not happening here.  What prevents us from doing so?

Thanks.