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

On Thu, Apr 14, 2016 at 11:08 PM, Junio C Hamano [off-list ref] wrote:

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

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.

quoted

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.

quoted

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

quoted

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

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.

quoted

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

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

quoted

  [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