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

2013-04-17 Thread Jens Lehmann
Am 17.04.2013 07:16, schrieb Junio C Hamano:
 Phil Hord phil.h...@gmail.com writes:
 
 On Mon, Apr 1, 2013 at 3:02 PM, Jens Lehmann jens.lehm...@web.de 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)
}
 
 Hrm, -maxdepth and -empty are not even in POSIX.  Folks on GNU
 platforms and BSDs (I checked NetBSD 6 and OpenBSD 5.2) should be
 fine, but it makes other platforms unhappy.

Ok, that is not acceptable.

 What is this check used for?  To avoid running rmdir on non-empty
 ones?  Saying cleaning foo/ (or cleaned foo/) when foo/ is
 already empty is not a crime; not omitting an empty one may actually
 be a better behaviour from the point of view of repeatability and
 uniformity.

It's no big issue, but 'init' issues the Submodule ... registered
for path ... message only once and is quiet on subsequent calls,
'deinit' does the same with Submodule ... unregistered for path
..., only the Cleared directory ... message appears each time
'deinit' is called, which makes it stand out. I do not believe
this little inconsistency is important enough to write a helper in
C (to have a portable way to see if the directory has already been
cleared), but this simple additional if looked easy enough. That
should have made me suspicious ;-)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-04-16 Thread Phil Hord
On Mon, Apr 1, 2013 at 3:02 PM, Jens Lehmann jens.lehm...@web.de 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-04-16 Thread Junio C Hamano
Phil Hord phil.h...@gmail.com writes:

 On Mon, Apr 1, 2013 at 3:02 PM, Jens Lehmann jens.lehm...@web.de 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)
}

Hrm, -maxdepth and -empty are not even in POSIX.  Folks on GNU
platforms and BSDs (I checked NetBSD 6 and OpenBSD 5.2) should be
fine, but it makes other platforms unhappy.

What is this check used for?  To avoid running rmdir on non-empty
ones?  Saying cleaning foo/ (or cleaned foo/) when foo/ is
already empty is not a crime; not omitting an empty one may actually
be a better behaviour from the point of view of repeatability and
uniformity.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] submodule deinit: clarify work tree removal message

2013-04-01 Thread Jens Lehmann
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 phil.h...@gmail.com
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---


Am 19.03.2013 02:45, schrieb Junio C Hamano:
 Jens Lehmann jens.lehm...@web.de writes:
 Am 12.03.2013 17:22, schrieb Junio C Hamano:
 Phil Hord phil.h...@gmail.com writes:

 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