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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:

> 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@xxxxxxxxx>
---
 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 () {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]