[PATCH 0/3] Fixes to mh/rebase-fixup

[PATCH 0/3] Fixes to mh/rebase-fixup

[Michael Haggerty]

This patch series contains fixes to the mh/rebase-fixup patch series
as suggested by reviewers on the mailing list.

Since the mh/rebase-fixup patch series is already in "next", I have
written these fixup patches against "next".  I hope that is OK.

The third patch removes the attempt to use one-shot export of
variables in do_with_author(), as the "$@" command that it calls might
be a shell function.  This issue was discussed on the mailing list
[1].  This change has the side effect that the GIT_AUTHOR_* variables
remain exported after do_with_author() returns, but I don't think that
this is a problem in the context of git-rebase--interactive.sh.
However, additional review would be welcome here.

[1] http://marc.info/?l=git&m=126345851831751&w=2

Michael Haggerty (3):
  rebase -i: Avoid non-portable "test X -a Y"
  rebase -i: Enclose sed command substitution in quotes
  rebase -i: Export GIT_AUTHOR_* variables explicitly

 git-rebase--interactive.sh |    6 ++----
 t/lib-rebase.sh            |    2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

[PATCH 1/3] rebase -i: Avoid non-portable "test X -a Y"

[Michael Haggerty]

Reported by: Eric Blake <ebb9@byu.net>

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e551906..c2f6089 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -235,7 +235,7 @@ pick_one () {
 	parent_sha1=$(git rev-parse --verify $sha1^) ||
 		die "Could not get the parent of $sha1"
 	current_sha1=$(git rev-parse --verify HEAD)
-	if test -z "$no_ff" -a "$current_sha1" = "$parent_sha1"
+	if test -z "$no_ff" && test "$current_sha1" = "$parent_sha1"
 	then
 		output git reset --hard $sha1
 		output warn Fast-forward to $(git rev-parse --short $sha1)
-- 
1.6.6

[PATCH 2/3] rebase -i: Enclose sed command substitution in quotes

[Michael Haggerty]

Reported by: Johannes Sixt <j.sixt@viscovery.net>

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/lib-rebase.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 2d922ae..6aefe27 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -27,7 +27,7 @@ set_fake_editor () {
 case "$1" in
 */COMMIT_EDITMSG)
 	test -z "$EXPECT_HEADER_COUNT" ||
-		test "$EXPECT_HEADER_COUNT" = $(sed -n '1s/^# This is a combination of \(.*\) commits\./\1/p' < "$1") ||
+		test "$EXPECT_HEADER_COUNT" = "$(sed -n '1s/^# This is a combination of \(.*\) commits\./\1/p' < "$1")" ||
 		exit
 	test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
 	test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
-- 
1.6.6

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

[Michael Haggerty]

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

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-rebase--interactive.sh |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c2f6089..c8603f0 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -215,9 +215,7 @@ 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 &&
 	"$@"
 }
 
-- 
1.6.6

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

[Johannes Schindelin]

Hi,

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.

Just imagine, for example, some git commit --amend which forgets to set 
the author information; I am not saying that this is happening, but I 
cannot be sure, because every possible code path to a git 
commit/commit-tree has to be checked, and this does not mean that future 
patches will not introduce such broken code, either.

It might also be possible that some people scripted rebase -i with a 
custom "editor" as in the tests (I have done so in the past).  They would 
be affected.

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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@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 () {

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

[Michael Haggerty]

Junio C Hamano wrote:
> [...]
> 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

Your version is fine with me.

Michael