On Thu, Jun 08, 2017 at 09:53:41PM +0200, Johannes Schindelin wrote:
> So far, when we invoked the early config code path, we implicitly
> determined the top-level directory of the worktree while discovering the
> .git/ directory.
>
> And then we simply forgot that information.
>
> However, when we expand aliases, we very much need that information, as
> aliases expanding to shell commands, i.e. whose value starts with an
> exclamation point, have to be executed in the top-level directory of the
> worktree. There are exceptions abound, not only with non-shell aliases
> (which are supposed to be executed in the original working directory
> instead), but also when being started inside the .git/ directory or in a
> worktree created via `git worktree add`.
I understand why you wrote it this way, but it really feels backwards.
We're calling a function that's about config, and oh by the way
sometimes it tells us about which git directory we're in (and sometimes
not because it didn't actually run discover_git_directory anyway).
It would make a lot more sense to me if we told read_early_config() "oh
by the way, I discovered the git directory already, here it is".
The same applies to the later patch to pass the information through
alias_lookup().
Taking a step back, this is really just an optimization, right? The
cleanest thing would be for the alias code, right before launching a
shell alias, to discover_git_directory(). But you don't want to do that
because it's too expensive?
If so, my questions would be:
1. Is it actually that expensive, or is this premature optimization?
We are, after all, about to fork and execute a shell. Have we
measured?
2. Can we cache the results of discovery_git_directory() between
calls? That would not only fix your issue here, but it would
optimize the calls we make when we call read_early_config() for
multiple items (e.g., both pager and alias config).
The only trick is making sure our previous value is still valid.
I suspect it would work to just ignore this, as we only chdir when
doing setup_git_directory(), and that should generally take
precedence over discover_git_directory() being called at all. But
for extra safety, we should be able to key it to the cwd, like:
strbuf_getcwd(&now_cwd);
if (!strbuf_cmp(&cached_cwd, &now_cwd) {
strbuf_addbuf(cdup_dir, &cached_cdup_dir));
return xstrdup(cached_gitdir);
}
strbuf_swap(&cached_cwd, &now_cwd);
strbuf_release(&now_cwd);
... rest of function, but store result in cached_gitdir ...
-Peff