worktree: allow to (re)move worktrees with uninitialized submodules
diff mbox series

Message ID 20181216121239.10017-1-pclouds@gmail.com
State New
Headers show
Series
  • worktree: allow to (re)move worktrees with uninitialized submodules
Related show

Commit Message

Duy Nguyen Dec. 16, 2018, 12:12 p.m. UTC
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>
---
 builtin/worktree.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Comments

Eric Sunshine Dec. 16, 2018, 1:18 p.m. UTC | #1
On Sun, Dec 16, 2018 at 7:12 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> 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.
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -724,20 +725,32 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
>  static void validate_no_submodules(const struct worktree *wt)
>  {
> +       if (is_directory(worktree_git_path(wt, "modules")))
> +               die(_("working trees containing submodules cannot be moved or removed"));
> +
>         if (read_index_from(&istate, worktree_git_path(wt, "index"),
>                             get_worktree_git_dir(wt)) > 0) {
>                 [...]
> +                       found_submodules = 1;
> +                       break;
>         }
>         if (found_submodules)
>                 die(_("working trees containing submodules cannot be moved or removed"));

Not worth a re-roll, but an alternate way to structure this to avoid
duplicating the die() message would be:

    if (is_directory(...))
        found_submodules = 1;
    else if (read_index_from(...)) {
        ...
        found_submodules = 1;
        break;
    }
    if (found_submodules)
        die(...);

Patch
diff mbox series

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5e84026177..af03b83a26 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,32 @@  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 (is_directory(worktree_git_path(wt, "modules")))
+		die(_("working trees containing submodules cannot be moved or removed"));
+
 	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"));