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

"Kyle J. McKay" [off-list ref] 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 ...

quoted

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/

On Apr 11, 2014, at 10:30, Matthieu Moy wrote:

"Kyle J. McKay" [off-list ref] writes:

quoted

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

Currently in maint:

The current code in maint does this:

git-rebase.sh: top-level
   git-rebase.sh: run_specific_rebase()
     git-rebase.sh: run_specific_rebase_internal() -- contains "dot"
       git-rebase--interactive.sh: top-level (using --continue or -- 
skip)
         git-rebase--interactive.sh: do_rest
           git-rebase--interactive.sh: do_next
             git-rebase--interactive.sh: record_in_rewritten
               git-rebase--interactive.sh: flush_rewritten_pending

So I really do not see the additional level of nesting as an issue  
since we've already got much more than 3 levels of nesting going on  
now.  If nesting was going to be a problem, something would have  
broken already.  In fact, since the follow on patch removes the  
run_specific_rebase_internal function what we would have after the  
originally proposed first two patches is:

git-rebase.sh: top-level
   git-rebase.sh: run_specific_rebase() -- contains "dot"
     git-rebase--interactive.sh: top-level (using --continue or --skip)
       git-rebase--interactive.sh: git_rebase__interactive
         git-rebase--interactive.sh: do_rest
           git-rebase--interactive.sh: do_next
             git-rebase--interactive.sh: record_in_rewritten
               git-rebase--interactive.sh: flush_rewritten_pending

Which has exactly the same nesting depth albeit the "dot" has moved up  
one level.

quoted

quoted

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:

The problem with these changes, particularly the git-rebase-- 
interactive.sh one is that a bunch of code is still run when the file  
is "dot" included.  With the changes to git-rebase.sh, that code will  
now run regardless of the action and it will run before it would have  
now.  So if any of the variables it sets affect the functions like  
read_basic_state or finish_rebase (they don't currently appear to),  
then there's a potential for new bugs.  That initial code would not  
previously have run in the --abort case at all.

But, you say the tests pass with those changes, so the changes are  
probably okay.  However, they create a potential situation where some  
code is added to the top of one of the git-rebase--$type.sh scripts  
and suddenly git rebase --abort stops working right because that code  
is being run when it shouldn't or the operation of read_basic_state  
and/or finish_rebase is adversely affected.  Hopefully the rebase  
tests would catch any such issue right away though.

So, in light of the fact that function nesting seems to be a non-issue  
here, and it seems to me the originally proposed changes have much  
less potential to introduce breakage either now or in the future, I  
still prefer them.

--Kyle