{"thread":{"id":"36880","subject":"[PATCH] git-submodule.sh: avoid \"test <cond> -a/-o <cond>\"","startedAt":"2014-06-10T12:28:33Z","lastAt":"2014-06-10T15:36:30Z","messageCount":8,"participants":["Elia Pinto","Junio C Hamano","Torsten Bögershausen","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"243724","messageId":"1402403313-22468-1-git-send-email-gitter.spiros@gmail.com","threadId":"36880","inReplyTo":null,"subject":"[PATCH] git-submodule.sh: avoid \"test <cond> -a/-o <cond>\"","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2014-06-10T12:28:33Z","receivedAt":"2014-06-10T12:28:33Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"The construct is error-prone; \"test\" being built-in in most modern\nshells, the reason to avoid \"test <cond> && test <cond>\" spawning\none extra process by using a single \"test <cond> -a <cond>\" no\nlonger exists.\n\nSigned-off-by: Elia Pinto <gitter.spiros@gmail.com>\n---\n\nThis is the fourth revision of this patch.\n\nChange based on Junio suggestions http://www.spinics.net/lists/git/msg233569.html\n\n git-submodule.sh |   32 ++++++++++++++++++++------------\n 1 file changed, 20 insertions(+), 12 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex e146b83..d6a1dea 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -393,7 +393,7 @@ cmd_add()\n \t\t\tsed -e 's|/$||' -e 's|:*/*\\.git$||' -e 's|.*[/:]||g')\n \tfi\n \n-\tif test -z \"$repo\" -o -z \"$sm_path\"; then\n+\tif test -z \"$repo\" || test -z \"$sm_path\"; then\n \t\tusage\n \tfi\n \n@@ -450,7 +450,7 @@ Use -f if you really want to add it.\" >&2\n \t# perhaps the path exists and is already a git repo, else clone it\n \tif test -e \"$sm_path\"\n \tthen\n-\t\tif test -d \"$sm_path\"/.git -o -f \"$sm_path\"/.git\n+\t\tif test -d \"$sm_path\"/.git || test -f \"$sm_path\"/.git\n \t\tthen\n \t\t\teval_gettextln \"Adding existing repo at '\\$sm_path' to the index\"\n \t\telse\n@@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\tcontinue\n \t\tfi\n \n-\t\tif ! test -d \"$sm_path\"/.git -o -f \"$sm_path\"/.git\n+\t\tif ! test -d \"$sm_path\"/.git || test -f \"$sm_path\"/.git\n \t\tthen\n \t\t\tmodule_clone \"$sm_path\" \"$name\" \"$url\" \"$reference\" \"$depth\" || exit\n \t\t\tcloned_modules=\"$cloned_modules;$name\"\n@@ -857,11 +857,11 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\tdie \"$(eval_gettext \"Unable to find current ${remote_name}/${branch} revision in submodule path '\\$sm_path'\")\"\n \t\tfi\n \n-\t\tif test \"$subsha1\" != \"$sha1\" -o -n \"$force\"\n+\t\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n \t\tthen\n \t\t\tsubforce=$force\n \t\t\t# If we don't already have a -f flag and the submodule has never been checked out\n-\t\t\tif test -z \"$subsha1\" -a -z \"$force\"\n+\t\t\tif test -z \"$subsha1\" && test -z \"$force\"\n \t\t\tthen\n \t\t\t\tsubforce=\"-f\"\n \t\t\tfi\n@@ -1031,7 +1031,7 @@ cmd_summary() {\n \tthen\n \t\thead=$rev\n \t\ttest $# = 0 || shift\n-\telif test -z \"$1\" -o \"$1\" = \"HEAD\"\n+\telif test -z \"$1\" || test \"$1\" = \"HEAD\"\n \tthen\n \t\t# before the first commit: compare with an empty tree\n \t\thead=$(git hash-object -w -t tree --stdin </dev/null)\n@@ -1056,13 +1056,17 @@ cmd_summary() {\n \t\twhile read mod_src mod_dst sha1_src sha1_dst status sm_path\n \t\tdo\n \t\t\t# Always show modules deleted or type-changed (blob<->module)\n-\t\t\ttest $status = D -o $status = T && echo \"$sm_path\" && continue\n+\t\t\tcase \"$status\" in\n+\t\t\t([DT])\n+\t\t\t\tprintf '%s\\n' \"$sm_path\" &&\n+\t\t\t\tcontinue\n+\t\t\tesac\n \t\t\t# Respect the ignore setting for --for-status.\n \t\t\tif test -n \"$for_status\"\n \t\t\tthen\n \t\t\t\tname=$(module_name \"$sm_path\")\n \t\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n-\t\t\t\ttest $status != A -a $ignore_config = all && continue\n+\t\t\t\ttest $status != A && test $ignore_config = all && continue\n \t\t\tfi\n \t\t\t# Also show added or modified modules which are checked out\n \t\t\tGIT_DIR=\"$sm_path/.git\" git-rev-parse --git-dir >/dev/null 2>&1 &&\n@@ -1122,7 +1126,7 @@ cmd_summary() {\n \t\t*)\n \t\t\terrmsg=\n \t\t\ttotal_commits=$(\n-\t\t\tif test $mod_src = 160000 -a $mod_dst = 160000\n+\t\t\tif test $mod_src = 160000 && test $mod_dst = 160000\n \t\t\tthen\n \t\t\t\trange=\"$sha1_src...$sha1_dst\"\n \t\t\telif test $mod_src = 160000\n@@ -1159,7 +1163,7 @@ cmd_summary() {\n \t\t\t# i.e. deleted or changed to blob\n \t\t\ttest $mod_dst = 160000 && echo \"$errmsg\"\n \t\telse\n-\t\t\tif test $mod_src = 160000 -a $mod_dst = 160000\n+\t\t\tif test $mod_src = 160000 && test $mod_dst = 160000\n \t\t\tthen\n \t\t\t\tlimit=\n \t\t\t\ttest $summary_limit -gt 0 && limit=\"-$summary_limit\"\n@@ -1230,7 +1234,11 @@ cmd_status()\n \t\t\tsay \"U$sha1 $displaypath\"\n \t\t\tcontinue\n \t\tfi\n-\t\tif test -z \"$url\" || ! test -d \"$sm_path\"/.git -o -f \"$sm_path\"/.git\n+\t\tif test -z \"$url\" ||\n+\t\t{\n+\t\t\t! test -d \"$sm_path\"/.git &&\n+\t\t\t! test -f \"$sm_path\"/.git\n+\t\t}\n \t\tthen\n \t\t\tsay \"-$sha1 $displaypath\"\n \t\t\tcontinue;\n@@ -1399,7 +1407,7 @@ then\n fi\n \n # \"--cached\" is accepted only by \"status\" and \"summary\"\n-if test -n \"$cached\" && test \"$command\" != status -a \"$command\" != summary\n+if test -n \"$cached\" && test \"$command\" != status && test \"$command\" != summary\n then\n \tusage\n fi\n-- \n1.7.10.4\n"},{"id":"243733","messageId":"xmqqbnu0bzcv.fsf@gitster.dls.corp.google.com","threadId":"36880","inReplyTo":"1402403313-22468-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH] git-submodule.sh: avoid \"test <cond> -a/-o <cond>\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T14:42:08Z","receivedAt":"2014-06-10T14:42:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto <gitter.spiros@gmail.com> writes:\n\n> The construct is error-prone; \"test\" being built-in in most modern\n> shells, the reason to avoid \"test <cond> && test <cond>\" spawning\n> one extra process by using a single \"test <cond> -a <cond>\" no\n> longer exists.\n>\n> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>\n> ---\n>\n> This is the fourth revision of this patch.\n\nHmmmm.\n\nWhen applied on top of 'master', this seems to break 7406; fails the\nsame way with either bash 4.2-2ubuntu2.1 which identifes itself as\n4.2.25 or dash 0.5.7-2ubuntu2.\n\n-- >8 --\n\nt7406-submodule-update.sh .. Dubious, test returned 1 (wstat 256, 0x100)\nFailed 14/43 subtests\n\nTest Summary Report\n-------------------\nt7406-submodule-update.sh (Wstat: 256 Tests: 43 Failed: 14)\n  Failed tests:  4-6, 10-15, 18, 30-33\n\n-- 8< --\n\nWhich shell did you test this patch with?\n"},{"id":"243734","messageId":"53971BBE.8030108@web.de","threadId":"36880","inReplyTo":"1402403313-22468-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH] git-submodule.sh: avoid \"test <cond> -a/-o <cond>\"","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-06-10T14:52:46Z","receivedAt":"2014-06-10T14:52:46Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-06-10 14.28, Elia Pinto wrote:\n[]\n>  \t\t# before the first commit: compare with an empty tree\n>  \t\thead=$(git hash-object -w -t tree --stdin </dev/null)\n> @@ -1056,13 +1056,17 @@ cmd_summary() {\n>  \t\twhile read mod_src mod_dst sha1_src sha1_dst status sm_path\n>  \t\tdo\n>  \t\t\t# Always show modules deleted or type-changed (blob<->module)\n> -\t\t\ttest $status = D -o $status = T && echo \"$sm_path\" && continue\n> +\t\t\tcase \"$status\" in\n> +\t\t\t([DT])\nDoes this look strange? ^\nShould it be\ncase \"$status\" in\nD|T)\n"},{"id":"243735","messageId":"xmqq7g4obypt.fsf@gitster.dls.corp.google.com","threadId":"36880","inReplyTo":"1402403313-22468-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH] git-submodule.sh: avoid \"test <cond> -a/-o <cond>\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T14:55:58Z","receivedAt":"2014-06-10T14:55:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto <gitter.spiros@gmail.com> writes:\n\n> @@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?\")\"\n>  \t\t\tcontinue\n>  \t\tfi\n>  \n> -\t\tif ! test -d \"$sm_path\"/.git -o -f \"$sm_path\"/.git\n> +\t\tif ! test -d \"$sm_path\"/.git || test -f \"$sm_path\"/.git\n\nWhich part of test conditions does that \"!\" apply in the original,\nand in the updated? \n\nI think the new test after || also needs negation, no?\n"},{"id":"243737","messageId":"53972229.5040104@viscovery.net","threadId":"36880","inReplyTo":"xmqq7g4obypt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-submodule.sh: avoid \"test <cond> -a/-o <cond>\"","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-06-10T15:20:09Z","receivedAt":"2014-06-10T15:20:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 6/10/2014 16:55, schrieb Junio C Hamano:\n> Elia Pinto <gitter.spiros@gmail.com> writes:\n> \n>> @@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?\")\"\n>>  \t\t\tcontinue\n>>  \t\tfi\n>>  \n>> -\t\tif ! test -d \"$sm_path\"/.git -o -f \"$sm_path\"/.git\n>> +\t\tif ! test -d \"$sm_path\"/.git || test -f \"$sm_path\"/.git\n> \n> Which part of test conditions does that \"!\" apply in the original,\n> and in the updated? \n> \n> I think the new test after || also needs negation, no?\n\nNot just that; the || must be turned into && as well.\n\nI noticed a similar construct later in the patch in a review of an earlier\niteration, but missed this one.\n\n-- Hannes\n"},{"id":"243739","messageId":"xmqq38fcbxf8.fsf@gitster.dls.corp.google.com","threadId":"36880","inReplyTo":"53971BBE.8030108@web.de","subject":"Re: [PATCH] git-submodule.sh: avoid \"test <cond> -a/-o <cond>\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T15:23:55Z","receivedAt":"2014-06-10T15:23:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> On 2014-06-10 14.28, Elia Pinto wrote:\n> []\n>>  \t\t# before the first commit: compare with an empty tree\n>>  \t\thead=$(git hash-object -w -t tree --stdin </dev/null)\n>> @@ -1056,13 +1056,17 @@ cmd_summary() {\n>>  \t\twhile read mod_src mod_dst sha1_src sha1_dst status sm_path\n>>  \t\tdo\n>>  \t\t\t# Always show modules deleted or type-changed (blob<->module)\n>> -\t\t\ttest $status = D -o $status = T && echo \"$sm_path\" && continue\n>> +\t\t\tcase \"$status\" in\n>> +\t\t\t([DT])\n> Does this look strange? ^\n> Should it be\n> case \"$status\" in\n> D|T)\n\nActually POSIX allows matching parentheses for case arm labels\n(surprise!).\n\nAnd some shells misparse\n\n\tvar=$( ... case arm) action ;; esac ... )\n\nas if the ')' after the arm label closes the whole command\nsubstitution.\n\nHaving said that, I'd prefer to see the following squashed into that\npatch.\n\nThe first hunk is purely a bugfix.  It used to be \n\n\tif ! test -d \"$sm_path\"/.git -o -f \"$sm_path\"/.git\n\nthat is: unless \"$sm_path/.git\" is directory or file, do this.\nAnd the rewrite broke that logic.\n\nThe second hunk is to avoid \"case\" that confuses without helping\nreadability that much.\n\nI would also have preferred to see the echo to printf substitution\nleft out of this patch.  There are other places where $sm_path is\nechoed and fixing only one of them in an otherwise unrelated patch\nfeels wrong---it should be a separate follow-up patch, I would\nthink.\n\n git-submodule.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex d6a1dea..27ca7d5 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\tcontinue\n \t\tfi\n \n-\t\tif ! test -d \"$sm_path\"/.git || test -f \"$sm_path\"/.git\n+\t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n \t\tthen\n \t\t\tmodule_clone \"$sm_path\" \"$name\" \"$url\" \"$reference\" \"$depth\" || exit\n \t\t\tcloned_modules=\"$cloned_modules;$name\"\n@@ -1056,11 +1056,11 @@ cmd_summary() {\n \t\twhile read mod_src mod_dst sha1_src sha1_dst status sm_path\n \t\tdo\n \t\t\t# Always show modules deleted or type-changed (blob<->module)\n-\t\t\tcase \"$status\" in\n-\t\t\t([DT])\n-\t\t\t\tprintf '%s\\n' \"$sm_path\" &&\n+\t\t\tif test \"$status\" = D || test \"$status\" = T\n+\t\t\tthen\n+\t\t\t\tprintf '%s\\n' \"$sm_path\"\n \t\t\t\tcontinue\n-\t\t\tesac\n+\t\t\tfi\n \t\t\t# Respect the ignore setting for --for-status.\n \t\t\tif test -n \"$for_status\"\n \t\t\tthen\n"},{"id":"243740","messageId":"xmqqy4x4aifq.fsf@gitster.dls.corp.google.com","threadId":"36880","inReplyTo":"xmqq38fcbxf8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-submodule.sh: avoid \"test <cond> -a/-o <cond>\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T15:32:57Z","receivedAt":"2014-06-10T15:32:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Torsten Bögershausen <tboegi@web.de> writes:\n>\n>> On 2014-06-10 14.28, Elia Pinto wrote:\n>> []\n>>>  \t\t# before the first commit: compare with an empty tree\n>>>  \t\thead=$(git hash-object -w -t tree --stdin </dev/null)\n>>> @@ -1056,13 +1056,17 @@ cmd_summary() {\n>>>  \t\twhile read mod_src mod_dst sha1_src sha1_dst status sm_path\n>>>  \t\tdo\n>>>  \t\t\t# Always show modules deleted or type-changed (blob<->module)\n>>> -\t\t\ttest $status = D -o $status = T && echo \"$sm_path\" && continue\n>>> +\t\t\tcase \"$status\" in\n>>> +\t\t\t([DT])\n>> Does this look strange? ^\n>> Should it be\n>> case \"$status\" in\n>> D|T)\n>\n> Actually POSIX allows matching parentheses for case arm labels\n> (surprise!).\n>\n> And some shells misparse\n>\n> \tvar=$( ... case arm) action ;; esac ... )\n>\n> as if the ')' after the arm label closes the whole command\n> substitution.\n>\n> Having said that, I'd prefer to see the following squashed into that\n> patch.\n> ...\n> I would also have preferred to see the echo to printf substitution\n> left out of this patch.  There are other places where $sm_path is\n> echoed and fixing only one of them in an otherwise unrelated patch\n> feels wrong---it should be a separate follow-up patch, I would\n> think.\n\n... which may look like this (after removing s/echo/printf/ in that\nhunk from this \"test -a/-o\" patch).\n\n git-submodule.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex d0d9b58..9245abf 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -235,7 +235,7 @@ module_name()\n \t\tsed -n -e 's|^submodule\\.\\(.*\\)\\.path '\"$re\"'$|\\1|p' )\n \ttest -z \"$name\" &&\n \tdie \"$(eval_gettext \"No submodule mapping found in .gitmodules for path '\\$sm_path'\")\"\n-\techo \"$name\"\n+\tprintf '%s\\n' \"$name\"\n }\n \n #\n@@ -305,10 +305,10 @@ module_clone()\n \tb=${b%/}\n \n \t# Turn each leading \"*/\" component into \"../\"\n-\trel=$(echo $b | sed -e 's|[^/][^/]*|..|g')\n-\techo \"gitdir: $rel/$a\" >\"$sm_path/.git\"\n+\trel=$(printf '%s\\n' \"$b\" | sed -e 's|[^/][^/]*|..|g')\n+\tprintf '%s\\n' \"gitdir: $rel/$a\" >\"$sm_path/.git\"\n \n-\trel=$(echo $a | sed -e 's|[^/][^/]*|..|g')\n+\trel=$(printf '%s\\n' \"$a\" | sed -e 's|[^/][^/]*|..|g')\n \t(clear_local_git_env; cd \"$sm_path\" && GIT_WORK_TREE=. git config core.worktree \"$rel/$b\")\n }\n \n@@ -389,7 +389,7 @@ cmd_add()\n \tsm_path=$2\n \n \tif test -z \"$sm_path\"; then\n-\t\tsm_path=$(echo \"$repo\" |\n+\t\tsm_path=$(printf '%s\\n' \"$repo\" |\n \t\t\tsed -e 's|/$||' -e 's|:*/*\\.git$||' -e 's|.*[/:]||g')\n \tfi\n \n@@ -1058,7 +1058,7 @@ cmd_summary() {\n \t\t\t# Always show modules deleted or type-changed (blob<->module)\n \t\t\tif test \"$status\" = D || test \"$status\" = T\n \t\t\tthen\n-\t\t\t\techo \"$sm_path\"\n+\t\t\t\tprintf '%s\\n' \"$sm_path\"\n \t\t\t\tcontinue\n \t\t\tfi\n \t\t\t# Respect the ignore setting for --for-status.\n@@ -1070,7 +1070,7 @@ cmd_summary() {\n \t\t\tfi\n \t\t\t# Also show added or modified modules which are checked out\n \t\t\tGIT_DIR=\"$sm_path/.git\" git-rev-parse --git-dir >/dev/null 2>&1 &&\n-\t\t\techo \"$sm_path\"\n+\t\t\tprintf '%s\\n' \"$sm_path\"\n \t\tdone\n \t)\n \n@@ -1311,7 +1311,7 @@ cmd_sync()\n \t\t./*|../*)\n \t\t\t# rewrite foo/bar as ../.. to find path from\n \t\t\t# submodule work tree to superproject work tree\n-\t\t\tup_path=\"$(echo \"$sm_path\" | sed \"s/[^/][^/]*/../g\")\" &&\n+\t\t\tup_path=\"$(printf '%s\\n' \"$sm_path\" | sed \"s/[^/][^/]*/../g\")\" &&\n \t\t\t# guarantee a trailing /\n \t\t\tup_path=${up_path%/}/ &&\n \t\t\t# path from submodule work tree to submodule origin repo\n"},{"id":"243741","messageId":"xmqqtx7sai9t.fsf@gitster.dls.corp.google.com","threadId":"36880","inReplyTo":"53972229.5040104@viscovery.net","subject":"Re: [PATCH] git-submodule.sh: avoid \"test <cond> -a/-o <cond>\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T15:36:30Z","receivedAt":"2014-06-10T15:36:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Am 6/10/2014 16:55, schrieb Junio C Hamano:\n>> Elia Pinto <gitter.spiros@gmail.com> writes:\n>> \n>>> @@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?\")\"\n>>>  \t\t\tcontinue\n>>>  \t\tfi\n>>>  \n>>> -\t\tif ! test -d \"$sm_path\"/.git -o -f \"$sm_path\"/.git\n>>> +\t\tif ! test -d \"$sm_path\"/.git || test -f \"$sm_path\"/.git\n>> \n>> Which part of test conditions does that \"!\" apply in the original,\n>> and in the updated? \n>> \n>> I think the new test after || also needs negation, no?\n>\n> Not just that; the || must be turned into && as well.\n\nI think our mails crossed ;-)\n"}]}