On Tue, Mar 27, 2018 at 6:47 PM, Jeff King <peff@xxxxxxxx> wrote: >> 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
