On Tue, Mar 27, 2018 at 6:47 PM, Jeff King [off-list ref] wrote:

quoted

So I would not mind papering over it right now (with an understanding
that absolute path pays some more overhead in path walking, which was
th reason we tried to avoid it in setup code). A slightly better patch
is trigger this path absolutization from setup_work_tree(), near
set_git_dir(). But then you face some ugliness there: how to find out
all ref stores to update, or just update the main ref store alone.

I don't quite get why f57f37e2 doesn't want to call git_path(). Is it to
avoid the way the path is munged? Or is it to avoid some lazy-setup that
is triggered by calling get_git_dir() at all (which doesn't make much
sense to me, because we'd already have called get_git_dir() much
earlier). Or is it just because we may sometimes fill in refs->git_dir
with something besides get_git_dir() (for a submodule or worktree or
something)?

None of those, I think. git_path() does some magic to translate paths
so that refs/... ends up with $GIT_COMMON_DIR/refs/... while "index"
ends up with $GIT_DIR/index. Michael wanted to avoid that magic and
keep the control within refs code (i.e. this code knows refs/ and
packed-refs are shared, and pseudo refs are not, what git_path()
decides does not matter).

I.e., can we do one of (depending on which of those answers is "yes"):

  1. Stop caching the value of get_git_dir(), and instead call it
     on-demand instead of looking at refs->git_dir? (If we just want to
     avoid git_path()).

This probably works, but I count it as papering over the problem too.

  2. If we need to avoid even calling get_git_dir(), can we add a
     "light" version of it that avoids whatever side effects we're
     trying to skip?

  3. If the problem is just that sometimes we need get_git_dir() and
     sometimes not, could we perhaps store NULL as a sentinel to mean
     "look up get_git_dir() when you need it"?

     That would let submodules and worktrees fill in their paths as
     necessary (assuming they never change after init), but handle the
     case of get_git_dir() changing.

Hmm. Typing that out, it seems like (3) is probably the right path.
Something like the patch below seems to fix it and passes the tests.

Honestly I think this is just another way to work around the problem
(with even more changes than your abspath approach). The problem is
with setup_work_tree(). We create a ref store at a specific location
and it should stay working without lazily calling get_git_dir(), which
has nothing to do (anymore) with the path we have given a ref store.
If somebody changes a global setting like $CWD, it should be well
communicated to everybody involved.

I would rather have something like ref_store_reinit() in the same
spirit as the second call of set_git_dir() in setup_work_tree. It is
hacky, but it works and keeps changes to minimal (so that it could be
easily replaced later).
-- 
Duy