[PATCH v2] worktree: allow to (re)move worktrees with uninitialized submodules
- From
Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
- Date
- Dec 16, 2018, 14:46 UTC
- Message-ID
- <20181216144657.31181-1-pclouds@gmail.com>
- In-Reply-To
- <20181216121239.10017-1-pclouds@gmail.com>
Uninitialized submodules have nothing valueable for us to be worried about. They are just SHA-1. Let "worktree remove" and "worktree move" continue in this case so that people can still use multiple worktrees on repos with optional submodules that are never populated, like sha1collisiondetection in git.git when checked out by doc-diff script.
Note that for "worktree remove", it is possible that a user initializes a submodule (*), makes some commits (but not push), then deinitializes it. At that point, the submodule is unpopulated, but the precious new commits are still in
$GIT_COMMON_DIR/worktrees/<worktree>/modules/<submodule>
directory and we should not allow removing the worktree or we lose those commits forever. The new directory check is added to prevent this.
(*) yes they are screwed anyway by doing this since "git submodule"
would add submodule.* in $GIT_COMMON_DIR/config, which is shared
across multiple worktrees. But it does not mean we let them be
screwed even more.Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- Fixed Eric's comment. I was a bit annoyed by the duplicate die() too but didn't think of adding "else" in front of "if (read_index"
builtin/worktree.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c index 5e84026177..3f9907fcc9 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -9,6 +9,7 @@ #include "refs.h" #include "run-command.h" #include "sigchain.h" +#include "submodule.h" #include "refs.h" #include "utf8.h" #include "worktree.h" @@ -724,20 +725,36 @@ static int unlock_worktree(int ac, const char **av, const char *prefix) static void validate_no_submodules(const struct worktree *wt) { struct index_state istate = { NULL }; + struct strbuf path = STRBUF_INIT; int i, found_submodules = 0; - if (read_index_from(&istate, worktree_git_path(wt, "index"), - get_worktree_git_dir(wt)) > 0) { + if (is_directory(worktree_git_path(wt, "modules"))) { + /* + * There could be false positives, e.g. the "modules" + * directory exists but is empty. But it's a rare case and + * this simpler check is probably good enough for now. + */ + found_submodules = 1; + } else if (read_index_from(&istate, worktree_git_path(wt, "index"), + get_worktree_git_dir(wt)) > 0) { for (i = 0; i < istate.cache_nr; i++) { struct cache_entry *ce = istate.cache[i]; + int err; - if (S_ISGITLINK(ce->ce_mode)) { - found_submodules = 1; - break; - } + if (!S_ISGITLINK(ce->ce_mode)) + continue; + + strbuf_reset(&path); + strbuf_addf(&path, "%s/%s", wt->path, ce->name); + if (!is_submodule_populated_gently(path.buf, &err)) + continue; + + found_submodules = 1; + break; } } discard_index(&istate); + strbuf_release(&path); if (found_submodules) die(_("working trees containing submodules cannot be moved or removed"));
-- 2.20.0.482.g66447595a7