[PATCH 0/3] Fix support for FreeBSD's /bin/sh

[PATCH 0/3] Fix support for FreeBSD's /bin/sh

[Kyle J. McKay]

This patch series words around the FreeBSD /bin/sh problems that cause rebase
to fail on FreeBSD as well as test t5560-http-backend-noserver.

The rebase issue was first introduced in Git v1.8.4 which started using the
"return" statement to return from a "dot" command script where the "dot"
command itself was located inside a function.  This behaves badly when using
FreeBSD's /bin/sh.  An attempt was made to fix this in Git v1.8.4.1, but it
only addressed part of the problem.

Patch 1/3 fixes the problem by eliminating such use of "return", patch 2/3
then reverts the earlier workaround since it is no longer needed and did
not fully solve the problem.

The t5560 issue was first introduced in Git v1.7.0 which started using a
backslash escapes in ${...} expansions.  The FreeBSD /bin/sh does not
properly support such escapes, but there is a simple change that works
everywhere (using [?] instead of \?).

There are more details in the individual patches.

This patch series is based on maint since these are bug fixes and that's
what SubmittingPatches says to do...

Kyle J. McKay (3):
  rebase: avoid non-function use of "return" on FreeBSD
  Revert "rebase: fix run_specific_rebase's use of "return" on FreeBSD"
  test: fix t5560 on FreeBSD

 git-rebase--am.sh                | 131 +++++++--------
 git-rebase--interactive.sh       | 341 ++++++++++++++++++++-------------------
 git-rebase--merge.sh             |  87 +++++-----
 git-rebase.sh                    |  11 +-
 t/t5560-http-backend-noserver.sh |   4 +-
 5 files changed, 287 insertions(+), 287 deletions(-)

The above stat may seem like a lot, but when using diff -w --stat you get this:

 git-rebase--am.sh                |  3 +++
 git-rebase--interactive.sh       |  3 +++
 git-rebase--merge.sh             |  3 +++
 git-rebase.sh                    | 11 +----------
 t/t5560-http-backend-noserver.sh |  4 ++--
 5 files changed, 12 insertions(+), 12 deletions(-)

which more accurately reflects what was actually touched.

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

[Kyle J. McKay]

Since a1549e10, 15d4bf2e and 01a1e646 (first appearing in v1.8.4) the
git-rebase--*.sh scripts have used a "return" to return from the "dot"
command that runs them.  While this is allowed by POSIX, the FreeBSD
/bin/sh utility behaves poorly under some circumstances when such a
"return" is executed.

In particular, if the "dot" command is contained within a function,
then when a "return" is executed by the script it runs (that is not
itself inside a function), control will return from the function
that contains the "dot" command skipping any statements that might
follow the dot command inside that function.  Commit 99855ddf (first
appearing in v1.8.4.1) addresses this by making the "dot" command
the last line in the function.  Unfortunately the FreeBSD /bin/sh
may also execute some statements in the script run by the "dot"
command that appear after the troublesome "return".  The fix in
99855ddf does not address this problem.

For example, if you have script1.sh with these contents:

#!/bin/sh
# script1.sh
run_script2() {
        . "$(dirname -- "$0")/script2.sh"
        _e=$?
        echo only this line should show
        [ $_e -eq 5 ] || echo expected status 5 got $_e
        return 3
}
run_script2
e=$?
[ $e -eq 3 ] || { echo expected status 3 got $e; exit 1; }

And script2.sh with these contents:

# script2.sh
if [ 5 -gt 3 ]; then
        return 5
fi
case bad in *)
        echo always shows
esac
echo should not get here
! :

When running script1.sh (e.g. '/bin/sh script1.sh' or './script1.sh'
after making it executable), the expected output from a POSIX shell
is simply the single line:

only this line should show

However, when run using FreeBSD's /bin/sh, the following output
appears instead:

should not get here
expected status 3 got 1

Not only did the lines following the "dot" command in the run_script2
function in script1.sh get skipped, but additional lines in script2.sh
following the "return" got executed -- but not all of them (e.g. the
"echo always shows" line did not run).

These issues can be avoided by not using a top-level "return" in
script2.sh.  If script2.sh is changed to this:

# script2.sh fixed
main() {
        if [ 5 -gt 3 ]; then
                return 5
        fi
        case bad in *)
                echo always shows
        esac
        echo should not get here
        ! :
}
main

Then it behaves the same when using FreeBSD's /bin/sh as when using
other more POSIX compliant /bin/sh implementations.

We fix the git-rebase--*.sh scripts in a similar fashion by moving
the top-level code that contains "return" statements into its own
function and then calling that as the last line in the script.

The changes introduced by this commit are best viewed with the
--ignore-all-space (-w) diff option.

Signed-off-by: Kyle J. McKay <mackyle@gmail.com>

---
 git-rebase--am.sh          | 117 ++++++++--------
 git-rebase--interactive.sh | 335 +++++++++++++++++++++++----------------------
 git-rebase--merge.sh       |  87 ++++++------
 3 files changed, 274 insertions(+), 265 deletions(-)


For convenience, here are the diffs using -w:

|--- a/git-rebase--am.sh
|+++ b/git-rebase--am.sh
|@@ -4,6 +4,7 @@
 # Copyright (c) 2010 Junio C Hamano.
 #
 
+git_rebase__am() {
 	case "$action" in
 	continue)
 		git am --resolved --resolvemsg="$resolvemsg" &&
|@@ -73,3 +74,5 @@ then
 	fi
 
 	move_to_original_branch
+}
+git_rebase__am

|--- a/git-rebase--interactive.sh
|+++ b/git-rebase--interactive.sh
|@@ -810,6 +810,7 @@ add_exec_commands () {
 	mv "$1.new" "$1"
 }
 
+git_rebase__interactive() {
 	case "$action" in
 	continue)
 		# do we have anything to commit?
|@@ -1042,3 +1043,5 @@ 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
+}
+git_rebase__interactive

|--- a/git-rebase--merge.sh
|+++ b/git-rebase--merge.sh
|@@ -101,6 +101,7 @@ finish_rb_merge () {
 	say All done.
 }
 
+git_rebase__merge() {
 	case "$action" in
 	continue)
 		read_state
|@@ -151,3 +152,5 @@ do
 	done
 
 	finish_rb_merge
+}
+git_rebase__merge


diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index a4f683a5..2d3f6d55 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -4,72 +4,75 @@
 # Copyright (c) 2010 Junio C Hamano.
 #
 
-case "$action" in
-continue)
-	git am --resolved --resolvemsg="$resolvemsg" &&
-	move_to_original_branch
-	return
-	;;
-skip)
-	git am --skip --resolvemsg="$resolvemsg" &&
-	move_to_original_branch
-	return
-	;;
-esac
-
-test -n "$rebase_root" && root_flag=--root
-
-ret=0
-if test -n "$keep_empty"
-then
-	# we have to do this the hard way.  git format-patch completely squashes
-	# empty commits and even if it didn't the format doesn't really lend
-	# itself well to recording empty patches.  fortunately, cherry-pick
-	# makes this easy
-	git cherry-pick --allow-empty "$revisions"
-	ret=$?
-else
-	rm -f "$GIT_DIR/rebased-patches"
+git_rebase__am() {
+	case "$action" in
+	continue)
+		git am --resolved --resolvemsg="$resolvemsg" &&
+		move_to_original_branch
+		return
+		;;
+	skip)
+		git am --skip --resolvemsg="$resolvemsg" &&
+		move_to_original_branch
+		return
+		;;
+	esac
 
-	git format-patch -k --stdout --full-index --ignore-if-in-upstream \
-		--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \
-		$root_flag "$revisions" >"$GIT_DIR/rebased-patches"
-	ret=$?
+	test -n "$rebase_root" && root_flag=--root
 
-	if test 0 != $ret
+	ret=0
+	if test -n "$keep_empty"
 	then
+		# we have to do this the hard way.  git format-patch completely squashes
+		# empty commits and even if it didn't the format doesn't really lend
+		# itself well to recording empty patches.  fortunately, cherry-pick
+		# makes this easy
+		git cherry-pick --allow-empty "$revisions"
+		ret=$?
+	else
 		rm -f "$GIT_DIR/rebased-patches"
-		case "$head_name" in
-		refs/heads/*)
-			git checkout -q "$head_name"
-			;;
-		*)
-			git checkout -q "$orig_head"
-			;;
-		esac
 
-		cat >&2 <<-EOF
+		git format-patch -k --stdout --full-index --ignore-if-in-upstream \
+			--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \
+			$root_flag "$revisions" >"$GIT_DIR/rebased-patches"
+		ret=$?
 
-		git encountered an error while preparing the patches to replay
-		these revisions:
+		if test 0 != $ret
+		then
+			rm -f "$GIT_DIR/rebased-patches"
+			case "$head_name" in
+			refs/heads/*)
+				git checkout -q "$head_name"
+				;;
+			*)
+				git checkout -q "$orig_head"
+				;;
+			esac
 
-		    $revisions
+			cat >&2 <<-EOF
 
-		As a result, git cannot rebase them.
-		EOF
-		return $?
-	fi
+			git encountered an error while preparing the patches to replay
+			these revisions:
+
+			    $revisions
 
-	git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" <"$GIT_DIR/rebased-patches"
-	ret=$?
+			As a result, git cannot rebase them.
+			EOF
+			return $?
+		fi
 
-	rm -f "$GIT_DIR/rebased-patches"
-fi
+		git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" <"$GIT_DIR/rebased-patches"
+		ret=$?
 
-if test 0 != $ret
-then
-	test -d "$state_dir" && write_basic_state
-	return $ret
-fi
+		rm -f "$GIT_DIR/rebased-patches"
+	fi
 
-move_to_original_branch
+	if test 0 != $ret
+	then
+		test -d "$state_dir" && write_basic_state
+		return $ret
+	fi
+
+	move_to_original_branch
+}
+git_rebase__am
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 43631b47..42164f11 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -810,16 +810,17 @@ add_exec_commands () {
 	mv "$1.new" "$1"
 }
 
-case "$action" in
-continue)
-	# do we have anything to commit?
-	if git diff-index --cached --quiet HEAD --
-	then
-		: Nothing to commit -- skip this
-	else
-		if ! test -f "$author_script"
+git_rebase__interactive() {
+	case "$action" in
+	continue)
+		# do we have anything to commit?
+		if git diff-index --cached --quiet HEAD --
 		then
-			die "You have staged changes in your working tree. If these changes are meant to be
+			: Nothing to commit -- skip this
+		else
+			if ! test -f "$author_script"
+			then
+				die "You have staged changes in your working tree. If these changes are meant to be
 squashed into the previous commit, run:
 
   git commit --amend
@@ -832,42 +833,42 @@ In both case, once you're done, continue with:
 
   git rebase --continue
 "
-		fi
-		. "$author_script" ||
-			die "Error trying to find the author identity to amend commit"
-		if test -f "$amend"
-		then
-			current_head=$(git rev-parse --verify HEAD)
-			test "$current_head" = $(cat "$amend") ||
-			die "\
+			fi
+			. "$author_script" ||
+				die "Error trying to find the author identity to amend commit"
+			if test -f "$amend"
+			then
+				current_head=$(git rev-parse --verify HEAD)
+				test "$current_head" = $(cat "$amend") ||
+				die "\
 You have uncommitted changes in your working tree. Please, commit them
 first and then run 'git rebase --continue' again."
-			do_with_author git commit --amend --no-verify -F "$msg" -e ||
-				die "Could not commit staged changes."
-		else
-			do_with_author git commit --no-verify -F "$msg" -e ||
-				die "Could not commit staged changes."
+				do_with_author git commit --amend --no-verify -F "$msg" -e ||
+					die "Could not commit staged changes."
+			else
+				do_with_author git commit --no-verify -F "$msg" -e ||
+					die "Could not commit staged changes."
+			fi
 		fi
-	fi
 
-	record_in_rewritten "$(cat "$state_dir"/stopped-sha)"
+		record_in_rewritten "$(cat "$state_dir"/stopped-sha)"
 
-	require_clean_work_tree "rebase"
-	do_rest
-	return 0
-	;;
-skip)
-	git rerere clear
+		require_clean_work_tree "rebase"
+		do_rest
+		return 0
+		;;
+	skip)
+		git rerere clear
 
-	do_rest
-	return 0
-	;;
-edit-todo)
-	git stripspace --strip-comments <"$todo" >"$todo".new
-	mv -f "$todo".new "$todo"
-	collapse_todo_ids
-	append_todo_help
-	git stripspace --comment-lines >>"$todo" <<\EOF
+		do_rest
+		return 0
+		;;
+	edit-todo)
+		git stripspace --strip-comments <"$todo" >"$todo".new
+		mv -f "$todo".new "$todo"
+		collapse_todo_ids
+		append_todo_help
+		git stripspace --comment-lines >>"$todo" <<\EOF
 
 You are editing the todo file of an ongoing interactive rebase.
 To continue rebase after editing, run:
@@ -875,170 +876,172 @@ To continue rebase after editing, run:
 
 EOF
 
-	git_sequence_editor "$todo" ||
-		die "Could not execute editor"
-	expand_todo_ids
+		git_sequence_editor "$todo" ||
+			die "Could not execute editor"
+		expand_todo_ids
 
-	exit
-	;;
-esac
+		exit
+		;;
+	esac
 
-git var GIT_COMMITTER_IDENT >/dev/null ||
-	die "You need to set your committer info first"
+	git var GIT_COMMITTER_IDENT >/dev/null ||
+		die "You need to set your committer info first"
 
-comment_for_reflog start
+	comment_for_reflog start
 
-if test ! -z "$switch_to"
-then
-	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
-	output git checkout "$switch_to" -- ||
-	die "Could not checkout $switch_to"
+	if test ! -z "$switch_to"
+	then
+		GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
+		output git checkout "$switch_to" -- ||
+		die "Could not checkout $switch_to"
 
-	comment_for_reflog start
-fi
+		comment_for_reflog start
+	fi
 
-orig_head=$(git rev-parse --verify HEAD) || die "No HEAD?"
-mkdir -p "$state_dir" || die "Could not create temporary $state_dir"
+	orig_head=$(git rev-parse --verify HEAD) || die "No HEAD?"
+	mkdir -p "$state_dir" || die "Could not create temporary $state_dir"
 
-: > "$state_dir"/interactive || die "Could not mark as interactive"
-write_basic_state
-if test t = "$preserve_merges"
-then
-	if test -z "$rebase_root"
+	: > "$state_dir"/interactive || die "Could not mark as interactive"
+	write_basic_state
+	if test t = "$preserve_merges"
 	then
-		mkdir "$rewritten" &&
-		for c in $(git merge-base --all $orig_head $upstream)
-		do
-			echo $onto > "$rewritten"/$c ||
+		if test -z "$rebase_root"
+		then
+			mkdir "$rewritten" &&
+			for c in $(git merge-base --all $orig_head $upstream)
+			do
+				echo $onto > "$rewritten"/$c ||
+					die "Could not init rewritten commits"
+			done
+		else
+			mkdir "$rewritten" &&
+			echo $onto > "$rewritten"/root ||
 				die "Could not init rewritten commits"
-		done
+		fi
+		# No cherry-pick because our first pass is to determine
+		# parents to rewrite and skipping dropped commits would
+		# prematurely end our probe
+		merges_option=
 	else
-		mkdir "$rewritten" &&
-		echo $onto > "$rewritten"/root ||
-			die "Could not init rewritten commits"
+		merges_option="--no-merges --cherry-pick"
 	fi
-	# No cherry-pick because our first pass is to determine
-	# parents to rewrite and skipping dropped commits would
-	# prematurely end our probe
-	merges_option=
-else
-	merges_option="--no-merges --cherry-pick"
-fi
 
-shorthead=$(git rev-parse --short $orig_head)
-shortonto=$(git rev-parse --short $onto)
-if test -z "$rebase_root"
-	# this is now equivalent to ! -z "$upstream"
-then
-	shortupstream=$(git rev-parse --short $upstream)
-	revisions=$upstream...$orig_head
-	shortrevisions=$shortupstream..$shorthead
-else
-	revisions=$onto...$orig_head
-	shortrevisions=$shorthead
-fi
-git rev-list $merges_option --pretty=oneline --abbrev-commit \
-	--abbrev=7 --reverse --left-right --topo-order \
-	$revisions | \
-	sed -n "s/^>//p" |
-while read -r shortsha1 rest
-do
-
-	if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1
+	shorthead=$(git rev-parse --short $orig_head)
+	shortonto=$(git rev-parse --short $onto)
+	if test -z "$rebase_root"
+		# this is now equivalent to ! -z "$upstream"
 	then
-		comment_out="$comment_char "
+		shortupstream=$(git rev-parse --short $upstream)
+		revisions=$upstream...$orig_head
+		shortrevisions=$shortupstream..$shorthead
 	else
-		comment_out=
+		revisions=$onto...$orig_head
+		shortrevisions=$shorthead
 	fi
+	git rev-list $merges_option --pretty=oneline --abbrev-commit \
+		--abbrev=7 --reverse --left-right --topo-order \
+		$revisions | \
+		sed -n "s/^>//p" |
+	while read -r shortsha1 rest
+	do
 
-	if test t != "$preserve_merges"
-	then
-		printf '%s\n' "${comment_out}pick $shortsha1 $rest" >>"$todo"
-	else
-		sha1=$(git rev-parse $shortsha1)
-		if test -z "$rebase_root"
+		if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1
 		then
-			preserve=t
-			for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)
-			do
-				if test -f "$rewritten"/$p
-				then
-					preserve=f
-				fi
-			done
+			comment_out="$comment_char "
 		else
-			preserve=f
-		fi
-		if test f = "$preserve"
-		then
-			touch "$rewritten"/$sha1
-			printf '%s\n' "${comment_out}pick $shortsha1 $rest" >>"$todo"
+			comment_out=
 		fi
-	fi
-done
 
-# Watch for commits that been dropped by --cherry-pick
-if test t = "$preserve_merges"
-then
-	mkdir "$dropped"
-	# Save all non-cherry-picked changes
-	git rev-list $revisions --left-right --cherry-pick | \
-		sed -n "s/^>//p" > "$state_dir"/not-cherry-picks
-	# Now all commits and note which ones are missing in
-	# not-cherry-picks and hence being dropped
-	git rev-list $revisions |
-	while read rev
-	do
-		if test -f "$rewritten"/$rev -a "$(sane_grep "$rev" "$state_dir"/not-cherry-picks)" = ""
+		if test t != "$preserve_merges"
 		then
-			# Use -f2 because if rev-list is telling us this commit is
-			# not worthwhile, we don't want to track its multiple heads,
-			# just the history of its first-parent for others that will
-			# be rebasing on top of it
-			git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev
-			short=$(git rev-list -1 --abbrev-commit --abbrev=7 $rev)
-			sane_grep -v "^[a-z][a-z]* $short" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
-			rm "$rewritten"/$rev
+			printf '%s\n' "${comment_out}pick $shortsha1 $rest" >>"$todo"
+		else
+			sha1=$(git rev-parse $shortsha1)
+			if test -z "$rebase_root"
+			then
+				preserve=t
+				for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)
+				do
+					if test -f "$rewritten"/$p
+					then
+						preserve=f
+					fi
+				done
+			else
+				preserve=f
+			fi
+			if test f = "$preserve"
+			then
+				touch "$rewritten"/$sha1
+				printf '%s\n' "${comment_out}pick $shortsha1 $rest" >>"$todo"
+			fi
 		fi
 	done
-fi
 
-test -s "$todo" || echo noop >> "$todo"
-test -n "$autosquash" && rearrange_squash "$todo"
-test -n "$cmd" && add_exec_commands "$todo"
+	# Watch for commits that been dropped by --cherry-pick
+	if test t = "$preserve_merges"
+	then
+		mkdir "$dropped"
+		# Save all non-cherry-picked changes
+		git rev-list $revisions --left-right --cherry-pick | \
+			sed -n "s/^>//p" > "$state_dir"/not-cherry-picks
+		# Now all commits and note which ones are missing in
+		# not-cherry-picks and hence being dropped
+		git rev-list $revisions |
+		while read rev
+		do
+			if test -f "$rewritten"/$rev -a "$(sane_grep "$rev" "$state_dir"/not-cherry-picks)" = ""
+			then
+				# Use -f2 because if rev-list is telling us this commit is
+				# not worthwhile, we don't want to track its multiple heads,
+				# just the history of its first-parent for others that will
+				# be rebasing on top of it
+				git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev
+				short=$(git rev-list -1 --abbrev-commit --abbrev=7 $rev)
+				sane_grep -v "^[a-z][a-z]* $short" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
+				rm "$rewritten"/$rev
+			fi
+		done
+	fi
+
+	test -s "$todo" || echo noop >> "$todo"
+	test -n "$autosquash" && rearrange_squash "$todo"
+	test -n "$cmd" && add_exec_commands "$todo"
 
-cat >>"$todo" <<EOF
+	cat >>"$todo" <<EOF
 
 $comment_char Rebase $shortrevisions onto $shortonto
 EOF
-append_todo_help
-git stripspace --comment-lines >>"$todo" <<\EOF
+	append_todo_help
+	git stripspace --comment-lines >>"$todo" <<\EOF
 
 However, if you remove everything, the rebase will be aborted.
 
 EOF
 
-if test -z "$keep_empty"
-then
-	printf '%s\n' "$comment_char Note that empty commits are commented out" >>"$todo"
-fi
+	if test -z "$keep_empty"
+	then
+		printf '%s\n' "$comment_char Note that empty commits are commented out" >>"$todo"
+	fi
 
 
-has_action "$todo" ||
-	die_abort "Nothing to do"
+	has_action "$todo" ||
+		die_abort "Nothing to do"
 
-cp "$todo" "$todo".backup
-git_sequence_editor "$todo" ||
-	die_abort "Could not execute editor"
+	cp "$todo" "$todo".backup
+	git_sequence_editor "$todo" ||
+		die_abort "Could not execute editor"
 
-has_action "$todo" ||
-	die_abort "Nothing to do"
+	has_action "$todo" ||
+		die_abort "Nothing to do"
 
-expand_todo_ids
+	expand_todo_ids
 
-test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
+	test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
 
-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
+	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
+}
+git_rebase__interactive
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index e7d96de9..b5f05bf5 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -101,53 +101,56 @@ finish_rb_merge () {
 	say All done.
 }
 
-case "$action" in
-continue)
-	read_state
-	continue_merge
-	while test "$msgnum" -le "$end"
-	do
-		call_merge "$msgnum"
+git_rebase__merge() {
+	case "$action" in
+	continue)
+		read_state
 		continue_merge
+		while test "$msgnum" -le "$end"
+		do
+			call_merge "$msgnum"
+			continue_merge
+		done
+		finish_rb_merge
+		return
+		;;
+	skip)
+		read_state
+		git rerere clear
+		msgnum=$(($msgnum + 1))
+		while test "$msgnum" -le "$end"
+		do
+			call_merge "$msgnum"
+			continue_merge
+		done
+		finish_rb_merge
+		return
+		;;
+	esac
+
+	mkdir -p "$state_dir"
+	echo "$onto_name" > "$state_dir/onto_name"
+	write_basic_state
+
+	msgnum=0
+	for cmt in `git rev-list --reverse --no-merges "$revisions"`
+	do
+		msgnum=$(($msgnum + 1))
+		echo "$cmt" > "$state_dir/cmt.$msgnum"
 	done
-	finish_rb_merge
-	return
-	;;
-skip)
-	read_state
-	git rerere clear
-	msgnum=$(($msgnum + 1))
+
+	echo 1 >"$state_dir/msgnum"
+	echo $msgnum >"$state_dir/end"
+
+	end=$msgnum
+	msgnum=1
+
 	while test "$msgnum" -le "$end"
 	do
 		call_merge "$msgnum"
 		continue_merge
 	done
-	finish_rb_merge
-	return
-	;;
-esac
-
-mkdir -p "$state_dir"
-echo "$onto_name" > "$state_dir/onto_name"
-write_basic_state
-
-msgnum=0
-for cmt in `git rev-list --reverse --no-merges "$revisions"`
-do
-	msgnum=$(($msgnum + 1))
-	echo "$cmt" > "$state_dir/cmt.$msgnum"
-done
-
-echo 1 >"$state_dir/msgnum"
-echo $msgnum >"$state_dir/end"
-
-end=$msgnum
-msgnum=1
 
-while test "$msgnum" -le "$end"
-do
-	call_merge "$msgnum"
-	continue_merge
-done
-
-finish_rb_merge
+	finish_rb_merge
+}
+git_rebase__merge
-- 
tg: (0bc85abb..) t/freebsd-sh-return (depends on: maint)

[PATCH 2/3] Revert "rebase: fix run_specific_rebase's use of "return" on FreeBSD"

[Kyle J. McKay]

This reverts commit 99855ddf4bd319cd06a0524e755ab1c1b7d39f3b.

The workaround 99855ddf introduced to deal with problematic
"return" statements in scripts run by "dot" commands located
inside functions only handles one part of the problem.  The
issue has now been addressed by not using "return" statements
in this way in the git-rebase--*.sh scripts.

This workaround is therefore no longer necessary, so clean
up the code by reverting it.

Conflicts:
        git-rebase.sh

Signed-off-by: Kyle J. McKay <mackyle@gmail.com>

---
 git-rebase.sh | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 8a3efa29..07e2bd48 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -169,22 +169,13 @@ You can run "git stash pop" or "git stash drop" at any time.
 	rm -rf "$state_dir"
 }
 
-run_specific_rebase_internal () {
+run_specific_rebase () {
 	if [ "$interactive_rebase" = implied ]; then
 		GIT_EDITOR=:
 		export GIT_EDITOR
 		autosquash=
 	fi
-	# On FreeBSD, the shell's "return" returns from the current
-	# function, not from the current file inclusion.
-	# 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 () {
-	run_specific_rebase_internal
 	ret=$?
 	if test $ret -eq 0
 	then
-- 
tg: (1bb252a9..) t/revert-99855ddf (depends on: t/freebsd-sh-return)

[PATCH 3/3] test: fix t5560 on FreeBSD

[Kyle J. McKay]

Since fd0a8c2e (first appearing in v1.7.0), the
t/t5560-http-backend-noserver.sh test has used a backslash escape
inside a ${} expansion in order to specify a literal '?' character.

Unfortunately the FreeBSD /bin/sh does not interpret this correctly.

In a POSIX compliant shell, the following:

x='one?two?three'
echo "${x#*\?}"

Would be expected to produce this:

two?three

When using the FreeBSD /bin/sh instead you get this:

one?two?three

In fact the FreeBSD /bin/sh treats the backslash as a literal
character to match so that this:

y='one\two\three'
echo "${y#*\?}"

Produces this unexpected value:

wo\three

In this case the backslash is not only treated literally, it also
fails to defeat the special meaning of the '?' character.

Instead, we can use the [...] construct to defeat the special meaning
of the '?' character and match it exactly in a way that works for the
FreeBSD /bin/sh as well as other POSIX /bin/sh implementations.

Changing the example like so:

x='one?two?three'
echo "${x#*[?]}"

Produces the expected output using the FreeBSD /bin/sh.

Therefore, change the use of \? to [?] in order to be compatible with
the FreeBSD /bin/sh which allows t/t5560-http-backend-noserver.sh to
pass on FreeBSD again.

Signed-off-by: Kyle J. McKay <mackyle@gmail.com>

---
 t/t5560-http-backend-noserver.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index 9be9ae34..5abd11a5 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -9,8 +9,8 @@ test_have_prereq GREP_STRIPS_CR && export GREP_OPTIONS=-U
 
 run_backend() {
 	echo "$2" |
-	QUERY_STRING="${1#*\?}" \
-	PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%\?*}" \
+	QUERY_STRING="${1#*[?]}" \
+	PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}" \
 	git http-backend >act.out 2>act.err
 }
 
-- 
tg: (532c2992..) t/freebsd-t5560 (depends on: t/revert-99855ddf)

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

[Matthieu Moy]

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

> If script2.sh is changed to this:
>
> # script2.sh fixed
> main() {
>         if [ 5 -gt 3 ]; then
>                 return 5
>         fi
>         case bad in *)
>                 echo always shows
>         esac
>         echo should not get here
>         ! :
> }
> main

Wouldn't it be better to just stop using . within function?

The .-ed script could define the complete function, and then the
function would be used from the toplevel script.

If I understand correctly, your version uses nested functions with file
inclusion between both levels of nesting. That might work for the shells
you tested, but if the goal is to avoid using tricky features that may
trigger bugs on some shells, that seems backward.

IOW, why not move the whole run_specific_rebase_internal function to
git-rebase--$type?

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

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

[Kyle J. McKay]

On Apr 11, 2014, at 01:48, Matthieu Moy wrote:
> "Kyle J. McKay" <mackyle@gmail.com> writes:
>
>> If script2.sh is changed to this:
>>
>> # script2.sh fixed
>> main() {
>>        if [ 5 -gt 3 ]; then
>>                return 5
>>        fi
>>        case bad in *)
>>                echo always shows
>>        esac
>>        echo should not get here
>>        ! :
>> }
>> main
>
> Wouldn't it be better to just stop using . within function?
>
> The .-ed script could define the complete function, and then the
> function would be used from the toplevel script.
>
> If I understand correctly, your version uses nested functions with  
> file
> inclusion between both levels of nesting. That might work for the  
> shells
> you tested, but if the goal is to avoid using tricky features that may
> trigger bugs on some shells, that seems backward.

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.

> IOW, why not move the whole run_specific_rebase_internal function to
> git-rebase--$type?

So what change are you proposing exactly?

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--$type.sh: top-level -- has "return"

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 when included which would require some code movement to  
avoid having two "main" functions -- either that or there need to be  
multiple "dot" includes in git-rebase.sh so the code not in a function  
only executes when the selected case that uses it is active -- or the  
entire contents of each git-rebase--$type script gets indented and  
becomes a function definition, but that would introduce functions  
defining functions which seems like it would add use of a new tricky  
feature not previously used.

I'm not saying it's a bad idea, it's just somewhat more invasive than  
simply inserting 3 lines.

--Kyle

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

[Matthieu Moy]

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

Re: [PATCH 3/3] test: fix t5560 on FreeBSD

[Junio C Hamano]

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

> Instead, we can use the [...] construct to defeat the special meaning
> of the '?' character and match it exactly in a way that works for the
> FreeBSD /bin/sh as well as other POSIX /bin/sh implementations.
>
> Changing the example like so:
>
> x='one?two?three'
> echo "${x#*[?]}"
>
> Produces the expected output using the FreeBSD /bin/sh.

I'll queue this in the meantime, while 1&2/3 are sorted out between
you and Matthieu.

I will also take "cp -a" patch with "-R -P -p", not just "-R" for
the reasons stated in the other thread.

Thanks.

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

[Kyle J. McKay]

On Apr 11, 2014, at 10:30, Matthieu Moy wrote:
> "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 ...

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.

>>> 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--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
> +}


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

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

[Matthieu Moy]

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

> On Apr 11, 2014, at 10:30, Matthieu Moy wrote:
>> "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 ...
>
> 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

You're confusing function calls and function nesting. do_rest calls
do_next, but the definition of do_next is not nested within do_rest.

When I talk about nested function, I mean

f() {
	g() {
		...
	}
}

Obviously, having functions call each other is not an issue. That's what
functions are meant to be.

Now, having run_specific_rebase_internal include a file which defines
functions which contain nested functions _is_ something I find weird. It
both stresses the shell in a buggy area and makes the code harder to
understand.

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

Function definitions, and variables assignments. Is it so much of an
issue?

What's the difference between a function definition or variable
assignment within git-rebase--*.sh and within git-rebase.sh?

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

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

[Kyle J. McKay]

On Apr 12, 2014, at 10:07, Matthieu Moy wrote:
> "Kyle J. McKay" <mackyle@gmail.com> writes:
>
>> On Apr 11, 2014, at 10:30, Matthieu Moy wrote:
>>> "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 ...
>>
>> 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
>
> You're confusing function calls and function nesting. do_rest calls
> do_next, but the definition of do_next is not nested within do_rest.
>
> When I talk about nested function, I mean
>
> f() {
> 	g() {
> 		...
> 	}
> }
>
> Obviously, having functions call each other is not an issue. That's  
> what
> functions are meant to be.
>
> Now, having run_specific_rebase_internal include a file which defines
> functions which contain nested functions _is_ something I find  
> weird. It
> both stresses the shell in a buggy area and makes the code harder to
> understand.

I meant: "nested functions" = "nested function calls"
You meant: "nested functions" = "nested function definitions"

Okay.  But nested function definitions is not something new to the
rebase code.

>> 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.
>
> Function definitions, and variables assignments. Is it so much of an
> issue?
>
> What's the difference between a function definition or variable
> assignment within git-rebase--*.sh and within git-rebase.sh?

As I said, this is the issue:

On Apr 11, 2014, at 16:08, Kyle J. McKay wrote:

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

Let me rephrase.

Patch 1/3 does not change the order in which individual statements are  
executed in the rebase code.  Nor does it change the logic.  It simply  
introduces one additional function callstack level, but the same  
individual statements are executed in the same order for all control  
flows.  No additional statements other than the introduced callstack  
level.  Nothing's executed in a different order.  No control paths  
execute additional statements they did not before.

The changes you propose introduce exactly the same additional function  
callstack level.  Then they proceed to alter the order in which  
statements are executed.  Statements that did not execute in some  
control paths before are now executed in those control paths.  Other  
statements are moved around to execute earlier/later than they did  
before.

My point is not that this is wrong.  It's nice, really, to move the  
"dot" include to the top level.  The point is that's not necessary to  
fix the problem and moving statements around and adding statements to  
some control paths increases the risk of introducing an uncaught bug  
when it's not necessary to fix the problem.

I would like to get a fix in so that rebase works out-of-the-box when  
Git version 1.8.4 or later is built on FreeBSD.

So I'm not going to belabor the point anymore.

There's a follow-up patch 4/3 attached to the end of this message that  
makes the changes you have proposed in your earlier email on top of  
patches 1/3 and 2/3.  The log message and all changes are taken from  
your emails and so this patch is assigned to you and has a
'Needs-Signed-off-by:' line.

On Apr 11, 2014, at 10:30, Matthieu Moy wrote:
> 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.

Unfortunately, after applying the below patch, some of the rebase  
tests (3403, 3407, 3418, 3420, 3421) start failing for me on systems  
where they did not fail previously.  Even though some of the previously  
failing tests on FreeBSD now pass with the patch, 3421 now fails on  
FreeBSD where it did not before.

Here's a test summary of the t34xx*.sh tests:

NOTE: at the time of these tests, maint and v1.9.2 were at the same  
commit.

______________SYSTEM____t34xx*.sh_results________________

maint         FreeBSD   FAIL: 3403,3404,3407,3409,3410,3412,3418,3419,3420
maint         Darwin    PASS
maint         Linux     PASS

maint+1-3/3   FreeBSD   PASS
maint+1-3/3   Darwin    PASS
maint+1-3/3   Linux     PASS

maint+1-4/3   FreeBSD   FAIL[*]: 3403,3407,3418,3420,3421
maint+1-4/3   Darwin    FAIL[*]: 3403,3407,3418,3420,3421
maint+1-4/3   Linux     FAIL[*]: 3403,3407,3418,3420,3421

[*]: The failing test reports for all three systems are identical for  
the "maint+1-4/3" run (except for the wallclock secs part):

Test Summary Report
-------------------
t3403-rebase-skip.sh                   (Wstat: 256 Tests: 10 Failed: 3)
   Failed tests:  8-10
   Non-zero exit status: 1
t3407-rebase-abort.sh                  (Wstat: 256 Tests: 11 Failed: 3)
   Failed tests:  7-9
   Non-zero exit status: 1
t3418-rebase-continue.sh               (Wstat: 256 Tests: 6 Failed: 2)
   Failed tests:  5-6
   Non-zero exit status: 1
t3420-rebase-autostash.sh              (Wstat: 256 Tests: 24 Failed: 11)
   Failed tests:  14-24
   Non-zero exit status: 1
t3421-rebase-topology-linear.sh        (Wstat: 256 Tests: 76 Failed: 16)
   Failed tests:  51-53, 55, 57, 59-63, 65, 67, 73-76
   Non-zero exit status: 1
Files=22, Tests=374, 354 wallclock secs ( 0.31 usr  0.13 sys + 79.57 cusr 233.99 csys = 314.00 CPU)
Result: FAIL
make: *** [prove] Error 1


So I suggest that in the interest of fixing rebase on FreeBSD in an  
expeditious fashion, patches 1/3 and 2/3 get picked up (see note  
below) now and that the follow-on patch below, after being enhanced to  
pass all tests, be submitted separately at some future point.

--Kyle

P.S.  Note to JCH: the below patch requires the previous 1/3 and 2/3  
be applied first.  As per SubmittingPatches for bug fixes those are  
based on maint.  Because of the whitespace change 1/3 introduces it  
does not apply cleanly to master, next or pu.  :(  Please let me know  
if you would like me to resend the initial series (1 & 2 -- 3 has  
already been picked up) based on a different branch instead of maint.

---- 8< ----
From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Subject: [PATCH 4/3] rebase: stop using . within function

Move the whole run_specific_rebase_internal function to
git-rebase--$type.

The .-ed script defines the complete function, and then the
function is used from the toplevel script.

The goal is to avoid using tricky features that may trigger
bugs on some shells.

The result is simpler, just using the basic pattern:

    1. use '. file' to import a set of functions
    2. then use these functions

Needs-Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 git-rebase--am.sh          |  3 +--
 git-rebase--interactive.sh |  3 +--
 git-rebase--merge.sh       |  3 +--
 git-rebase.sh              | 40 +++++++++++++++++++++-------------------
 4 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 2d3f6d55..b48b3e90 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -4,7 +4,7 @@
 # Copyright (c) 2010 Junio C Hamano.
 #
 
-git_rebase__am() {
+run_specific_rebase_infile() {
 	case "$action" in
 	continue)
 		git am --resolved --resolvemsg="$resolvemsg" &&
@@ -75,4 +75,3 @@ git_rebase__am() {
 
 	move_to_original_branch
 }
-git_rebase__am
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 42164f11..a7670eb0 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -810,7 +810,7 @@ add_exec_commands () {
 	mv "$1.new" "$1"
 }
 
-git_rebase__interactive() {
+run_specific_rebase_infile() {
 	case "$action" in
 	continue)
 		# do we have anything to commit?
@@ -1044,4 +1044,3 @@ EOF
 	git update-ref ORIG_HEAD $orig_head
 	do_rest
 }
-git_rebase__interactive
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index b5f05bf5..9550e656 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -101,7 +101,7 @@ finish_rb_merge () {
 	say All done.
 }
 
-git_rebase__merge() {
+run_specific_rebase_infile() {
 	case "$action" in
 	continue)
 		read_state
@@ -153,4 +153,3 @@ git_rebase__merge() {
 
 	finish_rb_merge
 }
-git_rebase__merge
diff --git a/git-rebase.sh b/git-rebase.sh
index 07e2bd48..9e105626 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -175,7 +175,7 @@ run_specific_rebase () {
 		export GIT_EDITOR
 		autosquash=
 	fi
-	. git-rebase--$type
+	run_specific_rebase_infile
 	ret=$?
 	if test $ret -eq 0
 	then
@@ -353,6 +353,26 @@ 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 -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
+
+. git-rebase--$type
+
 case "$action" in
 continue)
 	# Sanity check
@@ -407,24 +427,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
-- 
1.8.5

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

[Matthieu Moy]

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

> So I suggest that in the interest of fixing rebase on FreeBSD in an  
> expeditious fashion, patches 1/3 and 2/3 get picked up (see note  
> below) now and that the follow-on patch below, after being enhanced to  
> pass all tests, be submitted separately at some future point.

Seems a good plan to me.

> Needs-Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>

Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>

> From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> Subject: [PATCH 4/3] rebase: stop using . within function
> 
> Move the whole run_specific_rebase_internal function to
> git-rebase--$type.
> 
> The .-ed script defines the complete function, and then the
> function is used from the toplevel script.
> 
> The goal is to avoid using tricky features that may trigger
> bugs on some shells.
> 
> The result is simpler, just using the basic pattern:
> 
>     1. use '. file' to import a set of functions
>     2. then use these functions
> 
> Needs-Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> ---
>  git-rebase--am.sh          |  3 +--
>  git-rebase--interactive.sh |  3 +--
>  git-rebase--merge.sh       |  3 +--
>  git-rebase.sh              | 40 +++++++++++++++++++++-------------------
>  4 files changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
> index 2d3f6d55..b48b3e90 100644
> --- a/git-rebase--am.sh
> +++ b/git-rebase--am.sh
> @@ -4,7 +4,7 @@
>  # Copyright (c) 2010 Junio C Hamano.
>  #
>  
> -git_rebase__am() {
> +run_specific_rebase_infile() {
>  	case "$action" in
>  	continue)
>  		git am --resolved --resolvemsg="$resolvemsg" &&
> @@ -75,4 +75,3 @@ git_rebase__am() {
>  
>  	move_to_original_branch
>  }
> -git_rebase__am
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 42164f11..a7670eb0 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -810,7 +810,7 @@ add_exec_commands () {
>  	mv "$1.new" "$1"
>  }
>  
> -git_rebase__interactive() {
> +run_specific_rebase_infile() {
>  	case "$action" in
>  	continue)
>  		# do we have anything to commit?
> @@ -1044,4 +1044,3 @@ EOF
>  	git update-ref ORIG_HEAD $orig_head
>  	do_rest
>  }
> -git_rebase__interactive
> diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
> index b5f05bf5..9550e656 100644
> --- a/git-rebase--merge.sh
> +++ b/git-rebase--merge.sh
> @@ -101,7 +101,7 @@ finish_rb_merge () {
>  	say All done.
>  }
>  
> -git_rebase__merge() {
> +run_specific_rebase_infile() {
>  	case "$action" in
>  	continue)
>  		read_state
> @@ -153,4 +153,3 @@ git_rebase__merge() {
>  
>  	finish_rb_merge
>  }
> -git_rebase__merge
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 07e2bd48..9e105626 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -175,7 +175,7 @@ run_specific_rebase () {
>  		export GIT_EDITOR
>  		autosquash=
>  	fi
> -	. git-rebase--$type
> +	run_specific_rebase_infile
>  	ret=$?
>  	if test $ret -eq 0
>  	then
> @@ -353,6 +353,26 @@ 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 -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
> +
> +. git-rebase--$type
> +
>  case "$action" in
>  continue)
>  	# Sanity check
> @@ -407,24 +427,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
> -- 
> 1.8.5

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

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> "Kyle J. McKay" <mackyle@gmail.com> writes:
>
>> So I suggest that in the interest of fixing rebase on FreeBSD in an  
>> expeditious fashion, patches 1/3 and 2/3 get picked up (see note  
>> below) now and that the follow-on patch below, after being enhanced to  
>> pass all tests, be submitted separately at some future point.
>
> Seems a good plan to me.

OK, should I take that an Ack on 1 & 2?

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

[Junio C Hamano]

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

> For convenience, here are the diffs using -w:
>
> |--- a/git-rebase--am.sh
> |+++ b/git-rebase--am.sh
> |@@ -4,6 +4,7 @@
>  # Copyright (c) 2010 Junio C Hamano.
>  #
>  
> +git_rebase__am() {
>  	case "$action" in
>  	continue)
>  		git am --resolved --resolvemsg="$resolvemsg" &&
> |@@ -73,3 +74,5 @@ then
>  	fi
>  
>  	move_to_original_branch
> +}
> +git_rebase__am

I think we would want to see the actual change formatted this way
(without needing to pass "-w" to "git show"), as it will make it
clear that this artificial extra level of "define the whole thing
inside a function and then make a single call to it" is a workaround
of specific shell's glitch that we would not have to have in an
ideal world ;-)

Besides that would make it less likely to cause conflicts with the
real changes in flight.

Please double check what I queued on 'pu' when I push out today's
integration result.

Thanks.

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

[Kyle J. McKay]

On Apr 14, 2014, at 15:51, Junio C Hamano wrote:
> I think we would want to see the actual change formatted this way
> (without needing to pass "-w" to "git show"), as it will make it
> clear that this artificial extra level of "define the whole thing
> inside a function and then make a single call to it" is a workaround
> of specific shell's glitch that we would not have to have in an
> ideal world ;-)
>
> Besides that would make it less likely to cause conflicts with the
> real changes in flight.

Sounds good to me.

> Please double check what I queued on 'pu' when I push out today's
> integration result.

> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
> index a4f683a5..2ab514ea 100644
> --- a/git-rebase--am.sh
> +++ b/git-rebase--am.sh
> @@ -4,6 +4,13 @@
>  # Copyright (c) 2010 Junio C Hamano.
>  #
>
> +# The whole contents of the file is run by dot-sourcing this file  
> from
> +# inside a shell function, and "return"s we see below are expected to
> +# return from that function that dot-sources us.  However, FreeBSD
> +# /bin/sh misbehaves on such a construct, so we will work it around  
> by
> +# enclosing the whole thing inside an extra layer of a function.
> +git_rebase__am () {

I think the wording may be just a bit off:

> and "return"s we see below are expected to return from that function
> that dot-sources us.

I thought that was one of the buggy behaviors we are working around?

Maybe I'm just reading it wrong...

Does this wording seem any clearer?

	and "return"s we see below are expected not to return
	from the function that dot-sources us, but rather to
	the next command after the one in the function that
	dot-sources us.

Otherwise the patch in pu looks fine (all t34*.sh tests pass for me on  
FreeBSD with pu at 8d8dc6db).

Thank you for adding the comments.

If I'm the only one getting a wrong meaning from the comments, then no  
reason to change them.

--Kyle

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

[Junio C Hamano]

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

> On Apr 14, 2014, at 15:51, Junio C Hamano wrote:
>> I think we would want to see the actual change formatted this way
>> (without needing to pass "-w" to "git show"), as it will make it
>> clear that this artificial extra level of "define the whole thing
>> inside a function and then make a single call to it" is a workaround
>> of specific shell's glitch that we would not have to have in an
>> ideal world ;-)
>>
>> Besides that would make it less likely to cause conflicts with the
>> real changes in flight.
>
> Sounds good to me.
>
>> Please double check what I queued on 'pu' when I push out today's
>> integration result.
>
>> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
>> index a4f683a5..2ab514ea 100644
>> --- a/git-rebase--am.sh
>> +++ b/git-rebase--am.sh
>> @@ -4,6 +4,13 @@
>>  # Copyright (c) 2010 Junio C Hamano.
>>  #
>>
>> +# The whole contents of the file is run by dot-sourcing this file
>> from
>> +# inside a shell function, and "return"s we see below are expected to
>> +# return from that function that dot-sources us.  However, FreeBSD
>> +# /bin/sh misbehaves on such a construct, so we will work it around
>> by
>> +# enclosing the whole thing inside an extra layer of a function.
>> +git_rebase__am () {
>
> I think the wording may be just a bit off:
>
>> and "return"s we see below are expected to return from that function
>> that dot-sources us.
>
> I thought that was one of the buggy behaviors we are working around?

Yeah, it is written as if every reader has the knowledge that the
extra

	extra__func () {
        ...
        }
        extra__func

around did not originally exist.  The description does not read well
with the work-around already there.

> Maybe I'm just reading it wrong...
>
> Does this wording seem any clearer?
>
> 	and "return"s we see below are expected not to return
> 	from the function that dot-sources us, but rather to
> 	the next command after the one in the function that
> 	dot-sources us.

Not really.  The comment was not about explaining "return"s.  When a
reader reads the text with the work-around, it is clear that these
"return"s are inside this extra function and there is no possible
interpretation other than that they are to return from the extra
function.

The comment was meant to explain why a seemingly strange "define a
function and then immediately call it just once" pattern is used,
and "Earlier, these returns were not inside any function when this
file is viewed standalone.  Because this file is to be dot-sourced
within a function, they were to return from that dot-sourcing
function --- but some shells do not behave that way, hence this
strange construct." was meant to be that explanation, but apparently
it failed to convey what I meant to say.

> If I'm the only one getting a wrong meaning from the comments, then no
> reason to change them.

I agree that the description does not read well with the work-around
already there.  I am not sure what would be a better way to phrase
it, though.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Kyle J. McKay" <mackyle@gmail.com> writes:
>
>> If I'm the only one getting a wrong meaning from the comments, then no
>> reason to change them.
>
> I agree that the description does not read well with the work-around
> already there.  I am not sure what would be a better way to phrase
> it, though.

Here is a tentative rewrite at the beginning and the end of rebase-am:

    # The whole contents of the file is run by dot-sourcing this
    # file from inside a shell function.  It used to be that
    # "return"s we see below were not inside any function, and
    # expected to return from the function that dot-sourced us.
    #
    # However, FreeBSD /bin/sh misbehaves on such a construct and
    # continues to run the statements that follow such a "return".
    # As a work-around, we introduce an extra layer of a function
    # here, and immediately call it after defining it.
    git_rebase__am () {

    ...

    }
    # ... and then we call the whole thing.
    git_rebase__am


By the way, you have this in your log message:

    ... the git-rebase--*.sh scripts have used a "return" to return
    from the "dot" command that runs them.  While this is allowed by
    POSIX,...


Is it "this is allowed", or is it "this should be the way and shells
that do not do so are buggy"?

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> By the way, you have this in your log message:
>
>     ... the git-rebase--*.sh scripts have used a "return" to return
>     from the "dot" command that runs them.  While this is allowed by
>     POSIX,...
>
>
> Is it "this is allowed", or is it "this should be the way and shells
> that do not do so are buggy"?

Answering myself...

The only "unspecified" I see is this:

    If the shell is not currently executing a function or dot
    script, the results are unspecified.

which clearly does not apply to the version before this patch (we
are executing a dot script).  And

    The return utility shall cause the shell to stop executing the
    current function or dot script.

would mean that we are correct to expect that "should not get here"
is not reached, as the "return 5" would cause the shell to stop
executing the dot script there.

So "while this is allowed by POSIX" may be a bit misleading and
needs to be reworded, I guess?

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

[Kyle J. McKay]

On Apr 16, 2014, at 11:11, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> "Kyle J. McKay" <mackyle@gmail.com> writes:
>>
>>> If I'm the only one getting a wrong meaning from the comments,  
>>> then no
>>> reason to change them.
>>
>> I agree that the description does not read well with the work-around
>> already there.  I am not sure what would be a better way to phrase
>> it, though.
>
> Here is a tentative rewrite at the beginning and the end of rebase-am:
>
>    # The whole contents of the file is run by dot-sourcing this
>    # file from inside a shell function.  It used to be that
>    # "return"s we see below were not inside any function, and
>    # expected to return from the function that dot-sourced us.

I think it's the "return from the function that dot-sourced us" that  
gives me the wrong meaning.  The "return from" part says to me that  
function will be returning which it will not unless the dot command  
was the last command in the function.

Either "return to the function that dot-sourced us" or "return from  
the dot command that dot-sourced us", but using the original wording  
implies to me that the function that dot-sourced us will return as  
soon as the dot-sourced script executes the return and that is exactly  
one of the bugs we're working around.

I think just the s/from/to/ would fix it so it does not give me the  
wrong impression, but that doesn't mean that would not confuse  
everyone else.  ;)

>    #
>    # However, FreeBSD /bin/sh misbehaves on such a construct and
>    # continues to run the statements that follow such a "return".
>    # As a work-around, we introduce an extra layer of a function
>    # here, and immediately call it after defining it.
>    git_rebase__am () {
>
>    ...
>
>    }
>    # ... and then we call the whole thing.
>    git_rebase__am

On Apr 16, 2014, at 11:23, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> By the way, you have this in your log message:
>>
>>    ... the git-rebase--*.sh scripts have used a "return" to return
>>    from the "dot" command that runs them.  While this is allowed by
>>    POSIX,...
>>
>>
>> Is it "this is allowed", or is it "this should be the way and shells
>> that do not do so are buggy"?
>
> Answering myself...
>
> The only "unspecified" I see is this:
>
>    If the shell is not currently executing a function or dot
>    script, the results are unspecified.
>
> which clearly does not apply to the version before this patch (we
> are executing a dot script).  And
>
>    The return utility shall cause the shell to stop executing the
>    current function or dot script.
>
> would mean that we are correct to expect that "should not get here"
> is not reached, as the "return 5" would cause the shell to stop
> executing the dot script there.
>
> So "while this is allowed by POSIX" may be a bit misleading and
> needs to be reworded, I guess?

"allowed by POSIX" is not incorrect. ;)

But perhaps the log message should say 'While POSIX specifies that a  
"return" may be used to exit a "dot" sourced script,' there instead to  
be clearer.  Which would make that whole first paragraph become:

     Since a1549e10, 15d4bf2e and 01a1e646 (first appearing in v1.8.4)  
the
     git-rebase--*.sh scripts have used a "return" to return from the  
"dot"
     command that runs them.  While POSIX specifies that a "return"  
may be
     used to exit a "dot" sourced script, the FreeBSD /bin/sh utility
     behaves poorly under some circumstances when such a "return" is  
executed.

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

[Junio C Hamano]

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

> Either "return to the function that dot-sourced us" or "return from
> the dot command that dot-sourced us",

> but using the original wording
> implies to me that the function that dot-sourced us will return as
> soon as the dot-sourced script executes the return and that is exactly
> one of the bugs we're working around.

True. "return to the function that dot-sourced us" is what I meant.
To be more pedantic, "return to the point in the function that
dot-sourced this file".

> I think just the s/from/to/ would fix it so it does not give me the
> wrong impression, but that doesn't mean that would not confuse
> everyone else.  ;)

Yeah, let's do that.  Thanks for carefully reading them.

>> So "while this is allowed by POSIX" may be a bit misleading and
>> needs to be reworded, I guess?
>
> "allowed by POSIX" is not incorrect. ;)

Calling something that is required as "allowed" is a bit misleading,
if not outright dishonest.  Allowed implies that you are free not to
do so and can still be in compliance, but I do not think that is the
case.

I'd think it makes it clearer to say that we take "return stopping
the dot-sourced file" as a given and FreeBSD does not behave that
way.

-- >8 --
From: "Kyle J. McKay" <mackyle@gmail.com>
Date: Fri, 11 Apr 2014 01:28:17 -0700
Subject: [PATCH] rebase: avoid non-function use of "return" on FreeBSD

Since a1549e10, 15d4bf2e and 01a1e646 (first appearing in v1.8.4)
the git-rebase--*.sh scripts have used a "return" to stop execution
of the dot-sourced file and return to the "dot" command that
dot-sourced it.  The /bin/sh utility on FreeBSD however behaves
poorly under some circumstances when such a "return" is executed.

In particular, if the "dot" command is contained within a function,
then when a "return" is executed by the script it runs (that is not
itself inside a function), control will return from the function
that contains the "dot" command skipping any statements that might
follow the dot command inside that function.  Commit 99855ddf (first
appearing in v1.8.4.1) addresses this by making the "dot" command
the last line in the function.

Unfortunately the FreeBSD /bin/sh may also execute some statements
in the script run by the "dot" command that appear after the
troublesome "return".  The fix in 99855ddf does not address this
problem.

For example, if you have script1.sh with these contents:

run_script2() {
        . "$(dirname -- "$0")/script2.sh"
        _e=$?
        echo only this line should show
        [ $_e -eq 5 ] || echo expected status 5 got $_e
        return 3
}
run_script2
e=$?
[ $e -eq 3 ] || { echo expected status 3 got $e; exit 1; }

And script2.sh with these contents:

if [ 5 -gt 3 ]; then
        return 5
fi
case bad in *)
        echo always shows
esac
echo should not get here
! :

When running script1.sh (e.g. '/bin/sh script1.sh' or './script1.sh'
after making it executable), the expected output from a POSIX shell
is simply the single line:

only this line should show

However, when run using FreeBSD's /bin/sh, the following output
appears instead:

should not get here
expected status 3 got 1

Not only did the lines following the "dot" command in the run_script2
function in script1.sh get skipped, but additional lines in script2.sh
following the "return" got executed -- but not all of them (e.g. the
"echo always shows" line did not run).

These issues can be avoided by not using a top-level "return" in
script2.sh.  If script2.sh is changed to this:

main() {
        if [ 5 -gt 3 ]; then
                return 5
        fi
        case bad in *)
                echo always shows
        esac
        echo should not get here
        ! :
}
main

Then it behaves the same when using FreeBSD's /bin/sh as when using
other more POSIX compliant /bin/sh implementations.

We fix the git-rebase--*.sh scripts in a similar fashion by moving
the top-level code that contains "return" statements into its own
function and then calling that as the last line in the script.

Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
Acked-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-rebase--am.sh          | 15 +++++++++++++++
 git-rebase--interactive.sh | 15 +++++++++++++++
 git-rebase--merge.sh       | 15 +++++++++++++++
 3 files changed, 45 insertions(+)

diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index a4f683a..1cdc139 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -4,6 +4,17 @@
 # Copyright (c) 2010 Junio C Hamano.
 #
 
+# The whole contents of this file is run by dot-sourcing it from
+# inside a shell function.  It used to be that "return"s we see
+# below were not inside any function, and expected to return
+# to the function that dot-sourced us.
+#
+# However, FreeBSD /bin/sh misbehaves on such a construct and
+# continues to run the statements that follow such a "return".
+# As a work-around, we introduce an extra layer of a function
+# here, and immediately call it after defining it.
+git_rebase__am () {
+
 case "$action" in
 continue)
 	git am --resolved --resolvemsg="$resolvemsg" &&
@@ -73,3 +84,7 @@ then
 fi
 
 move_to_original_branch
+
+}
+# ... and then we call the whole thing.
+git_rebase__am
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 43631b4..9e1dd1e 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -810,6 +810,17 @@ add_exec_commands () {
 	mv "$1.new" "$1"
 }
 
+# The whole contents of this file is run by dot-sourcing it from
+# inside a shell function.  It used to be that "return"s we see
+# below were not inside any function, and expected to return
+# to the function that dot-sourced us.
+#
+# However, FreeBSD /bin/sh misbehaves on such a construct and
+# continues to run the statements that follow such a "return".
+# As a work-around, we introduce an extra layer of a function
+# here, and immediately call it after defining it.
+git_rebase__interactive () {
+
 case "$action" in
 continue)
 	# do we have anything to commit?
@@ -1042,3 +1053,7 @@ 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
+
+}
+# ... and then we call the whole thing.
+git_rebase__interactive
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index e7d96de..838fbed 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -101,6 +101,17 @@ finish_rb_merge () {
 	say All done.
 }
 
+# The whole contents of this file is run by dot-sourcing it from
+# inside a shell function.  It used to be that "return"s we see
+# below were not inside any function, and expected to return
+# to the function that dot-sourced us.
+#
+# However, FreeBSD /bin/sh misbehaves on such a construct and
+# continues to run the statements that follow such a "return".
+# As a work-around, we introduce an extra layer of a function
+# here, and immediately call it after defining it.
+git_rebase__merge () {
+
 case "$action" in
 continue)
 	read_state
@@ -151,3 +162,7 @@ do
 done
 
 finish_rb_merge
+
+}
+# ... and then we call the whole thing.
+git_rebase__merge
-- 
1.9.2-630-gc3ddd0c

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

[Kyle J. McKay]

On Apr 17, 2014, at 10:15, Junio C Hamano wrote:

>> I think just the s/from/to/ would fix it so it does not give me the
>> wrong impression, but that doesn't mean that would not confuse
>> everyone else.  ;)
>
> Yeah, let's do that.  Thanks for carefully reading them.

> I'd think it makes it clearer to say that we take "return stopping
> the dot-sourced file" as a given and FreeBSD does not behave that
> way.
>
> -- > 8 --
> From: "Kyle J. McKay" <mackyle@gmail.com>
> Date: Fri, 11 Apr 2014 01:28:17 -0700
> Subject: [PATCH] rebase: avoid non-function use of "return" on FreeBSD
[...]

The new version of the patch looks great, let's use that.  Thanks for  
adjusting it.