{"thread":{"id":"30924","subject":"git submodule return status","startedAt":"2012-06-28T08:28:00Z","lastAt":"2012-08-14T20:59:01Z","messageCount":9,"participants":["Daniel Milde","Heiko Voigt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"194408","messageId":"1340872080.2103.92.camel@athena.dnet","threadId":"30924","inReplyTo":null,"subject":"git submodule return status","fromName":"Daniel Milde","fromEmail":"daniel@milde.cz","sentAt":"2012-06-28T08:28:00Z","receivedAt":"2012-06-28T08:28:00Z","isPatch":false,"sender":{"key":"daniel@milde.cz","avatar":null},"body":"Hello,\nif command git submodule update fails, return status is not set\nproperly.\n\nExample:\n$ git submodule update unknown\nerror: pathspec 'unknown' did not match any file(s) known to git.\nDid you forget to 'git add'?\nerror: pathspec 'unknown' did not match any file(s) known to git.\nDid you forget to 'git add'?\n$ echo $?\n0\n\nI suppose the exit status should be 1 or something else, but not 0.\n\nBest regards\nDaniel Milde\n"},{"id":"196753","messageId":"20120809200302.GA93203@book.hvoigt.net","threadId":"30924","inReplyTo":"1340872080.2103.92.camel@athena.dnet","subject":"[PATCH] Let submodule command exit with error status if path does not exist","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-08-09T20:03:04Z","receivedAt":"2012-08-09T20:03:04Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Previously the exit status of git submodule was zero for various\nsubcommands even though the user specified an unknown path.\n\nThe reason behind that was that they all pipe the output of module_list\ninto the while loop which then does the action on the paths specified by\nthe commandline. Since piped commands are run in parallel the status\ncode of module_list was swallowed.\n\nWe work around this by introducing a new function module_list_valid\nwhich is used to check the leftover commandline parameters passed to\nmodule_list.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\n git-submodule.sh           | 19 ++++++++++++++++++-\n t/t7400-submodule-basic.sh | 26 ++++++++++++++++++++++----\n 2 files changed, 40 insertions(+), 5 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex aac575e..1fd21da 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -103,13 +103,21 @@ resolve_relative_url ()\n \techo \"${is_relative:+${up_path}}${remoteurl#./}\"\n }\n \n+module_list_ls_files() {\n+\tgit ls-files --error-unmatch --stage -- \"$@\"\n+}\n+\n+module_list_valid() {\n+\tmodule_list_ls_files \"$@\" >/dev/null\n+}\n+\n #\n # Get submodule info for registered submodules\n # $@ = path to limit submodule list\n #\n module_list()\n {\n-\tgit ls-files --error-unmatch --stage -- \"$@\" |\n+\tmodule_list_ls_files \"$@\" |\n \tperl -e '\n \tmy %unmerged = ();\n \tmy ($null_sha1) = (\"0\" x 40);\n@@ -434,6 +442,8 @@ cmd_init()\n \t\tshift\n \tdone\n \n+\tmodule_list_valid \"$@\" || exit 1\n+\n \tmodule_list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n@@ -532,6 +542,8 @@ cmd_update()\n \t\tcmd_init \"--\" \"$@\" || return\n \tfi\n \n+\tmodule_list_valid \"$@\" || exit 1\n+\n \tcloned_modules=\n \tmodule_list \"$@\" | {\n \terr=\n@@ -929,6 +941,8 @@ cmd_status()\n \t\tshift\n \tdone\n \n+\tmodule_list_valid \"$@\" || exit 1\n+\n \tmodule_list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n@@ -996,6 +1010,9 @@ cmd_sync()\n \t\t\t;;\n \t\tesac\n \tdone\n+\n+\tmodule_list_valid \"$@\" || exit 1\n+\n \tcd_to_toplevel\n \tmodule_list \"$@\" |\n \twhile read mode sha1 stage sm_path\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex c73bec9..3a40334 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -258,6 +258,27 @@ test_expect_success 'init should register submodule url in .git/config' '\n \ttest_cmp expect url\n '\n \n+test_failure_with_unknown_submodule() {\n+\ttest_must_fail git submodule $1 no-such-submodule 2>output.err &&\n+\tgrep \"^error: .*no-such-submodule\" output.err\n+}\n+\n+test_expect_success 'init should fail with unknown submodule' '\n+\ttest_failure_with_unknown_submodule init\n+'\n+\n+test_expect_success 'update should fail with unknown submodule' '\n+\ttest_failure_with_unknown_submodule update\n+'\n+\n+test_expect_success 'status should fail with unknown submodule' '\n+\ttest_failure_with_unknown_submodule status\n+'\n+\n+test_expect_success 'sync should fail with unknown submodule' '\n+\ttest_failure_with_unknown_submodule sync\n+'\n+\n test_expect_success 'update should fail when path is used by a file' '\n \techo hello >expect &&\n \n@@ -418,10 +439,7 @@ test_expect_success 'moving to a commit without submodule does not leave empty d\n '\n \n test_expect_success 'submodule <invalid-path> warns' '\n-\n-\tgit submodule no-such-submodule 2> output.err &&\n-\tgrep \"^error: .*no-such-submodule\" output.err\n-\n+\ttest_failure_with_unknown_submodule\n '\n \n test_expect_success 'add submodules without specifying an explicit path' '\n-- \n1.7.12.rc2.10.g45a4861\n"},{"id":"196754","messageId":"7vboijol03.fsf@alter.siamese.dyndns.org","threadId":"30924","inReplyTo":"20120809200302.GA93203@book.hvoigt.net","subject":"Re: [PATCH] Let submodule command exit with error status if path does not exist","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-09T20:42:20Z","receivedAt":"2012-08-09T20:42:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> Previously the exit status of git submodule was zero for various\n> subcommands even though the user specified an unknown path.\n>\n> The reason behind that was that they all pipe the output of module_list\n> into the while loop which then does the action on the paths specified by\n> the commandline. Since piped commands are run in parallel the status\n> code of module_list was swallowed.\n\nIt is more like that the shell ignores the exit status of command\nthat is on the upstream side of a pipeline.\n\n> We work around this by introducing a new function module_list_valid\n> which is used to check the leftover commandline parameters passed to\n> module_list.\n\nDoesn't it slow things down for the normal case, though?\n\nA plausible hack, assuming all the problematic readers of the pipe\nare of the form \"... | while read mode sha1 stage sm_path\", might be\nto update module_list () to do something like:\n\n\t(\n\t\tgit ls-files --error-unmatch ... ||\n                echo \"#unmatched\"\n\t)\n\nand then update the readers to catch \"#unmatched\" token, e.g.\n\n\tmodule_list \"$@\" |\n        while read mode sha1 stage sm_path\n        do\n\t\tif test \"$mode\" = \"#unmatched\"\n                then\n        \t\t... do the necessary error thing ...\n                        continue\n\t\tfi\n                ... whatever the loop originally did ...\n\tdone\n\nOne thing to note is that the above is not good if you want to\natomically reject\n\n\tgit submodule foo module1 moduel2\n\nand error the whole thing out without touching module1 (which\nexists) because of misspelt module2.\n\nBut is it what we want to see happen in these codepaths?\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index aac575e..1fd21da 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -103,13 +103,21 @@ resolve_relative_url ()\n>  \techo \"${is_relative:+${up_path}}${remoteurl#./}\"\n>  }\n>  \n> +module_list_ls_files() {\n> +\tgit ls-files --error-unmatch --stage -- \"$@\"\n> +}\n> +\n> +module_list_valid() {\n> +\tmodule_list_ls_files \"$@\" >/dev/null\n> +}\n> +\n\nThis is a tangent, but among the 170 hits\n\n\tgit grep -e '^[a-z][a-z0-9A-Z_]* *(' -- './git-*.sh'\n\ngives, about 120 have SP after funcname, i.e.\n\n\tfuncname () {\n\nand 50 don't, i.e.\n\n\tfuncname() {\n\nThis file has 12 such definitions, among which 10 are the latter\nform.  There is no \"rational\" reason to choose between the two, but\nhaving two forms in the same project hurts greppability.  Updating\nthe style of existing code shouldn't be done in the same patch, but\nplease do not make things worse.\n\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index c73bec9..3a40334 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -258,6 +258,27 @@ test_expect_success 'init should register submodule url in .git/config' '\n>  \ttest_cmp expect url\n>  '\n>  \n> +test_failure_with_unknown_submodule() {\n\nLikewise, even though inside t/ directory we seem to have more\noffenders (190/480 ~ 40%, vs 50/170 ~ 30%).\n\n> +\ttest_must_fail git submodule $1 no-such-submodule 2>output.err &&\n> +\tgrep \"^error: .*no-such-submodule\" output.err\n> +}\n\nI think the latter half already passes with the current code, but\nthe exit code from \"git submodule $1\" would be corrected with this\npatch, which is good.\n\nThanks.\n"},{"id":"196854","messageId":"20120811064912.GA83365@book.hvoigt.net","threadId":"30924","inReplyTo":"7vboijol03.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] Let submodule command exit with error status if path does not exist","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-08-11T06:49:14Z","receivedAt":"2012-08-11T06:49:14Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Previously the exit status of git submodule was zero for various\nsubcommands even though the user specified an unknown path.\n\nThe reason behind that was that they all pipe the output of module_list\ninto the while loop which then does the action on the paths specified by\nthe commandline. Since the exit code of piped commands is ignored by the\nshell, the status code of module_list was swallowed.\n\nWe work around this by piping a submodule with an empty path and a null\nsha1 as commit. This is necessary to pass through the perl snippet that\nis used to select submodule entries. The while loop now checks for such\na submodule specification, exits with 1 and the exit code is propagated.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\nOn Thu, Aug 09, 2012 at 01:42:20PM -0700, Junio C Hamano wrote:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> A plausible hack, assuming all the problematic readers of the pipe\n> are of the form \"... | while read mode sha1 stage sm_path\", might be\n> to update module_list () to do something like:\n> \n> \t(\n> \t\tgit ls-files --error-unmatch ... ||\n>                 echo \"#unmatched\"\n> \t)\n> \n> and then update the readers to catch \"#unmatched\" token, e.g.\n> \n> \tmodule_list \"$@\" |\n>         while read mode sha1 stage sm_path\n>         do\n> \t\tif test \"$mode\" = \"#unmatched\"\n>                 then\n>         \t\t... do the necessary error thing ...\n>                         continue\n> \t\tfi\n>                 ... whatever the loop originally did ...\n> \tdone\n\nUnfortunately it does not work that simple, but I have implemented\nsomething like this in this patch.\n\n> One thing to note is that the above is not good if you want to\n> atomically reject\n> \n> \tgit submodule foo module1 moduel2\n\n> \n> and error the whole thing out without touching module1 (which\n> exists) because of misspelt module2.\n> \n> But is it what we want to see happen in these codepaths?\n\nI think it is fine if we are working atomically for each submodule and\nstop once we hit an unknown path. If we want to continue propagating the\nerror code will make the code more complex than is justified (IMO).\n\nIf the user wants to proceed its easier for him to correct his spelling. :-)\n\n> This is a tangent, but among the 170 hits\n> \n> \tgit grep -e '^[a-z][a-z0-9A-Z_]* *(' -- './git-*.sh'\n> \n> gives, about 120 have SP after funcname, i.e.\n> \n> \tfuncname () {\n> \n> and 50 don't, i.e.\n> \n> \tfuncname() {\n> \n> This file has 12 such definitions, among which 10 are the latter\n> form.  There is no \"rational\" reason to choose between the two, but\n> having two forms in the same project hurts greppability.  Updating\n> the style of existing code shouldn't be done in the same patch, but\n> please do not make things worse.\n\n[...]\n\n> > +test_failure_with_unknown_submodule() {\n> \n> Likewise, even though inside t/ directory we seem to have more\n> offenders (190/480 ~ 40%, vs 50/170 ~ 30%).\n\nI did not know that you prefer a space after the function name. I simply\nimitated the style from C and there we do not have spaces. It makes the\nstyle rules a bit more complicated. Wouldn't it be nicer to have the\nsame as in C so we have less rules?\n\nNevertheless, I adjusted the patch.\n\n git-submodule.sh           | 23 ++++++++++++++++++++++-\n t/t7400-submodule-basic.sh | 26 ++++++++++++++++++++++----\n 2 files changed, 44 insertions(+), 5 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex aac575e..48014f2 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -109,7 +109,8 @@ resolve_relative_url ()\n #\n module_list()\n {\n-\tgit ls-files --error-unmatch --stage -- \"$@\" |\n+\t(git ls-files --error-unmatch --stage -- \"$@\" ||\n+\t\techo '160000 0000000000000000000000000000000000000000 0\t') |\n \tperl -e '\n \tmy %unmerged = ();\n \tmy ($null_sha1) = (\"0\" x 40);\n@@ -385,6 +386,10 @@ cmd_foreach()\n \tmodule_list |\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tif test -z \"$sm_path\"; then\n+\t\t\texit 1\n+\t\tfi\n+\n \t\tif test -e \"$sm_path\"/.git\n \t\tthen\n \t\t\tsay \"$(eval_gettext \"Entering '\\$prefix\\$sm_path'\")\"\n@@ -437,6 +442,10 @@ cmd_init()\n \tmodule_list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tif test -z \"$sm_path\"; then\n+\t\t\texit 1\n+\t\tfi\n+\n \t\tname=$(module_name \"$sm_path\") || exit\n \n \t\t# Copy url setting when it is not set yet\n@@ -537,6 +546,10 @@ cmd_update()\n \terr=\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tif test -z \"$sm_path\"; then\n+\t\t\texit 1\n+\t\tfi\n+\n \t\tif test \"$stage\" = U\n \t\tthen\n \t\t\techo >&2 \"Skipping unmerged submodule $sm_path\"\n@@ -932,6 +945,10 @@ cmd_status()\n \tmodule_list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tif test -z \"$sm_path\"; then\n+\t\t\texit 1\n+\t\tfi\n+\n \t\tname=$(module_name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tdisplaypath=\"$prefix$sm_path\"\n@@ -1000,6 +1017,10 @@ cmd_sync()\n \tmodule_list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tif test -z \"$sm_path\"; then\n+\t\t\texit 1\n+\t\tfi\n+\n \t\tname=$(module_name \"$sm_path\")\n \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex c73bec9..56a81cd 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -258,6 +258,27 @@ test_expect_success 'init should register submodule url in .git/config' '\n \ttest_cmp expect url\n '\n \n+test_failure_with_unknown_submodule () {\n+\ttest_must_fail git submodule $1 no-such-submodule 2>output.err &&\n+\tgrep \"^error: .*no-such-submodule\" output.err\n+}\n+\n+test_expect_success 'init should fail with unknown submodule' '\n+\ttest_failure_with_unknown_submodule init\n+'\n+\n+test_expect_success 'update should fail with unknown submodule' '\n+\ttest_failure_with_unknown_submodule update\n+'\n+\n+test_expect_success 'status should fail with unknown submodule' '\n+\ttest_failure_with_unknown_submodule status\n+'\n+\n+test_expect_success 'sync should fail with unknown submodule' '\n+\ttest_failure_with_unknown_submodule sync\n+'\n+\n test_expect_success 'update should fail when path is used by a file' '\n \techo hello >expect &&\n \n@@ -418,10 +439,7 @@ test_expect_success 'moving to a commit without submodule does not leave empty d\n '\n \n test_expect_success 'submodule <invalid-path> warns' '\n-\n-\tgit submodule no-such-submodule 2> output.err &&\n-\tgrep \"^error: .*no-such-submodule\" output.err\n-\n+\ttest_failure_with_unknown_submodule\n '\n \n test_expect_success 'add submodules without specifying an explicit path' '\n-- \n1.7.12.rc2.10.gaf2525e\n"},{"id":"196879","messageId":"7vy5lkfyx1.fsf@alter.siamese.dyndns.org","threadId":"30924","inReplyTo":"20120811064912.GA83365@book.hvoigt.net","subject":"Re: [PATCH v2] Let submodule command exit with error status if path does not exist","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-12T05:43:22Z","receivedAt":"2012-08-12T05:43:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> I did not know that you prefer a space after the function name. I simply\n> imitated the style from C and there we do not have spaces. It makes the\n> style rules a bit more complicated. Wouldn't it be nicer to have the\n> same as in C so we have less rules?\n\nI do not think so, as they are different languages.  The call site\nof C functions have name and opening parenthesis without a SP in\nbetween.  The call site of shell functions do not even have\nparentheses.\n\nIn any case, personal preferences (including mine) do not matter\nmuch, as there is no \"this is scientificly superiour\" in styles.\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index aac575e..48014f2 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -109,7 +109,8 @@ resolve_relative_url ()\n>  #\n>  module_list()\n>  {\n> -\tgit ls-files --error-unmatch --stage -- \"$@\" |\n> +\t(git ls-files --error-unmatch --stage -- \"$@\" ||\n> +\t\techo '160000 0000000000000000000000000000000000000000 0\t') |\n\nIs there a reason why the sentinel has to have the same mode pattern\nas a GITLINK entry, NULL SHA-1, stage #0?  Or is the \"path\" being\nempty all that matters?\n\nAh, OK, you did not want to touch the perl script downstream.  I\nwould have preferred a comment to document that, i.e.\n\n\t(\n        \tgit ls-files --error-unmatch --stage -- \"$@\" ||\n                # an entry with an empty $path used as a signal\n                echo '160000 0.... 0 '\n\t) |\n\tperl -e '...\n\nor\n\n\t(\n\t\tgit ls-files --error-unmatch --stage -- \"$@\" ||\n\t\techo 'unmatched pathspec exists'\n\t) |\n\tperl -e '\n\t...\n\twhile (<STDIN>) {\n\t\tif (/^unmatched pathspec/) {\n\t\t\tprint;\n                        next;\n\t\t}\n\t\tchomp;\n\t\tmy ($mode, $sha1, $stage, $path) = ...\n\nEither way, the reader of the code would not have to scratch her\nhead that way.\n\n>  \tperl -e '\n>  \tmy %unmerged = ();\n>  \tmy ($null_sha1) = (\"0\" x 40);\n> @@ -385,6 +386,10 @@ cmd_foreach()\n>  \tmodule_list |\n>  \twhile read mode sha1 stage sm_path\n>  \tdo\n> +\t\tif test -z \"$sm_path\"; then\n> +\t\t\texit 1\n\nStyle:\n\n\tif test -z \"$sm_path\"\n\tthen\n\t\texit 1\n\nI know module_list would have said \"error: pathspec 'no-such' did\nnot match any file(s) known to git.  Did you forget to git add\"\nalready, but because that comes at the very end of the input to the\nfilter written in perl (and with the way the filter is coded, it\nwill stay at the end), I am not sure if the user would notice it if\nwe exit like this.  By the time we hit this exit, we would have seen\n\"Entering $sm_path...\" followed by whatever message given while in\nthe submodule for all the submodules that comes out of module_list,\nno?\n\nHow about doing it this way to avoid that issue, to make sure we die\nimmediately after the typo is diagnosed without touching anything?\n\n git-submodule.sh | 32 +++++++++++++++++++++++++++++---\n 1 file changed, 29 insertions(+), 3 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 3aa7644..3f99d71 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -109,26 +109,47 @@ resolve_relative_url ()\n #\n module_list()\n {\n-\tgit ls-files --error-unmatch --stage -- \"$@\" |\n+\t(\n+\t\tgit ls-files --error-unmatch --stage -- \"$@\" ||\n+\t\techo \"unmatched pathspec exists\"\n+\t) |\n \tperl -e '\n \tmy %unmerged = ();\n \tmy ($null_sha1) = (\"0\" x 40);\n+\tmy @out = ();\n+\tmy $unmatched = 0;\n \twhile (<STDIN>) {\n+\t\tif (/^unmatched pathspec/) {\n+\t\t\t$unmatched = 1;\n+\t\t\tnext;\n+\t\t}\n \t\tchomp;\n \t\tmy ($mode, $sha1, $stage, $path) =\n \t\t\t/^([0-7]+) ([0-9a-f]{40}) ([0-3])\\t(.*)$/;\n \t\tnext unless $mode eq \"160000\";\n \t\tif ($stage ne \"0\") {\n \t\t\tif (!$unmerged{$path}++) {\n-\t\t\t\tprint \"$mode $null_sha1 U\\t$path\\n\";\n+\t\t\t\tpush @out, \"$mode $null_sha1 U\\t$path\\n\";\n \t\t\t}\n \t\t\tnext;\n \t\t}\n-\t\tprint \"$_\\n\";\n+\t\tpush @out, \"$_\\n\";\n+\t}\n+\tif ($unmatched) {\n+\t\tunshift @out, \"#unmatched\\n\";\n \t}\n+\tprint for (@out);\n \t'\n }\n \n+check_unmatched ()\n+{\n+\tif test \"$1\" = \"#unmatched\"\n+\tthen\n+\t\texit 1\n+\tfi\n+}\n+\n #\n # Map submodule path to submodule name\n #\n@@ -385,6 +406,7 @@ cmd_foreach()\n \tmodule_list |\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tcheck_unmatched \"$mode\"\n \t\tif test -e \"$sm_path\"/.git\n \t\tthen\n \t\t\tsay \"$(eval_gettext \"Entering '\\$prefix\\$sm_path'\")\"\n@@ -437,6 +459,7 @@ cmd_init()\n \tmodule_list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tcheck_unmatched \"$mode\"\n \t\tname=$(module_name \"$sm_path\") || exit\n \n \t\t# Copy url setting when it is not set yet\n@@ -537,6 +560,7 @@ cmd_update()\n \terr=\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tcheck_unmatched \"$mode\"\n \t\tif test \"$stage\" = U\n \t\tthen\n \t\t\techo >&2 \"Skipping unmerged submodule $sm_path\"\n@@ -932,6 +956,7 @@ cmd_status()\n \tmodule_list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tcheck_unmatched \"$mode\"\n \t\tname=$(module_name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tdisplaypath=\"$prefix$sm_path\"\n@@ -1000,6 +1025,7 @@ cmd_sync()\n \tmodule_list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tcheck_unmatched \"$mode\"\n \t\tname=$(module_name \"$sm_path\")\n \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n \n"},{"id":"196915","messageId":"20120813163911.GA6418@book.hvoigt.net","threadId":"30924","inReplyTo":"7vy5lkfyx1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Let submodule command exit with error status if path does not exist","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-08-13T16:39:17Z","receivedAt":"2012-08-13T16:39:17Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi Junio,\n\nthanks for such a thorough review.\n\nOn Sat, Aug 11, 2012 at 10:43:22PM -0700, Junio C Hamano wrote:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> \n> > I did not know that you prefer a space after the function name. I simply\n> > imitated the style from C and there we do not have spaces. It makes the\n> > style rules a bit more complicated. Wouldn't it be nicer to have the\n> > same as in C so we have less rules?\n> \n> I do not think so, as they are different languages.  The call site\n> of C functions have name and opening parenthesis without a SP in\n> between.  The call site of shell functions do not even have\n> parentheses.\n> \n> In any case, personal preferences (including mine) do not matter\n> much, as there is no \"this is scientificly superiour\" in styles.\n\nHow about I update CodingGuidelines according to the rules you\nsuggested? Then other people know how we prefer bash functions and if\nstatements to look like.\n\n> > diff --git a/git-submodule.sh b/git-submodule.sh\n> > index aac575e..48014f2 100755\n> > --- a/git-submodule.sh\n> > +++ b/git-submodule.sh\n> > @@ -109,7 +109,8 @@ resolve_relative_url ()\n> >  #\n> >  module_list()\n> >  {\n> > -\tgit ls-files --error-unmatch --stage -- \"$@\" |\n> > +\t(git ls-files --error-unmatch --stage -- \"$@\" ||\n> > +\t\techo '160000 0000000000000000000000000000000000000000 0\t') |\n> \n> Is there a reason why the sentinel has to have the same mode pattern\n> as a GITLINK entry, NULL SHA-1, stage #0?  Or is the \"path\" being\n> empty all that matters?\n> \n> Ah, OK, you did not want to touch the perl script downstream.  I\n> would have preferred a comment to document that, i.e.\n\nI only described it in the commit message, sorry. Next time I will add a\ncomment.\n\n> > @@ -385,6 +386,10 @@ cmd_foreach()\n> >  \tmodule_list |\n> >  \twhile read mode sha1 stage sm_path\n> >  \tdo\n> > +\t\tif test -z \"$sm_path\"; then\n> > +\t\t\texit 1\n> \n> Style:\n> \n> \tif test -z \"$sm_path\"\n> \tthen\n> \t\texit 1\n\nSee above. If you agree I would add this style to the guidelines.\n\n> I know module_list would have said \"error: pathspec 'no-such' did\n> not match any file(s) known to git.  Did you forget to git add\"\n> already, but because that comes at the very end of the input to the\n> filter written in perl (and with the way the filter is coded, it\n> will stay at the end), I am not sure if the user would notice it if\n> we exit like this.  By the time we hit this exit, we would have seen\n> \"Entering $sm_path...\" followed by whatever message given while in\n> the submodule for all the submodules that comes out of module_list,\n> no?\n> \n> How about doing it this way to avoid that issue, to make sure we die\n> immediately after the typo is diagnosed without touching anything?\n\nI like it, your approach combines the atomicity of my first patch with\nthe efficiency of not calling ls-files twice. I was hesitant to change\nto much code just to get the exit code right, but your approach looks\ngood to me.\n\nShould I send an updated patch? Or do you just want to squash this in.\nSince now only the tests are from me what should we do with the\nownership?\n\n>  git-submodule.sh | 32 +++++++++++++++++++++++++++++---\n>  1 file changed, 29 insertions(+), 3 deletions(-)\n[...]\n\nCheers Heiko\n"},{"id":"196918","messageId":"7vvcgmemyk.fsf@alter.siamese.dyndns.org","threadId":"30924","inReplyTo":"20120813163911.GA6418@book.hvoigt.net","subject":"Re: [PATCH v2] Let submodule command exit with error status if path does not exist","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-13T17:11:31Z","receivedAt":"2012-08-13T17:11:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> How about I update CodingGuidelines according to the rules you\n> suggested? Then other people know how we prefer bash functions and if\n> statements to look like.\n\nOK.  I was hoping that \"imitate surrounding code\" was sufficient,\nbut it seems many parts of the codebase have deteriorated enough to\nmake that rule no longer easy to follow.\n\n>> Style:\n>> \n>> \tif test -z \"$sm_path\"\n>> \tthen\n>> \t\texit 1\n>\n> See above. If you agree I would add this style to the guidelines.\n\nLikewise.\n\n>> I know module_list would have said \"error: pathspec 'no-such' did\n>> not match any file(s) known to git.  Did you forget to git add\"\n>> already, but because that comes at the very end of the input to the\n>> filter written in perl (and with the way the filter is coded, it\n>> will stay at the end), I am not sure if the user would notice it if\n>> we exit like this.  By the time we hit this exit, we would have seen\n>> \"Entering $sm_path...\" followed by whatever message given while in\n>> the submodule for all the submodules that comes out of module_list,\n>> no?\n>> \n>> How about doing it this way to avoid that issue, to make sure we die\n>> immediately after the typo is diagnosed without touching anything?\n>\n> I like it, your approach combines the atomicity of my first patch with\n> the efficiency of not calling ls-files twice. I was hesitant to change\n> to much code just to get the exit code right, but your approach looks\n> good to me.\n>\n> Should I send an updated patch? Or do you just want to squash this in.\n> Since now only the tests are from me what should we do with the\n> ownership?\n\nThat is your itch and the idea and the bulk of the changes remains\nto be yours in any case, methinks.\n\nBy the way, there is no reason for my patch to be unshifting the \"#unmatch\"\ntoken into the array and spewing the array out, if the readers are always\ngoing to stop upon seeing \"#unmatch\" without touching any submodule\nthat is named correctly on the command line.  In other words, the\nfollowing should suffice:\n\n\twhile (<>) {\n\t\t... accumulate in @out ...\n\t}\n\tif ($unmatched) {\n\t\tprint \"#unmatched\\n\";\n\t} else {\n\t\tprint for (@out);\n\t}\n"},{"id":"197015","messageId":"20120814203526.GA33843@book.hvoigt.net","threadId":"30924","inReplyTo":"7vvcgmemyk.fsf@alter.siamese.dyndns.org","subject":"[PATCH v3] Let submodule command exit with error status if path does not exist","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-08-14T20:35:27Z","receivedAt":"2012-08-14T20:35:27Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Previously the exit status of git submodule was zero for various\nsubcommands even though the user specified an unknown path.\n\nThe reason behind that was that they all pipe the output of module_list\ninto the while loop which then does the action on the paths specified by\nthe commandline. Since the exit code of piped commands is ignored by the\nshell, the status code of module_list was swallowed.\n\nIn case ls-files returns with an error code we pipe a special string\nthat is not possible in non error situations. If the perl filter behind\nthat encounters this string it outputs a single line with the special\ntag '#unmatched'. This is then used by all reader from module_list to\ndie with an error code.\n\nThe error message that there is an unmatched pathspec comes through\nstderr directly from ls-files. So the user still gets a hint whats going\non.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\nHi Junio,\n\nthis is an updated version with your proposal incorporated. I changed\nthe name of check_unmatched to die_if_unmatched because IMO it describes\nmore clearly what the function is doing.\n\nCheers Heiko\n\n git-submodule.sh           | 33 ++++++++++++++++++++++++++++++---\n t/t7400-submodule-basic.sh | 26 ++++++++++++++++++++++----\n 2 files changed, 52 insertions(+), 7 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex aac575e..0840524 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -109,26 +109,48 @@ resolve_relative_url ()\n #\n module_list()\n {\n-\tgit ls-files --error-unmatch --stage -- \"$@\" |\n+\t(\n+\t\tgit ls-files --error-unmatch --stage -- \"$@\" ||\n+\t\techo \"unmatched pathspec exists\"\n+\t) |\n \tperl -e '\n \tmy %unmerged = ();\n \tmy ($null_sha1) = (\"0\" x 40);\n+\tmy @out = ();\n+\tmy $unmatched = 0;\n \twhile (<STDIN>) {\n+\t\tif (/^unmatched pathspec/) {\n+\t\t\t$unmatched = 1;\n+\t\t\tnext;\n+\t\t}\n \t\tchomp;\n \t\tmy ($mode, $sha1, $stage, $path) =\n \t\t\t/^([0-7]+) ([0-9a-f]{40}) ([0-3])\\t(.*)$/;\n \t\tnext unless $mode eq \"160000\";\n \t\tif ($stage ne \"0\") {\n \t\t\tif (!$unmerged{$path}++) {\n-\t\t\t\tprint \"$mode $null_sha1 U\\t$path\\n\";\n+\t\t\t\tpush @out, \"$mode $null_sha1 U\\t$path\\n\";\n \t\t\t}\n \t\t\tnext;\n \t\t}\n-\t\tprint \"$_\\n\";\n+\t\tpush @out, \"$_\\n\";\n+\t}\n+\tif ($unmatched) {\n+\t\tprint \"#unmatched\\n\";\n+\t} else {\n+\t\tprint for (@out);\n \t}\n \t'\n }\n \n+die_if_unmatched ()\n+{\n+\tif test \"$1\" = \"#unmatched\"\n+\tthen\n+\t\texit 1\n+\tfi\n+}\n+\n #\n # Map submodule path to submodule name\n #\n@@ -385,6 +407,7 @@ cmd_foreach()\n \tmodule_list |\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tdie_if_unmatched \"$mode\"\n \t\tif test -e \"$sm_path\"/.git\n \t\tthen\n \t\t\tsay \"$(eval_gettext \"Entering '\\$prefix\\$sm_path'\")\"\n@@ -437,6 +460,7 @@ cmd_init()\n \tmodule_list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tdie_if_unmatched \"$mode\"\n \t\tname=$(module_name \"$sm_path\") || exit\n \n \t\t# Copy url setting when it is not set yet\n@@ -537,6 +561,7 @@ cmd_update()\n \terr=\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tdie_if_unmatched \"$mode\"\n \t\tif test \"$stage\" = U\n \t\tthen\n \t\t\techo >&2 \"Skipping unmerged submodule $sm_path\"\n@@ -932,6 +957,7 @@ cmd_status()\n \tmodule_list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tdie_if_unmatched \"$mode\"\n \t\tname=$(module_name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tdisplaypath=\"$prefix$sm_path\"\n@@ -1000,6 +1026,7 @@ cmd_sync()\n \tmodule_list \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n+\t\tdie_if_unmatched \"$mode\"\n \t\tname=$(module_name \"$sm_path\")\n \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex c73bec9..56a81cd 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -258,6 +258,27 @@ test_expect_success 'init should register submodule url in .git/config' '\n \ttest_cmp expect url\n '\n \n+test_failure_with_unknown_submodule () {\n+\ttest_must_fail git submodule $1 no-such-submodule 2>output.err &&\n+\tgrep \"^error: .*no-such-submodule\" output.err\n+}\n+\n+test_expect_success 'init should fail with unknown submodule' '\n+\ttest_failure_with_unknown_submodule init\n+'\n+\n+test_expect_success 'update should fail with unknown submodule' '\n+\ttest_failure_with_unknown_submodule update\n+'\n+\n+test_expect_success 'status should fail with unknown submodule' '\n+\ttest_failure_with_unknown_submodule status\n+'\n+\n+test_expect_success 'sync should fail with unknown submodule' '\n+\ttest_failure_with_unknown_submodule sync\n+'\n+\n test_expect_success 'update should fail when path is used by a file' '\n \techo hello >expect &&\n \n@@ -418,10 +439,7 @@ test_expect_success 'moving to a commit without submodule does not leave empty d\n '\n \n test_expect_success 'submodule <invalid-path> warns' '\n-\n-\tgit submodule no-such-submodule 2> output.err &&\n-\tgrep \"^error: .*no-such-submodule\" output.err\n-\n+\ttest_failure_with_unknown_submodule\n '\n \n test_expect_success 'add submodules without specifying an explicit path' '\n-- \n1.7.12.rc2.11.g5d52328\n"},{"id":"197022","messageId":"7vmx1x8a22.fsf@alter.siamese.dyndns.org","threadId":"30924","inReplyTo":"20120814203526.GA33843@book.hvoigt.net","subject":"Re: [PATCH v3] Let submodule command exit with error status if path does not exist","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-14T20:59:01Z","receivedAt":"2012-08-14T20:59:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> Previously the exit status of git submodule was zero for various\n> subcommands even though the user specified an unknown path.\n\nAs any patch that fixes behaviour deals with \"Previously\", I'd\nprefer to omit it and describe the current problem in present tense\ninstead.\n\nWill queue with minor tweaks.  Thanks.\n\n> this is an updated version with your proposal incorporated. I changed\n> the name of check_unmatched to die_if_unmatched because IMO it describes\n> more clearly what the function is doing.\n\nIn general, I would actually prefer to call this kind of function\n\"check\".  That way, all the call sites only need to be aware that\nthere is a check done there, without having to know what happens\nwhen the check triggers, and the implementation of \"check\" could\ndecide that dying is too much and weaken the behaviour to only warn\nin later updates.\n\nSuch an update would not easily apply for this particular case\nbecause you would need to spit out all of @out from the module_list\nbefore giving the \"#unmatched\" warning token, and find a way to\nbuffer the error message from ls-files so that it can be given when\nthe warning is issued at the end if we wanted to weaken this to\nwarn.  So in this particular case, I do not mind renaming it to\ndie-if.\n\nThanks.\n"}]}