* [PATCH] Get rid of the non portable shell export VAR=VALUE costruct @ 2014-05-22 12:48 Elia Pinto 2014-05-22 13:13 ` Torsten Bögershausen 0 siblings, 1 reply; 7+ messages in thread From: Elia Pinto @ 2014-05-22 12:48 UTC (permalink / raw) To: git; +Cc: Elia Pinto Found by check-non-portable-shell.pl Signed-off-by: Elia Pinto <gitter.spiros@gmail.com> --- contrib/subtree/t/t7900-subtree.sh | 2 +- git-remote-testgit.sh | 2 +- git-stash.sh | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh index 66ce4b0..c1d0b23 100755 --- a/contrib/subtree/t/t7900-subtree.sh +++ b/contrib/subtree/t/t7900-subtree.sh @@ -8,7 +8,7 @@ This test verifies the basic operation of the merge, pull, add and split subcommands of git subtree. ' -export TEST_DIRECTORY=$(pwd)/../../../t +TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY . ../../../t/test-lib.sh diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh index 1c006a0..9d1ce76 100755 --- a/git-remote-testgit.sh +++ b/git-remote-testgit.sh @@ -13,7 +13,7 @@ refspec="${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}" test -z "$refspec" && prefix="refs" -export GIT_DIR="$url/.git" +GIT_DIR="$url/.git" && export GIT_DIR force= diff --git a/git-stash.sh b/git-stash.sh index 4798bcf..0bb1af8 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -94,7 +94,7 @@ create_stash () { # ease of unpacking later. u_commit=$( untracked_files | ( - export GIT_INDEX_FILE="$TMPindex" + GIT_INDEX_FILE="$TMPindex" && export GIT_INDEX_FILE && rm -f "$TMPindex" && git update-index -z --add --remove --stdin && u_tree=$(git write-tree) && -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct 2014-05-22 12:48 [PATCH] Get rid of the non portable shell export VAR=VALUE costruct Elia Pinto @ 2014-05-22 13:13 ` Torsten Bögershausen 2014-05-22 13:19 ` David Kastrup 2014-05-22 13:57 ` Elia Pinto 0 siblings, 2 replies; 7+ messages in thread From: Torsten Bögershausen @ 2014-05-22 13:13 UTC (permalink / raw) To: Elia Pinto, git On 2014-05-22 14.48, Elia Pinto wrote: > Found by check-non-portable-shell.pl Thanks for picking this up > -export TEST_DIRECTORY=$(pwd)/../../../t > +TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY Minor remark: Both commands should go on their own line, like this: TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY And, unrelated to this patch, there seem to be a lot of && missing in git-remote-testgit.sh. But this should be a seperate patch, I think. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct 2014-05-22 13:13 ` Torsten Bögershausen @ 2014-05-22 13:19 ` David Kastrup 2014-05-22 14:28 ` Johannes Sixt 2014-05-22 13:57 ` Elia Pinto 1 sibling, 1 reply; 7+ messages in thread From: David Kastrup @ 2014-05-22 13:19 UTC (permalink / raw) To: Torsten Bögershausen; +Cc: Elia Pinto, git Torsten Bögershausen <tboegi@web.de> writes: > On 2014-05-22 14.48, Elia Pinto wrote: >> Found by check-non-portable-shell.pl > > Thanks for picking this up >> -export TEST_DIRECTORY=$(pwd)/../../../t >> +TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY > Minor remark: > Both commands should go on their own line, like this: > > TEST_DIRECTORY=$(pwd)/../../../t && > export TEST_DIRECTORY > > > And, unrelated to this patch, > there seem to be a lot of && missing in git-remote-testgit.sh. I have a hard time taking the above && seriously. pwd is a shell builtin (when we are not talking about Version 3 UNIX or something) that can hardly fail. And when your shell does not support assignment to a shell variable, you'll have a hard time getting the shell script to run. That's stuff of the if (1+1 != 2) { fputs("Warning: your CPU may be broken", stderr); } variety. If you have to check for that, you have bigger problems... -- David Kastrup ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct 2014-05-22 13:19 ` David Kastrup @ 2014-05-22 14:28 ` Johannes Sixt 2014-05-22 14:37 ` David Kastrup 0 siblings, 1 reply; 7+ messages in thread From: Johannes Sixt @ 2014-05-22 14:28 UTC (permalink / raw) To: David Kastrup, Torsten Bögershausen; +Cc: Elia Pinto, git Am 5/22/2014 15:19, schrieb David Kastrup: > Torsten Bögershausen <tboegi@web.de> writes: > >> On 2014-05-22 14.48, Elia Pinto wrote: >>> Found by check-non-portable-shell.pl >> >> Thanks for picking this up >>> -export TEST_DIRECTORY=$(pwd)/../../../t >>> +TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY >> Minor remark: >> Both commands should go on their own line, like this: >> >> TEST_DIRECTORY=$(pwd)/../../../t && >> export TEST_DIRECTORY >> >> >> And, unrelated to this patch, >> there seem to be a lot of && missing in git-remote-testgit.sh. > > I have a hard time taking the above && seriously. pwd is a shell > builtin (when we are not talking about Version 3 UNIX or something) that > can hardly fail. And when your shell does not support assignment to a > shell variable, you'll have a hard time getting the shell script to run. The && after an assignment makes a big difference when the assignment is part of an && chain. This is *very* common in our test suite, as you know. People tend to copy-and-paste. And then it is better to provide a more universally applicable precedent. -- Hannes ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct 2014-05-22 14:28 ` Johannes Sixt @ 2014-05-22 14:37 ` David Kastrup 0 siblings, 0 replies; 7+ messages in thread From: David Kastrup @ 2014-05-22 14:37 UTC (permalink / raw) To: Johannes Sixt; +Cc: Torsten Bögershausen, Elia Pinto, git Johannes Sixt <j.sixt@viscovery.net> writes: > Am 5/22/2014 15:19, schrieb David Kastrup: >> Torsten Bögershausen <tboegi@web.de> writes: >> >>> On 2014-05-22 14.48, Elia Pinto wrote: >>>> Found by check-non-portable-shell.pl >>> >>> Thanks for picking this up >>>> -export TEST_DIRECTORY=$(pwd)/../../../t >>>> +TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY >>> Minor remark: >>> Both commands should go on their own line, like this: >>> >>> TEST_DIRECTORY=$(pwd)/../../../t && >>> export TEST_DIRECTORY >>> >>> >>> And, unrelated to this patch, >>> there seem to be a lot of && missing in git-remote-testgit.sh. >> >> I have a hard time taking the above && seriously. pwd is a shell >> builtin (when we are not talking about Version 3 UNIX or something) that >> can hardly fail. And when your shell does not support assignment to a >> shell variable, you'll have a hard time getting the shell script to run. > > The && after an assignment makes a big difference when the assignment is > part of an && chain. This is *very* common in our test suite, as you know. > > People tend to copy-and-paste. And then it is better to provide a more > universally applicable precedent. Copy-and-paste will not magically add the second && that would be required for that usage, and the one in the line above might mislead you into thinking that the problem "has been dealt with already". So I'm not convinced that using && outside of a preexisting && chain makes any sense in this context. -- David Kastrup ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct 2014-05-22 13:13 ` Torsten Bögershausen 2014-05-22 13:19 ` David Kastrup @ 2014-05-22 13:57 ` Elia Pinto 2014-05-22 23:42 ` Junio C Hamano 1 sibling, 1 reply; 7+ messages in thread From: Elia Pinto @ 2014-05-22 13:57 UTC (permalink / raw) To: Torsten Bögershausen, dak; +Cc: git@vger.kernel.org 5-22 14.48, Elia Pinto wrote: >> Found by check-non-portable-shell.pl > > Thanks for picking this up >> -export TEST_DIRECTORY=$(pwd)/../../../t >> +TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY > Minor remark: > Both commands should go on their own line, like this: > > TEST_DIRECTORY=$(pwd)/../../../t && > export TEST_DIRECTORY > Apparently they are both common idioms and I find no indication in the CodingGuideline. I have no problems rerolling this simple patch, but i need to know what is the (git) right style in this case. Thanks ------ cd ./git-core grep "&&.*export" *.sh git-am.sh: GIT_MERGE_VERBOSITY=0 && export GIT_MERGE_VERBOSITY git-filter-branch.sh: echo "case \"\$GIT_$1_NAME\" in \"\") GIT_$1_NAME=\"\${GIT_$1_EMAIL%%@*}\" && export GIT_$1_NAME;; esac" git-filter-branch.sh: GIT_DIR="$ORIG_GIT_DIR" && export GIT_DIR git-rebase--merge.sh: GIT_MERGE_VERBOSITY=1 && export GIT_MERGE_VERBOSITY git-remote-testgit.sh:GIT_DIR="$url/.git" && export GIT_DIR git-stash.sh: GIT_INDEX_FILE="$TMPindex" && export GIT_INDEX_FILE && git-stash.sh: GIT_MERGE_VERBOSITY=0 && export GIT_MERGE_VERBOSITY grep 'export.*&&' *.sh git-stash.sh: GIT_INDEX_FILE="$TMPindex" && export GIT_INDEX_FILE && git-stash.sh: export GIT_INDEX_FILE && > > And, unrelated to this patch, > there seem to be a lot of && missing in git-remote-testgit.sh. > But this should be a seperate patch, I think. > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct 2014-05-22 13:57 ` Elia Pinto @ 2014-05-22 23:42 ` Junio C Hamano 0 siblings, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2014-05-22 23:42 UTC (permalink / raw) To: Elia Pinto; +Cc: Torsten Bögershausen, dak, git@vger.kernel.org Elia Pinto <gitter.spiros@gmail.com> writes: > I have no problems rerolling this simple patch, but i need to know > what is the (git) right style in this case. If I were doing this... diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh index 66ce4b0..c1d0b23 100755 --- a/contrib/subtree/t/t7900-subtree.sh +++ b/contrib/subtree/t/t7900-subtree.sh @@ -8,7 +8,7 @@ This test verifies the basic operation of the merge, pull, add and split subcommands of git subtree. ' -export TEST_DIRECTORY=$(pwd)/../../../t +TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY ... I'd say I would make this two lines without &&, i.e. TEST_DIRECTORY=$(pwd)/../../../t export TEST_DIRECTORY because we are not doing anything about a failure of $(pwd), other than skipping the export. If we were failing the entire thing, it would be a different story, but at this point of the test, it is unreasonable to do something like: TEST_DIRECTORY=$(pwd)/../../../t && export TEST_DIRECTORY || exit $? anyway. diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh index 1c006a0..9d1ce76 100755 --- a/git-remote-testgit.sh +++ b/git-remote-testgit.sh @@ -13,7 +13,7 @@ refspec="${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}" test -z "$refspec" && prefix="refs" -export GIT_DIR="$url/.git" +GIT_DIR="$url/.git" && export GIT_DIR force= Similarly, as we do not do anything if the assignment to GIT_DIR fails here, just like we ignore any failure from assignment to force, GIT_DIR="$url/.git" export GIT_DIR would be sufficient. The logic before the "mkdir -p" of this script may want to be cleaned up (can you tell how "if $refspec is empty, set prefix=refs" makes any sense, and what its implications are, without thinking for more than 10 seconds?), but that is clearly a separate topic [*1*]. diff --git a/git-stash.sh b/git-stash.sh index 4798bcf..0bb1af8 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -94,7 +94,7 @@ create_stash () { # ease of unpacking later. u_commit=$( untracked_files | ( - export GIT_INDEX_FILE="$TMPindex" + GIT_INDEX_FILE="$TMPindex" && export GIT_INDEX_FILE && rm -f "$TMPindex" && git update-index -z --add --remove --stdin && u_tree=$(git write-tree) && This one does care about failures. I'd rather have these on separate lines, i.e. GIT_INDEX_FILE="$TMPindex" && export GIT_INDEX_FILE && rm -f "$TMPindex" && ... [Footnote] *1* Perhaps something along these lines with the exact placements of blank lines to group similar things together: alias=$1 url=$2 if test -z "${GIT_REMOTE_TESTGIT_REFSPEC-notEmpty}" then # only if it is explicitly set to an empty string... prefix=refs else prefix="refs/testgit/$alias" fi dir="$GIT_DIR/testgit/$alias" default_refspec="refs/heads/*:${prefix}/heads/*" refspec="${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}" force= GIT_DIR="$url/.git" export GIT_DIR ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-05-22 23:42 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-05-22 12:48 [PATCH] Get rid of the non portable shell export VAR=VALUE costruct Elia Pinto 2014-05-22 13:13 ` Torsten Bögershausen 2014-05-22 13:19 ` David Kastrup 2014-05-22 14:28 ` Johannes Sixt 2014-05-22 14:37 ` David Kastrup 2014-05-22 13:57 ` Elia Pinto 2014-05-22 23:42 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).