| Message ID | 20200928154953.30396-1-rafaeloliveira.cs@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | teach `worktree list` to mark locked worktrees | expand |
Rafael Silva <rafaeloliveira.cs@gmail.com> writes: > 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. > > The intent is to improve the user experience to earlier sinalize that a linked > worktree is locked, instead of realising later when attempting to remove it > with `remove` command as it happened to me twice :) Change with a good intention, it seems. > The patches are divided into two parts. First part introduces > the new marker to the worktree list command and small documentation > change. And the second adds one test case into t2402 to test > if the (locked) text will be properly set for a locked worktree, and > not mistankely set to a unlocked or master worktree. Probably they belong together in a single patch (I am saying this after only seeing the above five lines, without reading either of these two patches, so there may be some things in them that makes it make sense to have them separate). > 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) Looks OK to me > This patches are marked with RFC mainly due to: > > - This will change the default behaviour of the worktree list, I am > not sure whether will be better to make this tuned via a config > and/or a git parameter. (assuming this change is a good idea ;) ) The default output is meant for human consumption (scripts that want to read from the command and expect stable output would be using the "--porcelain" option). The ideal case is a new output is universally useful for everybody, in which case we can just change it without any new configuration or command line option. > - Perhaps the `(locked)` marker is not the best suitable way to output > this information and we might need to come with a better way. It looks good enough to me. I am not qualified to have a strong opinion on this part, as I do not use the command all that often. $ git shortlog --no-merges --since=18.months builtin/worktree.c tells me that Eric Sunshine (CC'ed) may be a good source of wisdom on this command. > - 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. Welcome. > Rafael Silva (2): > teach `list` to mark locked worktree > t2402: add test to locked linked worktree marker > > Documentation/git-worktree.txt | 5 +++-- > builtin/worktree.c | 6 +++++- > t/t2402-worktree-list.sh | 13 +++++++++++++ > 3 files changed, 21 insertions(+), 3 deletions(-)
Thank you for all the feedback, I will work on a another patch that will address all the feedback provided on this series, including having the changes into a single patch. On Mon, Sep 28, 2020 at 02:19:53PM -0700, Junio C Hamano wrote: > Rafael Silva <rafaeloliveira.cs@gmail.com> writes: > > > 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. > > > > The intent is to improve the user experience to earlier sinalize that a linked > > worktree is locked, instead of realising later when attempting to remove it > > with `remove` command as it happened to me twice :) > > Change with a good intention, it seems. > > > The patches are divided into two parts. First part introduces > > the new marker to the worktree list command and small documentation > > change. And the second adds one test case into t2402 to test > > if the (locked) text will be properly set for a locked worktree, and > > not mistankely set to a unlocked or master worktree. > > Probably they belong together in a single patch (I am saying this > after only seeing the above five lines, without reading either of > these two patches, so there may be some things in them that makes it > make sense to have them separate). > Yes, I believe it make sense to have them in a single patch. > > 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) > > Looks OK to me > > > This patches are marked with RFC mainly due to: > > > > - This will change the default behaviour of the worktree list, I am > > not sure whether will be better to make this tuned via a config > > and/or a git parameter. (assuming this change is a good idea ;) ) > > The default output is meant for human consumption (scripts that want > to read from the command and expect stable output would be using the > "--porcelain" option). > > The ideal case is a new output is universally useful for everybody, > in which case we can just change it without any new configuration or > command line option. Thanks for the explanation, will keep that in mind. > > > - Perhaps the `(locked)` marker is not the best suitable way to output > > this information and we might need to come with a better way. > > It looks good enough to me. I am not qualified to have a strong > opinion on this part, as I do not use the command all that often. > > $ git shortlog --no-merges --since=18.months builtin/worktree.c > > tells me that Eric Sunshine (CC'ed) may be a good source of wisdom > on this command. > Thank you, I will keep Eric Sunshine in the loop as well. > > - 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. > > Welcome. Thank you. > > > Rafael Silva (2): > > teach `list` to mark locked worktree > > t2402: add test to locked linked worktree marker > > > > Documentation/git-worktree.txt | 5 +++-- > > builtin/worktree.c | 6 +++++- > > t/t2402-worktree-list.sh | 13 +++++++++++++ > > 3 files changed, 21 insertions(+), 3 deletions(-)
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/ 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". > 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). 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. > - 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>/)
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.
On Fri, Oct 2, 2020 at 12:28 PM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > 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. > > 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/ > 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. > 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. > > 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.
On Fri, Oct 09, 2020 at 06:50:02PM -0400, Eric Sunshine wrote: > > > > Sorry for the bit late response. > > Likewise. > > > > > 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/ Appreciate the tip, I will be working on the prunable annotations, verbose and other information that was proposed previously for the "worktree list" command. > > 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. > Thanks. I've just resubmitted the "locked" annotation patch, as you said, it's nice standalone and can be integrated and hopefully will be already useful for other git users and soon (hopefully :) ) will submit new patches for the other changes as proposed by [1]. [1]: https://lore.kernel.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/
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. The intent is to improve the user experience to earlier sinalize that a linked worktree is locked, instead of realising later when attempting to remove it with `remove` command as it happened to me twice :) The patches are divided into two parts. First part introduces the new marker to the worktree list command and small documentation change. And the second adds one test case into t2402 to test if the (locked) text will be properly set for a locked worktree, and not mistankely set to a unlocked or master worktree. 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) This patches are marked with RFC mainly due to: - This will change the default behaviour of the worktree list, I am not sure whether will be better to make this tuned via a config and/or a git parameter. (assuming this change is a good idea ;) ) - Perhaps the `(locked)` marker is not the best suitable way to output this information and we might need to come with a better way. - 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. Rafael Silva (2): teach `list` to mark locked worktree t2402: add test to locked linked worktree marker Documentation/git-worktree.txt | 5 +++-- builtin/worktree.c | 6 +++++- t/t2402-worktree-list.sh | 13 +++++++++++++ 3 files changed, 21 insertions(+), 3 deletions(-)