git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Rafael Silva <rafaeloliveira.cs@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Git List <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>
Subject: Re: [RFC PATCH 0/2] teach `worktree list` to mark locked worktrees
Date: Fri, 2 Oct 2020 16:28:02 +0000	[thread overview]
Message-ID: <20201002162802.GA15646@contrib-buster.localdomain> (raw)
In-Reply-To: <CAPig+cQXkP8vTNR+LJ4fZRT-an0vEgKxcFpfi+aQ-BdipTgq=A@mail.gmail.com>

On Wed, Sep 30, 2020 at 03:19:27AM -0400, Eric Sunshine wrote:

> On Mon, Sep 28, 2020 at 11:50 AM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
> > This patch series introduces a new information on the git `worktree list`
> > command output, to mark when a worktree is locked with a (locked) text mark.
> 
> Thanks for working on this. "locked" is one of several additional
> annotations to the output of "git worktree list" which have long been
> envisioned. For reference, here are some earlier messages related to
> this topic:
> 
> [1]: https://lore.kernel.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/
> [2]: https://lore.kernel.org/git/CAPig+cQF6V8HNdMX5AZbmz3_w2WhSfA4SFfNhQqxXBqPXTZL+w@mail.gmail.com/
> [3]: https://lore.kernel.org/git/CAPig+cSGXqJuaZPhUhOVX5X=LMrjVfv8ye_6ncMUbyKox1i7QA@mail.gmail.com/
> [4]: https://lore.kernel.org/git/CAPig+cTitWCs5vB=0iXuUyEY22c0gvjXvY1ZtTT90s74ydhE=A@mail.gmail.com/

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.

> 
> I'll leave a few review comments to supplement those already by Junio.
> 
> > This is the output of the worktree list with locked marker:
> >
> >  $ git worktree list
> >  /repo/to/main        abc123 [master]
> >  /path/to/unlocked-worktree1 456def [brancha]
> >  /path/to/locked-worktree   123abc (detached HEAD) (locked)
> 
> In [2], I gave an example of output similar to this but without
> encapsulating "locked" within parentheses:
> 
>     % git worktree list
>     giggle     89ea799ffc [master]
>     ../bobble  f172cb543d [feature1] locked
>     ../fumple  6453c84b7d (detached HEAD) prunable
> 
> I omitted the parentheses partly due to the extra noise they
> introduce, but mostly because I foresaw that a single entry might
> eventually have multiple annotations, for instance:
> 
>     % git worktree list
>     giggle     89ea799ffc [master]
>     ../bobble  f172cb543d [feature1] locked prunable
> 
> in which case, the eyes glide over "locked prunable" a bit more easily
> than over "(locked) (prunable)" or "(locked, prunable)" or some such.
> Generally speaking, "the less noise, the better".

Agreed. Without the parentheses is cleaner and it make easier to
extend to future annotations. I will drop the paretheses for the 
next patch version.

> > This patches are marked with RFC mainly due to:
> >
> >  - Perhaps the `(locked)` marker is not the best suitable way to output
> >   this information and we might need to come with a better way.
> 
> Taking [2] into consideration, I think it's fine to annotate the line
> with "locked" (sans the parentheses), and it's compatible with the the
> verbose mode, also proposed by [2]:
> 
>     % git worktree list -v
>     giggle     89ea799ffc [master]
>     ../bobble  f172cb543d [feature1]
>         locked: worktree on removable media
>     ../fumple  6453c84b7d (detached HEAD)
>         prunable: directory does not exist
> 
> in which case the short "locked" annotation gets moved to the next
> line, indented, and expanded to include the reason, if available
> (otherwise would probably not be moved to the next line).

The verbose mode seems like a very good extension for the worktree list
with the extended reason for `locked/prunable` on the next line.

> 
> 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()`.

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.

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].

> [2]: https://lore.kernel.org/git/CAPig+cQF6V8HNdMX5AZbmz3_w2WhSfA4SFfNhQqxXBqPXTZL+w@mail.gmail.com/

> >  - I am a new contributor to the code base, still learning a lot of git
> >   internals data structure and commands. Likely this patch will require
> >   updates.
> 
> Under normal circumstances, I would be hesitant to accept a
> contribution which makes an addition to the human-consumable "git
> worktree list" output without also making the corresponding addition
> to the --porcelain format. Thus, for instance, I would expect the
> porcelain format to be updated, as well, to produce output such as
> this (taken from [4]):
> 
>     worktree /blah
>     branch refs/heads/blah
>     locked Sneaker-net removable storage\nNot always mounted
> 
> That's a bit complicated because the lock reason may need escaping if
> it contains special characters (such as the newline in the example).
> Thus, I'm a bit hesitant to expect such a change from a newcomer.
> 
> More problematic, though, with regard to the porcelain format is that
> the documentation is so woefully under-specified, as explained in [4],
> that some people may interpret it as meaning that no additional
> information can be added. I'm not particularly sympathetic to that
> view since the intention from the start was that the porcelain format
> should be extensible[4], thus adding new attributes should be allowed.
> But I'm hesitant to ask a newcomer to undertake the task of addressing
> these shortcomings. As such, I think it may be okay merely to change
> the human-consumable output as this series does, and leave porcelain
> output for a later date if someone wants to tackle it.
> 
> 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>/)

Thank you for the detailed explanation. much appreciated.

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.

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.

  reply	other threads:[~2020-10-02 16:28 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-28 15:49 [RFC PATCH 0/2] teach `worktree list` to mark locked worktrees Rafael Silva
2020-09-28 15:49 ` [RFC PATCH 1/2] worktree: teach `list` to mark locked worktree Rafael Silva
2020-09-28 21:37   ` Junio C Hamano
2020-09-29 21:36     ` Rafael Silva
2020-09-30  7:35     ` Eric Sunshine
2020-09-28 15:49 ` [RFC PATCH 2/2] t2402: add test to locked linked worktree marker Rafael Silva
2020-09-28 21:54   ` Junio C Hamano
2020-09-29 21:37     ` Rafael Silva
2020-09-30  8:06   ` Eric Sunshine
2020-09-28 21:19 ` [RFC PATCH 0/2] teach `worktree list` to mark locked worktrees Junio C Hamano
2020-09-29 21:35   ` Rafael Silva
2020-09-30  7:19 ` Eric Sunshine
2020-10-02 16:28   ` Rafael Silva [this message]
2020-10-09 22:50     ` Eric Sunshine
2020-10-10 19:06       ` Rafael Silva
2020-10-10 18:55 ` [PATCH v2 0/1] Teach "worktree list" to annotate " Rafael Silva
2020-10-10 18:55   ` [PATCH v2 1/1] worktree: teach `list` to annotate locked worktree Rafael Silva
2020-10-11  6:26     ` Eric Sunshine
2020-10-11  6:34       ` Eric Sunshine
2020-10-11 10:04       ` Rafael Silva
2020-10-11 10:11   ` [PATCH v3 0/1] Teach "worktree list" to annotate locked worktrees Rafael Silva
2020-10-11 10:11     ` [PATCH v3] worktree: teach `list` to annotate locked worktree Rafael Silva
2020-10-12  2:24       ` Eric Sunshine

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201002162802.GA15646@contrib-buster.localdomain \
    --to=rafaeloliveira.cs@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).