Re: [PATCH v3 11/20] path: construct correct path to a worktree's index

[Brandon Williams <bmwill@google.com>]

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@google.com>
> > ---
> >  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@google.com>
> 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@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  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