Re: [PATCH 00/25] worktree lock, move, remove and unlock

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This is basically a resend from last time, which happened during rc
> time.

It would have made them a much more pleasant read if you re-read
them during that time and added the missing "why" to many of the
commit log message.

> It adds 4 more commands, basically cleaning up the "TODO" list
> in git-worktree.txt.
>
> So far I've only actually used move and remove (and maybe unlock once
> because worktree-add failed on me and I had to unlock it manually).
> And I don't get to move worktrees a lot either so not really extensive
> testing.
>
>   [01/25] usage.c: move format processing out of die_errno()
>   [02/25] usage.c: add sys_error() that prints strerror() automatically

This looks parallel to die_errno(); isn't error_errno() a better name?

>   [03/25] copy.c: import copy_file() from busybox
>   [04/25] copy.c: delete unused code in copy_file()
>   [05/25] copy.c: convert bb_(p)error_msg to (sys_)error
>   [06/25] copy.c: style fix
>   [07/25] copy.c: convert copy_file() to copy_dir_recursively()

Somewhere among these, there needs to be a single overview of why we
want "cp" implementation of busybox, e.g. what part of "cp" we want?
the whole thing?  or "because this is to be used from this and that
codepaths to make copy of these things, we only need these parts and
can remove other features like this and that?"

>   [08/25] completion: support git-worktree
>   [09/25] git-worktree.txt: keep subcommand listing in alphabetical order

I'd defer doing this immediately before 21 if I were doing this
series.

Offhand, I think it makes it easier to look things up in an
alphabetical list in the description section, but it probably is
easier to get an overview if the synopsis part groups things along
concepts and/or lists things along the order in typical workflows
(e.g. "create, list, rename, remove" would be a list along
lifecycle), not alphabetical.

But such judgement is better done when we know what are the final
elements that are to be listed, i.e. closer to where new things are
introduced.  This is especially true, as the log messages of patches
leading to 21 are all sketchy and do not give the readers a good
birds-view picture.

>   [10/25] path.c: add git_common_path() and strbuf_git_common_path()

Write "what for" when adding a new API function.  "Wanting to learn
X is very common and there are many existing code or new code that
repeats sequence A, B and C to compute it.  Give a helper function
to do so to refactor the existing codepaths" or something like that?

Move some part of [12/25] that is not about "store 'id'" but is
about this refactoring to this commit.

>   [11/25] worktree.c: use is_dot_or_dotdot()
>   [12/25] worktree.c: store "id" instead of "git_dir"

It is better to have these (and other conceptually "small and
obvious" ones) as preliminary clean-up to make the main body of the
series that may need to go through iterations smaller.

>   [13/25] worktree.c: add clear_worktree()
>   [14/25] worktree.c: add find_worktree_by_path()
>   [15/25] worktree.c: add is_main_worktree()
>   [16/25] worktree.c: add validate_worktree()
>   [17/25] worktree.c: add update_worktree_location()
>   [18/25] worktree.c: add is_worktree_locked()
>   [19/25] worktree: avoid 0{40}, too many zeroes, hard to read
>   [20/25] worktree: simplify prefixing paths
>   [21/25] worktree: add "lock" command
>   [22/25] worktree: add "unlock" command
>   [23/25] worktree: add "move" commmand
>   [24/25] worktree move: accept destination as directory
>   [25/25] worktree: add "remove" command
>
> Total 11 files changed, 1028 insertions(+), 48 deletions(-)