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

Heiko Voigt [off-list ref] 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.

quoted

Style:

	if test -z "$sm_path"
	then
		exit 1

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

Likewise.

quoted

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

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