On Fri, Nov 3, 2023 at 8:03 PM Edmundo Carmona Antoranz
<eantoranz@xxxxxxxxx> wrote:
> Given that worktrees are tracked in their own special fashion separately,
> it makes sense to _not_ report them as "untracked". Also, when seeing the
> directory of a worktree listed as Untracked, it might be tempting to try
> to do operations (like 'git add') on them from the parent worktree which,
> at the moment, will silently do nothing.
>
> With this patch, we check items against the list of worktrees to add
> them into the untracked items list effectively hiding them.
>
> END OF PATCH
>
> Here are a few questions more inline with the "RFC" part of the patch.
>
> About UI
> - Would it make more sense to separate them from Untracked files instead
> of hiding them (perhaps add a --worktrees option to display them)?
> - Follow-up if the previous answer is 'yes': List a worktree only if it
> is not clean?
>
> About code:
> - If keeping the idea/patch, Would it make more sense (performance-wise) to
> fist check an item in the list of worktrees before checking it in the
> index? In other words, reverse the conditions to add an item to the
> untracked list?
I have slightly mixed feelings about this idea since I'm sympathetic
to the motivation, however, my knee-jerk reaction is that these really
_are_ untracked considering that Git is a "content tracker" and
worktrees are not project content. Git already has general mechanisms
such as .git/info/exclude and .gitignore for suppressing certain
untracked items, so introducing special-purpose code to suppress
worktrees from being considered untracked may be a case of adding
complexity for little gain.
Moreover, although your personal workflow may be to create worktrees
within your main directory:
git worktree add new-feature
other people use a workflow in which worktrees are created at other
locations, such as making them siblings:
git worktree add ../new-feature
For the former case, it's easy enough to mention worktrees in
.git/info/exclude, especially if you use a standard naming convention
for your worktrees, in which case a single wildcard pattern may allow
you to set it once and forget about it. For the latter workflow, the
extra "is untracked" checking is simply wasteful.
Having said all that, I think that someone may have recently floated
the idea on the mailing list about suppressing dirty submodules from
showing up as "dirty" in git-status. Although the underlying concepts
and mechanisms are quite distinct (especially since a submodule _is_
content), perhaps there is some sort of analogy between worktrees and
dirty submodules which invalidates my knee-jerk reaction. Also, I'm
just one person responding without having put all that much thought
into it. Others may feel differently.
Regarding the patch itself...
> diff --git a/wt-status.c b/wt-status.c
> @@ -795,9 +796,12 @@ static void wt_status_collect_untracked(struct wt_status *s)
> + worktrees = get_worktrees();
> +
> for (i = 0; i < dir.nr; i++) {
> struct dir_entry *ent = dir.entries[i];
> - if (index_name_is_other(istate, ent->name, ent->len))
> + if (index_name_is_other(istate, ent->name, ent->len) &&
> + !find_worktree_by_path(worktrees, ent->name))
> string_list_insert(&s->untracked, ent->name);
> }
This first-stab implementation unfortunately has worse than quadratic
complexity, perhaps even cubic complexity since
find_worktree_by_path() performs a linear scan through the worktree
list. So, for each path in `dir`, it's performing a
character-by-character string comparison with each path in
`worktrees`. Worse, find_worktree_by_path() calls strbuf_realpath()
which hits the filesystem for each path in `worktrees` each time it's
called.
So, a real (non-RFC) implementation would probably need to perform a
preparatory step of creating a hash-table/set in which the keys are
the realpath'd elements from `worktrees`, and then simply consult the
hash-table/set for each path in `dir`.
> @@ -809,6 +813,9 @@ static void wt_status_collect_untracked(struct wt_status *s)
> + if (worktrees)
> + free_worktrees(worktrees);
Nit: At this point, we _know_ that `worktrees` is non-NULL, so
free_worktrees() can be called unconditionally.
