On Thu, Apr 14, 2016 at 11:08 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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. >> 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. >> [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. >> [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. >> [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. > >> [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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html