threads / patch / 36730

patchGet rid of the non portable shell export VAR=VALUE costruct

Subject: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct

## tl;dr

7 messages between May 22, 2014 and May 22, 2014. Diffs are folded; open one to read it.

replies: 6people: 5as markdown or json

Elia Pinto· May 22, 2014, 12:48 UTC · lore
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(-)
Show changes to 3 files +3 −3

contrib/subtree/t/t7900-subtree.sh, git-remote-testgit.sh, git-stash.sh

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
Torsten Bögershausen· May 22, 2014, 13:13 UTC · re: Elia Pinto · lore

Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct

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.

David Kastrup· May 22, 2014, 13:19 UTC · re: Torsten Bögershausen · lore

Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct

Torsten Bögershausen <tboegi@web.de> writes:
Show 15 quoted lines
> 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
Johannes Sixt· May 22, 2014, 14:28 UTC · re: David Kastrup · lore

Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct

Am 5/22/2014 15:19, schrieb David Kastrup:
Show 22 quoted lines
> 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
David Kastrup· May 22, 2014, 14:37 UTC · re: Johannes Sixt · lore

Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 29 quoted lines
> 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
Elia Pinto· May 22, 2014, 13:57 UTC · re: Torsten Bögershausen · lore

Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct

5-22 14.48, Elia Pinto wrote:
Show 11 quoted lines
>> 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 &&

Show 7 quoted lines
>
> 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.
>
>
>
Junio C Hamano· May 22, 2014, 23:42 UTC · re: Elia Pinto · lore

Re: [PATCH] Get rid of the non portable shell export VAR=VALUE costruct

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...
Show changes to 3 files +3 −3

contrib/subtree/t/t7900-subtree.sh, git-remote-testgit.sh, git-stash.sh

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

← back to recent threads