Re: [PATCH] worktree: add per-worktree config files

[Taylor Blau <me@ttaylorr.com>]

On Sun, Sep 23, 2018 at 07:04:38PM +0200, Nguyễn Thái Ngọc Duy wrote:
> A new repo extension is added, worktreeConfig. When it is present:

I was initially scratching my head at why this should be a repository
extension, but...

>  - The special treatment for core.bare and core.worktree, to stay
>    effective only in main worktree, is gone. These config files are
>    supposed to be in config.worktree.

This seems to be sufficient justification for it. A destructive action
(such as migrating configuration from one location to another, as you
have implemented in the patch below) should be something that the user
opts into, rather than having happen automatically.

> This extension is most useful in multiple worktree setup because you
> now have an option to store per-worktree config (which is either
> .git/config.worktree for main worktree, or
> .git/worktrees/xx/config.worktree for linked ones).

This sounds quite useful for these situations.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 8d85d1a324..c24abf5871 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2,8 +2,9 @@ CONFIGURATION FILE
>  ------------------
>
>  The Git configuration file contains a number of variables that affect
> -the Git commands' behavior. The `.git/config` file in each repository
> -is used to store the configuration for that repository, and
> +the Git commands' behavior. The files `.git/config` and optionally
> +`config.worktree` (see `extensions.worktreeConfig` below) are each
> +repository is used to store the configuration for that repository, and

Typo: 'are each'.

>  ENVIRONMENT
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index e2ee9fc21b..3f9112db56 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -204,6 +204,43 @@ working trees, it can be used to identify worktrees. For example if
>  you only have two working trees, at "/abc/def/ghi" and "/abc/def/ggg",
>  then "ghi" or "def/ghi" is enough to point to the former working tree.
>
> +CONFIGURATION FILE
> +------------------
> +By default, the repository "config" file is shared across all working
> +directories. If the config variables `core.bare` or `core.worktree`
> +are already present in the config file, they will be applied to the
> +main working directory only.
> +
> +In order to have configuration specific to working directories, you
> +can turn on "worktreeConfig" extension, e.g.:
> +
> +------------
> +$ git config extensions.worktreeConfig true
> +------------

Good, this matches my expectation from above.

> @@ -24,6 +25,7 @@ static char key_delim = ' ';
>  static char term = '\n';
>
>  static int use_global_config, use_system_config, use_local_config;
> +static int use_worktree_config;
>  static struct git_config_source given_config_source;
>  static int actions, type;
>  static char *default_value;
> @@ -123,6 +125,7 @@ static struct option builtin_config_options[] = {
>  	OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),
>  	OPT_BOOL(0, "system", &use_system_config, N_("use system config file")),
>  	OPT_BOOL(0, "local", &use_local_config, N_("use repository config file")),
> +	OPT_BOOL(0, "worktree", &use_worktree_config, N_("use per-worktree config file")),
>  	OPT_STRING('f', "file", &given_config_source.file, N_("file"), N_("use given config file")),
>  	OPT_STRING(0, "blob", &given_config_source.blob, N_("blob-id"), N_("read config from given blob object")),
>  	OPT_GROUP(N_("Action")),
> @@ -602,6 +605,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  			     PARSE_OPT_STOP_AT_NON_OPTION);
>
>  	if (use_global_config + use_system_config + use_local_config +
> +	    use_worktree_config +
>  	    !!given_config_source.file + !!given_config_source.blob > 1) {

I feel like we're getting into "let's extract a function" territory,
here, since this line is growing in width. Perhaps:

  static int config_files_count()
  {
    return use_global_config + use_system_config + use_local_config +
  		use_worktree_config +
  		!!given_config_source.file +
  		!!given_config_source.blob;
  }

Simplifying the call to:

> diff --git a/t/t2029-worktree-config.sh b/t/t2029-worktree-config.sh
> new file mode 100755
> index 0000000000..4ebdf13cf9
> --- /dev/null
> +++ b/t/t2029-worktree-config.sh
> @@ -0,0 +1,82 @@
> +#!/bin/sh
> +
> +test_description="config file in multi worktree"
> +
> +. ./test-lib.sh
> +
> +cmp_config() {
> +	if [ "$1" = "-C" ]; then
> +		shift &&
> +		GD="-C $1" &&
> +		shift
> +	else
> +		GD=
> +	fi &&
> +	echo "$1" >expected &&
> +	shift &&
> +	git $GD config "$@" >actual &&
> +	test_cmp expected actual
> +}

This cmp_config seems generally useful, perhaps beyond t2029. What do
you think about putting it in t/test-lib-functions.sh instead?

Thanks,
Taylor