Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

From: Jeff King <peff@peff.net>
Date: Mar 28, 2018, 09:47 UTC
Message-ID: <20180328094733.GA1523@sigill.intra.peff.net>
In-Reply-To: <CACsJy8Bog6U7X-jvzDhq14heQWx0HA_21HsSYR0nykU9aDsCYQ@mail.gmail.com>

On Tue, Mar 27, 2018 at 07:09:36PM +0200, Duy Nguyen wrote:

Show 14 quoted lines

> > 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).

Ah, OK (that is my first one, "avoid the way the path is munged", but obviously I didn't spell it out very clearly).

Show 10 quoted lines

> > 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.

Yeah, I agree that the root of the problem is not the caching of get_git_dir(), but that chdir() may invalidate assumptions made by other parts of the program.

> 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).

So the non-hacky solution is to inform all callers that we've changed directories, and they may need to recompute any relative paths.

It does seem backwards for setup_work_tree() to need to know about the refs code. Should we have a system by which interested code can register to learn about changes to global state? E.g., something like:

  typedef void (*chdir_notify_cb)(const char *old_cwd,
                                  const char *new_cwd,
                                  void *data);

  /* Register interest in hearing about chdir */
  void chdir_notify_register(chdir_notify_cb cb, void *data);

  /* Do a chdir and then tell everybody about it */
  void chdir_notify(const char *path);

Then the ref code (or anybody else) should be able to write a function to normalize a relative path from the old_cwd into a relative one from the new_cwd.

-Peff