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

Hi Junio,

thanks for such a thorough review.

On Sat, Aug 11, 2012 at 10:43:22PM -0700, Junio C Hamano wrote:

Heiko Voigt [off-list ref] writes:

quoted

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.

quoted

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.

quoted

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