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

From

Junio C Hamano <gitster@pobox.com>

Date

May 22, 2014, 23:42 UTC

Message-ID

<xmqqmwe9l6rq.fsf@gitster.dls.corp.google.com>

In-Reply-To

<CA+EOSBkmZRd7FiTNUfDPrUX4NivHR7HUL-+UqDzcpE+X7BQcOA@mail.gmail.com>

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