Re: [PATCH v4] submodule: add 'deinit' command

4 messages, 3 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH v4] submodule: add 'deinit' command

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:26

Jens Lehmann [off-list ref] writes:

Am 12.03.2013 17:22, schrieb Junio C Hamano:

quoted

Phil Hord [off-list ref] writes:

quoted

I think this would be clearer if 'git deinit' said

    rm 'submodule/*'

or maybe

    Removed workdir for 'submodule'

Is it just me?

The latter may probably be better.

Hmm, it doesn't really remove the directory but only empties it
(it recreates it a few lines after removing it together with its
contents). So what about

    Cleared directory 'submodule'

Sounds the cleanest among the suggested phrasing so far.

[PATCH] submodule deinit: clarify work tree removal message

From: Jens Lehmann <hidden>
Date: 2016-06-15 22:56:35

The output of "git submodule deinit sub" of a populated submodule prints

  rm 'sub'

as the first line unless used with the -f option.

The "rm 'sub'" line is exactly the same output the user gets when using
"git rm sub" (because that command is used with the --dry-run option under
the hood to determine if the submodule is clean), which can easily lead to
the false impression that the submodule would be permanently removed. Also
users might be confused that the "rm 'submodule'" line won't show up when
the -f option is used, as the test is skipped in this case.

Silence the "rm 'submodule'" output by using the --quiet option for "git
rm" and always print

  Cleared directory 'submodule'

instead as the first output line. This line is printed as long as the
directory exists, no matter if empty or not.

Also extend the tests in t7400 to make sure the "Cleared directory" line
is printed correctly.

Reported-by: Phil Hord <redacted>
Signed-off-by: Jens Lehmann <redacted>
---


Am 19.03.2013 02:45, schrieb Junio C Hamano:

Jens Lehmann [off-list ref] writes:

quoted

Am 12.03.2013 17:22, schrieb Junio C Hamano:

quoted

Phil Hord [off-list ref] writes:

quoted

I think this would be clearer if 'git deinit' said

    rm 'submodule/*'

or maybe

    Removed workdir for 'submodule'

Is it just me?

The latter may probably be better.

Hmm, it doesn't really remove the directory but only empties it
(it recreates it a few lines after removing it together with its
contents). So what about

    Cleared directory 'submodule'

Sounds the cleanest among the suggested phrasing so far.

Okay, so here is the patch for that. If someone could point out
a portable and efficient way to check if a directory is already
empty I would be happy to use that to silence the "Cleaned
directory" message currently printed also when deinit is run on
an already empty directory. Technically that is correct, as the
"rm -rf" is also executed, but I think it would be better to
skip the whole if block containing the "rm -rf" and "mkdir"
commands in that case as there is really nothing to do. Calling
"find" to see if there is anything inside the directory sounds
too expensive to me, as the directory will contain a lot of
files sometimes. But maybe this should be done in another patch.


 git-submodule.sh           |  6 ++++--
 t/t7400-submodule-basic.sh | 21 ++++++++++++++++-----
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 204bc78..d003e8a 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -601,10 +601,12 @@ cmd_deinit()

 			if test -z "$force"
 			then
-				git rm -n "$sm_path" ||
+				git rm -qn "$sm_path" ||
 				die "$(eval_gettext "Submodule work tree '\$sm_path' contains local modifications; use '-f' to discard them")"
 			fi
-			rm -rf "$sm_path" || say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")"
+			rm -rf "$sm_path" &&
+			say "$(eval_gettext "Cleared directory '\$sm_path'")" ||
+			say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")"
 		fi

 		mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$sm_path'")"

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 5030f1f..ff26535 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh

@@ -777,18 +777,22 @@ test_expect_success 'submodule deinit . deinits all initialized submodules' '
 	git config submodule.example.foo bar &&
 	git config submodule.example2.frotz nitfol &&
 	test_must_fail git submodule deinit &&
-	git submodule deinit . &&
+	git submodule deinit . >actual &&
 	test -z "$(git config --get-regexp "submodule\.example\.")" &&
 	test -z "$(git config --get-regexp "submodule\.example2\.")" &&
+	test_i18ngrep "Cleared directory .init" actual &&
+	test_i18ngrep "Cleared directory .example2" actual &&
 	rmdir init example2
 '

 test_expect_success 'submodule deinit deinits a submodule when its work tree is missing or empty' '
 	git submodule update --init &&
 	rm -rf init example2/* example2/.git &&
-	git submodule deinit init example2 &&
+	git submodule deinit init example2 >actual &&
 	test -z "$(git config --get-regexp "submodule\.example\.")" &&
 	test -z "$(git config --get-regexp "submodule\.example2\.")" &&
+	test_i18ngrep ! "Cleared directory .init" actual &&
+	test_i18ngrep "Cleared directory .example2" actual &&
 	rmdir init
 '

@@ -798,8 +802,9 @@ test_expect_success 'submodule deinit fails when the submodule contains modifica
 	test_must_fail git submodule deinit init &&
 	test -n "$(git config --get-regexp "submodule\.example\.")" &&
 	test -f example2/.git &&
-	git submodule deinit -f init &&
+	git submodule deinit -f init >actual &&
 	test -z "$(git config --get-regexp "submodule\.example\.")" &&
+	test_i18ngrep "Cleared directory .init" actual &&
 	rmdir init
 '

@@ -809,8 +814,9 @@ test_expect_success 'submodule deinit fails when the submodule contains untracke
 	test_must_fail git submodule deinit init &&
 	test -n "$(git config --get-regexp "submodule\.example\.")" &&
 	test -f example2/.git &&
-	git submodule deinit -f init &&
+	git submodule deinit -f init >actual &&
 	test -z "$(git config --get-regexp "submodule\.example\.")" &&
+	test_i18ngrep "Cleared directory .init" actual &&
 	rmdir init
 '

@@ -823,8 +829,9 @@ test_expect_success 'submodule deinit fails when the submodule HEAD does not mat
 	test_must_fail git submodule deinit init &&
 	test -n "$(git config --get-regexp "submodule\.example\.")" &&
 	test -f example2/.git &&
-	git submodule deinit -f init &&
+	git submodule deinit -f init >actual &&
 	test -z "$(git config --get-regexp "submodule\.example\.")" &&
+	test_i18ngrep "Cleared directory .init" actual &&
 	rmdir init
 '

@@ -832,14 +839,18 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su
 	git submodule update --init &&
 	git submodule deinit init >actual &&
 	test_i18ngrep "Submodule .example. (.*) unregistered for path .init" actual &&
+	test_i18ngrep "Cleared directory .init" actual &&
 	git submodule deinit init >actual &&
 	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
+	test_i18ngrep "Cleared directory .init" actual &&
 	git submodule deinit . >actual &&
 	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
 	test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual &&
+	test_i18ngrep "Cleared directory .init" actual &&
 	git submodule deinit . >actual &&
 	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
 	test_i18ngrep ! "Submodule .example2. (.*) unregistered for path .example2" actual &&
+	test_i18ngrep "Cleared directory .init" actual &&
 	rmdir init example2
 '

-- 
1.8.2.358.g5c06bcf

Re: [PATCH] submodule deinit: clarify work tree removal message

From: Phil Hord <hidden>
Date: 2016-06-15 22:56:51

On Mon, Apr 1, 2013 at 3:02 PM, Jens Lehmann [off-list ref] wrote:

Okay, so here is the patch for that. If someone could point out
a portable and efficient way to check if a directory is already
empty I would be happy to use that to silence the "Cleaned
directory" message currently printed also when deinit is run on
an already empty directory.

   isemptydir() {
        test -d "$(find $1 -maxdepth 0 -empty)"
   }

Sorry for the late reply.  I see this patch is already in master
(which is fine with me).

Thanks,
Phil

[PATCH] submodule deinit: don't output "Cleared directory" when directory is empty

From: Jens Lehmann <hidden>
Date: 2016-06-15 22:56:52

Currently a "submodule deinit" run on a non-populated submodule will still
print the "Cleared directory" message even though the directory is already
empty. While that is technically correct (as the directory is removed and
created again), it is rather surprising to see this message for an empty
submodule directory where nothing is to be cleared.

Fix that by using 'test ! -d "$(find "$sm_path" -maxdepth 0 -empty)"' to
test for the directory being not empty before removing and recreating it.

Thanks-to: Phil Hord [off-list ref]
Signed-off-by: Jens Lehmann <redacted>
---

Am 16.04.2013 15:32, schrieb Phil Hord:

On Mon, Apr 1, 2013 at 3:02 PM, Jens Lehmann [off-list ref] wrote:

quoted

Okay, so here is the patch for that. If someone could point out
a portable and efficient way to check if a directory is already
empty I would be happy to use that to silence the "Cleaned
directory" message currently printed also when deinit is run on
an already empty directory.

   isemptydir() {
        test -d "$(find $1 -maxdepth 0 -empty)"
   }

Sorry for the late reply.  I see this patch is already in master
(which is fine with me).

Thanks, I managed to miss that solution when googling for it.


 git-submodule.sh           | 35 +++++++++++++++++++----------------
 t/t7400-submodule-basic.sh |  8 ++++----
 2 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 79bfaac..52ecbf1 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -594,27 +594,30 @@ cmd_deinit()
 		die_if_unmatched "$mode"
 		name=$(module_name "$sm_path") || exit

-		# Remove the submodule work tree (unless the user already did it)
-		if test -d "$sm_path"
+		# Remove the submodule work tree (unless the user already did it or it is empty)
+		if test ! -d "$(find "$sm_path" -maxdepth 0 -empty)"
 		then
-			# Protect submodules containing a .git directory
-			if test -d "$sm_path/.git"
+			if test -d "$sm_path"
 			then
-				echo >&2 "$(eval_gettext "Submodule work tree '\$sm_path' contains a .git directory")"
-				die "$(eval_gettext "(use 'rm -rf' if you really want to remove it including all of its history)")"
-			fi
+				# Protect submodules containing a .git directory
+				if test -d "$sm_path/.git"
+				then
+					echo >&2 "$(eval_gettext "Submodule work tree '\$sm_path' contains a .git directory")"
+					die "$(eval_gettext "(use 'rm -rf' if you really want to remove it including all of its history)")"
+				fi

-			if test -z "$force"
-			then
-				git rm -qn "$sm_path" ||
-				die "$(eval_gettext "Submodule work tree '\$sm_path' contains local modifications; use '-f' to discard them")"
+				if test -z "$force"
+				then
+					git rm -qn "$sm_path" ||
+					die "$(eval_gettext "Submodule work tree '\$sm_path' contains local modifications; use '-f' to discard them")"
+				fi
+				rm -rf "$sm_path" &&
+				say "$(eval_gettext "Cleared directory '\$sm_path'")" ||
+				say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")"
 			fi
-			rm -rf "$sm_path" &&
-			say "$(eval_gettext "Cleared directory '\$sm_path'")" ||
-			say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")"
-		fi

-		mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$sm_path'")"
+			mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$sm_path'")"
+		fi

 		# Remove the .git/config entries (unless the user already did it)
 		if test -n "$(git config --get-regexp submodule."$name\.")"

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index ff26535..b56e4a5 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh

@@ -792,7 +792,7 @@ test_expect_success 'submodule deinit deinits a submodule when its work tree is
 	test -z "$(git config --get-regexp "submodule\.example\.")" &&
 	test -z "$(git config --get-regexp "submodule\.example2\.")" &&
 	test_i18ngrep ! "Cleared directory .init" actual &&
-	test_i18ngrep "Cleared directory .example2" actual &&
+	test_i18ngrep ! "Cleared directory .example2" actual &&
 	rmdir init
 '

@@ -842,15 +842,15 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su
 	test_i18ngrep "Cleared directory .init" actual &&
 	git submodule deinit init >actual &&
 	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
-	test_i18ngrep "Cleared directory .init" actual &&
+	test_i18ngrep ! "Cleared directory .init" actual &&
 	git submodule deinit . >actual &&
 	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
 	test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual &&
-	test_i18ngrep "Cleared directory .init" actual &&
+	test_i18ngrep ! "Cleared directory .init" actual &&
 	git submodule deinit . >actual &&
 	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
 	test_i18ngrep ! "Submodule .example2. (.*) unregistered for path .example2" actual &&
-	test_i18ngrep "Cleared directory .init" actual &&
+	test_i18ngrep ! "Cleared directory .init" actual &&
 	rmdir init example2
 '

-- 
1.8.2.1.419.g33f67ba

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help