Re: [RFC PATCH 1/1] maintenance: add config option for config-file

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

Matthew Hughes <matthewhughes934@gmail.com> writes:

> This is to allow splitting out this configuration from the global config
> file, e.g.:

I cannot guess what "this" refers to in this sentence.

>     # in ~/.config/git/config
>     [include]
>         path = maintenance.config
>     [maintenance]
>         # use a separate files for reads/writes from
>         # 'git maintenance {un,}register'
>         configFile = ~/.config/git/maintenance.config
>
>     # in ~/.config/git/maintenance.config
>     [maintenance]
>         repo = /path/to/some/repo
>         repo = /path/to/another/repo

You are burdening your readers too heavily.  After reading the above
three times and then trying to guess what you are trying to do for
several minutes, what I am guessing is:

 * maintenance.configFile specifies an additional file to which
   maintenance.repo configuration items are written out when "git
   maintenance register/unregister" works.  

 * "git config" is not affected, so "git config set --global
   --append maintenance.repo foo" would still write into the
   per-user configuration file.

 * Also, the general config API does not pay maintenance.configFile
   at all, so setting it does not affect "git config list", for
   example.

 * You'd need an extra "[include] path = maintenance.config" in the
   configuration file because of the previous point.

Am I following you well so far?  Giving an explanation on your
_intent_, along with the sample configuration, would help your
readers, and I would expect something with a similar degree of
detail as above in the log message.

> My motivation for this is that I track my global config in git, so I'd
> like to avoid changes in there that depend on specific repos/workflows
> that I'm working with.

I am not sure if singling out "maintenance" is the right approach to
solve that issue.  If we had a mechanism to have two per-user
configuration file, where one is read-only (as far as Git is
concerned) which is covered/overlayed with a separate read-write
file, not just "maintenance register/unregister" but all other
things that writes into "git config" would use that overlayed file
without touching the base configuration that is read-only.  Wouldn't
that be closer to what you want?

> Signed-off-by: Matthew Hughes <matthewhughes934@gmail.com>
> ---
>  builtin/gc.c           |  8 ++++++++
>  t/t7900-maintenance.sh | 13 +++++++++++++
>  2 files changed, 21 insertions(+)

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 92c6e7b954..257cceecf6 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -2124,6 +2124,10 @@ static int maintenance_register(int argc, const char **argv, const char *prefix,
>  		usage_with_options(builtin_maintenance_register_usage,
>  				   options);
>  
> +	if (config_file == NULL) {
> +		repo_config_get_pathname(the_repository, "maintenance.configFile", &config_file);
> +	}

 * Comparison with 0 or NULL should be spelled "if (!config_file)"
   (meaning, 'is NULL') or "if (config_file)" (meaning, 'not NULL'),
   in this project.

 * This project omits {} around a single statement block.

 * The function call is overly long. wrap to comfortably fit on
   80-column terminal after getting quoted in an e-mail review twice
   or so, which means ~72 columns is the practical width limit.

	repo_config_get_pathname(the_repository,
        			"maintenance.configFile", &config_file);

> +test_expect_success 'register and unregister config from maintenance.configFile' '
> +	test_when_finished git config --global --unset-all maintenance.configFile &&
> +
> +	git config set --global maintenance.configFile ./maintenance.config &&
> +	git maintenance register &&
> +	pwd >>expect &&

Readers would wonder "To what existing contents is this being
appended?  Do we have something that we care?"  If not, do not use
">>" to mislead them.

Would the output of the pwd command match what "maintenance
register" writes into the file even on Windows?  We often see
breakage between $(pwd) and $PWD and I can never get this right
without looking at past discussions.

> +	git config get --file ./maintenance.config maintenance.repo >actual &&
> +	test_cmp expect actual &&

For this particular case, would it be sufficient to ask

    git config get --all --no-includes --file maintenance.config \
	maintenance.repo

    git config get --all --no-includes --file ./git/config \
	maintenance.repo

and see if the former gives output and the latter does not, or
something?

Making sure the maintenance.repo file gets written is good, but for
your purpose, it is equally if not more important that the base
configuration file is not affected, no?

> +	git maintenance unregister &&
> +	test_must_be_empty ./maintenance.config

Ditto.

> +'
> +
>  test_expect_success 'register with no value for maintenance.repo' '
>  	cp .git/config .git/config.orig &&
>  	test_when_finished mv .git/config.orig .git/config &&