Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Apr 11, 2014, 17:30 UTC
Message-ID: <vpq4n1zg4f9.fsf@anie.imag.fr>
In-Reply-To: <D2BF8546-87B4-431A-B86D-6EFAF205279C@gmail.com>

"Kyle J. McKay" <mackyle@gmail.com> writes:

> There are already nested functions with file inclusion between both
> levels of nesting in git-rebase--interactive.sh and git-rebase--
> merge.sh now, so it's not introducing anything new.

OK, so it's less serious than I thought. But still, we're introducing a function with 3 levels of nesting, split accross files, in an area where we know that at least one shell is buggy ...

>> IOW, why not move the whole run_specific_rebase_internal function to
>> git-rebase--$type?
>
> So what change are you proposing exactly?

Something along the lines of this:

diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index df46f4c..4f7b22d 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -4,6 +4,8 @@
 # Copyright (c) 2010 Junio C Hamano.
 #
 
+run_specific_rebase_infile() {
+
 case "$action" in
 continue)
 	git am --resolved --resolvemsg="$resolvemsg" \
@@ -75,3 +77,4 @@ then
 fi
 
 move_to_original_branch
+}
[ Same patch for other git-rebase--*.sh variants]
index 76f7f71..1a150bd 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -186,7 +186,7 @@ run_specific_rebase_internal () {
 	# run_specific_rebase_internal has the file inclusion as a
 	# last statement, so POSIX and FreeBSD's return will do the
 	# same thing.
-	. git-rebase--$type
+	run_specific_rebase_infile
 }
 
 run_specific_rebase () {
@@ -438,6 +438,8 @@ else
 	state_dir="$apply_dir"
 fi
 
+. git-rebase--$type
+
 if test -z "$rebase_root"
 then
 	case "$#" in

I minimized patch size, so it would obviously need a reidentation, and
would require some cleanup so that run_specific_rebase_internal is
merged back into run_specific_rebase (a bit like your PATCH 2).

I find the result simpler, just using the basic pattern "use '. file' to
import a set of functions, and then use these functions".

The real patch is a bit more tricky though, because we need to run the
". git-rebase--$type" after computing type properly. A patch passing the
tests but requiring cleanup is given below.

> To make the kind of change I think you're proposing would be somewhat
> more invasive than the proposed patch.  Each of the git-rebase--$type
> scripts would have to be modified not to do anything other than define
> functions

That's almost what your patch does already: move everything into a
function, and call it. Except, I'd put the function call outside the
file inclusion.



diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index df46f4c..4f7b22d 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -4,6 +4,8 @@
 # Copyright (c) 2010 Junio C Hamano.
 #
 
+run_specific_rebase_infile() {
+
 case "$action" in
 continue)
 	git am --resolved --resolvemsg="$resolvemsg" \
@@ -75,3 +77,4 @@ then
 fi
 
 move_to_original_branch
+}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6046778..5dfbf14 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -820,6 +820,7 @@ add_exec_commands () {
 	mv "$1.new" "$1"
 }
 
+run_specific_rebase_infile() {
 case "$action" in
 continue)
 	# do we have anything to commit?
@@ -1055,3 +1056,4 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
 output git checkout $onto || die_abort "could not detach HEAD"
 git update-ref ORIG_HEAD $orig_head
 do_rest
+}
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index d84f412..907aa46 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -99,6 +99,7 @@ finish_rb_merge () {
 	say All done.
 }
 
+run_specific_rebase_infile () {
 case "$action" in
 continue)
 	read_state
@@ -149,3 +150,4 @@ do
 done
 
 finish_rb_merge
+}
diff --git a/git-rebase.sh b/git-rebase.sh
index 76f7f71..63e0e68 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -186,7 +186,7 @@ run_specific_rebase_internal () {
 	# run_specific_rebase_internal has the file inclusion as a
 	# last statement, so POSIX and FreeBSD's return will do the
 	# same thing.
-	. git-rebase--$type
+	run_specific_rebase_infile
 }
 
 run_specific_rebase () {
@@ -366,6 +366,29 @@ then
 	die "$(gettext "The --edit-todo action can only be used during interactive rebase.")"
 fi
 
+if test -n "$rebase_root" && test -z "$onto"
+then
+	test -z "$interactive_rebase" && interactive_rebase=implied
+fi
+
+if test -z "$in_progress"
+then
+	if test -n "$interactive_rebase"
+	then
+		type=interactive
+		state_dir="$merge_dir"
+	elif test -n "$do_merge"
+	then
+		type=merge
+		state_dir="$merge_dir"
+	else
+		type=am
+		state_dir="$apply_dir"
+	fi
+fi
+
+. git-rebase--$type
+
 case "$action" in
 continue)
 	# Sanity check
@@ -420,24 +443,6 @@ and run me again.  I am stopping in case you still have something
 valuable there.')"
 fi
 
-if test -n "$rebase_root" && test -z "$onto"
-then
-	test -z "$interactive_rebase" && interactive_rebase=implied
-fi
-
-if test -n "$interactive_rebase"
-then
-	type=interactive
-	state_dir="$merge_dir"
-elif test -n "$do_merge"
-then
-	type=merge
-	state_dir="$merge_dir"
-else
-	type=am
-	state_dir="$apply_dir"
-fi
-
 if test -z "$rebase_root"
 then
 	case "$#" in

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/