Re: [PATCH 6/6] worktree remove: new command

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

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

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

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

> +		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?

> +		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;
> +	}