{"thread":{"id":"36882","subject":"[PATCH] git-submodule.sh: avoid \"test <cond> -a/-o <cond>\"","startedAt":"2014-06-10T16:43:14Z","lastAt":"2014-06-10T19:47:03Z","messageCount":4,"participants":["Elia Pinto","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"243745","messageId":"1402418594-1377-1-git-send-email-gitter.spiros@gmail.com","threadId":"36882","inReplyTo":null,"subject":"[PATCH] git-submodule.sh: avoid \"test <cond> -a/-o <cond>\"","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2014-06-10T16:43:14Z","receivedAt":"2014-06-10T16:43:14Z","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---\nThis is the fifth revision.\n\nChange based on Junio bugfix and better rewrite of the case condition\nhttp://permalink.gmane.org/gmane.comp.version-control.git/251198\n\nI dropped also the echo -> printf replacement for doing\nit in another patch.\n\nPass all the t/*submodule* tests. Finally ! :=)\n\nThank you all very much and sorry for the mess.\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..e128a4a 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\tif test \"$status\" = D || test \"$status\" = T\n+                        then\n+\t\t\t\techo \"$sm_path\" &&\n+\t\t\t\tcontinue\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 \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":"243746","messageId":"xmqqegywaeb5.fsf@gitster.dls.corp.google.com","threadId":"36882","inReplyTo":"1402418594-1377-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-10T17:02:06Z","receivedAt":"2014-06-10T17:02:06Z","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\nHmmmm.  Is the above correct?\n\n    $ if ! false && true; then echo true; else echo false; fi\n    true\n\nIn other words, \"! cmd1 && cmd2\" parses not as \"! (cmd1 && cmd2)\"\nbut as \"(! cmd1) && cmd2\".\n\nIt almost makes me wonder if the code may become simpler if we did\nit the way in the attached.  That is, \"if $sm_path/.git is there\n(whether as an embedded repository, or a file pointing to a\nrepository elsewhere), then grab its HEAD, otherwise $sm_path is a\nsubmodule that hasn't been run 'submodule init' on, so run the whole\nnine yards starting from module_clone\".\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 e146b83..018f1bb 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -832,15 +832,15 @@ 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 -e \"$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-\t\t\tsubsha1=\n-\t\telse\n \t\t\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n \t\t\tdie \"$(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n+\t\telse\n+\t\t\tmodule_clone \"$sm_path\" \"$name\" \"$url\" \"$reference\" \"$depth\" || exit\n+\t\t\tcloned_modules=\"$cloned_modules;$name\"\n+\t\t\tsubsha1=\n \t\tfi\n \n \t\tif test -n \"$remote\"\n"},{"id":"243747","messageId":"xmqqd2egae9f.fsf@gitster.dls.corp.google.com","threadId":"36882","inReplyTo":"1402418594-1377-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-10T17:03:08Z","receivedAt":"2014-06-10T17:03: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> @@ -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\nHmmmm.  Is the above correct?\n\n    $ if ! false && true; then echo true; else echo false; fi\n    true\n\nIn other words, \"! cmd1 && cmd2\" parses not as \"! (cmd1 && cmd2)\"\nbut as \"(! cmd1) && cmd2\".\n\nIt almost makes me wonder if the code may become simpler if we did\nit the way in the attached.  That is, \"if $sm_path/.git is there\n(whether as an embedded repository, or a file pointing to a\nrepository elsewhere), then grab its HEAD, otherwise $sm_path is a\nsubmodule that hasn't been run 'submodule init' on, so run the whole\nnine yards starting from module_clone\".\n\nSuch a rewrite is not within the scope of this series, so \n\n\tif ! test -d \"$sm_path/.git\" && ! test -f \"$sm_path/.git\"\n\nmay be the closest to the original intent, I would think.\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 e146b83..018f1bb 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -832,15 +832,15 @@ 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 -e \"$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-\t\t\tsubsha1=\n-\t\telse\n \t\t\tsubsha1=$(clear_local_git_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n \t\t\tdie \"$(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n+\t\telse\n+\t\t\tmodule_clone \"$sm_path\" \"$name\" \"$url\" \"$reference\" \"$depth\" || exit\n+\t\t\tcloned_modules=\"$cloned_modules;$name\"\n+\t\t\tsubsha1=\n \t\tfi\n \n \t\tif test -n \"$remote\"\n"},{"id":"243776","messageId":"CAPig+cTTOCe0-U_bMbzo6fp3PFsN+8eJHi9iTxoE9d3UHyRMhg@mail.gmail.com","threadId":"36882","inReplyTo":"1402418594-1377-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH] git-submodule.sh: avoid \"test <cond> -a/-o <cond>\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-10T19:47:03Z","receivedAt":"2014-06-10T19:47:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 10, 2014 at 12:43 PM, Elia Pinto <gitter.spiros@gmail.com> wrote:\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> This is the fifth revision.\n>\n> Change based on Junio bugfix and better rewrite of the case condition\n> http://permalink.gmane.org/gmane.comp.version-control.git/251198\n>\n> I dropped also the echo -> printf replacement for doing\n> it in another patch.\n>\n> Pass all the t/*submodule* tests. Finally ! :=)\n>\n> Thank you all very much and sorry for the mess.\n>\n>  git-submodule.sh |   32 ++++++++++++++++++++------------\n>  1 file changed, 20 insertions(+), 12 deletions(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index e146b83..e128a4a 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n>                 while read mod_src mod_dst sha1_src sha1_dst status sm_path\n>                 do\n>                         # Always show modules deleted or type-changed (blob<->module)\n> -                       test $status = D -o $status = T && echo \"$sm_path\" && continue\n> +                       if test \"$status\" = D || test \"$status\" = T\n> +                        then\n> +                               echo \"$sm_path\" &&\n\nUnnecessary &&.\n\n> +                               continue\n> +                       fi\n>                         # Respect the ignore setting for --for-status.\n>                         if test -n \"$for_status\"\n>                         then\n>                                 name=$(module_name \"$sm_path\")\n>                                 ignore_config=$(get_submodule_config \"$name\" ignore none)\n> -                               test $status != A -a $ignore_config = all && continue\n> +                               test $status != A && test $ignore_config = all && continue\n>                         fi\n>                         # Also show added or modified modules which are checked out\n>                         GIT_DIR=\"$sm_path/.git\" git-rev-parse --git-dir >/dev/null 2>&1 &&\n"}]}