On Tue, Mar 27, 2018 at 07:09:36PM +0200, Duy Nguyen wrote:
> > 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).
> > 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
