git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 0/7] teach `worktree list` verbose mode and prunable annotations

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Jan 6, 2021, 05:36 UTC
Message-ID
<CAPig+cSMA0J9fBvG=L0ojWvd5ePwxb6ya67umX1HdOy4LEiZpg@mail.gmail.com>
In-Reply-To
<20210104162128.95281-1-rafaeloliveira.cs@gmail.com>

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote:

Show 15 quoted lines
> In c57b3367be (worktree: teach `list` to annotate locked worktree,
> 2020-10-11) we taught `git worktree list` to annotate working tree that
> is locked by appending "locked" text in order to signalize to the user
> that a working tree is locked.  During the review, there was some
> discussion about additional annotations and information that `list`
> command could provide to the user that has long been envisioned and
> mentioned in [2], [3] and [4].
>
> This patch series address some of these changes by teaching
> `worktree list` to show "prunable" annotation, adding verbose mode and
> extending the --porcelain format with prunable and locked annotation as
> follow up from [1]. Additionally, it address one shortcoming for porcelain
> format to escape any newline characters (LF and CRLF) for the lock reason
> to prevent breaking format mentioned in [4] and [1] during the review
> cycle.

Thank you for working on this. I'm happy to see these long-envisioned enhancements finally taking shape. Before even reviewing the patches, I decided to apply them and play with the new features, and I'm very pleased to see that they behave exactly as I had envisioned all those years ago.

Very nicely done.
I'll review the patches when I finish responding to this cover letter.
Show 15 quoted lines
> The first moves the should_prune_worktree() machinery to the top-level
> worktree.c exposing the function as general API, that will be reference
> by should_prune_worktree() wrapper implemented on the second patch. The
> original idea was to not only move should_prune_worktree() but also
> refactor to accept "struct worktree" and load the information directly,
> which can simplify the `prune` command by reusing get_worktrees().
> However this seems to also require refactoring get_worktrees() itself
> to return "non-valid" working trees that can/should be pruned. This is
> also mentioned in [5]. Having the wrapper function makes it easier to add
> the prunable annotation without touching the get_worktrees() and the
> other worktree sub commands.  The refactoring can be addressed in a
> future patch, if this turns out to be good idea.  One possible approach
> is to teach get_worktrees() to take additional flags that will tell
> whether to return only valid or all worktrees in GIT_DIR/worktrees/
> directory and address its own possible shortcoming, if any.
I haven't looked at the patches yet, so I can't respond to this right now.
Show 5 quoted lines
> The third patch changes worktree_lock_reason() to be more gentle for the
> main working tree to simply returning NULL instead of aborting the
> program via assert() macro. This allow us to simplify the code that
> checks if the working tree is locked for default and porcelain format.
> This changes is also mentioned in [6].
Sounds good.
Show 5 quoted lines
> The forth patch is the one that teaches `list` command to show prunable
> annotation, for both default and porcelain format, and adds the verbose
> option. These changes are also done for the "locked" annotation that was
> not done on in [1]. Additionally, `list` learns to fail when both
> --verbose and --porcelain format as used together.

My knee-jerk response to disallowing --verbose and --porcelain together was that it seemed unnecessarily harsh, but upon reflection, I think it's the correct thing to do. After all, porcelain output, even though extensible and flexible, should be predictable, so it doesn't make sense for options such as --verbose to affect it.

Show 9 quoted lines
> The fifth patch adds worktree_escape_reason() that accepts a (char *)
> text and returned the text with any LF or CRLF escaped. The caller is
> responsible to freeing the escaped text. This is used by the locked
> annotation in porcelain format. Currently, this is defined within
> builtin/worktree.c as I was not sure whether libfying the function as
> part of this series is a good idea. At this time it seems more sensible
> to leave the code internally and libfying later once we are confident
> about the implementation and whether it can be used in other part of the
> code base but I'm open for suggestion.

Perhaps I misunderstand, but I had envisioned employing one of the codebase's existing quoting/escaping functions rather than crafting a new one from scratch. However, I'll reserve judgment until I actually read the patch.

Previous: Rafael SilvaNext: Rafael Silva
Message 33 of 88 in “teach `worktree list` verbose mode and prunable annotations”
  1. 0/7 teach `worktree list` verbose mode and prunable annotationsRafael Silva, Jan 4, 2021
  2. 3/7 worktree: teach worktree_lock_reason() to gently handle main worktreeRafael Silva, Jan 4, 2021
  3. Eric SunshineJan 6, 2021
  4. Rafael SilvaJan 8, 2021
  5. 6/7 worktree: add tests for `list` verbose and annotationsRafael Silva, Jan 4, 2021
  6. Eric SunshineJan 6, 2021
  7. Eric SunshineJan 7, 2021
  8. Rafael SilvaJan 8, 2021
  9. 4/7 worktree: teach `list` prunable annotation and verboseRafael Silva, Jan 4, 2021
  10. Eric SunshineJan 6, 2021
  11. Rafael SilvaJan 8, 2021
  12. 7/7 worktree: document `list` verbose and prunable annotationsRafael Silva, Jan 4, 2021
  13. Eric SunshineJan 6, 2021
  14. Rafael SilvaJan 8, 2021
  15. 5/7 worktree: `list` escape lock reason in --porcelainRafael Silva, Jan 4, 2021
  16. Phillip WoodJan 5, 2021
  17. worktree: add -z option for list subcommandPhillip Wood, Jan 5, 2021
  18. Eric SunshineJan 7, 2021
  19. Phillip WoodJan 8, 2021
  20. Eric SunshineJan 10, 2021
  21. Eric SunshineJan 6, 2021
  22. Rafael SilvaJan 8, 2021
  23. Eric SunshineJan 6, 2021
  24. 1/7 worktree: move should_prune_worktree() to worktree.cRafael Silva, Jan 4, 2021
  25. Eric SunshineJan 6, 2021
  26. Rafael SilvaJan 8, 2021
  27. Eric SunshineJan 6, 2021
  28. Eric SunshineJan 7, 2021
  29. Rafael SilvaJan 8, 2021
  30. 2/7 worktree: implement worktree_prune_reason() wrapperRafael Silva, Jan 4, 2021
  31. Eric SunshineJan 6, 2021
  32. Rafael SilvaJan 8, 2021
  33. Eric SunshineJan 6, 2021
  34. Rafael SilvaJan 8, 2021
  35. Eric SunshineJan 8, 2021
  36. 0/6 teach `worktree list` verbose mode and prunable annotationsRafael Silva, Jan 17, 2021
  37. 1/6 worktree: libify should_prune_worktree()Rafael Silva, Jan 17, 2021
  38. 4/6 worktree: teach `list --porcelain` to annotate locked worktreeRafael Silva, Jan 17, 2021
  39. Eric SunshineJan 18, 2021
  40. Rafael SilvaJan 19, 2021
  41. Eric SunshineJan 19, 2021
  42. 5/6 worktree: teach `list` to annotate prunable worktreeRafael Silva, Jan 17, 2021
  43. Eric SunshineJan 18, 2021
  44. Rafael SilvaJan 19, 2021
  45. Eric SunshineJan 19, 2021
  46. 6/6 worktree: teach `list` verbose modeRafael Silva, Jan 17, 2021
  47. Eric SunshineJan 18, 2021
  48. Eric SunshineJan 18, 2021
  49. 2/6 worktree: teach worktree to lazy-load "prunable" reasonRafael Silva, Jan 17, 2021
  50. Eric SunshineJan 18, 2021
  51. Rafael SilvaJan 19, 2021
  52. 3/6 worktree: teach worktree_lock_reason() to gently handle main worktreeRafael Silva, Jan 17, 2021
  53. Eric SunshineJan 18, 2021
  54. Rafael SilvaJan 19, 2021
  55. 0/7 teach `worktree list` verbose mode and prunable annotationsRafael Silva, Jan 19, 2021
  56. 1/7 worktree: libify should_prune_worktree()Rafael Silva, Jan 19, 2021
  57. 7/7 worktree: teach `list` verbose modeRafael Silva, Jan 19, 2021
  58. Eric SunshineJan 24, 2021
  59. Rafael SilvaJan 24, 2021
  60. 2/7 worktree: teach worktree to lazy-load "prunable" reasonRafael Silva, Jan 19, 2021
  61. 6/7 worktree: teach `list` to annotate prunable worktreeRafael Silva, Jan 19, 2021
  62. Junio C HamanoJan 21, 2021
  63. Rafael SilvaJan 21, 2021
  64. Junio C HamanoJan 21, 2021
  65. 3/7 worktree: teach worktree_lock_reason() to gently handle main worktreeRafael Silva, Jan 19, 2021
  66. 4/7 t2402: ensure locked worktree is properly cleaned upRafael Silva, Jan 19, 2021
  67. Eric SunshineJan 24, 2021
  68. Rafael SilvaJan 24, 2021
  69. 5/7 worktree: teach `list --porcelain` to annotate locked worktreeRafael Silva, Jan 19, 2021
  70. Phillip WoodJan 20, 2021
  71. Junio C HamanoJan 21, 2021
  72. Rafael SilvaJan 21, 2021
  73. Eric SunshineJan 24, 2021
  74. Eric SunshineJan 24, 2021
  75. Rafael SilvaJan 24, 2021
  76. Eric SunshineJan 24, 2021
  77. Rafael SilvaJan 27, 2021
  78. 0/7 teach `worktree list` verbose mode and prunable annotationsRafael Silva, Jan 27, 2021
  79. 2/7 worktree: teach worktree to lazy-load "prunable" reasonRafael Silva, Jan 27, 2021
  80. 6/7 worktree: teach `list` to annotate prunable worktreeRafael Silva, Jan 27, 2021
  81. 5/7 worktree: teach `list --porcelain` to annotate locked worktreeRafael Silva, Jan 27, 2021
  82. 7/7 worktree: teach `list` verbose modeRafael Silva, Jan 27, 2021
  83. 3/7 worktree: teach worktree_lock_reason() to gently handle main worktreeRafael Silva, Jan 27, 2021
  84. 1/7 worktree: libify should_prune_worktree()Rafael Silva, Jan 27, 2021
  85. 4/7 t2402: ensure locked worktree is properly cleaned upRafael Silva, Jan 27, 2021
  86. Eric SunshineJan 30, 2021
  87. Rafael SilvaJan 30, 2021
  88. Junio C HamanoJan 30, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.