[PATCH] Let submodule command exit with error status if path does not exist
Previously the exit status of git submodule was zero for various
subcommands even though the user specified an unknown path.
The reason behind that was that they all pipe the output of module_list
into the while loop which then does the action on the paths specified by
the commandline. Since piped commands are run in parallel the status
code of module_list was swallowed.
We work around this by introducing a new function module_list_valid
which is used to check the leftover commandline parameters passed to
module_list.
Signed-off-by: Heiko Voigt hvo...@hvoigt.net
---
git-submodule.sh | 19 ++-
t/t7400-submodule-basic.sh | 26 ++
2 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index aac575e..1fd21da 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -103,13 +103,21 @@ resolve_relative_url ()
echo ${is_relative:+${up_path}}${remoteurl#./}
}
+module_list_ls_files() {
+ git ls-files --error-unmatch --stage -- $@
+}
+
+module_list_valid() {
+ module_list_ls_files $@ /dev/null
+}
+
#
# Get submodule info for registered submodules
# $@ = path to limit submodule list
#
module_list()
{
- git ls-files --error-unmatch --stage -- $@ |
+ module_list_ls_files $@ |
perl -e '
my %unmerged = ();
my ($null_sha1) = (0 x 40);
@@ -434,6 +442,8 @@ cmd_init()
shift
done
+ module_list_valid $@ || exit 1
+
module_list $@ |
while read mode sha1 stage sm_path
do
@@ -532,6 +542,8 @@ cmd_update()
cmd_init -- $@ || return
fi
+ module_list_valid $@ || exit 1
+
cloned_modules=
module_list $@ | {
err=
@@ -929,6 +941,8 @@ cmd_status()
shift
done
+ module_list_valid $@ || exit 1
+
module_list $@ |
while read mode sha1 stage sm_path
do
@@ -996,6 +1010,9 @@ cmd_sync()
;;
esac
done
+
+ module_list_valid $@ || exit 1
+
cd_to_toplevel
module_list $@ |
while read mode sha1 stage sm_path
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index c73bec9..3a40334 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -258,6 +258,27 @@ test_expect_success 'init should register submodule url in
.git/config' '
test_cmp expect url
'
+test_failure_with_unknown_submodule() {
+ test_must_fail git submodule $1 no-such-submodule 2output.err
+ grep ^error: .*no-such-submodule output.err
+}
+
+test_expect_success 'init should fail with unknown submodule' '
+ test_failure_with_unknown_submodule init
+'
+
+test_expect_success 'update should fail with unknown submodule' '
+ test_failure_with_unknown_submodule update
+'
+
+test_expect_success 'status should fail with unknown submodule' '
+ test_failure_with_unknown_submodule status
+'
+
+test_expect_success 'sync should fail with unknown submodule' '
+ test_failure_with_unknown_submodule sync
+'
+
test_expect_success 'update should fail when path is used by a file' '
echo hello expect
@@ -418,10 +439,7 @@ test_expect_success 'moving to a commit without submodule
does not leave empty d
'
test_expect_success 'submodule invalid-path warns' '
-
- git submodule no-such-submodule 2 output.err
- grep ^error: .*no-such-submodule output.err
-
+ test_failure_with_unknown_submodule
'
test_expect_success 'add submodules without specifying an explicit path' '
--
1.7.12.rc2.10.g45a4861
--
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] Let submodule command exit with error status if path does not exist
Heiko Voigt hvo...@hvoigt.net writes:
Previously the exit status of git submodule was zero for various
subcommands even though the user specified an unknown path.
The reason behind that was that they all pipe the output of module_list
into the while loop which then does the action on the paths specified by
the commandline. Since piped commands are run in parallel the status
code of module_list was swallowed.
It is more like that the shell ignores the exit status of command
that is on the upstream side of a pipeline.
We work around this by introducing a new function module_list_valid
which is used to check the leftover commandline parameters passed to
module_list.
Doesn't it slow things down for the normal case, though?
A plausible hack, assuming all the problematic readers of the pipe
are of the form ... | while read mode sha1 stage sm_path, might be
to update module_list () to do something like:
(
git ls-files --error-unmatch ... ||
echo #unmatched
)
and then update the readers to catch #unmatched token, e.g.
module_list $@ |
while read mode sha1 stage sm_path
do
if test $mode = #unmatched
then
... do the necessary error thing ...
continue
fi
... whatever the loop originally did ...
done
One thing to note is that the above is not good if you want to
atomically reject
git submodule foo module1 moduel2
and error the whole thing out without touching module1 (which
exists) because of misspelt module2.
But is it what we want to see happen in these codepaths?
diff --git a/git-submodule.sh b/git-submodule.sh
index aac575e..1fd21da 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -103,13 +103,21 @@ resolve_relative_url ()
echo ${is_relative:+${up_path}}${remoteurl#./}
}
+module_list_ls_files() {
+ git ls-files --error-unmatch --stage -- $@
+}
+
+module_list_valid() {
+ module_list_ls_files $@ /dev/null
+}
+
This is a tangent, but among the 170 hits
git grep -e '^[a-z][a-z0-9A-Z_]* *(' -- './git-*.sh'
gives, about 120 have SP after funcname, i.e.
funcname () {
and 50 don't, i.e.
funcname() {
This file has 12 such definitions, among which 10 are the latter
form. There is no rational reason to choose between the two, but
having two forms in the same project hurts greppability. Updating
the style of existing code shouldn't be done in the same patch, but
please do not make things worse.
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index c73bec9..3a40334 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -258,6 +258,27 @@ test_expect_success 'init should register submodule url
in .git/config' '
test_cmp expect url
'
+test_failure_with_unknown_submodule() {
Likewise, even though inside t/ directory we seem to have more
offenders (190/480 ~ 40%, vs 50/170 ~ 30%).
+ test_must_fail git submodule $1 no-such-submodule 2output.err
+ grep ^error: .*no-such-submodule output.err
+}
I think the latter half already passes with the current code, but
the exit code from git submodule $1 would be corrected with this
patch, which is good.
Thanks.
--
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
