Nguyễn Thái Ngọc Duy [off-list ref] 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(-)
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
On Thu, Apr 14, 2016 at 5:40 PM, Duy Nguyen [off-list ref] wrote:
On Thu, Apr 14, 2016 at 11:08 PM, Junio C Hamano [off-list ref] wrote:
quoted
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.
I think you've been here long enough to know that absense of comments
does not mean anything more than that: lack of interest at that moment in time.
quoted
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.
So in the end error_errno() would be a better name to you after all ;-)
I agree the stuttering sound coming from repeating error twice feels
somewhat odd, but warning_errno() would be so natural and obvious
future direction, so...
quoted
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.
Yeah, I know all that but you do not want to explain that to me only when
I asked in a mailing list response. You want to get into the habit of having
that in the log message to help reviewers, not just me, before they ask such
a question.
quoted
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.
Just so that there is no confusion, I am not opposed to reordering.
I was just saying that the decision on what the right ordering is can
be easier to make when the readers more or less know what the final
set of things in the set are, and because that becomes only clear when
they start reading 21, and because the log messages of patches
leading to 21 do not show the direction clearly enough, it is hard to
make that judgment at this point so early in the series.