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

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

[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

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

[Torsten Bögershausen]

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.

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

[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.

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

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

[Elia Pinto]

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.
>
>
>

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

[Johannes Sixt]

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

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

[David Kastrup]

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

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

[Junio C Hamano]

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