git submodule return status

git submodule return status

[Daniel Milde]

Hello,
if command git submodule update fails, return status is not set
properly.

Example:
$ git submodule update unknown
error: pathspec 'unknown' did not match any file(s) known to git.
Did you forget to 'git add'?
error: pathspec 'unknown' did not match any file(s) known to git.
Did you forget to 'git add'?
$ echo $?
0

I suppose the exit status should be 1 or something else, but not 0.

Best regards
Daniel Milde

[PATCH] Let submodule command exit with error status if path does not exist

[Heiko Voigt]

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 <hvoigt@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 2>output.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

Re: [PATCH] Let submodule command exit with error status if path does not exist

[Junio C Hamano]

Heiko Voigt <hvoigt@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 2>output.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.

[PATCH v2] Let submodule command exit with error status if path does not exist

[Heiko Voigt]

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 the exit code of piped commands is ignored by the
shell, the status code of module_list was swallowed.

We work around this by piping a submodule with an empty path and a null
sha1 as commit. This is necessary to pass through the perl snippet that
is used to select submodule entries. The while loop now checks for such
a submodule specification, exits with 1 and the exit code is propagated.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
On Thu, Aug 09, 2012 at 01:42:20PM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 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

Unfortunately it does not work that simple, but I have implemented
something like this in this patch.

> 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?

I think it is fine if we are working atomically for each submodule and
stop once we hit an unknown path. If we want to continue propagating the
error code will make the code more complex than is justified (IMO).

If the user wants to proceed its easier for him to correct his spelling. :-)

> 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.

[...]

> > +test_failure_with_unknown_submodule() {
> 
> Likewise, even though inside t/ directory we seem to have more
> offenders (190/480 ~ 40%, vs 50/170 ~ 30%).

I did not know that you prefer a space after the function name. I simply
imitated the style from C and there we do not have spaces. It makes the
style rules a bit more complicated. Wouldn't it be nicer to have the
same as in C so we have less rules?

Nevertheless, I adjusted the patch.

 git-submodule.sh           | 23 ++++++++++++++++++++++-
 t/t7400-submodule-basic.sh | 26 ++++++++++++++++++++++----
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index aac575e..48014f2 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -109,7 +109,8 @@ resolve_relative_url ()
 #
 module_list()
 {
-	git ls-files --error-unmatch --stage -- "$@" |
+	(git ls-files --error-unmatch --stage -- "$@" ||
+		echo '160000 0000000000000000000000000000000000000000 0	') |
 	perl -e '
 	my %unmerged = ();
 	my ($null_sha1) = ("0" x 40);
@@ -385,6 +386,10 @@ cmd_foreach()
 	module_list |
 	while read mode sha1 stage sm_path
 	do
+		if test -z "$sm_path"; then
+			exit 1
+		fi
+
 		if test -e "$sm_path"/.git
 		then
 			say "$(eval_gettext "Entering '\$prefix\$sm_path'")"
@@ -437,6 +442,10 @@ cmd_init()
 	module_list "$@" |
 	while read mode sha1 stage sm_path
 	do
+		if test -z "$sm_path"; then
+			exit 1
+		fi
+
 		name=$(module_name "$sm_path") || exit
 
 		# Copy url setting when it is not set yet
@@ -537,6 +546,10 @@ cmd_update()
 	err=
 	while read mode sha1 stage sm_path
 	do
+		if test -z "$sm_path"; then
+			exit 1
+		fi
+
 		if test "$stage" = U
 		then
 			echo >&2 "Skipping unmerged submodule $sm_path"
@@ -932,6 +945,10 @@ cmd_status()
 	module_list "$@" |
 	while read mode sha1 stage sm_path
 	do
+		if test -z "$sm_path"; then
+			exit 1
+		fi
+
 		name=$(module_name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
 		displaypath="$prefix$sm_path"
@@ -1000,6 +1017,10 @@ cmd_sync()
 	module_list "$@" |
 	while read mode sha1 stage sm_path
 	do
+		if test -z "$sm_path"; then
+			exit 1
+		fi
+
 		name=$(module_name "$sm_path")
 		url=$(git config -f .gitmodules --get submodule."$name".url)
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index c73bec9..56a81cd 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 2>output.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.gaf2525e

Re: [PATCH v2] Let submodule command exit with error status if path does not exist

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> I did not know that you prefer a space after the function name. I simply
> imitated the style from C and there we do not have spaces. It makes the
> style rules a bit more complicated. Wouldn't it be nicer to have the
> same as in C so we have less rules?

I do not think so, as they are different languages.  The call site
of C functions have name and opening parenthesis without a SP in
between.  The call site of shell functions do not even have
parentheses.

In any case, personal preferences (including mine) do not matter
much, as there is no "this is scientificly superiour" in styles.

> diff --git a/git-submodule.sh b/git-submodule.sh
> index aac575e..48014f2 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -109,7 +109,8 @@ resolve_relative_url ()
>  #
>  module_list()
>  {
> -	git ls-files --error-unmatch --stage -- "$@" |
> +	(git ls-files --error-unmatch --stage -- "$@" ||
> +		echo '160000 0000000000000000000000000000000000000000 0	') |

Is there a reason why the sentinel has to have the same mode pattern
as a GITLINK entry, NULL SHA-1, stage #0?  Or is the "path" being
empty all that matters?

Ah, OK, you did not want to touch the perl script downstream.  I
would have preferred a comment to document that, i.e.

	(
        	git ls-files --error-unmatch --stage -- "$@" ||
                # an entry with an empty $path used as a signal
                echo '160000 0.... 0 '
	) |
	perl -e '...

or

	(
		git ls-files --error-unmatch --stage -- "$@" ||
		echo 'unmatched pathspec exists'
	) |
	perl -e '
	...
	while (<STDIN>) {
		if (/^unmatched pathspec/) {
			print;
                        next;
		}
		chomp;
		my ($mode, $sha1, $stage, $path) = ...

Either way, the reader of the code would not have to scratch her
head that way.

>  	perl -e '
>  	my %unmerged = ();
>  	my ($null_sha1) = ("0" x 40);
> @@ -385,6 +386,10 @@ cmd_foreach()
>  	module_list |
>  	while read mode sha1 stage sm_path
>  	do
> +		if test -z "$sm_path"; then
> +			exit 1

Style:

	if test -z "$sm_path"
	then
		exit 1

I know module_list would have said "error: pathspec 'no-such' did
not match any file(s) known to git.  Did you forget to git add"
already, but because that comes at the very end of the input to the
filter written in perl (and with the way the filter is coded, it
will stay at the end), I am not sure if the user would notice it if
we exit like this.  By the time we hit this exit, we would have seen
"Entering $sm_path..." followed by whatever message given while in
the submodule for all the submodules that comes out of module_list,
no?

How about doing it this way to avoid that issue, to make sure we die
immediately after the typo is diagnosed without touching anything?

 git-submodule.sh | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 3aa7644..3f99d71 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -109,26 +109,47 @@ resolve_relative_url ()
 #
 module_list()
 {
-	git ls-files --error-unmatch --stage -- "$@" |
+	(
+		git ls-files --error-unmatch --stage -- "$@" ||
+		echo "unmatched pathspec exists"
+	) |
 	perl -e '
 	my %unmerged = ();
 	my ($null_sha1) = ("0" x 40);
+	my @out = ();
+	my $unmatched = 0;
 	while (<STDIN>) {
+		if (/^unmatched pathspec/) {
+			$unmatched = 1;
+			next;
+		}
 		chomp;
 		my ($mode, $sha1, $stage, $path) =
 			/^([0-7]+) ([0-9a-f]{40}) ([0-3])\t(.*)$/;
 		next unless $mode eq "160000";
 		if ($stage ne "0") {
 			if (!$unmerged{$path}++) {
-				print "$mode $null_sha1 U\t$path\n";
+				push @out, "$mode $null_sha1 U\t$path\n";
 			}
 			next;
 		}
-		print "$_\n";
+		push @out, "$_\n";
+	}
+	if ($unmatched) {
+		unshift @out, "#unmatched\n";
 	}
+	print for (@out);
 	'
 }
 
+check_unmatched ()
+{
+	if test "$1" = "#unmatched"
+	then
+		exit 1
+	fi
+}
+
 #
 # Map submodule path to submodule name
 #
@@ -385,6 +406,7 @@ cmd_foreach()
 	module_list |
 	while read mode sha1 stage sm_path
 	do
+		check_unmatched "$mode"
 		if test -e "$sm_path"/.git
 		then
 			say "$(eval_gettext "Entering '\$prefix\$sm_path'")"
@@ -437,6 +459,7 @@ cmd_init()
 	module_list "$@" |
 	while read mode sha1 stage sm_path
 	do
+		check_unmatched "$mode"
 		name=$(module_name "$sm_path") || exit
 
 		# Copy url setting when it is not set yet
@@ -537,6 +560,7 @@ cmd_update()
 	err=
 	while read mode sha1 stage sm_path
 	do
+		check_unmatched "$mode"
 		if test "$stage" = U
 		then
 			echo >&2 "Skipping unmerged submodule $sm_path"
@@ -932,6 +956,7 @@ cmd_status()
 	module_list "$@" |
 	while read mode sha1 stage sm_path
 	do
+		check_unmatched "$mode"
 		name=$(module_name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
 		displaypath="$prefix$sm_path"
@@ -1000,6 +1025,7 @@ cmd_sync()
 	module_list "$@" |
 	while read mode sha1 stage sm_path
 	do
+		check_unmatched "$mode"
 		name=$(module_name "$sm_path")
 		url=$(git config -f .gitmodules --get submodule."$name".url)
 

Re: [PATCH v2] Let submodule command exit with error status if path does not exist

[Heiko Voigt]

Hi Junio,

thanks for such a thorough review.

On Sat, Aug 11, 2012 at 10:43:22PM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> > I did not know that you prefer a space after the function name. I simply
> > imitated the style from C and there we do not have spaces. It makes the
> > style rules a bit more complicated. Wouldn't it be nicer to have the
> > same as in C so we have less rules?
> 
> I do not think so, as they are different languages.  The call site
> of C functions have name and opening parenthesis without a SP in
> between.  The call site of shell functions do not even have
> parentheses.
> 
> In any case, personal preferences (including mine) do not matter
> much, as there is no "this is scientificly superiour" in styles.

How about I update CodingGuidelines according to the rules you
suggested? Then other people know how we prefer bash functions and if
statements to look like.

> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index aac575e..48014f2 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -109,7 +109,8 @@ resolve_relative_url ()
> >  #
> >  module_list()
> >  {
> > -	git ls-files --error-unmatch --stage -- "$@" |
> > +	(git ls-files --error-unmatch --stage -- "$@" ||
> > +		echo '160000 0000000000000000000000000000000000000000 0	') |
> 
> Is there a reason why the sentinel has to have the same mode pattern
> as a GITLINK entry, NULL SHA-1, stage #0?  Or is the "path" being
> empty all that matters?
> 
> Ah, OK, you did not want to touch the perl script downstream.  I
> would have preferred a comment to document that, i.e.

I only described it in the commit message, sorry. Next time I will add a
comment.

> > @@ -385,6 +386,10 @@ cmd_foreach()
> >  	module_list |
> >  	while read mode sha1 stage sm_path
> >  	do
> > +		if test -z "$sm_path"; then
> > +			exit 1
> 
> Style:
> 
> 	if test -z "$sm_path"
> 	then
> 		exit 1

See above. If you agree I would add this style to the guidelines.

> I know module_list would have said "error: pathspec 'no-such' did
> not match any file(s) known to git.  Did you forget to git add"
> already, but because that comes at the very end of the input to the
> filter written in perl (and with the way the filter is coded, it
> will stay at the end), I am not sure if the user would notice it if
> we exit like this.  By the time we hit this exit, we would have seen
> "Entering $sm_path..." followed by whatever message given while in
> the submodule for all the submodules that comes out of module_list,
> no?
> 
> How about doing it this way to avoid that issue, to make sure we die
> immediately after the typo is diagnosed without touching anything?

I like it, your approach combines the atomicity of my first patch with
the efficiency of not calling ls-files twice. I was hesitant to change
to much code just to get the exit code right, but your approach looks
good to me.

Should I send an updated patch? Or do you just want to squash this in.
Since now only the tests are from me what should we do with the
ownership?

>  git-submodule.sh | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
[...]

Cheers Heiko

Re: [PATCH v2] Let submodule command exit with error status if path does not exist

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> How about I update CodingGuidelines according to the rules you
> suggested? Then other people know how we prefer bash functions and if
> statements to look like.

OK.  I was hoping that "imitate surrounding code" was sufficient,
but it seems many parts of the codebase have deteriorated enough to
make that rule no longer easy to follow.

>> Style:
>> 
>> 	if test -z "$sm_path"
>> 	then
>> 		exit 1
>
> See above. If you agree I would add this style to the guidelines.

Likewise.

>> I know module_list would have said "error: pathspec 'no-such' did
>> not match any file(s) known to git.  Did you forget to git add"
>> already, but because that comes at the very end of the input to the
>> filter written in perl (and with the way the filter is coded, it
>> will stay at the end), I am not sure if the user would notice it if
>> we exit like this.  By the time we hit this exit, we would have seen
>> "Entering $sm_path..." followed by whatever message given while in
>> the submodule for all the submodules that comes out of module_list,
>> no?
>> 
>> How about doing it this way to avoid that issue, to make sure we die
>> immediately after the typo is diagnosed without touching anything?
>
> I like it, your approach combines the atomicity of my first patch with
> the efficiency of not calling ls-files twice. I was hesitant to change
> to much code just to get the exit code right, but your approach looks
> good to me.
>
> Should I send an updated patch? Or do you just want to squash this in.
> Since now only the tests are from me what should we do with the
> ownership?

That is your itch and the idea and the bulk of the changes remains
to be yours in any case, methinks.

By the way, there is no reason for my patch to be unshifting the "#unmatch"
token into the array and spewing the array out, if the readers are always
going to stop upon seeing "#unmatch" without touching any submodule
that is named correctly on the command line.  In other words, the
following should suffice:

	while (<>) {
		... accumulate in @out ...
	}
	if ($unmatched) {
		print "#unmatched\n";
	} else {
		print for (@out);
	}

[PATCH v3] Let submodule command exit with error status if path does not exist

[Heiko Voigt]

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 the exit code of piped commands is ignored by the
shell, the status code of module_list was swallowed.

In case ls-files returns with an error code we pipe a special string
that is not possible in non error situations. If the perl filter behind
that encounters this string it outputs a single line with the special
tag '#unmatched'. This is then used by all reader from module_list to
die with an error code.

The error message that there is an unmatched pathspec comes through
stderr directly from ls-files. So the user still gets a hint whats going
on.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
Hi Junio,

this is an updated version with your proposal incorporated. I changed
the name of check_unmatched to die_if_unmatched because IMO it describes
more clearly what the function is doing.

Cheers Heiko

 git-submodule.sh           | 33 ++++++++++++++++++++++++++++++---
 t/t7400-submodule-basic.sh | 26 ++++++++++++++++++++++----
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index aac575e..0840524 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -109,26 +109,48 @@ resolve_relative_url ()
 #
 module_list()
 {
-	git ls-files --error-unmatch --stage -- "$@" |
+	(
+		git ls-files --error-unmatch --stage -- "$@" ||
+		echo "unmatched pathspec exists"
+	) |
 	perl -e '
 	my %unmerged = ();
 	my ($null_sha1) = ("0" x 40);
+	my @out = ();
+	my $unmatched = 0;
 	while (<STDIN>) {
+		if (/^unmatched pathspec/) {
+			$unmatched = 1;
+			next;
+		}
 		chomp;
 		my ($mode, $sha1, $stage, $path) =
 			/^([0-7]+) ([0-9a-f]{40}) ([0-3])\t(.*)$/;
 		next unless $mode eq "160000";
 		if ($stage ne "0") {
 			if (!$unmerged{$path}++) {
-				print "$mode $null_sha1 U\t$path\n";
+				push @out, "$mode $null_sha1 U\t$path\n";
 			}
 			next;
 		}
-		print "$_\n";
+		push @out, "$_\n";
+	}
+	if ($unmatched) {
+		print "#unmatched\n";
+	} else {
+		print for (@out);
 	}
 	'
 }
 
+die_if_unmatched ()
+{
+	if test "$1" = "#unmatched"
+	then
+		exit 1
+	fi
+}
+
 #
 # Map submodule path to submodule name
 #
@@ -385,6 +407,7 @@ cmd_foreach()
 	module_list |
 	while read mode sha1 stage sm_path
 	do
+		die_if_unmatched "$mode"
 		if test -e "$sm_path"/.git
 		then
 			say "$(eval_gettext "Entering '\$prefix\$sm_path'")"
@@ -437,6 +460,7 @@ cmd_init()
 	module_list "$@" |
 	while read mode sha1 stage sm_path
 	do
+		die_if_unmatched "$mode"
 		name=$(module_name "$sm_path") || exit
 
 		# Copy url setting when it is not set yet
@@ -537,6 +561,7 @@ cmd_update()
 	err=
 	while read mode sha1 stage sm_path
 	do
+		die_if_unmatched "$mode"
 		if test "$stage" = U
 		then
 			echo >&2 "Skipping unmerged submodule $sm_path"
@@ -932,6 +957,7 @@ cmd_status()
 	module_list "$@" |
 	while read mode sha1 stage sm_path
 	do
+		die_if_unmatched "$mode"
 		name=$(module_name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
 		displaypath="$prefix$sm_path"
@@ -1000,6 +1026,7 @@ cmd_sync()
 	module_list "$@" |
 	while read mode sha1 stage sm_path
 	do
+		die_if_unmatched "$mode"
 		name=$(module_name "$sm_path")
 		url=$(git config -f .gitmodules --get submodule."$name".url)
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index c73bec9..56a81cd 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 2>output.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.11.g5d52328

Re: [PATCH v3] Let submodule command exit with error status if path does not exist

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> Previously the exit status of git submodule was zero for various
> subcommands even though the user specified an unknown path.

As any patch that fixes behaviour deals with "Previously", I'd
prefer to omit it and describe the current problem in present tense
instead.

Will queue with minor tweaks.  Thanks.

> this is an updated version with your proposal incorporated. I changed
> the name of check_unmatched to die_if_unmatched because IMO it describes
> more clearly what the function is doing.

In general, I would actually prefer to call this kind of function
"check".  That way, all the call sites only need to be aware that
there is a check done there, without having to know what happens
when the check triggers, and the implementation of "check" could
decide that dying is too much and weaken the behaviour to only warn
in later updates.

Such an update would not easily apply for this particular case
because you would need to spit out all of @out from the module_list
before giving the "#unmatched" warning token, and find a way to
buffer the error message from ls-files so that it can be given when
the warning is issued at the end if we wanted to weaken this to
warn.  So in this particular case, I do not mind renaming it to
die-if.

Thanks.