Re: [PATCH v2 4/8] read_early_config(): optionally return the worktree's top-level directory

[Jeff King <peff@peff.net>]

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