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,
> 
> 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(-)
> 
> Thanks --- this is subtle.  I don't think that what this patch does is
> right.  Commenting below.
> 
> > --- a/path.c
> > +++ b/path.c
> > @@ -372,13 +372,20 @@ void report_linked_checkout_garbage(void)
> >  }
> >  
> >  static void adjust_git_path(const struct repository *repo,
> > +			    const struct worktree *wt,
> >  			    struct strbuf *buf, int git_dir_len)
> >  {
> >  	const char *base = buf->buf + git_dir_len;
> >  	if (is_dir_file(base, "info", "grafts"))
> >  		strbuf_splice(buf, 0, buf->len,
> >  			      repo->graft_file, strlen(repo->graft_file));
> > -	else if (!strcmp(base, "index"))
> > +	/*
> > +	 * Only try to replace the path '$gitdir/index' with the index file
> > +	 * recorded in the repository when not constructing a path for a
> > +	 * worktree.  This way we can retrieve the correct path to a particular
> > +	 * worktree's index file.
> > +	 */
> > +	else if (!wt && !strcmp(base, "index"))
> >  		strbuf_splice(buf, 0, buf->len,
> >  			      repo->index_file, strlen(repo->index_file));
> 
> Some context that may have been missing: GIT_INDEX_FILE is a low-level
> tool to allow script authors to specify an alternate index file to use
> when running commands like "git read-tree" or "git checkout-index".
> 
> The above would make it not take effect for git_path() callers when
> 'wt != NULL'.  As a result, if any caller reaches this code path, then
> scripts specifying GIT_INDEX_FILE would stop working when run from a
> worktree that borrows refs and objects from a separate repository.
> I'm pretty sure that such a subtle flip in behavior based on whether
> they are in "git worktree" created worktree or the main working tree
> is not what the end user would intend, so this looks like a step in
> the wrong direction.
> 
> Fortunately this code path doesn't actually get called.
> 
> In fact, rewriting git_path("index") in this function feels to me like
> a layering violation.  Shouldn't callers be using get_index_file() to
> express their intent more clearly?  That's what all current callers
> do.
> 
> IIUC this came up when a patch from nd/prune-in-worktree (e7a6a3b15,
> revision.c: --indexed-objects add objects from all worktrees), which
> is currently not in pu, introduced a caller that does call
> git_path("index").  The old behavior of replacing git_path("index")
> with $GIT_INDEX_FILE when the latter is set was mostly harmless
> because typically GIT_INDEX_FILE is not set, especially when people
> are running "git prune".  Patch 05/20 (environment: place key
> repository state in the_repository) made the substitution harmful: now
> we would use repo->index_file unconditionally instead of allowing the
> ordinary worktree-relative resolution as a fallback when
> GIT_INDEX_FILE is unset.
> 
> Possible next steps:
> 
>  1. I think we should make git_path less magical and discourage
>     callers from relying on it to handle the GIT_INDEX_FILE envvar.
>     We can do that by removing the !strcmp(base, "index") case
>     completely.

From our conversation off-line it seems like we agree that this is
probably the best course of action.  So I'll plan to drop the case
completely since it isn't even used currently.  I'll also go ahead and
drop the graft_file one as well since it doesn't make much sense to keep
it around when it is only queried for once.

> 
>  2. Optionally, it is possible to be more cautious by keeping the
>     !strcmp(base, "index") case and making it call BUG() to force
>     people not to use it.  This would help steer callers toward
>     get_index_file().  But given that the only caller did not actually
>     want GIT_INDEX_FILE substitution, I don't think that that is
>     necessary or useful.
> 
>  3. A docstring for git_path should explain the substitutions it
>     currently makes and more straightforward alternatives that callers
>     can use.
> 
>  4. Specifying GIT_INDEX_FILE when running "git prune" is a
>     meaningless combination.  It would be nice for "git prune" to
>     error out to save the user from confusion.
> 
>  5. There are likely other commands that don't make sense with
>     GIT_INDEX_FILE.  Tracking them all down to make them print a
>     meaningful error message might be a bit of a slog, though.
> 
> >  	else if (dir_prefix(base, "objects"))
> > @@ -411,7 +418,7 @@ static void do_git_path(const struct repository *repo,
> >  		strbuf_addch(buf, '/');
> >  	gitdir_len = buf->len;
> >  	strbuf_vaddf(buf, fmt, args);
> > -	adjust_git_path(repo, buf, gitdir_len);
> > +	adjust_git_path(repo, wt, buf, gitdir_len);
> >  	strbuf_cleanup_path(buf);
> >  }
> 
> Thanks and hope that helps,
> Jonathan

-- 
Brandon Williams