v3, 6 partsAvoid problem where git_dir is set after alias expansion

> 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.

> >  alias.c          | 31 ++++++++++++++++++++++++-------
> >  git.c            | 55 ++++---------------------------------------------------
> >  t/t7006-pager.sh |  2 +-
> >  3 files changed, 29 insertions(+), 59 deletions(-)
> 
> Happy to see the deletion of all the save/restore-env stuff.
> 
> Except for the puzzlement in one paragraph in the log, looks very
> good.  Thanks for a pleasant reading.

> On Tue, Jun 13, 2017 at 09:21:30AM -0700, Junio C Hamano wrote:
> 
> > >  alias.c          | 31 ++++++++++++++++++++++++-------
> > >  git.c            | 55 ++++---------------------------------------------------
> > >  t/t7006-pager.sh |  2 +-
> > >  3 files changed, 29 insertions(+), 59 deletions(-)
> > 
> > Happy to see the deletion of all the save/restore-env stuff.
> > 
> > Except for the puzzlement in one paragraph in the log, looks very
> > good.  Thanks for a pleasant reading.
> 
> The whole thing looks good to me, too, though we should decide what to
> do with the point that Brandon raised.
> 
> As far as the "streamline" puzzlement goes, I'm OK with either of:
> 
>   - taking your suggestion from the other thread and actually doing that
>     streamlining, which is one less thing to have to explain
> 
>   - leaving the code as-is and tweaking the commit message (though it
>     made sense to me)

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > 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.
> 
> s/read/&ing/, I think.  My reading hiccupped while trying to figure
> out what two alternative approaches are being compared.

> > 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.
> 
> The mention of "streamline" is puzzling to me.  When we are trying
> "git xyz", "alias.xyz" is the key we would look up, not "xyz"; it is
> not clear to anybody who didn't read the discussion on v2 (which
> includes the readers of "git log" in a few months) what kind of flawed
> streamlining could look up "xyz" and result in a bad configuration
> reported on it.

> >  alias.c          | 31 ++++++++++++++++++++++++-------
> >  git.c            | 55 ++++---------------------------------------------------
> >  t/t7006-pager.sh |  2 +-
> >  3 files changed, 29 insertions(+), 59 deletions(-)
> 
> Happy to see the deletion of all the save/restore-env stuff.

> 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>

> ---
>  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

> 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.

> On Tue, Jun 13, 2017 at 11:26:06AM -0700, Brandon Williams wrote:
> 
> > 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.
> 
> Good catch.

> > 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.
> 
> I think that read_early_config() really meant to set the commondir, as
> it was always about actual config-file lookup. So it was sort-of buggy
> already, though I suspect it was pretty hard to trigger.
> 
> But I agree that since include_by_gitdir now reads the same struct
> field, swapping it out fixes the config-reading at the cost of breaking
> that function. And we really need to pass both in.
> 
> I'm not sure whether we should care about this for Dscho's series or
> not. I think his series _does_ make the problem easier to trigger. But
> it's a minor enough bug that I think I'd be OK with letting your
> solution proceed independently.

> Hi Peff & Brandon,
> 
> On Wed, 14 Jun 2017, Jeff King wrote:
> 
> > On Tue, Jun 13, 2017 at 11:26:06AM -0700, Brandon Williams wrote:
> > 
> > > 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.
> > 
> > Good catch.
> 
> Oh wow.
> 
> > > 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.
> > 
> > I think that read_early_config() really meant to set the commondir, as
> > it was always about actual config-file lookup. So it was sort-of buggy
> > already, though I suspect it was pretty hard to trigger.
> > 
> > But I agree that since include_by_gitdir now reads the same struct
> > field, swapping it out fixes the config-reading at the cost of breaking
> > that function. And we really need to pass both in.
> > 
> > I'm not sure whether we should care about this for Dscho's series or
> > not. I think his series _does_ make the problem easier to trigger. But
> > it's a minor enough bug that I think I'd be OK with letting your
> > solution proceed independently.
> 
> Brandon, how hard would it be to build on top of my series? I ask because
> I have to take care of some other things and would not have the time to
> adjust my patch series on top of yours anytime soon.