On 06/20, Jonathan Nieder wrote:
> Hi again,
>
> Brandon Williams wrote:
>
> > When working with worktrees the git directory is split into two part,
> > the per-worktree gitdir and a commondir which contains things which are
> > shared among all worktrees (like the object store). With this notion of
> > having a split git directory, 557bd833b (git_path(): be aware of file
> > relocation in $GIT_DIR) and c7b3a3d2f ($GIT_COMMON_DIR: a new
> > environment variable) changed the way that 'git_path()' functioned so
> > that paths would be adjusted if they referred to files or directories
> > that are stored in the commondir or have been changed via an environment
> > variable.
> >
> > One interesting problem with this is the index file as it is not shared
> > across worktrees yet when asking for a path to a specific worktree's
> > index it will be replaced with a path to the current worktree's index.
> > In order to prevent this, teach 'adjuct_git_path' to replace the
> > path to the index with the path recorded in a repository (which would be
> > the current, active worktree) only when not asked to construct a path
> > for a specific worktree.
> >
> > Signed-off-by: Brandon Williams <bmwill@xxxxxxxxxx>
> > ---
> > path.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
>
> Digging more into it with your help, I ran into v2.5.0-rc0~143^2~34
> and v2.11.0-rc0~15^2 (git-svn: "git worktree" awareness, 2016-10-14),
> which uses this function:
>
> - my $index = $ENV{GIT_INDEX_FILE} || "$ENV{GIT_DIR}/index";
> + my $index = command_oneline(qw(rev-parse --git-path index));
>
> That rules out my hope of making git_path stop being aware of the index
> at all.
>
> That also made it a little clearer to me what is going on with this
> function. I had previously misread !wt as meaning that git_dir is
> not under worktrees/ (or in other words as !wt->id). Instead, it
> means that the caller does not want to use some other work tree in
> preference to git_dir.
>
> With that piece in place, the patch starts making more sense to me.
> When !wt, repo->index_file is going to have the same value as
> getenv("GIT_INDEX_FILE") ?: buf->buf already, since repo->index_file
> refers to the current work tree git dir's index file. When wt != NULL,
> we don't want to use that file and have requested to grab the index
> from a specific work tree git dir instead.
>
> And patch 05/20 (environment: place key repository state in
> the_repository) had no effect since no one calls worktree_git_path
> with argument "index", nor with a user-specified path.
>
> How about the following? I found it a little easier to understand.
So your suggestion is to completely avoid doing any location when asking
for a worktree_git_path, I guess those code paths which request those
paths should be aware enough that if they need something in commondir to
use git_common_path instead. My only worry is that it may be difficult
to catch misuse of worktree_git_path during code review, at least that
was one of the motivating factors for originally respecting
GIT_INDEX_FILE and the like.
>
> -- >8 --
> From: Brandon Williams <bmwill@xxxxxxxxxx>
> Subject: worktree_git_path: do not let GIT_INDEX_FILE override path to index
>
> git_path (and variants like strbuf_git_path) is designed to behave
> like a convenience function that produces a string $GIT_DIR/<path>.
> In the process, callers automatically get support for path relocation
> variables like $GIT_OBJECT_DIRECTORY:
>
> - git_path("index") is $GIT_INDEX_FILE when set
> - git_path("info/grafts") is $GIT_GRAFTS_FILE when set
> - git_path("objects/<foo>") is $GIT_OBJECT_DIRECTORY/<foo> when set
> - git_path("hooks/<foo>") is <foo> under core.hookspath when set
> - git_path("refs/<foo>") etc (see path.c::common_list) is relative
> to $GIT_COMMON_DIR instead of $GIT_DIR
>
> worktree_git_path, by comparison, is designed to resolve files in a
> specific worktree's git dir and should not perform such relocation.
> Do so by skipping the relocation step in worktree_git_path.
>
> Fortunately no current callers of worktree_git_path pass such
> arguments. Noticed due to an interaction between two patches under
> review, one of which introduced such a caller:
>
> * One made "git prune" check the index file in each worktree's git dir
> (using worktree_git_path(wt, "index")) for objects not to prune,
> triggering the unwanted relocation code by mistake and allowing
> objects reachable from worktree indices to be pruned if
> GIT_INDEX_FILE is set.
>
> * The other simplified the relocation logic for index, info/grafts,
> objects, and hooks to happen unconditionally instead of based on
> whether environment or configuration variables are set, causing the
> relocation to trigger even when GIT_INDEX_FILE is not set.
>
> [jn: rewrote commit message; skipped all relocations instead of just
> the index]
>
> Change-Id: I2ba0a48a48b7e9a9c2e3ef97648cf53cb913bdd9
> Signed-off-by: Brandon Williams <bmwill@xxxxxxxxxx>
> Signed-off-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
> ---
> path.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/path.c b/path.c
> index c1cb1cf62..4c3a27a8e 100644
> --- a/path.c
> +++ b/path.c
> @@ -397,7 +397,8 @@ static void do_git_path(const struct worktree *wt, struct strbuf *buf,
> strbuf_addch(buf, '/');
> gitdir_len = buf->len;
> strbuf_vaddf(buf, fmt, args);
> - adjust_git_path(buf, gitdir_len);
> + if (!wt)
> + adjust_git_path(buf, gitdir_len);
> strbuf_cleanup_path(buf);
> }
>
> --
> 2.13.1.611.g7e3b11ae1
>
--
Brandon Williams
