git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] worktree: allow to (re)move worktrees with uninitialized submodules
@ 2018-12-16 12:12 Nguyễn Thái Ngọc Duy
  2018-12-16 13:18 ` Eric Sunshine
  2018-12-16 14:46 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
  0 siblings, 2 replies; 6+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-12-16 12:12 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy

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(-)

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"));
-- 
2.20.0.482.g66447595a7


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] worktree: allow to (re)move worktrees with uninitialized submodules
  2018-12-16 12:12 [PATCH] worktree: allow to (re)move worktrees with uninitialized submodules Nguyễn Thái Ngọc Duy
@ 2018-12-16 13:18 ` Eric Sunshine
  2018-12-16 14:46 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
  1 sibling, 0 replies; 6+ messages in thread
From: Eric Sunshine @ 2018-12-16 13:18 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: Git List

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(...);

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] worktree: allow to (re)move worktrees with uninitialized submodules
  2018-12-16 12:12 [PATCH] worktree: allow to (re)move worktrees with uninitialized submodules Nguyễn Thái Ngọc Duy
  2018-12-16 13:18 ` Eric Sunshine
@ 2018-12-16 14:46 ` Nguyễn Thái Ngọc Duy
  2019-01-04 22:51   ` Junio C Hamano
  2019-01-05  5:08   ` [PATCH v3] " Nguyễn Thái Ngọc Duy
  1 sibling, 2 replies; 6+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-12-16 14:46 UTC (permalink / raw)
  To: pclouds, sunshine; +Cc: git

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] worktree: allow to (re)move worktrees with uninitialized submodules
  2018-12-16 14:46 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
@ 2019-01-04 22:51   ` Junio C Hamano
  2019-01-06  7:01     ` Eric Sunshine
  2019-01-05  5:08   ` [PATCH v3] " Nguyễn Thái Ngọc Duy
  1 sibling, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2019-01-04 22:51 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: sunshine, git

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

> 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(-)

Is this a fair description for this 1-patch topic?

	"git worktree remove" and "git worktree move" failed to work
	when there is an uninitialized submodule, which has been fixed.

If so, can we have a test case to cover this fix?

Thanks.

> 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"));

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3] worktree: allow to (re)move worktrees with uninitialized submodules
  2018-12-16 14:46 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
  2019-01-04 22:51   ` Junio C Hamano
@ 2019-01-05  5:08   ` Nguyễn Thái Ngọc Duy
  1 sibling, 0 replies; 6+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-01-05  5:08 UTC (permalink / raw)
  To: pclouds; +Cc: git, sunshine, Junio C Hamano

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>
---
 v3 adds tests. This test file is being renamed by
 tg/checkout-no-overlay but it seems rename is handled correctly, no
 merge conflicts on 'pu'.

 builtin/worktree.c       | 29 +++++++++++++++++++++++------
 t/t2028-worktree-move.sh | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 60 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"));
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 33c0337733..939d18d728 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -112,6 +112,26 @@ test_expect_success 'move locked worktree (force)' '
 	git worktree move --force --force flump ploof
 '
 
+test_expect_success 'move a repo with uninitialized submodule' '
+	git init withsub &&
+	(
+		cd withsub &&
+		test_commit initial &&
+		git submodule add "$PWD"/.git sub &&
+		git commit -m withsub &&
+		git worktree add second HEAD &&
+		git worktree move second third
+	)
+'
+
+test_expect_success 'not move a repo with initialized submodule' '
+	(
+		cd withsub &&
+		git -C third submodule update &&
+		test_must_fail git worktree move third forth
+	)
+'
+
 test_expect_success 'remove main worktree' '
 	test_must_fail git worktree remove .
 '
@@ -185,4 +205,21 @@ test_expect_success 'remove cleans up .git/worktrees when empty' '
 	)
 '
 
+test_expect_success 'remove a repo with uninitialized submodule' '
+	(
+		cd withsub &&
+		git worktree add to-remove HEAD &&
+		git worktree remove to-remove
+	)
+'
+
+test_expect_success 'not remove a repo with initialized submodule' '
+	(
+		cd withsub &&
+		git worktree add to-remove HEAD &&
+		git -C to-remove submodule update &&
+		test_must_fail git worktree remove to-remove
+	)
+'
+
 test_done
-- 
2.20.0.482.g66447595a7


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] worktree: allow to (re)move worktrees with uninitialized submodules
  2019-01-04 22:51   ` Junio C Hamano
@ 2019-01-06  7:01     ` Eric Sunshine
  0 siblings, 0 replies; 6+ messages in thread
From: Eric Sunshine @ 2019-01-06  7:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, Git List

On Fri, Jan 4, 2019 at 5:51 PM Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> > 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.
>
> Is this a fair description for this 1-patch topic?
>
>         "git worktree remove" and "git worktree move" failed to work
>         when there is an uninitialized submodule, which has been fixed.

Saying "failed to work" makes it sound as if those two subcommands are
broken or buggy, which is not the case. Instead, they are overly
cautious and refuse to allow the operation if _any_ submodule
(initialized or not) is detected. This patch just loosens that check
to allow the operations to succeed for uninitialized submodules, thus
avoiding this (existing) annoying behavior, using git.git as an
example:

    % cd git
    % git worktree add --detach ../foo
    % git worktree remove ../foo
    fatal: working trees containing submodules cannot be moved or removed

Perhaps the change could instead be summarized as:

    "git worktree remove" and "git worktree move" were overly
    cautious, refusing to operate if any submodule was detected, even
    an uninitialized one. This safeguard has been refined to take only
    initialized submodules into account since worktrees with
    uninitialized ones can be safely removed and moved.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-01-06  7:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-16 12:12 [PATCH] worktree: allow to (re)move worktrees with uninitialized submodules Nguyễn Thái Ngọc Duy
2018-12-16 13:18 ` Eric Sunshine
2018-12-16 14:46 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
2019-01-04 22:51   ` Junio C Hamano
2019-01-06  7:01     ` Eric Sunshine
2019-01-05  5:08   ` [PATCH v3] " Nguyễn Thái Ngọc Duy

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git