Re: [RFC PATCH 0/2] teach `worktree list` to mark locked worktrees

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 9, 2020, 22:50 UTC
Message-ID: <CAPig+cR8D13cM8OewRVYfg7wNjVC05tVQw80-dm4B5XPmjHJWw@mail.gmail.com>
In-Reply-To: <20201002162802.GA15646@contrib-buster.localdomain>

On Fri, Oct 2, 2020 at 12:28 PM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote:

Show 9 quoted lines

> On Wed, Sep 30, 2020 at 03:19:27AM -0400, Eric Sunshine wrote:
> > [...] For reference, here are some earlier messages related to
> > this topic:
>
> Thank you for all this reference, it's really helpful. It is nice to see
> that we already have few discussion on this topic that we can use and
> work on top of that.
>
> Sorry for the bit late response.

Likewise.

Show 7 quoted lines

> > I'm not suggesting that this patch series implement verbose mode, but
> > bring it to attention to make sure we don't paint ourselves into a
> > corner when deciding how the "locked" annotation should be presented.
>
> Doing a little investigation on the code, it seems the machinery for checking
> whether a worktree is prunable it seems is already there implemented
> on the `should_prune_worktree()`.

Yes, when I mentioned that in [1], I envisioned should_prune_worktree() being moved from builtin/worktree.c to top-level worktree.c and possibly generalized a bit if necessary.

One thing to note is that should_prune_worktree() is somewhat expensive, so we'd probably want to make determination of "prunable reason" lazy, much like the lock reason is retrieved lazily rather than doing it when get_worktrees() is called. Thus, like the lock reason, the prunable reason would be accessed indirectly via a function, say worktree_prunable_reason(), rather than directly from 'struct worktree'.

[1]: https://lore.kernel.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/

Show 5 quoted lines

> In such case, I would love to get started working on a bigger patch that
> will implemented not only the annotation, but the verbose mode as well.
> Specially because I was also thinking about how to make the "locked reason"
> message available to the command output and the design proposed by [2]
> sounds like a good way to manage that.

I'd be happy to see that implemented.

Show 6 quoted lines

> Additionally, having the ability to see the annotation and the reason in
> case you see the annotation seems like more complete work for the intention
> of the patch.
>
> Unless you think that is better to start with the annotation, and some time
> later addressing the other changes specified by [2].

Whatever you feel comfortable tackling is fine. The simple "locked" annotation is nicely standalone, so it could be resubmitted with the changes suggested by reviewers, and graduate without waiting for the more complex tasks which could be done as follow-up series. Or, expand the current series to tackle verbose mode and/or prunable status or both or any combination.

Show 27 quoted lines

> > A reason that it would be nice to address the shortcomings of
> > porcelain format is because there are several additional pieces of
> > information it could be providing. Summarizing from [1], in addition
> > to the worktree path, its head, checked out branch, whether its bare
> > or detached, for each worktree, porcelain could also show:
> >
> >   * whether it is locked
> >    - the lock reason (if available)
> >    - and whether the worktree is currently accessible (mounted)
> >   * whether it can be pruned
> >    - and the prune reason if so
> >   * worktree ID (the <id> of .git/worktrees/<id>/)
>
> That something that can also work on. But I agreed that it could be bit
> more work for a newcomer. I was thinking that I can split the work in
> three series of patches.
>
>  1. Implementing the annotation for the standards "list" command, implementing
>   not only the locked but the prunable as on aforementioned in [2].
>
>  2. A second series of patch that will introduce the verbose as defined in [2]
>
>  3. Third and final series that extend the porcelain format.
>
> I would like to kindly ask your opnion on this. Whether you think it will
> be a good idea to implement all these changes this way and I can start
> working on that.

Such an organization would be fine. Tackle what you feel is appropriate and what "scratches your itch". Breaking the changes down into smaller chunks, as you propose, also helps reviewers since it's easier to review a shorter series than a long one.

> I will change this series to become the first part of annotations, specially
> because after reading your response and references, it seems this will be
> much complete functionality that I would like to have on Git.

Makes sense.