Re: [PATCH v3 6/6] Use the early config machinery to expand aliases

[Brandon Williams <bmwill@google.com>]

On 06/13, Johannes Schindelin wrote:
> Instead of discovering the .git/ directory, read the config and then
> trying to painstakingly reset all the global state if we did not find a
> matching alias, let's use the early config machinery instead.
> 
> It may look like unnecessary work to discover the .git/ directory in the
> early config machinery and then call setup_git_directory_gently() in the
> case of a shell alias, repeating the very same discovery *again*.
> However, we have to do this as the early config machinery takes pains
> *not* to touch any global state, while shell aliases expect a possibly
> changed working directory and at least the GIT_PREFIX and GIT_DIR
> variables to be set.
> 
> Also, one might be tempted to streamline the code in alias_lookup() to
> *not* use a strbuf for the key. However, if the config reports an error,
> it is far superior to tell the user that the `alias.xyz` key had a
> problem than to claim that it was the `xyz` key.
> 
> This change also fixes a known issue where Git tried to read the pager
> config from an incorrect path in a subdirectory of a Git worktree if an
> alias expanded to a shell command.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

So because I've been looking at the config machinery lately, I've
noticed a lot of issues with how things are handled with respect to
gitdir vs commondir.  Essentially the config resides at commondir/config
always, and only at gitdir/config when not working with a worktree.
Because of this, your patches point out a bug in how early config is
handled.  I'll illustrate this using aliases.

Before this series (because aliases are read using the standard config
machinery):

  > git init main
  > git -C main config alias.test '!echo hello'
  > git -C main test
    hello
  > git -C main worktree add ../worktree
  > git -C worktree test
    hello

After this series (using read_early_config()):

  > git init main
  > git -C main config alias.test '!echo hello'
  > git -C main test
    hello
  > git -C main worktree add ../worktree
  > git -C worktree test
    git: 'test' is not a git command. See 'git --help'.

The issue is that read_early_config passes the gitdir and not the
commondir when reading the config.

The solution would be to add a 'commondir' field to the config_options
struct and populate that before reading the config.  I'm planning on
fixing this in v2 of my config cleanup series which I'll hopefully have
finished by the end of the day.

> ---
>  alias.c          | 31 ++++++++++++++++++++++++-------
>  git.c            | 55 ++++---------------------------------------------------
>  t/t7006-pager.sh |  2 +-
>  3 files changed, 29 insertions(+), 59 deletions(-)
> 
> diff --git a/alias.c b/alias.c
> index 3b90397a99d..6bdc9363037 100644
> --- a/alias.c
> +++ b/alias.c
> @@ -1,14 +1,31 @@
>  #include "cache.h"
>  
> +struct config_alias_data
> +{
> +	struct strbuf key;
> +	char *v;
> +};
> +
> +static int config_alias_cb(const char *key, const char *value, void *d)
> +{
> +	struct config_alias_data *data = d;
> +
> +	if (!strcmp(key, data->key.buf))
> +		return git_config_string((const char **)&data->v, key, value);
> +
> +	return 0;
> +}
> +
>  char *alias_lookup(const char *alias)
>  {
> -	char *v = NULL;
> -	struct strbuf key = STRBUF_INIT;
> -	strbuf_addf(&key, "alias.%s", alias);
> -	if (git_config_key_is_valid(key.buf))
> -		git_config_get_string(key.buf, &v);
> -	strbuf_release(&key);
> -	return v;
> +	struct config_alias_data data = { STRBUF_INIT, NULL };
> +
> +	strbuf_addf(&data.key, "alias.%s", alias);
> +	if (git_config_key_is_valid(data.key.buf))
> +		read_early_config(config_alias_cb, &data);
> +	strbuf_release(&data.key);
> +
> +	return data.v;
>  }
>  
>  #define SPLIT_CMDLINE_BAD_ENDING 1
> diff --git a/git.c b/git.c
> index 8ff44f081d4..58ef570294d 100644
> --- a/git.c
> +++ b/git.c
> @@ -16,50 +16,6 @@ const char git_more_info_string[] =
>  	   "to read about a specific subcommand or concept.");
>  
>  static int use_pager = -1;
> -static char *orig_cwd;
> -static const char *env_names[] = {
> -	GIT_DIR_ENVIRONMENT,
> -	GIT_WORK_TREE_ENVIRONMENT,
> -	GIT_IMPLICIT_WORK_TREE_ENVIRONMENT,
> -	GIT_PREFIX_ENVIRONMENT
> -};
> -static char *orig_env[4];
> -static int save_restore_env_balance;
> -
> -static void save_env_before_alias(void)
> -{
> -	int i;
> -
> -	assert(save_restore_env_balance == 0);
> -	save_restore_env_balance = 1;
> -	orig_cwd = xgetcwd();
> -	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
> -		orig_env[i] = getenv(env_names[i]);
> -		orig_env[i] = xstrdup_or_null(orig_env[i]);
> -	}
> -}
> -
> -static void restore_env(int external_alias)
> -{
> -	int i;
> -
> -	assert(save_restore_env_balance == 1);
> -	save_restore_env_balance = 0;
> -	if (!external_alias && orig_cwd && chdir(orig_cwd))
> -		die_errno("could not move to %s", orig_cwd);
> -	free(orig_cwd);
> -	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
> -		if (external_alias &&
> -		    !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))
> -			continue;
> -		if (orig_env[i]) {
> -			setenv(env_names[i], orig_env[i], 1);
> -			free(orig_env[i]);
> -		} else {
> -			unsetenv(env_names[i]);
> -		}
> -	}
> -}
>  
>  static void commit_pager_choice(void) {
>  	switch (use_pager) {
> @@ -250,19 +206,18 @@ static int handle_alias(int *argcp, const char ***argv)
>  	const char **new_argv;
>  	const char *alias_command;
>  	char *alias_string;
> -	int unused_nongit;
> -
> -	save_env_before_alias();
> -	setup_git_directory_gently(&unused_nongit);
>  
>  	alias_command = (*argv)[0];
>  	alias_string = alias_lookup(alias_command);
>  	if (alias_string) {
>  		if (alias_string[0] == '!') {
>  			struct child_process child = CHILD_PROCESS_INIT;
> +			int nongit_ok;
> +
> +			/* Aliases expect GIT_PREFIX, GIT_DIR etc to be set */
> +			setup_git_directory_gently(&nongit_ok);
>  
>  			commit_pager_choice();
> -			restore_env(1);
>  
>  			child.use_shell = 1;
>  			argv_array_push(&child.args, alias_string + 1);
> @@ -308,8 +263,6 @@ static int handle_alias(int *argcp, const char ***argv)
>  		ret = 1;
>  	}
>  
> -	restore_env(0);
> -
>  	errno = saved_errno;
>  
>  	return ret;
> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index 83881ec3a0c..20b4d83c281 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -391,7 +391,7 @@ test_expect_success TTY 'core.pager in repo config works and retains cwd' '
>  	)
>  '
>  
> -test_expect_failure TTY 'core.pager is found via alias in subdirectory' '
> +test_expect_success TTY 'core.pager is found via alias in subdirectory' '
>  	sane_unset GIT_PAGER &&
>  	test_config core.pager "cat >via-alias" &&
>  	(
> -- 
> 2.13.0.windows.1.460.g13f583bedb5

-- 
Brandon Williams