Re: [PATCH 3/3] rebase -i: Export GIT_AUTHOR_* variables explicitly

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2010, 21:09 UTC
Message-ID: <7viqatu9rv.fsf@alter.siamese.dyndns.org>
In-Reply-To: <alpine.DEB.1.00.1001221212350.4985@pacific.mpi-cbg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 9 quoted lines

> On Fri, 22 Jan 2010, Michael Haggerty wrote:
>
>> As pointed out on the mailing list, one-shot shell variable exports do
>> not necessarily work with shell functions.  So export the GIT_AUTHOR_*
>> variables explicitly using "export".
>
> This one's a bit hairy; I really was not sure about unintended side 
> effects, that is why I avoided the export.
> ... (sensible worries omitted) ...

My reading of "git grep -C3 AUTHOR git-rebase--interactive.sh" is that
GIT_AUTHOR_NAME/EMAIL/DATE are set by:
 
   - redoing a merge (while preserving history) by reading from the merge;

   - squashing/fixing into the last one (relying on the last one did the
     right thing) by reading from the HEAD; or

   - sourcing AUTHOR_SCRIPT upon --continue, reading from the file left
     by make_patch immediately before the previous invocation gave control
     back to the user (e.g. "e"dit, or conflict).

The places we want to see these variables in effect are while doing one of the above three places, and they all do this with do_with_author.

Nothing that is called with do_with_author has a side effect of setting shell variables for the caller of do_with_author.

So I tend to think this patch would be the cleanest and safest alternative, albeit it may cost an extra fork.

What do you think?

-- >8 --
Subject: rebase -i: Export GIT_AUTHOR_* variables explicitly

There is no point doing self-assignments of these variables. Instead, just export them to the environment, but do so in a sub-shell, because

	VAR1=VAL1 VAR2=VAL2 ... command arg1 arg2...

does not mark the variables exported if command that is run is a shell function, according to POSIX.1.

The callers of do_with_author do not rely on seeing the effect of any shell variable assignments that may happen inside what was called through this shell function (currently "output" is the only one), so running it in the subshell doesn't have an adverse semantic effect.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-rebase--interactive.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c2f6089..9187e9b 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -215,10 +215,10 @@ has_action () {
 # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
 # GIT_AUTHOR_DATE exported from the current environment.
 do_with_author () {
-	GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
-	GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
-	GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
-	"$@"
+	(
+		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE
+		"$@"
+	)
 }
 
 pick_one () {