{"thread":{"id":"36730","subject":"[PATCH] Get rid of the non portable shell export VAR=VALUE costruct","startedAt":"2014-05-22T12:48:16Z","lastAt":"2014-05-22T23:42:49Z","messageCount":7,"participants":["Elia Pinto","Torsten Bögershausen","David Kastrup","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"242480","messageId":"1400762896-24324-1-git-send-email-gitter.spiros@gmail.com","threadId":"36730","inReplyTo":null,"subject":"[PATCH] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2014-05-22T12:48:16Z","receivedAt":"2014-05-22T12:48:16Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Found by check-non-portable-shell.pl\n\nSigned-off-by: Elia Pinto <gitter.spiros@gmail.com>\n---\n contrib/subtree/t/t7900-subtree.sh |    2 +-\n git-remote-testgit.sh              |    2 +-\n git-stash.sh                       |    2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 66ce4b0..c1d0b23 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -8,7 +8,7 @@ This test verifies the basic operation of the merge, pull, add\n and split subcommands of git subtree.\n '\n \n-export TEST_DIRECTORY=$(pwd)/../../../t\n+TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY\n \n . ../../../t/test-lib.sh\n \ndiff --git a/git-remote-testgit.sh b/git-remote-testgit.sh\nindex 1c006a0..9d1ce76 100755\n--- a/git-remote-testgit.sh\n+++ b/git-remote-testgit.sh\n@@ -13,7 +13,7 @@ refspec=\"${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}\"\n \n test -z \"$refspec\" && prefix=\"refs\"\n \n-export GIT_DIR=\"$url/.git\"\n+GIT_DIR=\"$url/.git\" && export GIT_DIR\n \n force=\n \ndiff --git a/git-stash.sh b/git-stash.sh\nindex 4798bcf..0bb1af8 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -94,7 +94,7 @@ create_stash () {\n \t\t# ease of unpacking later.\n \t\tu_commit=$(\n \t\t\tuntracked_files | (\n-\t\t\t\texport GIT_INDEX_FILE=\"$TMPindex\"\n+\t\t\t\tGIT_INDEX_FILE=\"$TMPindex\" && export GIT_INDEX_FILE &&\n \t\t\t\trm -f \"$TMPindex\" &&\n \t\t\t\tgit update-index -z --add --remove --stdin &&\n \t\t\t\tu_tree=$(git write-tree) &&\n-- \n1.7.10.4\n"},{"id":"242481","messageId":"537DF7F2.8000401@web.de","threadId":"36730","inReplyTo":"1400762896-24324-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-05-22T13:13:22Z","receivedAt":"2014-05-22T13:13:22Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-05-22 14.48, Elia Pinto wrote:\n> Found by check-non-portable-shell.pl\n\nThanks for picking this up\n> -export TEST_DIRECTORY=$(pwd)/../../../t\n> +TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY\nMinor remark:\nBoth commands should go on their own line, like this:\n\nTEST_DIRECTORY=$(pwd)/../../../t &&\nexport TEST_DIRECTORY\n\n\nAnd, unrelated to this patch, \nthere seem to be a lot of && missing in git-remote-testgit.sh.\nBut this should be a seperate patch, I think.\n"},{"id":"242482","messageId":"87a9aa7xxt.fsf@fencepost.gnu.org","threadId":"36730","inReplyTo":"537DF7F2.8000401@web.de","subject":"Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-05-22T13:19:42Z","receivedAt":"2014-05-22T13:19:42Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> On 2014-05-22 14.48, Elia Pinto wrote:\n>> Found by check-non-portable-shell.pl\n>\n> Thanks for picking this up\n>> -export TEST_DIRECTORY=$(pwd)/../../../t\n>> +TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY\n> Minor remark:\n> Both commands should go on their own line, like this:\n>\n> TEST_DIRECTORY=$(pwd)/../../../t &&\n> export TEST_DIRECTORY\n>\n>\n> And, unrelated to this patch, \n> there seem to be a lot of && missing in git-remote-testgit.sh.\n\nI have a hard time taking the above && seriously.  pwd is a shell\nbuiltin (when we are not talking about Version 3 UNIX or something) that\ncan hardly fail.  And when your shell does not support assignment to a\nshell variable, you'll have a hard time getting the shell script to run.\n\nThat's stuff of the\n\n    if (1+1 != 2) { fputs(\"Warning: your CPU may be broken\", stderr); }\n\nvariety.  If you have to check for that, you have bigger problems...\n\n-- \nDavid Kastrup\n"},{"id":"242484","messageId":"CA+EOSBkmZRd7FiTNUfDPrUX4NivHR7HUL-+UqDzcpE+X7BQcOA@mail.gmail.com","threadId":"36730","inReplyTo":"537DF7F2.8000401@web.de","subject":"Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2014-05-22T13:57:40Z","receivedAt":"2014-05-22T13:57:40Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"5-22 14.48, Elia Pinto wrote:\n>> Found by check-non-portable-shell.pl\n>\n> Thanks for picking this up\n>> -export TEST_DIRECTORY=$(pwd)/../../../t\n>> +TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY\n> Minor remark:\n> Both commands should go on their own line, like this:\n>\n> TEST_DIRECTORY=$(pwd)/../../../t &&\n> export TEST_DIRECTORY\n>\nApparently they are both common idioms and I find no indication in the\nCodingGuideline.\n\nI have no problems rerolling  this simple patch, but i need to know\nwhat is the (git) right style in this case.\n\nThanks\n------\n\ncd ./git-core\n\n grep \"&&.*export\" *.sh\ngit-am.sh:          GIT_MERGE_VERBOSITY=0 && export GIT_MERGE_VERBOSITY\ngit-filter-branch.sh:   echo \"case \\\"\\$GIT_$1_NAME\\\" in \\\"\\\")\nGIT_$1_NAME=\\\"\\${GIT_$1_EMAIL%%@*}\\\" && export GIT_$1_NAME;; esac\"\ngit-filter-branch.sh:   GIT_DIR=\"$ORIG_GIT_DIR\" && export GIT_DIR\ngit-rebase--merge.sh:           GIT_MERGE_VERBOSITY=1 && export\nGIT_MERGE_VERBOSITY\ngit-remote-testgit.sh:GIT_DIR=\"$url/.git\" && export GIT_DIR\ngit-stash.sh:                           GIT_INDEX_FILE=\"$TMPindex\" &&\nexport GIT_INDEX_FILE &&\ngit-stash.sh:           GIT_MERGE_VERBOSITY=0 && export GIT_MERGE_VERBOSITY\n\ngrep 'export.*&&' *.sh\ngit-stash.sh:                           GIT_INDEX_FILE=\"$TMPindex\" &&\nexport GIT_INDEX_FILE &&\ngit-stash.sh:                   export GIT_INDEX_FILE &&\n\n>\n> And, unrelated to this patch,\n> there seem to be a lot of && missing in git-remote-testgit.sh.\n> But this should be a seperate patch, I think.\n>\n>\n>\n"},{"id":"242488","messageId":"537E09AA.7030901@viscovery.net","threadId":"36730","inReplyTo":"87a9aa7xxt.fsf@fencepost.gnu.org","subject":"Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-05-22T14:28:58Z","receivedAt":"2014-05-22T14:28:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/22/2014 15:19, schrieb David Kastrup:\n> Torsten Bögershausen <tboegi@web.de> writes:\n> \n>> On 2014-05-22 14.48, Elia Pinto wrote:\n>>> Found by check-non-portable-shell.pl\n>>\n>> Thanks for picking this up\n>>> -export TEST_DIRECTORY=$(pwd)/../../../t\n>>> +TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY\n>> Minor remark:\n>> Both commands should go on their own line, like this:\n>>\n>> TEST_DIRECTORY=$(pwd)/../../../t &&\n>> export TEST_DIRECTORY\n>>\n>>\n>> And, unrelated to this patch, \n>> there seem to be a lot of && missing in git-remote-testgit.sh.\n> \n> I have a hard time taking the above && seriously.  pwd is a shell\n> builtin (when we are not talking about Version 3 UNIX or something) that\n> can hardly fail.  And when your shell does not support assignment to a\n> shell variable, you'll have a hard time getting the shell script to run.\n\nThe && after an assignment makes a big difference when the assignment is\npart of an && chain. This is *very* common in our test suite, as you know.\n\nPeople tend to copy-and-paste. And then it is better to provide a more\nuniversally applicable precedent.\n\n-- Hannes\n"},{"id":"242489","messageId":"871tvl98we.fsf@fencepost.gnu.org","threadId":"36730","inReplyTo":"537E09AA.7030901@viscovery.net","subject":"Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-05-22T14:37:37Z","receivedAt":"2014-05-22T14:37:37Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Am 5/22/2014 15:19, schrieb David Kastrup:\n>> Torsten Bögershausen <tboegi@web.de> writes:\n>> \n>>> On 2014-05-22 14.48, Elia Pinto wrote:\n>>>> Found by check-non-portable-shell.pl\n>>>\n>>> Thanks for picking this up\n>>>> -export TEST_DIRECTORY=$(pwd)/../../../t\n>>>> +TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY\n>>> Minor remark:\n>>> Both commands should go on their own line, like this:\n>>>\n>>> TEST_DIRECTORY=$(pwd)/../../../t &&\n>>> export TEST_DIRECTORY\n>>>\n>>>\n>>> And, unrelated to this patch, \n>>> there seem to be a lot of && missing in git-remote-testgit.sh.\n>> \n>> I have a hard time taking the above && seriously.  pwd is a shell\n>> builtin (when we are not talking about Version 3 UNIX or something) that\n>> can hardly fail.  And when your shell does not support assignment to a\n>> shell variable, you'll have a hard time getting the shell script to run.\n>\n> The && after an assignment makes a big difference when the assignment is\n> part of an && chain. This is *very* common in our test suite, as you know.\n>\n> People tend to copy-and-paste. And then it is better to provide a more\n> universally applicable precedent.\n\nCopy-and-paste will not magically add the second && that would be\nrequired for that usage, and the one in the line above might mislead you\ninto thinking that the problem \"has been dealt with already\".\n\nSo I'm not convinced that using && outside of a preexisting && chain\nmakes any sense in this context.\n\n-- \nDavid Kastrup\n"},{"id":"242545","messageId":"xmqqmwe9l6rq.fsf@gitster.dls.corp.google.com","threadId":"36730","inReplyTo":"CA+EOSBkmZRd7FiTNUfDPrUX4NivHR7HUL-+UqDzcpE+X7BQcOA@mail.gmail.com","subject":"Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-22T23:42:49Z","receivedAt":"2014-05-22T23:42:49Z","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> I have no problems rerolling  this simple patch, but i need to know\n> what is the (git) right style in this case.\n\nIf I were doing this...\n\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 66ce4b0..c1d0b23 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -8,7 +8,7 @@ This test verifies the basic operation of the merge, pull, add\n and split subcommands of git subtree.\n '\n \n-export TEST_DIRECTORY=$(pwd)/../../../t\n+TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY\n\n... I'd say I would make this two lines without &&, i.e.\n\n\tTEST_DIRECTORY=$(pwd)/../../../t\n        export TEST_DIRECTORY\n\nbecause we are not doing anything about a failure of $(pwd), other\nthan skipping the export.  If we were failing the entire thing, it\nwould be a different story, but at this point of the test, it is\nunreasonable to do something like:\n\n\tTEST_DIRECTORY=$(pwd)/../../../t &&\n        export TEST_DIRECTORY || exit $?\n\nanyway.\n \ndiff --git a/git-remote-testgit.sh b/git-remote-testgit.sh\nindex 1c006a0..9d1ce76 100755\n--- a/git-remote-testgit.sh\n+++ b/git-remote-testgit.sh\n@@ -13,7 +13,7 @@ refspec=\"${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}\"\n \n test -z \"$refspec\" && prefix=\"refs\"\n \n-export GIT_DIR=\"$url/.git\"\n+GIT_DIR=\"$url/.git\" && export GIT_DIR\n \n force=\n\nSimilarly, as we do not do anything if the assignment to GIT_DIR\nfails here, just like we ignore any failure from assignment to\nforce,\n\n    GIT_DIR=\"$url/.git\"\n    export GIT_DIR\n\nwould be sufficient.\n\nThe logic before the \"mkdir -p\" of this script may want to be\ncleaned up (can you tell how \"if $refspec is empty, set prefix=refs\"\nmakes any sense, and what its implications are, without thinking for\nmore than 10 seconds?), but that is clearly a separate topic [*1*].\n\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 4798bcf..0bb1af8 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -94,7 +94,7 @@ create_stash () {\n \t\t# ease of unpacking later.\n \t\tu_commit=$(\n \t\t\tuntracked_files | (\n-\t\t\t\texport GIT_INDEX_FILE=\"$TMPindex\"\n+\t\t\t\tGIT_INDEX_FILE=\"$TMPindex\" && export GIT_INDEX_FILE &&\n \t\t\t\trm -f \"$TMPindex\" &&\n \t\t\t\tgit update-index -z --add --remove --stdin &&\n \t\t\t\tu_tree=$(git write-tree) &&\n\nThis one does care about failures.  I'd rather have these on\nseparate lines, i.e.\n\n                                GIT_INDEX_FILE=\"$TMPindex\" &&\n                                export GIT_INDEX_FILE &&\n                                rm -f \"$TMPindex\" &&\n                                ...\n\n\n[Footnote]\n\n*1* Perhaps something along these lines with the exact placements of\n    blank lines to group similar things together:\n\n\talias=$1 url=$2\n\n        if test -z \"${GIT_REMOTE_TESTGIT_REFSPEC-notEmpty}\"\n        then\n        \t# only if it is explicitly set to an empty string...\n\t\tprefix=refs\n\telse\n\t\tprefix=\"refs/testgit/$alias\"\n\tfi\n        dir=\"$GIT_DIR/testgit/$alias\"\n\tdefault_refspec=\"refs/heads/*:${prefix}/heads/*\"\n\trefspec=\"${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}\"\n\tforce=\n\n\tGIT_DIR=\"$url/.git\"\n\texport GIT_DIR\n"}]}