[RFC PATCH v3 0/9] rebase-interactive:

[RFC PATCH v3 0/9] rebase-interactive:

[Wink Saville]

I've incorporated review feed back to date. I'm split the change
into 9 commits with each commit do a single class of operation.

I've prepared these commits using github and have Travis-CI setup to
test my changes. Of the 9 commits 2 failed, the 1st and 5th commits, I
tested those two locally and they were fine. I then restarted the builds
on Travis-CI they finished fine so it seems the errors were spurious.

Wink Saville (9):
  Simplify pick_on_preserving_merges
  Call git_rebase__interactive from run_specific_rebase
  Indent function git_rebase__interactive
  Extract functions out of git_rebase__interactive
  Use new functions in git_rebase__interactive
  Add and use git_rebase__interactive__preserve_merges
  Remove unused code paths from git_rebase__interactive
  Remove unused code paths from git_rebase__interactive__preserve_merges
  Remove merges_option and a blank line

 git-rebase--interactive.sh | 407 ++++++++++++++++++++++++---------------------
 git-rebase.sh              |  16 +-
 2 files changed, 229 insertions(+), 194 deletions(-)

-- 
2.16.2

[RFC PATCH v3 1/9] Simplify pick_on_preserving_merges

[Wink Saville]

Signed-off-by: Wink Saville <wink@saville.com>
---
 git-rebase--interactive.sh | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 331c8dfea..561e2660e 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -307,17 +307,14 @@ pick_one_preserving_merges () {
 	esac
 	sha1=$(git rev-parse $sha1)
 
-	if test -f "$state_dir"/current-commit
+	if test -f "$state_dir"/current-commit && test "$fast_forward" = t
 	then
-		if test "$fast_forward" = t
-		then
-			while read current_commit
-			do
-				git rev-parse HEAD > "$rewritten"/$current_commit
-			done <"$state_dir"/current-commit
-			rm "$state_dir"/current-commit ||
-				die "$(gettext "Cannot write current commit's replacement sha1")"
-		fi
+		while read current_commit
+		do
+			git rev-parse HEAD > "$rewritten"/$current_commit
+		done <"$state_dir"/current-commit
+		rm "$state_dir"/current-commit ||
+			die "$(gettext "Cannot write current commit's replacement sha1")"
 	fi
 
 	echo $sha1 >> "$state_dir"/current-commit
-- 
2.16.2

[RFC PATCH v3 2/9] Call git_rebase__interactive from run_specific_rebase

[Wink Saville]

Instead of indirectly invoking git_rebase__interactive this invokes
it directly after sourcing.

Signed-off-by: Wink Saville <wink@saville.com>
---
 git-rebase--interactive.sh | 11 -----------
 git-rebase.sh              | 11 +++++++++--
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 561e2660e..213d75f43 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -740,15 +740,6 @@ get_missing_commit_check_level () {
 	printf '%s' "$check_level" | tr 'A-Z' 'a-z'
 }
 
-# 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, older (9.x) versions of FreeBSD /bin/sh misbehave on such a
-# construct and continue 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
@@ -1029,5 +1020,3 @@ fi
 do_rest
 
 }
-# ... and then we call the whole thing.
-git_rebase__interactive
diff --git a/git-rebase.sh b/git-rebase.sh
index a1f6e5de6..c4ec7c21b 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -196,8 +196,15 @@ run_specific_rebase () {
 		export GIT_EDITOR
 		autosquash=
 	fi
-	. git-rebase--$type
-	ret=$?
+	if test "$type" = interactive
+	then
+		. git-rebase--interactive
+		git_rebase__interactive
+		ret=$?
+	else
+		. git-rebase--$type
+		ret=$?
+	fi
 	if test $ret -eq 0
 	then
 		finish_rebase
-- 
2.16.2

[RFC PATCH v3 3/9] Indent function git_rebase__interactive

[Wink Saville]

Signed-off-by: Wink Saville <wink@saville.com>
---
 git-rebase--interactive.sh | 432 ++++++++++++++++++++++-----------------------
 1 file changed, 215 insertions(+), 217 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 213d75f43..a79330f45 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -741,27 +741,26 @@ get_missing_commit_check_level () {
 }
 
 git_rebase__interactive () {
-
-case "$action" in
-continue)
-	if test ! -d "$rewritten"
-	then
-		exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
-			--continue
-	fi
-	# do we have anything to commit?
-	if git diff-index --cached --quiet HEAD --
-	then
-		# Nothing to commit -- skip this commit
-
-		test ! -f "$GIT_DIR"/CHERRY_PICK_HEAD ||
-		rm "$GIT_DIR"/CHERRY_PICK_HEAD ||
-		die "$(gettext "Could not remove CHERRY_PICK_HEAD")"
-	else
-		if ! test -f "$author_script"
+	case "$action" in
+	continue)
+		if test ! -d "$rewritten"
 		then
-			gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")}
-			die "$(eval_gettext "\
+			exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
+				--continue
+		fi
+		# do we have anything to commit?
+		if git diff-index --cached --quiet HEAD --
+		then
+			# Nothing to commit -- skip this commit
+
+			test ! -f "$GIT_DIR"/CHERRY_PICK_HEAD ||
+			rm "$GIT_DIR"/CHERRY_PICK_HEAD ||
+			die "$(gettext "Could not remove CHERRY_PICK_HEAD")"
+		else
+			if ! test -f "$author_script"
+			then
+				gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")}
+				die "$(eval_gettext "\
 You have staged changes in your working tree.
 If these changes are meant to be
 squashed into the previous commit, run:
@@ -776,197 +775,197 @@ In both cases, once you're done, continue with:
 
   git rebase --continue
 ")"
-		fi
-		. "$author_script" ||
-			die "$(gettext "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 "$(gettext "\
+			fi
+			. "$author_script" ||
+				die "$(gettext "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 "$(gettext "\
 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 \
-				${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
-				die "$(gettext "Could not commit staged changes.")"
-		else
-			do_with_author git commit --no-verify -F "$msg" -e \
-				${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
-				die "$(gettext "Could not commit staged changes.")"
+				do_with_author git commit --amend --no-verify -F "$msg" -e \
+					${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
+					die "$(gettext "Could not commit staged changes.")"
+			else
+				do_with_author git commit --no-verify -F "$msg" -e \
+					${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
+					die "$(gettext "Could not commit staged changes.")"
+			fi
 		fi
-	fi
 
-	if test -r "$state_dir"/stopped-sha
-	then
-		record_in_rewritten "$(cat "$state_dir"/stopped-sha)"
-	fi
+		if test -r "$state_dir"/stopped-sha
+		then
+			record_in_rewritten "$(cat "$state_dir"/stopped-sha)"
+		fi
 
-	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
 
-	if test ! -d "$rewritten"
-	then
-		exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
-			--continue
-	fi
-	do_rest
-	return 0
-	;;
-edit-todo)
-	git stripspace --strip-comments <"$todo" >"$todo".new
-	mv -f "$todo".new "$todo"
-	collapse_todo_ids
-	append_todo_help
-	gettext "
+		if test ! -d "$rewritten"
+		then
+			exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
+				--continue
+		fi
+		do_rest
+		return 0
+		;;
+	edit-todo)
+		git stripspace --strip-comments <"$todo" >"$todo".new
+		mv -f "$todo".new "$todo"
+		collapse_todo_ids
+		append_todo_help
+		gettext "
 You are editing the todo file of an ongoing interactive rebase.
 To continue rebase after editing, run:
     git rebase --continue
 
 " | git stripspace --comment-lines >>"$todo"
 
-	git_sequence_editor "$todo" ||
-		die "$(gettext "Could not execute editor")"
-	expand_todo_ids
-
-	exit
-	;;
-show-current-patch)
-	exec git show REBASE_HEAD --
-	;;
-esac
-
-comment_for_reflog start
+		git_sequence_editor "$todo" ||
+			die "$(gettext "Could not execute editor")"
+		expand_todo_ids
 
-if test ! -z "$switch_to"
-then
-	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
-	output git checkout "$switch_to" -- ||
-		die "$(eval_gettext "Could not checkout \$switch_to")"
+		exit
+		;;
+	show-current-patch)
+		exec git show REBASE_HEAD --
+		;;
+	esac
 
 	comment_for_reflog start
-fi
-
-orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
-mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
-rm -f "$(git rev-parse --git-path REBASE_HEAD)"
 
-: > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
-write_basic_state
-if test t = "$preserve_merges"
-then
-	if test -z "$rebase_root"
+	if test ! -z "$switch_to"
 	then
-		mkdir "$rewritten" &&
-		for c in $(git merge-base --all $orig_head $upstream)
-		do
-			echo $onto > "$rewritten"/$c ||
-				die "$(gettext "Could not init rewritten commits")"
-		done
-	else
-		mkdir "$rewritten" &&
-		echo $onto > "$rewritten"/root ||
-			die "$(gettext "Could not init rewritten commits")"
+		GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
+		output git checkout "$switch_to" -- ||
+			die "$(eval_gettext "Could not checkout \$switch_to")"
+
+		comment_for_reflog start
 	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
-if test t != "$preserve_merges"
-then
-	git rebase--helper --make-script ${keep_empty:+--keep-empty} \
-		$revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
-	die "$(gettext "Could not generate todo list")"
-else
-	format=$(git config --get rebase.instructionFormat)
-	# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse
-	git rev-list $merges_option --format="%m%H ${format:-%s}" \
-		--reverse --left-right --topo-order \
-		$revisions ${restrict_revision+^$restrict_revision} | \
-		sed -n "s/^>//p" |
-	while read -r sha1 rest
-	do
 
-		if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1
-		then
-			comment_out="$comment_char "
-		else
-			comment_out=
-		fi
+	orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
+	mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
+	rm -f "$(git rev-parse --git-path REBASE_HEAD)"
 
+	: > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
+	write_basic_state
+	if test t = "$preserve_merges"
+	then
 		if test -z "$rebase_root"
 		then
-			preserve=t
-			for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)
+			mkdir "$rewritten" &&
+			for c in $(git merge-base --all $orig_head $upstream)
 			do
-				if test -f "$rewritten"/$p
-				then
-					preserve=f
-				fi
+				echo $onto > "$rewritten"/$c ||
+					die "$(gettext "Could not init rewritten commits")"
 			done
 		else
-			preserve=f
-		fi
-		if test f = "$preserve"
-		then
-			touch "$rewritten"/$sha1
-			printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo"
-		fi
-	done
-fi
-
-# 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 &&
-		   ! sane_grep "$rev" "$state_dir"/not-cherry-picks >/dev/null
-		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
-			sha1=$(git rev-list -1 $rev)
-			sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
-			rm "$rewritten"/$rev
+			mkdir "$rewritten" &&
+			echo $onto > "$rewritten"/root ||
+				die "$(gettext "Could not init rewritten commits")"
 		fi
-	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
+		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
+	if test t != "$preserve_merges"
+	then
+		git rebase--helper --make-script ${keep_empty:+--keep-empty} \
+			$revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
+		die "$(gettext "Could not generate todo list")"
+	else
+		format=$(git config --get rebase.instructionFormat)
+		# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse
+		git rev-list $merges_option --format="%m%H ${format:-%s}" \
+			--reverse --left-right --topo-order \
+			$revisions ${restrict_revision+^$restrict_revision} | \
+			sed -n "s/^>//p" |
+		while read -r sha1 rest
+		do
+
+			if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1
+			then
+				comment_out="$comment_char "
+			else
+				comment_out=
+			fi
+
+			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 $sha1 $rest" >>"$todo"
+			fi
+		done
+	fi
+
+	# 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 &&
+			   ! sane_grep "$rev" "$state_dir"/not-cherry-picks >/dev/null
+			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
+				sha1=$(git rev-list -1 $rev)
+				sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
+				rm "$rewritten"/$rev
+			fi
+		done
+	fi
 
-test -s "$todo" || echo noop >> "$todo"
-test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
-test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd"
+	test -s "$todo" || echo noop >> "$todo"
+	test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
+	test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd"
 
-todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
-todocount=${todocount##* }
+	todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
+	todocount=${todocount##* }
 
 cat >>"$todo" <<EOF
 
@@ -975,48 +974,47 @@ $comment_char $(eval_ngettext \
 	"Rebase \$shortrevisions onto \$shortonto (\$todocount commands)" \
 	"$todocount")
 EOF
-append_todo_help
-gettext "
-However, if you remove everything, the rebase will be aborted.
-
-" | git stripspace --comment-lines >>"$todo"
+	append_todo_help
+	gettext "
+	However, if you remove everything, the rebase will be aborted.
 
-if test -z "$keep_empty"
-then
-	printf '%s\n' "$comment_char $(gettext "Note that empty commits are commented out")" >>"$todo"
-fi
+	" | git stripspace --comment-lines >>"$todo"
 
+	if test -z "$keep_empty"
+	then
+		printf '%s\n' "$comment_char $(gettext "Note that empty commits are commented out")" >>"$todo"
+	fi
 
-has_action "$todo" ||
-	return 2
 
-cp "$todo" "$todo".backup
-collapse_todo_ids
-git_sequence_editor "$todo" ||
-	die_abort "$(gettext "Could not execute editor")"
+	has_action "$todo" ||
+		return 2
 
-has_action "$todo" ||
-	return 2
+	cp "$todo" "$todo".backup
+	collapse_todo_ids
+	git_sequence_editor "$todo" ||
+		die_abort "$(gettext "Could not execute editor")"
 
-git rebase--helper --check-todo-list || {
-	ret=$?
-	checkout_onto
-	exit $ret
-}
+	has_action "$todo" ||
+		return 2
 
-expand_todo_ids
+	git rebase--helper --check-todo-list || {
+		ret=$?
+		checkout_onto
+		exit $ret
+	}
 
-test -d "$rewritten" || test -n "$force_rebase" ||
-onto="$(git rebase--helper --skip-unnecessary-picks)" ||
-die "Could not skip unnecessary pick commands"
+	expand_todo_ids
 
-checkout_onto
-if test -z "$rebase_root" && test ! -d "$rewritten"
-then
-	require_clean_work_tree "rebase"
-	exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
-		--continue
-fi
-do_rest
+	test -d "$rewritten" || test -n "$force_rebase" ||
+	onto="$(git rebase--helper --skip-unnecessary-picks)" ||
+	die "Could not skip unnecessary pick commands"
 
+	checkout_onto
+	if test -z "$rebase_root" && test ! -d "$rewritten"
+	then
+		require_clean_work_tree "rebase"
+		exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
+			--continue
+	fi
+	do_rest
 }
-- 
2.16.2

[RFC PATCH v3 4/9] Extract functions out of git_rebase__interactive

[Wink Saville]

The extracted functions are:
  - initiate_action
  - setup_reflog_action
  - init_basic_state
  - init_revisions_and_shortrevisions
  - complete_action

Signed-off-by: Wink Saville <wink@saville.com>
---
 git-rebase--interactive.sh | 211 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 211 insertions(+)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a79330f45..b72f80ae8 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -740,6 +740,217 @@ get_missing_commit_check_level () {
 	printf '%s' "$check_level" | tr 'A-Z' 'a-z'
 }
 
+# Initiate an action. If the cannot be any
+# further action it  may exec a command
+# or exit and not return.
+#
+# TODO: Consider a cleaner return model so it
+# never exits and always return 0 if process
+# is complete.
+#
+# Parameter 1 is the action to initiate.
+#
+# Returns 0 if the action was able to complete
+# and if 1 if further processing is required.
+initiate_action () {
+	case "$1" in
+	continue)
+		if test ! -d "$rewritten"
+		then
+			exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
+				--continue
+		fi
+		# do we have anything to commit?
+		if git diff-index --cached --quiet HEAD --
+		then
+			# Nothing to commit -- skip this commit
+
+			test ! -f "$GIT_DIR"/CHERRY_PICK_HEAD ||
+			rm "$GIT_DIR"/CHERRY_PICK_HEAD ||
+			die "$(gettext "Could not remove CHERRY_PICK_HEAD")"
+		else
+			if ! test -f "$author_script"
+			then
+				gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")}
+				die "$(eval_gettext "\
+You have staged changes in your working tree.
+If these changes are meant to be
+squashed into the previous commit, run:
+
+  git commit --amend \$gpg_sign_opt_quoted
+
+If they are meant to go into a new commit, run:
+
+  git commit \$gpg_sign_opt_quoted
+
+In both cases, once you're done, continue with:
+
+  git rebase --continue
+")"
+			fi
+			. "$author_script" ||
+				die "$(gettext "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 "$(gettext "\
+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 \
+					${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
+					die "$(gettext "Could not commit staged changes.")"
+			else
+				do_with_author git commit --no-verify -F "$msg" -e \
+					${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
+					die "$(gettext "Could not commit staged changes.")"
+			fi
+		fi
+
+		if test -r "$state_dir"/stopped-sha
+		then
+			record_in_rewritten "$(cat "$state_dir"/stopped-sha)"
+		fi
+
+		require_clean_work_tree "rebase"
+		do_rest
+		return 0
+		;;
+	skip)
+		git rerere clear
+
+		if test ! -d "$rewritten"
+		then
+			exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
+				--continue
+		fi
+		do_rest
+		return 0
+		;;
+	edit-todo)
+		git stripspace --strip-comments <"$todo" >"$todo".new
+		mv -f "$todo".new "$todo"
+		collapse_todo_ids
+		append_todo_help
+		gettext "
+You are editing the todo file of an ongoing interactive rebase.
+To continue rebase after editing, run:
+    git rebase --continue
+
+" | git stripspace --comment-lines >>"$todo"
+
+		git_sequence_editor "$todo" ||
+			die "$(gettext "Could not execute editor")"
+		expand_todo_ids
+
+		exit
+		;;
+	show-current-patch)
+		exec git show REBASE_HEAD --
+		;;
+	*)
+		return 1 # continue
+		;;
+	esac
+}
+
+setup_reflog_action () {
+	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 "$(eval_gettext "Could not checkout \$switch_to")"
+
+		comment_for_reflog start
+	fi
+}
+
+init_basic_state () {
+	orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
+	mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
+	rm -f "$(git rev-parse --git-path REBASE_HEAD)"
+
+	: > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
+	write_basic_state
+}
+
+init_revisions_and_shortrevisions () {
+	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
+}
+
+complete_action() {
+	test -s "$todo" || echo noop >> "$todo"
+	test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
+	test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd"
+
+	todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
+	todocount=${todocount##* }
+
+cat >>"$todo" <<EOF
+
+$comment_char $(eval_ngettext \
+	"Rebase \$shortrevisions onto \$shortonto (\$todocount command)" \
+	"Rebase \$shortrevisions onto \$shortonto (\$todocount commands)" \
+	"$todocount")
+EOF
+	append_todo_help
+	gettext "
+	However, if you remove everything, the rebase will be aborted.
+
+	" | git stripspace --comment-lines >>"$todo"
+
+	if test -z "$keep_empty"
+	then
+		printf '%s\n' "$comment_char $(gettext "Note that empty commits are commented out")" >>"$todo"
+	fi
+
+
+	has_action "$todo" ||
+		return 2
+
+	cp "$todo" "$todo".backup
+	collapse_todo_ids
+	git_sequence_editor "$todo" ||
+		die_abort "$(gettext "Could not execute editor")"
+
+	has_action "$todo" ||
+		return 2
+
+	git rebase--helper --check-todo-list || {
+		ret=$?
+		checkout_onto
+		exit $ret
+	}
+
+	expand_todo_ids
+
+	test -d "$rewritten" || test -n "$force_rebase" ||
+	onto="$(git rebase--helper --skip-unnecessary-picks)" ||
+	die "Could not skip unnecessary pick commands"
+
+	checkout_onto
+	if test -z "$rebase_root" && test ! -d "$rewritten"
+	then
+		require_clean_work_tree "rebase"
+		exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
+			--continue
+	fi
+	do_rest
+}
+
 git_rebase__interactive () {
 	case "$action" in
 	continue)
-- 
2.16.2

[RFC PATCH v3 5/9] Use new functions in git_rebase__interactive

[Wink Saville]

Use initiate_action, setup_reflog_action, init_basic_state,
init_revisions_and_shortrevisions and complete_action.

Signed-off-by: Wink Saville <wink@saville.com>
---
 git-rebase--interactive.sh | 187 ++-------------------------------------------
 1 file changed, 8 insertions(+), 179 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index b72f80ae8..2c10a7f1a 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -952,120 +952,15 @@ EOF
 }
 
 git_rebase__interactive () {
-	case "$action" in
-	continue)
-		if test ! -d "$rewritten"
-		then
-			exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
-				--continue
-		fi
-		# do we have anything to commit?
-		if git diff-index --cached --quiet HEAD --
-		then
-			# Nothing to commit -- skip this commit
-
-			test ! -f "$GIT_DIR"/CHERRY_PICK_HEAD ||
-			rm "$GIT_DIR"/CHERRY_PICK_HEAD ||
-			die "$(gettext "Could not remove CHERRY_PICK_HEAD")"
-		else
-			if ! test -f "$author_script"
-			then
-				gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")}
-				die "$(eval_gettext "\
-You have staged changes in your working tree.
-If these changes are meant to be
-squashed into the previous commit, run:
-
-  git commit --amend \$gpg_sign_opt_quoted
-
-If they are meant to go into a new commit, run:
-
-  git commit \$gpg_sign_opt_quoted
-
-In both cases, once you're done, continue with:
-
-  git rebase --continue
-")"
-			fi
-			. "$author_script" ||
-				die "$(gettext "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 "$(gettext "\
-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 \
-					${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
-					die "$(gettext "Could not commit staged changes.")"
-			else
-				do_with_author git commit --no-verify -F "$msg" -e \
-					${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
-					die "$(gettext "Could not commit staged changes.")"
-			fi
-		fi
-
-		if test -r "$state_dir"/stopped-sha
-		then
-			record_in_rewritten "$(cat "$state_dir"/stopped-sha)"
-		fi
-
-		require_clean_work_tree "rebase"
-		do_rest
-		return 0
-		;;
-	skip)
-		git rerere clear
-
-		if test ! -d "$rewritten"
-		then
-			exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
-				--continue
-		fi
-		do_rest
+	initiate_action "$action"
+	ret=$?
+	if test $ret = 0; then
 		return 0
-		;;
-	edit-todo)
-		git stripspace --strip-comments <"$todo" >"$todo".new
-		mv -f "$todo".new "$todo"
-		collapse_todo_ids
-		append_todo_help
-		gettext "
-You are editing the todo file of an ongoing interactive rebase.
-To continue rebase after editing, run:
-    git rebase --continue
-
-" | git stripspace --comment-lines >>"$todo"
-
-		git_sequence_editor "$todo" ||
-			die "$(gettext "Could not execute editor")"
-		expand_todo_ids
-
-		exit
-		;;
-	show-current-patch)
-		exec git show REBASE_HEAD --
-		;;
-	esac
-
-	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 "$(eval_gettext "Could not checkout \$switch_to")"
-
-		comment_for_reflog start
 	fi
 
-	orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
-	mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
-	rm -f "$(git rev-parse --git-path REBASE_HEAD)"
+	setup_reflog_action
+	init_basic_state
 
-	: > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
-	write_basic_state
 	if test t = "$preserve_merges"
 	then
 		if test -z "$rebase_root"
@@ -1089,18 +984,8 @@ To continue rebase after editing, run:
 		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
+	init_revisions_and_shortrevisions
+
 	if test t != "$preserve_merges"
 	then
 		git rebase--helper --make-script ${keep_empty:+--keep-empty} \
@@ -1171,61 +1056,5 @@ To continue rebase after editing, run:
 		done
 	fi
 
-	test -s "$todo" || echo noop >> "$todo"
-	test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
-	test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd"
-
-	todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
-	todocount=${todocount##* }
-
-cat >>"$todo" <<EOF
-
-$comment_char $(eval_ngettext \
-	"Rebase \$shortrevisions onto \$shortonto (\$todocount command)" \
-	"Rebase \$shortrevisions onto \$shortonto (\$todocount commands)" \
-	"$todocount")
-EOF
-	append_todo_help
-	gettext "
-	However, if you remove everything, the rebase will be aborted.
-
-	" | git stripspace --comment-lines >>"$todo"
-
-	if test -z "$keep_empty"
-	then
-		printf '%s\n' "$comment_char $(gettext "Note that empty commits are commented out")" >>"$todo"
-	fi
-
-
-	has_action "$todo" ||
-		return 2
-
-	cp "$todo" "$todo".backup
-	collapse_todo_ids
-	git_sequence_editor "$todo" ||
-		die_abort "$(gettext "Could not execute editor")"
-
-	has_action "$todo" ||
-		return 2
-
-	git rebase--helper --check-todo-list || {
-		ret=$?
-		checkout_onto
-		exit $ret
-	}
-
-	expand_todo_ids
-
-	test -d "$rewritten" || test -n "$force_rebase" ||
-	onto="$(git rebase--helper --skip-unnecessary-picks)" ||
-	die "Could not skip unnecessary pick commands"
-
-	checkout_onto
-	if test -z "$rebase_root" && test ! -d "$rewritten"
-	then
-		require_clean_work_tree "rebase"
-		exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
-			--continue
-	fi
-	do_rest
+	complete_action
 }
-- 
2.16.2

[RFC PATCH v3 6/9] Add and use git_rebase__interactive__preserve_merges

[Wink Saville]

At the moment it's an exact copy of git_rebase__interactive except
the name has changed.

Signed-off-by: Wink Saville <wink@saville.com>
---
 git-rebase--interactive.sh | 108 +++++++++++++++++++++++++++++++++++++++++++++
 git-rebase.sh              |   7 ++-
 2 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2c10a7f1a..ab5513d80 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1058,3 +1058,111 @@ git_rebase__interactive () {
 
 	complete_action
 }
+
+git_rebase__interactive__preserve_merges () {
+	initiate_action "$action"
+	ret=$?
+	if test $ret = 0; then
+		return 0
+	fi
+
+	setup_reflog_action
+	init_basic_state
+
+	if test t = "$preserve_merges"
+	then
+		if test -z "$rebase_root"
+		then
+			mkdir "$rewritten" &&
+			for c in $(git merge-base --all $orig_head $upstream)
+			do
+				echo $onto > "$rewritten"/$c ||
+					die "$(gettext "Could not init rewritten commits")"
+			done
+		else
+			mkdir "$rewritten" &&
+			echo $onto > "$rewritten"/root ||
+				die "$(gettext "Could not init rewritten commits")"
+		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
+
+	init_revisions_and_shortrevisions
+
+	if test t != "$preserve_merges"
+	then
+		git rebase--helper --make-script ${keep_empty:+--keep-empty} \
+			$revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
+		die "$(gettext "Could not generate todo list")"
+	else
+		format=$(git config --get rebase.instructionFormat)
+		# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse
+		git rev-list $merges_option --format="%m%H ${format:-%s}" \
+			--reverse --left-right --topo-order \
+			$revisions ${restrict_revision+^$restrict_revision} | \
+			sed -n "s/^>//p" |
+		while read -r sha1 rest
+		do
+
+			if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1
+			then
+				comment_out="$comment_char "
+			else
+				comment_out=
+			fi
+
+			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 $sha1 $rest" >>"$todo"
+			fi
+		done
+	fi
+
+	# 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 &&
+			   ! sane_grep "$rev" "$state_dir"/not-cherry-picks >/dev/null
+			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
+				sha1=$(git rev-list -1 $rev)
+				sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
+				rm "$rewritten"/$rev
+			fi
+		done
+	fi
+
+	complete_action
+}
diff --git a/git-rebase.sh b/git-rebase.sh
index c4ec7c21b..b32282f4c 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -199,7 +199,12 @@ run_specific_rebase () {
 	if test "$type" = interactive
 	then
 		. git-rebase--interactive
-		git_rebase__interactive
+		if test "$preserve_merges" = t
+		then
+			git_rebase__interactive__preserve_merges
+		else
+			git_rebase__interactive
+		fi
 		ret=$?
 	else
 		. git-rebase--$type
-- 
2.16.2

[RFC PATCH v3 7/9] Remove unused code paths from git_rebase__interactive

[Wink Saville]

Since git_rebase__interactive is now never called with
$preserve_merges = t we can remove those code paths.

Signed-off-by: Wink Saville <wink@saville.com>
---
 git-rebase--interactive.sh | 95 ++--------------------------------------------
 1 file changed, 4 insertions(+), 91 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ab5513d80..346da0f67 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -961,100 +961,13 @@ git_rebase__interactive () {
 	setup_reflog_action
 	init_basic_state
 
-	if test t = "$preserve_merges"
-	then
-		if test -z "$rebase_root"
-		then
-			mkdir "$rewritten" &&
-			for c in $(git merge-base --all $orig_head $upstream)
-			do
-				echo $onto > "$rewritten"/$c ||
-					die "$(gettext "Could not init rewritten commits")"
-			done
-		else
-			mkdir "$rewritten" &&
-			echo $onto > "$rewritten"/root ||
-				die "$(gettext "Could not init rewritten commits")"
-		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
+	merges_option="--no-merges --cherry-pick"
 
 	init_revisions_and_shortrevisions
 
-	if test t != "$preserve_merges"
-	then
-		git rebase--helper --make-script ${keep_empty:+--keep-empty} \
-			$revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
-		die "$(gettext "Could not generate todo list")"
-	else
-		format=$(git config --get rebase.instructionFormat)
-		# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse
-		git rev-list $merges_option --format="%m%H ${format:-%s}" \
-			--reverse --left-right --topo-order \
-			$revisions ${restrict_revision+^$restrict_revision} | \
-			sed -n "s/^>//p" |
-		while read -r sha1 rest
-		do
-
-			if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1
-			then
-				comment_out="$comment_char "
-			else
-				comment_out=
-			fi
-
-			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 $sha1 $rest" >>"$todo"
-			fi
-		done
-	fi
-
-	# 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 &&
-			   ! sane_grep "$rev" "$state_dir"/not-cherry-picks >/dev/null
-			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
-				sha1=$(git rev-list -1 $rev)
-				sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
-				rm "$rewritten"/$rev
-			fi
-		done
-	fi
+	git rebase--helper --make-script ${keep_empty:+--keep-empty} \
+		$revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
+	die "$(gettext "Could not generate todo list")"
 
 	complete_action
 }
-- 
2.16.2

[RFC PATCH v3 8/9] Remove unused code paths from git_rebase__interactive__preserve_merges

[Wink Saville]

Since git_rebase__interactive__preserve_merges is now always called with
$preserve_merges = t we can remove the unused code paths.

Signed-off-by: Wink Saville <wink@saville.com>
---
 git-rebase--interactive.sh | 152 ++++++++++++++++++++-------------------------
 1 file changed, 69 insertions(+), 83 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 346da0f67..ddbd126f2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -982,100 +982,86 @@ git_rebase__interactive__preserve_merges () {
 	setup_reflog_action
 	init_basic_state
 
-	if test t = "$preserve_merges"
+	if test -z "$rebase_root"
 	then
-		if test -z "$rebase_root"
-		then
-			mkdir "$rewritten" &&
-			for c in $(git merge-base --all $orig_head $upstream)
-			do
-				echo $onto > "$rewritten"/$c ||
-					die "$(gettext "Could not init rewritten commits")"
-			done
-		else
-			mkdir "$rewritten" &&
-			echo $onto > "$rewritten"/root ||
+		mkdir "$rewritten" &&
+		for c in $(git merge-base --all $orig_head $upstream)
+		do
+			echo $onto > "$rewritten"/$c ||
 				die "$(gettext "Could not init rewritten commits")"
-		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=
+		done
 	else
-		merges_option="--no-merges --cherry-pick"
+		mkdir "$rewritten" &&
+		echo $onto > "$rewritten"/root ||
+			die "$(gettext "Could not init rewritten commits")"
 	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=
+
 	init_revisions_and_shortrevisions
 
-	if test t != "$preserve_merges"
-	then
-		git rebase--helper --make-script ${keep_empty:+--keep-empty} \
-			$revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
-		die "$(gettext "Could not generate todo list")"
-	else
-		format=$(git config --get rebase.instructionFormat)
-		# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse
-		git rev-list $merges_option --format="%m%H ${format:-%s}" \
-			--reverse --left-right --topo-order \
-			$revisions ${restrict_revision+^$restrict_revision} | \
-			sed -n "s/^>//p" |
-		while read -r sha1 rest
-		do
+	format=$(git config --get rebase.instructionFormat)
+	# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse
+	git rev-list $merges_option --format="%m%H ${format:-%s}" \
+		--reverse --left-right --topo-order \
+		$revisions ${restrict_revision+^$restrict_revision} | \
+		sed -n "s/^>//p" |
+	while read -r sha1 rest
+	do
 
-			if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1
-			then
-				comment_out="$comment_char "
-			else
-				comment_out=
-			fi
+		if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1
+		then
+			comment_out="$comment_char "
+		else
+			comment_out=
+		fi
 
-			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 $sha1 $rest" >>"$todo"
-			fi
-		done
-	fi
+		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 $sha1 $rest" >>"$todo"
+		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 &&
-			   ! sane_grep "$rev" "$state_dir"/not-cherry-picks >/dev/null
-			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
-				sha1=$(git rev-list -1 $rev)
-				sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
-				rm "$rewritten"/$rev
-			fi
-		done
-	fi
+	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 &&
+		   ! sane_grep "$rev" "$state_dir"/not-cherry-picks >/dev/null
+		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
+			sha1=$(git rev-list -1 $rev)
+			sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo"
+			rm "$rewritten"/$rev
+		fi
+	done
 
 	complete_action
 }
-- 
2.16.2

[RFC PATCH v3 9/9] Remove merges_option and a blank line

[Wink Saville]

merges_option is unused in git_rebase__interactive and always empty in
git_rebase__interactive__preserve_merges so it can be removed.

Signed-off-by: Wink Saville <wink@saville.com>
---
 git-rebase--interactive.sh | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ddbd126f2..50323fc27 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -961,8 +961,6 @@ git_rebase__interactive () {
 	setup_reflog_action
 	init_basic_state
 
-	merges_option="--no-merges --cherry-pick"
-
 	init_revisions_and_shortrevisions
 
 	git rebase--helper --make-script ${keep_empty:+--keep-empty} \
@@ -996,22 +994,16 @@ git_rebase__interactive__preserve_merges () {
 			die "$(gettext "Could not init rewritten commits")"
 	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=
-
 	init_revisions_and_shortrevisions
 
 	format=$(git config --get rebase.instructionFormat)
 	# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse
-	git rev-list $merges_option --format="%m%H ${format:-%s}" \
+	git rev-list --format="%m%H ${format:-%s}" \
 		--reverse --left-right --topo-order \
 		$revisions ${restrict_revision+^$restrict_revision} | \
 		sed -n "s/^>//p" |
 	while read -r sha1 rest
 	do
-
 		if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1
 		then
 			comment_out="$comment_char "
-- 
2.16.2

Re: [RFC PATCH v3 2/9] Call git_rebase__interactive from run_specific_rebase

[Junio C Hamano]

Wink Saville <wink@saville.com> writes:

> Instead of indirectly invoking git_rebase__interactive this invokes
> it directly after sourcing.
>
> Signed-off-by: Wink Saville <wink@saville.com>
> ---
>  git-rebase--interactive.sh | 11 -----------
>  git-rebase.sh              | 11 +++++++++--
>  2 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 561e2660e..213d75f43 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -740,15 +740,6 @@ get_missing_commit_check_level () {
>  	printf '%s' "$check_level" | tr 'A-Z' 'a-z'
>  }
>  
> -# 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, older (9.x) versions of FreeBSD /bin/sh misbehave on such a
> -# construct and continue 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.

We still enclose the whole thing (including the returns that are
problematic for older FreeBSD shells) in a shell function, so it's
not like we are dropping the workaround for these systems.  It's
just the caller of the function moved.

I think the removal of this large comment is justifiable, but the
structure still needs a bit of explanation, especially given that
the caller in git-rebase.sh needs to treat this scriptlet a bit
differently from others.

If we were not in the (longer term) process of getting rid of
git-rebase.sh, it might even make sense to port the same
"dot-sourced scriptlet defines a shell function to be called, and
the caller calls it after dot-sourcing it" pattern to other rebase
backends, so that the calling side can be unified again to become
something like:

	. git-rebase--$type
	git_rebase__$type
	ret=$?




>  git_rebase__interactive () {
>  
>  case "$action" in
> @@ -1029,5 +1020,3 @@ fi
>  do_rest
>  
>  }
> -# ... and then we call the whole thing.
> -git_rebase__interactive
> diff --git a/git-rebase.sh b/git-rebase.sh
> index a1f6e5de6..c4ec7c21b 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -196,8 +196,15 @@ run_specific_rebase () {
>  		export GIT_EDITOR
>  		autosquash=
>  	fi
> -	. git-rebase--$type
> -	ret=$?
> +	if test "$type" = interactive
> +	then
> +		. git-rebase--interactive
> +		git_rebase__interactive
> +		ret=$?
> +	else
> +		. git-rebase--$type
> +		ret=$?
> +	fi
>  	if test $ret -eq 0
>  	then
>  		finish_rebase

Re: [RFC PATCH v3 2/9] Call git_rebase__interactive from run_specific_rebase

[Wink Saville]

On Thu, Mar 22, 2018 at 11:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Wink Saville <wink@saville.com> writes:
>
>> Instead of indirectly invoking git_rebase__interactive this invokes
>> it directly after sourcing.
>>
>> Signed-off-by: Wink Saville <wink@saville.com>
>> ---
>>  git-rebase--interactive.sh | 11 -----------
>>  git-rebase.sh              | 11 +++++++++--
>>  2 files changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index 561e2660e..213d75f43 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -740,15 +740,6 @@ get_missing_commit_check_level () {
>>       printf '%s' "$check_level" | tr 'A-Z' 'a-z'
>>  }
>>
>> -# 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, older (9.x) versions of FreeBSD /bin/sh misbehave on such a
>> -# construct and continue 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.
>
> We still enclose the whole thing (including the returns that are
> problematic for older FreeBSD shells) in a shell function, so it's
> not like we are dropping the workaround for these systems.  It's
> just the caller of the function moved.
>
> I think the removal of this large comment is justifiable, but the
> structure still needs a bit of explanation, especially given that
> the caller in git-rebase.sh needs to treat this scriptlet a bit
> differently from others.
>
> If we were not in the (longer term) process of getting rid of
> git-rebase.sh, it might even make sense to port the same
> "dot-sourced scriptlet defines a shell function to be called, and
> the caller calls it after dot-sourcing it" pattern to other rebase
> backends, so that the calling side can be unified again to become
> something like:
>
>         . git-rebase--$type
>         git_rebase__$type
>         ret=$?

I've gone with the above suggestion and added back some
of the header.

[RFC PATCH v3.1 2/9 2/2] rebase-interactive: Do not automatically run code

[Wink Saville]

At Junio's suggestion have git-rebase--am and git-rebase--merge work the
same way as git-rebase--interactive. This makes the code more consistent.

Signed-off-by: Wink Saville <wink@saville.com>
---
 git-rebase--am.sh          | 17 ++++++-----------
 git-rebase--interactive.sh |  8 +++++++-
 git-rebase--merge.sh       | 17 ++++++-----------
 git-rebase.sh              | 13 ++++---------
 4 files changed, 23 insertions(+), 32 deletions(-)

diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index be3f06892..47dc69ed9 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -4,17 +4,14 @@
 # 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.
+# The whole contents of this file is loaded by dot-sourcing it from
+# inside another shell function, hence no shebang on the first line
+# and then the caller invokes git_rebase__am.
 #
-# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a
-# construct and continue 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.
+# Previously this file was sourced and it called itself to get this
+# was to get around a bug in older (9.x) versions of FreeBSD.
 git_rebase__am () {
-
+echo "git_rebase_am:+" 1>&5
 case "$action" in
 continue)
 	git am --resolved --resolvemsg="$resolvemsg" \
@@ -105,5 +102,3 @@ 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 213d75f43..48f358333 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -740,8 +740,14 @@ get_missing_commit_check_level () {
 	printf '%s' "$check_level" | tr 'A-Z' 'a-z'
 }
 
+# The whole contents of this file is loaded by dot-sourcing it from
+# inside another shell function, hence no shebang on the first line
+# and then the caller invokes git_rebase__interactive.
+#
+# Previously this file was sourced and it called itself to get this
+# was to get around a bug in older (9.x) versions of FreeBSD.
 git_rebase__interactive () {
-
+echo "git_rebase_interactive:+" 1>&5
 case "$action" in
 continue)
 	if test ! -d "$rewritten"
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index ceb715453..71de80788 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -104,17 +104,14 @@ 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.
+# The whole contents of this file is loaded by dot-sourcing it from
+# inside another shell function, hence no shebang on the first line
+# and then the caller invokes git_rebase__merge.
 #
-# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a
-# construct and continue 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.
+# Previously this file was sourced and it called itself to get this
+# was to get around a bug in older (9.x) versions of FreeBSD.
 git_rebase__merge () {
-
+echo "git_rebase_merge:+" 1>&5
 case "$action" in
 continue)
 	read_state
@@ -171,5 +168,3 @@ done
 finish_rb_merge
 
 }
-# ... and then we call the whole thing.
-git_rebase__merge
diff --git a/git-rebase.sh b/git-rebase.sh
index c4ec7c21b..4595a316a 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -196,15 +196,10 @@ run_specific_rebase () {
 		export GIT_EDITOR
 		autosquash=
 	fi
-	if test "$type" = interactive
-	then
-		. git-rebase--interactive
-		git_rebase__interactive
-		ret=$?
-	else
-		. git-rebase--$type
-		ret=$?
-	fi
+	# Source the code and invoke it
+	. git-rebase--$type
+	git_rebase__$type
+	ret=$?
 	if test $ret -eq 0
 	then
 		finish_rebase
-- 
2.16.2

Re: [RFC PATCH v3.1 2/9 2/2] rebase-interactive: Do not automatically run code

[Junio C Hamano]

Wink Saville <wink@saville.com> writes:

> At Junio's suggestion have git-rebase--am and git-rebase--merge work the
> same way as git-rebase--interactive. This makes the code more consistent.

I mumbled about making git_rebase__$type functions for all $type in
my previous response, but that was done without even looking at
git-rebase--$type.sh scriptlets.  It seems that they all shared the
same structure (i.e. define git_rebase__$type function and then at
the end clla it) and were consistent already.  It was the v3 that
changed the calling convention only for interactive, which made it
inconsistent.  If you are making git-rebase.sh call the helper shell
function for all backend $type, you are keeping the existing
consistency.

This is no longer about "interactive" alone, though, and need to be
retitled ;-)

> Signed-off-by: Wink Saville <wink@saville.com>
> ---
>  git-rebase--am.sh          | 17 ++++++-----------
>  git-rebase--interactive.sh |  8 +++++++-
>  git-rebase--merge.sh       | 17 ++++++-----------
>  git-rebase.sh              | 13 ++++---------
>  4 files changed, 23 insertions(+), 32 deletions(-)
>
> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
> index be3f06892..47dc69ed9 100644
> --- a/git-rebase--am.sh
> +++ b/git-rebase--am.sh
> @@ -4,17 +4,14 @@
>  # Copyright (c) 2010 Junio C Hamano.
>  #
>  
> +# The whole contents of this file is loaded by dot-sourcing it from
> +# inside another shell function, hence no shebang on the first line
> +# and then the caller invokes git_rebase__am.

Is this comment necessary?

> +# Previously this file was sourced and it called itself to get this
> +# was to get around a bug in older (9.x) versions of FreeBSD.

ECANTPARSE.  But this probably is no longer needed here, even though
it may make sense to explain why this comment is no longer relevant
in the log message.  E.g.

	The backend scriptlets for "git rebase" are structured in a
	bit unusual way for historical reasons.  Originally, it was
	designed in such a way that dot-sourcing them from "git
	rebase" would be sufficient to invoke the specific backend.
	When it was discovered that some shell implementations
	(e.g. FreeBSD 9.x) misbehaved by exiting when "return" is
	executed at the top level of a dot-sourced script (the
	original was expecting that the control returns to the next
	command in "git rebase" after dot-sourcing the scriptlet),
	the whole body of git-rebase--$backend.sh was made into a
	shell function git_rebase__$backend and then the scriptlet
        was made to call this function at the end as a workaround.

	Move the call to "git rebase" side, instead of at the end of
	each scriptlet.  This would give us a more normal
	arrangement where a function library lives in a scriptlet
	that is dot-sourced, and then these helper functions are
	called by the script that dot-sourced the scriptlet.

	While at it, remove the large comment that explains why this
	rather unusual structure was used from these scriptlets.

or something like that in the log message, and then we can get rid
of these in-code comments, I would think.

>  git_rebase__am () {
> -
> +echo "git_rebase_am:+" 1>&5

debuggin'?  I see similar stuff left in other parts (snipped) of
this patch.

Re: [RFC PATCH v3.1 2/9 2/2] rebase-interactive: Do not automatically run code

[Wink Saville]

On Thu, Mar 22, 2018 at 1:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Wink Saville <wink@saville.com> writes:
>
>> At Junio's suggestion have git-rebase--am and git-rebase--merge work the
>> same way as git-rebase--interactive. This makes the code more consistent.
>
> I mumbled about making git_rebase__$type functions for all $type in
> my previous response, but that was done without even looking at
> git-rebase--$type.sh scriptlets.  It seems that they all shared the
> same structure (i.e. define git_rebase__$type function and then at
> the end clla it) and were consistent already.  It was the v3 that
> changed the calling convention only for interactive, which made it
> inconsistent.  If you are making git-rebase.sh call the helper shell
> function for all backend $type, you are keeping the existing
> consistency.
>
> This is no longer about "interactive" alone, though, and need to be
> retitled ;-)
>
>> Signed-off-by: Wink Saville <wink@saville.com>
>> ---
>>  git-rebase--am.sh          | 17 ++++++-----------
>>  git-rebase--interactive.sh |  8 +++++++-
>>  git-rebase--merge.sh       | 17 ++++++-----------
>>  git-rebase.sh              | 13 ++++---------
>>  4 files changed, 23 insertions(+), 32 deletions(-)
>>
>> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
>> index be3f06892..47dc69ed9 100644
>> --- a/git-rebase--am.sh
>> +++ b/git-rebase--am.sh
>> @@ -4,17 +4,14 @@
>>  # Copyright (c) 2010 Junio C Hamano.
>>  #
>>
>> +# The whole contents of this file is loaded by dot-sourcing it from
>> +# inside another shell function, hence no shebang on the first line
>> +# and then the caller invokes git_rebase__am.
>
> Is this comment necessary?

Removed

>
>> +# Previously this file was sourced and it called itself to get this
>> +# was to get around a bug in older (9.x) versions of FreeBSD.
>
> ECANTPARSE.  But this probably is no longer needed here, even though
> it may make sense to explain why this comment is no longer relevant
> in the log message.  E.g.
>
>         The backend scriptlets for "git rebase" are structured in a
>         bit unusual way for historical reasons.  Originally, it was
>         designed in such a way that dot-sourcing them from "git
>         rebase" would be sufficient to invoke the specific backend.
>         When it was discovered that some shell implementations
>         (e.g. FreeBSD 9.x) misbehaved by exiting when "return" is
>         executed at the top level of a dot-sourced script (the
>         original was expecting that the control returns to the next
>         command in "git rebase" after dot-sourcing the scriptlet),
>         the whole body of git-rebase--$backend.sh was made into a
>         shell function git_rebase__$backend and then the scriptlet
>         was made to call this function at the end as a workaround.
>
>         Move the call to "git rebase" side, instead of at the end of
>         each scriptlet.  This would give us a more normal
>         arrangement where a function library lives in a scriptlet
>         that is dot-sourced, and then these helper functions are
>         called by the script that dot-sourced the scriptlet.
>
>         While at it, remove the large comment that explains why this
>         rather unusual structure was used from these scriptlets.
>
> or something like that in the log message, and then we can get rid
> of these in-code comments, I would think.

Updated commit message

>>  git_rebase__am () {
>> -
>> +echo "git_rebase_am:+" 1>&5
>
> debuggin'?  I see similar stuff left in other parts (snipped) of
> this patch.

Removed debugging :(

Currently I'm not rebasing the other commits (3..9)
to reduce the amount of work I have to do in each
review cycle, is that OK?

Also, will you merge commits 1 and 2 before the other
commits or is the procedure to merge the complete set
at once?

Re: [RFC PATCH v3.1 2/9 2/2] rebase-interactive: Do not automatically run code

[Junio C Hamano]

Wink Saville <wink@saville.com> writes:

> Currently I'm not rebasing the other commits (3..9)
> to reduce the amount of work I have to do in each
> review cycle, is that OK?

Yeah, I want to see others more heavily involved in this part of the
system to comment on your patches.  As to the organization of the
changes, I have some more opinions of my own, primarily regarding to
reviewability, but they are of secondary importance than reviews by
area experts.  I think it would be helpful to give them a target
that is not moving too rapidly ;-)

> Also, will you merge commits 1 and 2 before the other
> commits or is the procedure to merge the complete set
> at once?

I am inclined to take the early preliminary clean-up steps before
the remainder.

Re: [RFC PATCH v3 5/9] Use new functions in git_rebase__interactive

[Johannes Schindelin]

Hi Wink,

On Thu, 22 Mar 2018, Wink Saville wrote:

> Use initiate_action, setup_reflog_action, init_basic_state,
> init_revisions_and_shortrevisions and complete_action.
> 
> Signed-off-by: Wink Saville <wink@saville.com>
> ---
>  git-rebase--interactive.sh | 187 ++-------------------------------------------

If you fold this into the previous patch, I am sure that after your 3/9
indenting the function properly, the splitting into functions will look
more or less like this:

-git_rebase__interactive () {
+initiate_action () {
+	action="$1"
 
 	[... unchanged code ...]
+}
+
+<next function> () {
+	[... setting up variables ...]
 
 	[.. unchanged code ...]
+}
[... more of the same ...]
+
+git_rebase__interactive () {
+	initiate_action "$action" &&
+ 	<next function> <arguments> &&
+	...
+}

In other words, it would be easier to review and to verify that the
previous code is left unchanged (although that would have to be verified
manually by applying 3/9 and then looking at the diff with the -w option,
anyway).

Ciao,
Johannes

Re: [RFC PATCH v3 3/9] Indent function git_rebase__interactive

[Johannes Schindelin]

Hi Wink,

On Thu, 22 Mar 2018, Wink Saville wrote:

> Signed-off-by: Wink Saville <wink@saville.com>
> ---
>  git-rebase--interactive.sh | 432 ++++++++++++++++++++++-----------------------

It cannot be helped (at least for now) that this indent change produces
such a large diff.

I am fine with it, of course.

Thanks,
Johannes

Re: [RFC PATCH v3 5/9] Use new functions in git_rebase__interactive

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> If you fold this into the previous patch, I am sure that after your 3/9
> indenting the function properly, the splitting into functions will look
> more or less like this:
>
> -git_rebase__interactive () {
> +initiate_action () {
> +	action="$1"
>  
>  	[... unchanged code ...]
> +}
> +
> +<next function> () {
> +	[... setting up variables ...]
>  
>  	[.. unchanged code ...]
> +}
> [... more of the same ...]
> +
> +git_rebase__interactive () {
> +	initiate_action "$action" &&
> + 	<next function> <arguments> &&
> +	...
> +}
>
> In other words, it would be easier to review and to verify that the
> previous code is left unchanged (although that would have to be verified
> manually by applying 3/9 and then looking at the diff with the -w option,
> anyway).

We are on the same page on this.  A series structured to have a step
that looks exactly like the above would be very pleasant to review.

Thanks for a great suggestion.

Re: [RFC PATCH v3 5/9] Use new functions in git_rebase__interactive

[Wink Saville]

On Fri, Mar 23, 2018 at 11:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> If you fold this into the previous patch, I am sure that after your 3/9
>> indenting the function properly, the splitting into functions will look
>> more or less like this:
>>
>> -git_rebase__interactive () {
>> +initiate_action () {
>> +     action="$1"
>>
>>       [... unchanged code ...]
>> +}
>> +
>> +<next function> () {
>> +     [... setting up variables ...]
>>
>>       [.. unchanged code ...]
>> +}
>> [... more of the same ...]
>> +
>> +git_rebase__interactive () {
>> +     initiate_action "$action" &&
>> +     <next function> <arguments> &&
>> +     ...
>> +}
>>
>> In other words, it would be easier to review and to verify that the
>> previous code is left unchanged (although that would have to be verified
>> manually by applying 3/9 and then looking at the diff with the -w option,
>> anyway).
>
> We are on the same page on this.  A series structured to have a step
> that looks exactly like the above would be very pleasant to review.
>
> Thanks for a great suggestion.

SG and I'll rebase on top of my v5 for the first two commits which
I rebased on master.