Re: [PATCH] Fix "t0001: test git init when run via an alias"

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Commit 4ad8332 (t0001: test git init when run via an alias -
> 2010-11-26) noted breakages when running init via alias. The problem
> is for alias to be used, $GIT_DIR must be searched, but 'init' and
> 'clone' are not happy with that. So we start a new process like an
> external command, with clean environment in this case. Env variables
> that are set by command line (e.g. "git --git-dir=.. ") are kept.
>
> This should also fix autocorrecting a command typo to "init" because
> it's the same problem: aliases are read, then "init" is unhappy with
> $GIT_DIR already set up because of that.
>
> Reminded-by: David Turner <dturner@twopensource.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  git.c           | 52 ++++++++++++++++++++++++++++++++++++++++++++++++----

This goes far deeper than "Fix t0001", doesn't it?

>  t/t0001-init.sh |  4 ++--
>  2 files changed, 50 insertions(+), 6 deletions(-)
>
> diff --git a/git.c b/git.c
> index 7780572..d1e49da 100644
> --- a/git.c
> +++ b/git.c
> @@ -484,6 +521,10 @@ static void handle_builtin(int argc, const char **argv)
>  		struct cmd_struct *p = commands+i;
>  		if (strcmp(p->cmd, cmd))
>  			continue;
> +		if (saved_environment && (p->option & NO_SETUP)) {
> +			restore_env();
> +			break;
> +		}
>  		exit(run_builtin(p, argc, argv));
>  	}
>  }
> @@ -539,7 +580,10 @@ static int run_argv(int *argcp, const char ***argv)
>  		 * of overriding "git log" with "git show" by having
>  		 * alias.log = show
>  		 */
> -		if (done_alias || !handle_alias(argcp, argv))
> +		if (done_alias)
> +			break;
> +		save_env();
> +		if (!handle_alias(argcp, argv))
>  			break;
>  		done_alias = 1;
>  	}

So the save-env kicks in only after we tried the builtins and the
externals and didn't find any, and before expanding the alias (which
has to look at the config, which means we need to do discovery and
may contaminate the environment and the globals), and then when we
retry with the expanded alias, we restore when we know the command
will misbehave if we didn't do so?

That does not sound so bad.  Even though I wonder if that "save and
then restore" sequence logically belongs around handle_alias(), you
would not have sufficient clue to let you cheat by not restoring the
environment for commands that you happen to know that they do not
care, so that may be a reasonable optimization.

Is it too brittle a solution to force people to mark problematic
subcommands with NO_SETUP, though?  What kind of change to a
subcommand that currently does not have to be marked with NO_SETUP
would make it necessary to mark it with NO_SETUP?

Thanks.