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

From: Duy Nguyen <pclouds@gmail.com>
Date: Apr 15, 2016, 00:40 UTC
Message-ID: <CACsJy8DK863+rgseeYrQJ1db+xSeFfm8WsNvGBmJwD_pr1yMJQ@mail.gmail.com>
In-Reply-To: <xmqqfuuoi35o.fsf@gitster.mtv.corp.google.com>

On Thu, Apr 14, 2016 at 11:08 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 8 quoted lines

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

Hmm... I thought I didn't receive any comments last time.

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

To me, no. Duplicating the "err" looks weird. error_no() does not look good either. Though there's a couple of warning(..., strerror()), which could become warning_errno(). Then maybe error_errno() makes more sense because all three follow the same naming convention.

Show 11 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?"

We need directory move functionality. In the worst case when rename(<dir>, <dir>) wouldn't do the job, we have to fall back to copying the whole directory over, preserving metadata as much as possible, then delete the old directory. I don't want to write new code for this because I think it shows in busybox code that there are pitfalls in dealing with filesystems. And I don't want to fall back to /bin/cp either. Windows won't have one (long term I hope we won't need msys) and *nix is not famous for consistent command line behavior. Plus, if we want to clean up a failed cp operation, it's easier to do it in core by keeping track of what files have been copied.

Show 5 quoted lines

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

Will do.

Show 12 quoted lines

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

Well. I think all the commands are there now at the end of this series. So we have add, list, prune, move, remove, lock and unlock. I guess we can group list/add/move/remove together and the rest as support commands. I might add "git worktree migrate" for converting between worktree v0 and v1. But it's not clear yet.

Show 6 quoted lines

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

Pretty much for convenience. Will add some more in commit message.

Show 9 quoted lines

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

Sure.

-- 
Duy