Re: [PATCH 4/7] worktree move: accept destination as directory
From: Eric Sunshine <hidden>
Date: 2018-02-02 09:35:25
On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Similar to "mv a b/", which is actually "mv a b/a", we extract basename of source worktree and create a directory of the same name at destination if dst path is a directory. Signed-off-by: Nguyễn Thái Ngọc Duy <redacted> ---diff --git a/builtin/worktree.c b/builtin/worktree.c@@ -624,8 +624,6 @@ static int move_worktree(int ac, const char **av, const char *prefix) path = prefix_filename(prefix, av[1]); strbuf_addstr(&dst, path); free(path); - if (file_exists(dst.buf)) - die(_("target '%s' already exists"), av[1]);
Nit: The distance between this "removed" conditional and the code inserted below hampered review a bit since it made it slightly more onerous to check for unwanted logic changes. Had patch 3/7 located this conditional below the is_main_worktree() check (where the new code is inserted below), this 'if' would have merely mutated into an 'else if' but not otherwise moved, thus review would have been simplified. (Not itself worth a re-roll.)
quoted hunk ↗ jump to hunk
worktrees = get_worktrees(0); wt = find_worktree(worktrees, prefix, av[0]);@@ -633,6 +631,20 @@ static int move_worktree(int ac, const char **av, const char *prefix) die(_("'%s' is not a working tree"), av[0]); if (is_main_worktree(wt)) die(_("'%s' is a main working tree"), av[0]); + + if (is_directory(dst.buf)) { + const char *sep = find_last_dir_sep(wt->path); + + if (!sep) + die(_("could not figure out destination name from '%s'"), + wt->path); + strbuf_addstr(&dst, sep);
Do we know at this point whether 'dst' has a terminating "/"? If it does, then this addstr() could result in "//" in the path which might be problematic on Windows.
+ if (file_exists(dst.buf))
+ die(_("target '%s' already exists"), dst.buf);
+ } else if (file_exists(dst.buf)) {
+ die(_("target '%s' already exists"), av[1]);
+ }
I wonder if it makes sense to collapse the duplicated
file_exists()/"target already exists" code?
if (is_directory(...)) {
...
strbuf_addstr(&dst, sep);
}
if (file_exists(dst.buf))
die(_("target '%s' already exists"), dst.buf);
It changes the error message slightly for the non-directory case but
simplifies the code a bit.
reason = is_worktree_locked(wt);
if (reason) {
if (*reason)Perhaps add a test to t2028-worktree-move.sh to show that this new behavior works?