Re: [PATCH v2 8/8] Use the early config machinery to expand aliases
- From
Jeff King <peff@peff.net>
- Date
- Jun 10, 2017, 10:07 UTC
- Message-ID
- <20170610100730.j62kh2hayt6zknr5@sigill.intra.peff.net>
- In-Reply-To
- <af40ce088de03130a5edfae187bb8274b5a9d3a9.1496951503.git.johannes.schindelin@gmx.de>
On Thu, Jun 08, 2017 at 09:53:53PM +0200, Johannes Schindelin wrote:
Show 25 quoted lines
> @@ -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