Re: [PATCH 6/6] worktree remove: new command
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 21, 2017, 03:33 UTC
- Message-ID
- <xmqqo9vq5tag.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170420101024.7593-7-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 6 quoted lines
> + worktrees = get_worktrees(0);
> + wt = find_worktree(worktrees, prefix, av[0]);
> + if (!wt)
> + die(_("'%s' is not a working directory"), av[0]);
> + if (is_main_worktree(wt))
> + die(_("'%s' is a main working directory"), av[0]);The same comment as 3/6 applies here.
Show 6 quoted lines
> + reason = is_worktree_locked(wt);
> + if (reason) {
> + if (*reason)
> + die(_("already locked, reason: %s"), reason);
> + die(_("already locked, no reason"));
> + }The same comment as 3/6 applies here, too.
This is shared with 3/6 but I wonder if "--force" should be usable as a way to bust this refusal. There is an "unlock" operation, so probably such a short-cut is not necessary---if you want to repair your repository by moving or removing a working tree and if you cannot do so due to an outstanding lock, you can do a two-step dance "unlock followed by move or remove". So I am OK with "--force" that does not bust the lock.
Show 22 quoted lines
> + if (validate_worktree(wt, 0))
> + return -1;
> +
> + if (!force) {
> + struct argv_array child_env = ARGV_ARRAY_INIT;
> + struct child_process cp;
> + char buf[1];
> +
> + argv_array_pushf(&child_env, "%s=%s/.git",
> + GIT_DIR_ENVIRONMENT, wt->path);
> + argv_array_pushf(&child_env, "%s=%s",
> + GIT_WORK_TREE_ENVIRONMENT, wt->path);
> + memset(&cp, 0, sizeof(cp));
> + argv_array_pushl(&cp.args, "status", "--porcelain", NULL);
> + cp.env = child_env.argv;
> + cp.git_cmd = 1;
> + cp.dir = wt->path;
> + cp.out = -1;
> + ret = start_command(&cp);
> + if (ret)
> + die_errno(_("failed to run git-status on '%s', code %d"),
> + av[0], ret);Do we return "code" from start_command() that is usable like this?
Is this "git status --porcelain" call affected by settings like "submodule.*.ignore"? If so, is that a good thing?
Oh, submodules. Unlike "move" that may make their .git files pointing at strange places after the operation finishes, "remove" does not have to worry about them, because they are going to disappear--I think that is OK, but I could be missing some cases where a working tree that is not dirty may still want to be kept. I dunno.
Show 11 quoted lines
> + ret = xread(cp.out, buf, sizeof(buf));
> + if (ret)
> + die(_("'%s' is dirty, use --force to delete it"), av[0]);
> + close(cp.out);
> + ret = finish_command(&cp);
> + if (ret)
> + die_errno(_("failed to run git-status on '%s', code %d"),
> + av[0], ret);
> + }
> + strbuf_addstr(&sb, wt->path);
> + if (remove_dir_recursively(&sb, 0)) {Oh, submodules. If this working tree has submodules that are not yet absorbed, wouldn't this go into their ".git" recursively and end up losing everything?
Show 9 quoted lines
> + error_errno(_("failed to delete '%s'"), sb.buf);
> + ret = -1;
> + }
> + strbuf_reset(&sb);
> + strbuf_addstr(&sb, git_common_path("worktrees/%s", wt->id));
> + if (remove_dir_recursively(&sb, 0)) {
> + error_errno(_("failed to delete '%s'"), sb.buf);
> + ret = -1;
> + }