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