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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 14, 2016, 16:08 UTC
Message-ID: <xmqqfuuoi35o.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1460553346-12985-1-git-send-email-pclouds@gmail.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.

Show 10 quoted lines

> 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?

Show 5 quoted lines

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

Show 15 quoted lines

>   [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(-)