Re: [PATCH v2 8/8] Use the early config machinery to expand aliases

[Jeff King <peff@peff.net>]

On Thu, Jun 08, 2017 at 09:53:53PM +0200, Johannes Schindelin wrote:

> @@ -245,36 +201,37 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  
>  static int handle_alias(int *argcp, const char ***argv)
>  {
> +	struct strbuf cdup_dir = STRBUF_INIT;
>  	int envchanged = 0, ret = 0, saved_errno = errno;
>  	int count, option_count;
>  	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, NULL);
> +	alias_string = alias_lookup(alias_command, &cdup_dir);
>  	if (alias_string) {
>  		if (alias_string[0] == '!') {
>  			struct child_process child = CHILD_PROCESS_INIT;
>  
> +			if (cdup_dir.len)
> +				setup_git_directory();
> +

I'm really confused here. We went to all the trouble to record the
cdup_dir as part of the alias lookup so that we could make sure to
chdir() to it. But we don't seem to ever look at its value. We just use
it as a proxy for "did we find a git-dir". And if we did, we go to all
the trouble to re-discover it via setup_git_directory().

I understand why you must use setup_git_directory() and not just a
chdir(), because the latter sets up variables like GIT_PREFIX and
GIT_DIR that the shell alias may be depending on.

But couldn't we just unconditionally do:

  setup_git_directory_gently();

here to move into the top-level if there is one, without caring about
cdup_dir at all?

That should be equally correct. It does do an extra discovery when we
already know that there isn't a gitdir at all. But my earlier comments
about optimizing apply (measure, and possibly cache). And in the common
case that we are in a repo, it's exactly the same, since the setup call
here would re-discover from scratch (though again, if we care, caching
could solve that).

-Peff