{"thread":{"id":"36723","subject":"[PATCHv2 10/19] git-submodule.sh: convert test -a/-o to && and ||","startedAt":"2014-05-21T14:24:15Z","lastAt":"2014-05-22T09:10:02Z","messageCount":4,"participants":["Elia Pinto","Matthieu Moy","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"242381","messageId":"1400682255-17616-1-git-send-email-gitter.spiros@gmail.com","threadId":"36723","inReplyTo":null,"subject":"[PATCHv2 10/19] git-submodule.sh: convert test -a/-o to && and ||","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2014-05-21T14:24:15Z","receivedAt":"2014-05-21T14:24:15Z","isPatch":false,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"The interaction with unary operators and operator precedence\nfor && and || are better known than -a and -o, and for that\nreason we prefer them. Replace all existing instances\nof -a and -o to save readers from the burden of thinking\nabout such things.\n---\nThis is version 2 of the patch to git-submodule of the \npatch series \"convert test -a/-o to && and ||\". \nIt contains the fixes identified by Johannes and\nMatthieu. \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 b55d83a..1e3a5a6 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -396,7 +396,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@@ -453,7 +453,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@@ -835,7 +835,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@@ -860,11 +860,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@@ -1034,7 +1034,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 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@@ -1125,7 +1129,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@@ -1162,7 +1166,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@@ -1233,7 +1237,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@@ -1402,7 +1410,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":"242458","messageId":"vpqha4is3yd.fsf@anie.imag.fr","threadId":"36723","inReplyTo":"1400682255-17616-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCHv2 10/19] git-submodule.sh: convert test -a/-o to && and ||","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-05-22T06:49:30Z","receivedAt":"2014-05-22T06:49:30Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Elia Pinto writes:\n\n> This is version 2 of the patch to git-submodule of the \n> patch series \"convert test -a/-o to && and ||\". \n> It contains the fixes identified by Johannes and\n> Matthieu. \n\nThis version of the patch (not the whole series) is\n\nReviewed-by: Matthieu Moy \n\nSome comments for next time:\n\n* I agree with Johannes that splitting the series with one patch per\n file is not the right way to split. As a reviewer, I want to\n\n - check that -a trivially translates to &&\n - check that -o trivially translates to ||\n - check non-trivial cases\n\n Interleaving these cases (especially the trivial and non-trivial\n cases) makes the review much harder. For this kind of series, the\n change is trivial, but the review is not (Johannes caught a || Vs &&\n inversion that I didn't find for example, which is quite serious), so\n the \"optimize your patches for review\" is even more important than\n usual.\n\n* Again, to avoid mixing trivial and non-trivial changes, ...\n\n> @@ -1059,13 +1059,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\nturning a echo into a printf is good, but would be better done\nseparately.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"242462","messageId":"CA+EOSB=wqBUb_QB6XtK0v4Lsux1mG7wCvXP3XVeDe7o=ofsYFA@mail.gmail.com","threadId":"36723","inReplyTo":"vpqha4is3yd.fsf@anie.imag.fr","subject":"Re: [PATCHv2 10/19] git-submodule.sh: convert test -a/-o to && and ||","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2014-05-22T08:38:06Z","receivedAt":"2014-05-22T08:38:06Z","isPatch":false,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"2014-05-22 8:49 GMT+02:00 Matthieu Moy :\n> Elia Pinto writes:\n>\n>> This is version 2 of the patch to git-submodule of the\n>> patch series \"convert test -a/-o to && and ||\".\n>> It contains the fixes identified by Johannes and\n>> Matthieu.\n>\n> This version of the patch (not the whole series) is\n>\n> Reviewed-by: Matthieu Moy \n>\n> Some comments for next time:\n>\n> * I agree with Johannes that splitting the series with one patch per\n> file is not the right way to split. As a reviewer, I want to\n>\n> - check that -a trivially translates to &&\n> - check that -o trivially translates to ||\n> - check non-trivial cases\n>\n> Interleaving these cases (especially the trivial and non-trivial\n> cases) makes the review much harder. For this kind of series, the\n> change is trivial, but the review is not (Johannes caught a || Vs &&\n> inversion that I didn't find for example, which is quite serious), so\n> the \"optimize your patches for review\" is even more important than\n> usual.\n>\n> * Again, to avoid mixing trivial and non-trivial changes, ...\n\nFirst of all, thank you. I will take note of your valuable suggestions\nfor the future.\n\n>\n>> @@ -1059,13 +1059,17 @@ cmd_summary() {\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>> + case \"$status\" in\n>> + [DT])\n>> + printf '%s\\n' \"$sm_path\" &&\n>> + continue\n>> + esac\n>\n> turning a echo into a printf is good, but would be better done\n> separately.\n\nI had thought, but the change was in the fix of Johannes, and it did\nnot think was right to change this, especially that it was right\nanyway. But I understand very well the observation.\n\nBest Regards\n>\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n"},{"id":"242465","messageId":"537DBEEA.8010403@viscovery.net","threadId":"36723","inReplyTo":"CA+EOSB=wqBUb_QB6XtK0v4Lsux1mG7wCvXP3XVeDe7o=ofsYFA@mail.gmail.com","subject":"Re: [PATCHv2 10/19] git-submodule.sh: convert test -a/-o to && and ||","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-05-22T09:10:02Z","receivedAt":"2014-05-22T09:10:02Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/22/2014 10:38, schrieb Elia Pinto:\n> 2014-05-22 8:49 GMT+02:00 Matthieu Moy :\n>> Elia Pinto writes:\n>>> @@ -1059,13 +1059,17 @@ cmd_summary() {\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>>> + case \"$status\" in\n>>> + [DT])\n>>> + printf '%s\\n' \"$sm_path\" &&\n>>> + continue\n>>> + esac\n>>\n>> turning a echo into a printf is good, but would be better done\n>> separately.\n> \n> I had thought, but the change was in the fix of Johannes, and it did\n> not think was right to change this, especially that it was right\n> anyway. But I understand very well the observation.\n\nMy intent was to show the final state of this piece of code. I do agree\nwith Matthieu that the change from 'echo' to 'printf' is a different topic\n(in particular, since this is not the only point in the script that would\nneed that change). Sorry for having sent you in circles.\n\n-- Hannes\n"}]}