[RFC PATCH 0/3] rebase-interactive

[RFC PATCH 0/3] rebase-interactive

[Wink Saville]

I've not worked on the git sources before and while looking into
fixing test_expect_failure 'exchange two commits with -p' in
t3404-rebase-interactive.sh, I found it difficult to understand
the git testing infracture and git-rebase--interactive.sh.

So as part of learning my way around I thought I'd refactor
git-rebase--interactive to make it easier for me to understand.
At this point I do have some understanding and will be working
on fixing the bug. In the mean time I'm requesting comments on
this refactoring patch sequence.

Patch 0001 creates a library of functions which can be
used by git-rebase--interactive and
git-rebase--interactive--preserve-merges. The functions are
those that exist in git-rebase--interactive.sh plus new
functions created from the body of git_rebase_interactive
that will be used git_rebase_interactive in the third patch
and git_rebase_interactive_preserve_merges in the second
patch. None of the functions are invoked so there is no
logic changes and the system builds and passes all tests
on travis-ci.org.

Patch 0002 creates git-rebase--interactive--preserve-merges.sh
with the function git_rebase_interactive_preserve_merges. The contents
of the function are refactored from git_rebase_interactive and
uses existing and new functions in the library. A small modification
of git-rebase is also done to invoke the new function when the -p
switch is used with git-rebase. When this is applied on top of
0001 the system builds and passes all tests on travis-ci.org.

The final patch, 0003, removes all unused code from
git_rebase_interactive and uses the functions from the library
where appropriate. And, of course, when applied on top of 0002
the system builds and passes all tests on travis-ci.org.

Wink Saville (3):
  rebase-interactive: create git-rebase--interactive--lib.sh
  rebase-interactive: create git-rebase--interactive--preserve-merges
  rebase-interactive: refactor git-rebase--interactive to use library

 .gitignore                                  |    2 +
 Makefile                                    |    2 +
 git-rebase--interactive--lib.sh             |  944 +++++++++++++++++++++++++
 git-rebase--interactive--preserve-merges.sh |  134 ++++
 git-rebase--interactive.sh                  | 1019 +--------------------------
 git-rebase.sh                               |    7 +-
 6 files changed, 1107 insertions(+), 1001 deletions(-)
 create mode 100644 git-rebase--interactive--lib.sh
 create mode 100644 git-rebase--interactive--preserve-merges.sh

-- 
2.16.2

[RFC PATCH 1/3] rebase-interactive: create git-rebase--interactive--lib.sh

[Wink Saville]

Create a library that can be used for interactive rebasing by
separate scripts. In particular, the current git-rebase--interactive.sh
will be reduced to a single function which uses the library and a new
file, git-rebase--interactive--preserve-merges.sh will also be a single
function that uses the library.

Signed-off-by: Wink Saville <wink@saville.com>
---
 .gitignore                      |   1 +
 Makefile                        |   1 +
 git-rebase--interactive--lib.sh | 944 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 946 insertions(+)
 create mode 100644 git-rebase--interactive--lib.sh

diff --git a/.gitignore b/.gitignore
index 833ef3b0b..4ea246780 100644
--- a/.gitignore
+++ b/.gitignore
@@ -116,6 +116,7 @@
 /git-rebase--am
 /git-rebase--helper
 /git-rebase--interactive
+/git-rebase--interactive--lib
 /git-rebase--merge
 /git-receive-pack
 /git-reflog
diff --git a/Makefile b/Makefile
index de4b8f0c0..f13540da6 100644
--- a/Makefile
+++ b/Makefile
@@ -567,6 +567,7 @@ SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
 SCRIPT_LIB += git-rebase--interactive
+SCRIPT_LIB += git-rebase--interactive--lib
 SCRIPT_LIB += git-rebase--merge
 SCRIPT_LIB += git-sh-setup
 SCRIPT_LIB += git-sh-i18n
diff --git a/git-rebase--interactive--lib.sh b/git-rebase--interactive--lib.sh
new file mode 100644
index 000000000..0cb902f98
--- /dev/null
+++ b/git-rebase--interactive--lib.sh
@@ -0,0 +1,944 @@
+#!/bin/sh
+# This shell script fragment is sourced by git-rebase--interactive
+# and git-rebase--interactive--preserve-merges in suppor of the
+# interactive mode.  "git rebase --interactive" makes it easy
+# to fix up commits in the middle of a series and rearrange commits.
+#
+# Copyright (c) 2006 Johannes E. Schindelin
+#
+# The original idea comes from Eric W. Biederman, in
+# https://public-inbox.org/git/m1odwkyuf5.fsf_-_@ebiederm.dsl.xmission.com/
+#
+
+# The file containing rebase commands, comments, and empty lines.
+# This file is created by "git rebase -i" then edited by the user.  As
+# the lines are processed, they are removed from the front of this
+# file and written to the tail of $done.
+todo="$state_dir"/git-rebase-todo
+
+# The rebase command lines that have already been processed.  A line
+# is moved here when it is first handled, before any associated user
+# actions.
+done="$state_dir"/done
+
+# The commit message that is planned to be used for any changes that
+# need to be committed following a user interaction.
+msg="$state_dir"/message
+
+# The file into which is accumulated the suggested commit message for
+# squash/fixup commands.  When the first of a series of squash/fixups
+# is seen, the file is created and the commit message from the
+# previous commit and from the first squash/fixup commit are written
+# to it.  The commit message for each subsequent squash/fixup commit
+# is appended to the file as it is processed.
+#
+# The first line of the file is of the form
+#     # This is a combination of $count commits.
+# where $count is the number of commits whose messages have been
+# written to the file so far (including the initial "pick" commit).
+# Each time that a commit message is processed, this line is read and
+# updated.  It is deleted just before the combined commit is made.
+squash_msg="$state_dir"/message-squash
+
+# If the current series of squash/fixups has not yet included a squash
+# command, then this file exists and holds the commit message of the
+# original "pick" commit.  (If the series ends without a "squash"
+# command, then this can be used as the commit message of the combined
+# commit without opening the editor.)
+fixup_msg="$state_dir"/message-fixup
+
+# $rewritten is the name of a directory containing files for each
+# commit that is reachable by at least one merge base of $head and
+# $upstream. They are not necessarily rewritten, but their children
+# might be.  This ensures that commits on merged, but otherwise
+# unrelated side branches are left alone. (Think "X" in the man page's
+# example.)
+rewritten="$state_dir"/rewritten
+
+dropped="$state_dir"/dropped
+
+end="$state_dir"/end
+msgnum="$state_dir"/msgnum
+
+# A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
+# GIT_AUTHOR_DATE that will be used for the commit that is currently
+# being rebased.
+author_script="$state_dir"/author-script
+
+# When an "edit" rebase command is being processed, the SHA1 of the
+# commit to be edited is recorded in this file.  When "git rebase
+# --continue" is executed, if there are any staged changes then they
+# will be amended to the HEAD commit, but only provided the HEAD
+# commit is still the commit to be edited.  When any other rebase
+# command is processed, this file is deleted.
+amend="$state_dir"/amend
+
+# For the post-rewrite hook, we make a list of rewritten commits and
+# their new sha1s.  The rewritten-pending list keeps the sha1s of
+# commits that have been processed, but not committed yet,
+# e.g. because they are waiting for a 'squash' command.
+rewritten_list="$state_dir"/rewritten-list
+rewritten_pending="$state_dir"/rewritten-pending
+
+# Work around Git for Windows' Bash whose "read" does not strip CRLF
+# and leaves CR at the end instead.
+cr=$(printf "\015")
+
+strategy_args=${strategy:+--strategy=$strategy}
+test -n "$strategy_opts" &&
+eval '
+	for strategy_opt in '"$strategy_opts"'
+	do
+		strategy_args="$strategy_args -X$(git rev-parse --sq-quote "${strategy_opt#--}")"
+	done
+'
+
+GIT_CHERRY_PICK_HELP="$resolvemsg"
+export GIT_CHERRY_PICK_HELP
+
+comment_char=$(git config --get core.commentchar 2>/dev/null)
+case "$comment_char" in
+'' | auto)
+	comment_char="#"
+	;;
+?)
+	;;
+*)
+	comment_char=$(echo "$comment_char" | cut -c1)
+	;;
+esac
+
+warn () {
+	printf '%s\n' "$*" >&2
+}
+
+# Output the commit message for the specified commit.
+commit_message () {
+	git cat-file commit "$1" | sed "1,/^$/d"
+}
+
+orig_reflog_action="$GIT_REFLOG_ACTION"
+
+comment_for_reflog () {
+	case "$orig_reflog_action" in
+	''|rebase*)
+		GIT_REFLOG_ACTION="rebase -i ($1)"
+		export GIT_REFLOG_ACTION
+		;;
+	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
+}
+
+last_count=
+mark_action_done () {
+	sed -e 1q < "$todo" >> "$done"
+	sed -e 1d < "$todo" >> "$todo".new
+	mv -f "$todo".new "$todo"
+	new_count=$(( $(git stripspace --strip-comments <"$done" | wc -l) ))
+	echo $new_count >"$msgnum"
+	total=$(($new_count + $(git stripspace --strip-comments <"$todo" | wc -l)))
+	echo $total >"$end"
+	if test "$last_count" != "$new_count"
+	then
+		last_count=$new_count
+		eval_gettext "Rebasing (\$new_count/\$total)"; printf "\r"
+		test -z "$verbose" || echo
+	fi
+}
+
+# Put the last action marked done at the beginning of the todo list
+# again. If there has not been an action marked done yet, leave the list of
+# items on the todo list unchanged.
+reschedule_last_action () {
+	tail -n 1 "$done" | cat - "$todo" >"$todo".new
+	sed -e \$d <"$done" >"$done".new
+	mv -f "$todo".new "$todo"
+	mv -f "$done".new "$done"
+}
+
+append_todo_help () {
+	gettext "
+Commands:
+p, pick = use commit
+r, reword = use commit, but edit the commit message
+e, edit = use commit, but stop for amending
+s, squash = use commit, but meld into previous commit
+f, fixup = like \"squash\", but discard this commit's log message
+x, exec = run command (the rest of the line) using shell
+d, drop = remove commit
+
+These lines can be re-ordered; they are executed from top to bottom.
+" | git stripspace --comment-lines >>"$todo"
+
+	if test $(get_missing_commit_check_level) = error
+	then
+		gettext "
+Do not remove any line. Use 'drop' explicitly to remove a commit.
+" | git stripspace --comment-lines >>"$todo"
+	else
+		gettext "
+If you remove a line here THAT COMMIT WILL BE LOST.
+" | git stripspace --comment-lines >>"$todo"
+	fi
+}
+
+make_patch () {
+	sha1_and_parents="$(git rev-list --parents -1 "$1")"
+	case "$sha1_and_parents" in
+	?*' '?*' '?*)
+		git diff --cc $sha1_and_parents
+		;;
+	?*' '?*)
+		git diff-tree -p "$1^!"
+		;;
+	*)
+		echo "Root commit"
+		;;
+	esac > "$state_dir"/patch
+	test -f "$msg" ||
+		commit_message "$1" > "$msg"
+	test -f "$author_script" ||
+		get_author_ident_from_commit "$1" > "$author_script"
+}
+
+die_with_patch () {
+	echo "$1" > "$state_dir"/stopped-sha
+	git update-ref REBASE_HEAD "$1"
+	make_patch "$1"
+	die "$2"
+}
+
+exit_with_patch () {
+	echo "$1" > "$state_dir"/stopped-sha
+	git update-ref REBASE_HEAD "$1"
+	make_patch $1
+	git rev-parse --verify HEAD > "$amend"
+	gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")}
+	warn "$(eval_gettext "\
+You can amend the commit now, with
+
+	git commit --amend \$gpg_sign_opt_quoted
+
+Once you are satisfied with your changes, run
+
+	git rebase --continue")"
+	warn
+	exit $2
+}
+
+die_abort () {
+	apply_autostash
+	rm -rf "$state_dir"
+	die "$1"
+}
+
+has_action () {
+	test -n "$(git stripspace --strip-comments <"$1")"
+}
+
+is_empty_commit() {
+	tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null) || {
+		sha1=$1
+		die "$(eval_gettext "\$sha1: not a commit that can be picked")"
+	}
+	ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null) ||
+		ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+	test "$tree" = "$ptree"
+}
+
+is_merge_commit()
+{
+	git rev-parse --verify --quiet "$1"^2 >/dev/null 2>&1
+}
+
+# Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
+# GIT_AUTHOR_DATE exported from the current environment.
+do_with_author () {
+	(
+		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE
+		"$@"
+	)
+}
+
+git_sequence_editor () {
+	if test -z "$GIT_SEQUENCE_EDITOR"
+	then
+		GIT_SEQUENCE_EDITOR="$(git config sequence.editor)"
+		if [ -z "$GIT_SEQUENCE_EDITOR" ]
+		then
+			GIT_SEQUENCE_EDITOR="$(git var GIT_EDITOR)" || return $?
+		fi
+	fi
+
+	eval "$GIT_SEQUENCE_EDITOR" '"$@"'
+}
+
+pick_one () {
+	ff=--ff
+
+	case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
+	case "$force_rebase" in '') ;; ?*) ff= ;; esac
+	output git rev-parse --verify $sha1 ||
+		die "$(eval_gettext "Invalid commit name: \$sha1")"
+
+	if is_empty_commit "$sha1"
+	then
+		empty_args="--allow-empty"
+	fi
+
+	test -d "$rewritten" &&
+		pick_one_preserving_merges "$@" && return
+	output eval git cherry-pick $allow_rerere_autoupdate $allow_empty_message \
+			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
+			"$strategy_args" $empty_args $ff "$@"
+
+	# If cherry-pick dies it leaves the to-be-picked commit unrecorded.
+	# Reschedule previous task so this commit is not lost.
+	ret=$?
+	case "$ret" in [01]) ;; *) reschedule_last_action ;; esac
+	return $ret
+}
+
+pick_one_preserving_merges () {
+	fast_forward=t
+	case "$1" in
+	-n)
+		fast_forward=f
+		sha1=$2
+		;;
+	*)
+		sha1=$1
+		;;
+	esac
+	sha1=$(git rev-parse $sha1)
+
+	if test -f "$state_dir"/current-commit && 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
+
+	echo $sha1 >> "$state_dir"/current-commit
+
+	# rewrite parents; if none were rewritten, we can fast-forward.
+	new_parents=
+	pend=" $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)"
+	if test "$pend" = " "
+	then
+		pend=" root"
+	fi
+	while [ "$pend" != "" ]
+	do
+		p=$(expr "$pend" : ' \([^ ]*\)')
+		pend="${pend# $p}"
+
+		if test -f "$rewritten"/$p
+		then
+			new_p=$(cat "$rewritten"/$p)
+			# If the todo reordered commits, and our parent is
+			# marked for rewriting, but hasn't been gotten to yet,
+			# assume the user meant to drop it on top of the
+			# current HEAD
+			if test -z "$new_p"
+			then
+				new_p=$(git rev-parse HEAD)
+			fi
+
+			test $p != $new_p && fast_forward=f
+			case "$new_parents" in
+			*$new_p*)
+				;; # do nothing; that parent is already there
+			*)
+				new_parents="$new_parents $new_p"
+				;;
+			esac
+		else
+			if test -f "$dropped"/$p
+			then
+				fast_forward=f
+				replacement="$(cat "$dropped"/$p)"
+				test -z "$replacement" && replacement=root
+				pend=" $replacement$pend"
+			else
+				new_parents="$new_parents $p"
+			fi
+		fi
+	done
+	case $fast_forward in
+	t)
+		output warn "$(eval_gettext "Fast-forward to \$sha1")"
+		output git reset --hard $sha1 ||
+			die "$(eval_gettext "Cannot fast-forward to \$sha1")"
+		;;
+	f)
+		first_parent=$(expr "$new_parents" : ' \([^ ]*\)')
+
+		if [ "$1" != "-n" ]
+		then
+			# detach HEAD to current parent
+			output git checkout $first_parent 2> /dev/null ||
+			   die "$(eval_gettext "Cannot move HEAD to \$first_parent")"
+		fi
+
+		case "$new_parents" in
+		' '*' '*)
+			test "a$1" = a-n && die "$(eval_gettext "Refusing to squash a merge: \$sha1")"
+
+			# redo merge
+			author_script_content=$(get_author_ident_from_commit $sha1)
+			eval "$author_script_content"
+			msg_content="$(commit_message $sha1)"
+			# No point in merging the first parent, that's HEAD
+			new_parents=${new_parents# $first_parent}
+			merge_args="--no-log --no-ff"
+			if ! do_with_author output eval \
+				git merge ${gpg_sign_opt:+$(git rev-parse \
+					--sq-quote "$gpg_sign_opt")} \
+				$allow_rerere_autoupdate "$merge_args" \
+				"$strategy_args" \
+				-m "$(git rev-parse --sq-quote "$msg_content")" \
+				"$new_parents"
+			then
+				printf "%s\n" "$msg_content" > "$GIT_DIR"/MERGE_MSG
+				die_with_patch $sha1 "$(eval_gettext "Error redoing merge \$sha1")"
+			fi
+			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
+			;;
+		*)
+			output eval git cherry-pick $allow_rerere_autoupdate \
+				$allow_empty_message \
+				${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
+				"$strategy_args" "$@" ||
+				die_with_patch $sha1 "$(eval_gettext "Could not pick \$sha1")"
+			;;
+		esac
+		;;
+	esac
+}
+
+this_nth_commit_message () {
+	n=$1
+	eval_gettext "This is the commit message #\${n}:"
+}
+
+skip_nth_commit_message () {
+	n=$1
+	eval_gettext "The commit message #\${n} will be skipped:"
+}
+
+update_squash_messages () {
+	if test -f "$squash_msg"; then
+		mv "$squash_msg" "$squash_msg".bak || exit
+		count=$(($(sed -n \
+			-e "1s/^$comment_char[^0-9]*\([0-9][0-9]*\).*/\1/p" \
+			-e "q" < "$squash_msg".bak)+1))
+		{
+			printf '%s\n' "$comment_char $(eval_ngettext \
+				"This is a combination of \$count commit." \
+				"This is a combination of \$count commits." \
+				$count)"
+			sed -e 1d -e '2,/^./{
+				/^$/d
+			}' <"$squash_msg".bak
+		} >"$squash_msg"
+	else
+		commit_message HEAD >"$fixup_msg" ||
+		die "$(eval_gettext "Cannot write \$fixup_msg")"
+		count=2
+		{
+			printf '%s\n' "$comment_char $(gettext "This is a combination of 2 commits.")"
+			printf '%s\n' "$comment_char $(gettext "This is the 1st commit message:")"
+			echo
+			cat "$fixup_msg"
+		} >"$squash_msg"
+	fi
+	case $1 in
+	squash)
+		rm -f "$fixup_msg"
+		echo
+		printf '%s\n' "$comment_char $(this_nth_commit_message $count)"
+		echo
+		commit_message $2
+		;;
+	fixup)
+		echo
+		printf '%s\n' "$comment_char $(skip_nth_commit_message $count)"
+		echo
+		# Change the space after the comment character to TAB:
+		commit_message $2 | git stripspace --comment-lines | sed -e 's/ /	/'
+		;;
+	esac >>"$squash_msg"
+}
+
+peek_next_command () {
+	git stripspace --strip-comments <"$todo" | sed -n -e 's/ .*//p' -e q
+}
+
+# A squash/fixup has failed.  Prepare the long version of the squash
+# commit message, then die_with_patch.  This code path requires the
+# user to edit the combined commit message for all commits that have
+# been squashed/fixedup so far.  So also erase the old squash
+# messages, effectively causing the combined commit to be used as the
+# new basis for any further squash/fixups.  Args: sha1 rest
+die_failed_squash() {
+	sha1=$1
+	rest=$2
+	mv "$squash_msg" "$msg" || exit
+	rm -f "$fixup_msg"
+	cp "$msg" "$GIT_DIR"/MERGE_MSG || exit
+	warn
+	warn "$(eval_gettext "Could not apply \$sha1... \$rest")"
+	die_with_patch $sha1 ""
+}
+
+flush_rewritten_pending() {
+	test -s "$rewritten_pending" || return
+	newsha1="$(git rev-parse HEAD^0)"
+	sed "s/$/ $newsha1/" < "$rewritten_pending" >> "$rewritten_list"
+	rm -f "$rewritten_pending"
+}
+
+record_in_rewritten() {
+	oldsha1="$(git rev-parse $1)"
+	echo "$oldsha1" >> "$rewritten_pending"
+
+	case "$(peek_next_command)" in
+	squash|s|fixup|f)
+		;;
+	*)
+		flush_rewritten_pending
+		;;
+	esac
+}
+
+do_pick () {
+	sha1=$1
+	rest=$2
+	if test "$(git rev-parse HEAD)" = "$squash_onto"
+	then
+		# Set the correct commit message and author info on the
+		# sentinel root before cherry-picking the original changes
+		# without committing (-n).  Finally, update the sentinel again
+		# to include these changes.  If the cherry-pick results in a
+		# conflict, this means our behaviour is similar to a standard
+		# failed cherry-pick during rebase, with a dirty index to
+		# resolve before manually running git commit --amend then git
+		# rebase --continue.
+		git commit --allow-empty --allow-empty-message --amend \
+			   --no-post-rewrite -n -q -C $sha1 &&
+			pick_one -n $sha1 &&
+			git commit --allow-empty --allow-empty-message \
+				   --amend --no-post-rewrite -n -q -C $sha1 \
+				   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
+				   die_with_patch $sha1 "$(eval_gettext "Could not apply \$sha1... \$rest")"
+	else
+		pick_one $sha1 ||
+			die_with_patch $sha1 "$(eval_gettext "Could not apply \$sha1... \$rest")"
+	fi
+}
+
+do_next () {
+	rm -f "$msg" "$author_script" "$amend" "$state_dir"/stopped-sha || exit
+	read -r command sha1 rest < "$todo"
+	case "$command" in
+	"$comment_char"*|''|noop|drop|d)
+		mark_action_done
+		;;
+	"$cr")
+		# Work around CR left by "read" (e.g. with Git for Windows' Bash).
+		mark_action_done
+		;;
+	pick|p)
+		comment_for_reflog pick
+
+		mark_action_done $sha1 "$rest"
+		do_pick $sha1 "$rest"
+		record_in_rewritten $sha1
+		;;
+	reword|r)
+		comment_for_reflog reword
+
+		mark_action_done
+		do_pick $sha1 "$rest"
+		git commit --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} \
+			$allow_empty_message || {
+			warn "$(eval_gettext "\
+Could not amend commit after successfully picking \$sha1... \$rest
+This is most likely due to an empty commit message, or the pre-commit hook
+failed. If the pre-commit hook failed, you may need to resolve the issue before
+you are able to reword the commit.")"
+			exit_with_patch $sha1 1
+		}
+		record_in_rewritten $sha1
+		;;
+	edit|e)
+		comment_for_reflog edit
+
+		mark_action_done
+		do_pick $sha1 "$rest"
+		sha1_abbrev=$(git rev-parse --short $sha1)
+		warn "$(eval_gettext "Stopped at \$sha1_abbrev... \$rest")"
+		exit_with_patch $sha1 0
+		;;
+	squash|s|fixup|f)
+		case "$command" in
+		squash|s)
+			squash_style=squash
+			;;
+		fixup|f)
+			squash_style=fixup
+			;;
+		esac
+		comment_for_reflog $squash_style
+
+		test -f "$done" && has_action "$done" ||
+			die "$(eval_gettext "Cannot '\$squash_style' without a previous commit")"
+
+		mark_action_done
+		update_squash_messages $squash_style $sha1
+		author_script_content=$(get_author_ident_from_commit HEAD)
+		echo "$author_script_content" > "$author_script"
+		eval "$author_script_content"
+		if ! pick_one -n $sha1
+		then
+			git rev-parse --verify HEAD >"$amend"
+			die_failed_squash $sha1 "$rest"
+		fi
+		case "$(peek_next_command)" in
+		squash|s|fixup|f)
+			# This is an intermediate commit; its message will only be
+			# used in case of trouble.  So use the long version:
+			do_with_author output git commit --amend --no-verify -F "$squash_msg" \
+				${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
+				die_failed_squash $sha1 "$rest"
+			;;
+		*)
+			# This is the final command of this squash/fixup group
+			if test -f "$fixup_msg"
+			then
+				do_with_author git commit --amend --no-verify -F "$fixup_msg" \
+					${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
+					die_failed_squash $sha1 "$rest"
+			else
+				cp "$squash_msg" "$GIT_DIR"/SQUASH_MSG || exit
+				rm -f "$GIT_DIR"/MERGE_MSG
+				do_with_author git commit --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e \
+					${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
+					die_failed_squash $sha1 "$rest"
+			fi
+			rm -f "$squash_msg" "$fixup_msg"
+			;;
+		esac
+		record_in_rewritten $sha1
+		;;
+	x|"exec")
+		read -r command rest < "$todo"
+		mark_action_done
+		eval_gettextln "Executing: \$rest"
+		"${SHELL:-/bin/sh}" -c "$rest" # Actual execution
+		status=$?
+		# Run in subshell because require_clean_work_tree can die.
+		dirty=f
+		(require_clean_work_tree "rebase" 2>/dev/null) || dirty=t
+		if test "$status" -ne 0
+		then
+			warn "$(eval_gettext "Execution failed: \$rest")"
+			test "$dirty" = f ||
+				warn "$(gettext "and made changes to the index and/or the working tree")"
+
+			warn "$(gettext "\
+You can fix the problem, and then run
+
+	git rebase --continue")"
+			warn
+			if test $status -eq 127		# command not found
+			then
+				status=1
+			fi
+			exit "$status"
+		elif test "$dirty" = t
+		then
+			# TRANSLATORS: after these lines is a command to be issued by the user
+			warn "$(eval_gettext "\
+Execution succeeded: \$rest
+but left changes to the index and/or the working tree
+Commit or stash your changes, and then run
+
+	git rebase --continue")"
+			warn
+			exit 1
+		fi
+		;;
+	*)
+		warn "$(eval_gettext "Unknown command: \$command \$sha1 \$rest")"
+		fixtodo="$(gettext "Please fix this using 'git rebase --edit-todo'.")"
+		if git rev-parse --verify -q "$sha1" >/dev/null
+		then
+			die_with_patch $sha1 "$fixtodo"
+		else
+			die "$fixtodo"
+		fi
+		;;
+	esac
+	test -s "$todo" && return
+
+	comment_for_reflog finish &&
+	newhead=$(git rev-parse HEAD) &&
+	case $head_name in
+	refs/*)
+		message="$GIT_REFLOG_ACTION: $head_name onto $onto" &&
+		git update-ref -m "$message" $head_name $newhead $orig_head &&
+		git symbolic-ref \
+		  -m "$GIT_REFLOG_ACTION: returning to $head_name" \
+		  HEAD $head_name
+		;;
+	esac && {
+		test ! -f "$state_dir"/verbose ||
+			git diff-tree --stat $orig_head..HEAD
+	} &&
+	{
+		test -s "$rewritten_list" &&
+		git notes copy --for-rewrite=rebase < "$rewritten_list" ||
+		true # we don't care if this copying failed
+	} &&
+	hook="$(git rev-parse --git-path hooks/post-rewrite)"
+	if test -x "$hook" && test -s "$rewritten_list"; then
+		"$hook" rebase < "$rewritten_list"
+		true # we don't care if this hook failed
+	fi &&
+		warn "$(eval_gettext "Successfully rebased and updated \$head_name.")"
+
+	return 1 # not failure; just to break the do_rest loop
+}
+
+# can only return 0, when the infinite loop breaks
+do_rest () {
+	while :
+	do
+		do_next || break
+	done
+}
+
+expand_todo_ids() {
+	git rebase--helper --expand-ids
+}
+
+collapse_todo_ids() {
+	git rebase--helper --shorten-ids
+}
+
+# Switch to the branch in $into and notify it in the reflog
+checkout_onto () {
+	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
+	output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
+	git update-ref ORIG_HEAD $orig_head
+}
+
+get_missing_commit_check_level () {
+	check_level=$(git config --get rebase.missingCommitsCheck)
+	check_level=${check_level:-ignore}
+	# Don't be case sensitive
+	printf '%s' "$check_level" | tr 'A-Z' 'a-z'
+}
+
+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
+}
+
+# Initiate an action which is first parameter.
+# Returns 0 if the action was able to complete
+# or 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 # done
+		;;
+	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 # done
+		;;
+	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
+}
+
+# Complete the action
+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
+	#dbg_pause "invoke git_sequence_editor"
+	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
+	else
+		do_rest
+	fi
+}
-- 
2.16.2

[RFC PATCH 2/3] rebase-interactive: create git-rebase--interactive--preserve-merges

[Wink Saville]

Extract the code that is executed when $preserve_merges is t from
git-rebase--interactive to git-rebase--interactive--preserve-merges.sh.
The extracted code uses functions from git-rebase--interactve--lib.

Signed-off-by: Wink Saville <wink@saville.com>
---
 .gitignore                                  |   1 +
 Makefile                                    |   1 +
 git-rebase--interactive--preserve-merges.sh | 134 ++++++++++++++++++++++++++++
 git-rebase.sh                               |   7 +-
 4 files changed, 142 insertions(+), 1 deletion(-)
 create mode 100644 git-rebase--interactive--preserve-merges.sh

diff --git a/.gitignore b/.gitignore
index 4ea246780..c57a6b563 100644
--- a/.gitignore
+++ b/.gitignore
@@ -116,6 +116,7 @@
 /git-rebase--am
 /git-rebase--helper
 /git-rebase--interactive
+/git-rebase--interactive--preserve-merges
 /git-rebase--interactive--lib
 /git-rebase--merge
 /git-receive-pack
diff --git a/Makefile b/Makefile
index f13540da6..543e0a659 100644
--- a/Makefile
+++ b/Makefile
@@ -567,6 +567,7 @@ SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
 SCRIPT_LIB += git-rebase--interactive
+SCRIPT_LIB += git-rebase--interactive--preserve-merges
 SCRIPT_LIB += git-rebase--interactive--lib
 SCRIPT_LIB += git-rebase--merge
 SCRIPT_LIB += git-sh-setup
diff --git a/git-rebase--interactive--preserve-merges.sh b/git-rebase--interactive--preserve-merges.sh
new file mode 100644
index 000000000..e00b5c990
--- /dev/null
+++ b/git-rebase--interactive--preserve-merges.sh
@@ -0,0 +1,134 @@
+#!/bin/sh
+# This shell script fragment is sourced by git-rebase to implement
+# its interactive mode with --preserve-merges flag.
+# "git rebase --interactive" makes it easy to fix up commits in the
+# middle of a series and rearrange commits and adding --preserve-merges
+# requests it to preserve merges while rebase.
+#
+# Copyright (c) 2006 Johannes E. Schindelin
+#
+# The original idea comes from Eric W. Biederman, in
+# https://public-inbox.org/git/m1odwkyuf5.fsf_-_@ebiederm.dsl.xmission.com/
+
+. git-rebase--interactive--lib
+
+# 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__preserve_merges () {
+	initiate_action "$action"
+	ret=$?
+	if test $ret == 0; then
+		return 0
+	fi
+
+	setup_reflog_action
+	init_basic_state
+
+	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=
+
+	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
+
+	# The 'rev-list .. | sed'
+	#   requires %m to parse; where as the the instruction
+	#   requires %H to parse
+	format=$(git config --get rebase.instructionFormat)
+	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
+
+	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
+}
+# ... and then we call the whole thing.
+git_rebase__interactive__preserve_merges
diff --git a/git-rebase.sh b/git-rebase.sh
index a1f6e5de6..3c2fc35f7 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -196,7 +196,12 @@ run_specific_rebase () {
 		export GIT_EDITOR
 		autosquash=
 	fi
-	. git-rebase--$type
+	if test "$type" = interactive
+	then
+		. git-rebase--${type}${preserve_merges:+--preserve-merges}
+	else
+		. git-rebase--${type}
+	fi
 	ret=$?
 	if test $ret -eq 0
 	then
-- 
2.16.2

[RFC PATCH 3/3] rebase-interactive: refactor git-rebase--interactive to use library

[Wink Saville]

Refactor git-rebase--interactive to use git-rebase--interactive--lib
this simplifies and reduces the size of the code by about 1000 LoC.

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

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 331c8dfea..3d2b89e1c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1,3 +1,4 @@
+#!/bin/sh
 # This shell script fragment is sourced by git-rebase to implement
 # its interactive mode.  "git rebase --interactive" makes it easy
 # to fix up commits in the middle of a series and rearrange commits.
@@ -6,742 +7,8 @@
 #
 # The original idea comes from Eric W. Biederman, in
 # https://public-inbox.org/git/m1odwkyuf5.fsf_-_@ebiederm.dsl.xmission.com/
-#
-# The file containing rebase commands, comments, and empty lines.
-# This file is created by "git rebase -i" then edited by the user.  As
-# the lines are processed, they are removed from the front of this
-# file and written to the tail of $done.
-todo="$state_dir"/git-rebase-todo
-
-# The rebase command lines that have already been processed.  A line
-# is moved here when it is first handled, before any associated user
-# actions.
-done="$state_dir"/done
-
-# The commit message that is planned to be used for any changes that
-# need to be committed following a user interaction.
-msg="$state_dir"/message
-
-# The file into which is accumulated the suggested commit message for
-# squash/fixup commands.  When the first of a series of squash/fixups
-# is seen, the file is created and the commit message from the
-# previous commit and from the first squash/fixup commit are written
-# to it.  The commit message for each subsequent squash/fixup commit
-# is appended to the file as it is processed.
-#
-# The first line of the file is of the form
-#     # This is a combination of $count commits.
-# where $count is the number of commits whose messages have been
-# written to the file so far (including the initial "pick" commit).
-# Each time that a commit message is processed, this line is read and
-# updated.  It is deleted just before the combined commit is made.
-squash_msg="$state_dir"/message-squash
-
-# If the current series of squash/fixups has not yet included a squash
-# command, then this file exists and holds the commit message of the
-# original "pick" commit.  (If the series ends without a "squash"
-# command, then this can be used as the commit message of the combined
-# commit without opening the editor.)
-fixup_msg="$state_dir"/message-fixup
-
-# $rewritten is the name of a directory containing files for each
-# commit that is reachable by at least one merge base of $head and
-# $upstream. They are not necessarily rewritten, but their children
-# might be.  This ensures that commits on merged, but otherwise
-# unrelated side branches are left alone. (Think "X" in the man page's
-# example.)
-rewritten="$state_dir"/rewritten
-
-dropped="$state_dir"/dropped
-
-end="$state_dir"/end
-msgnum="$state_dir"/msgnum
-
-# A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
-# GIT_AUTHOR_DATE that will be used for the commit that is currently
-# being rebased.
-author_script="$state_dir"/author-script
-
-# When an "edit" rebase command is being processed, the SHA1 of the
-# commit to be edited is recorded in this file.  When "git rebase
-# --continue" is executed, if there are any staged changes then they
-# will be amended to the HEAD commit, but only provided the HEAD
-# commit is still the commit to be edited.  When any other rebase
-# command is processed, this file is deleted.
-amend="$state_dir"/amend
-
-# For the post-rewrite hook, we make a list of rewritten commits and
-# their new sha1s.  The rewritten-pending list keeps the sha1s of
-# commits that have been processed, but not committed yet,
-# e.g. because they are waiting for a 'squash' command.
-rewritten_list="$state_dir"/rewritten-list
-rewritten_pending="$state_dir"/rewritten-pending
-
-# Work around Git for Windows' Bash whose "read" does not strip CRLF
-# and leaves CR at the end instead.
-cr=$(printf "\015")
-
-strategy_args=${strategy:+--strategy=$strategy}
-test -n "$strategy_opts" &&
-eval '
-	for strategy_opt in '"$strategy_opts"'
-	do
-		strategy_args="$strategy_args -X$(git rev-parse --sq-quote "${strategy_opt#--}")"
-	done
-'
-
-GIT_CHERRY_PICK_HELP="$resolvemsg"
-export GIT_CHERRY_PICK_HELP
-
-comment_char=$(git config --get core.commentchar 2>/dev/null)
-case "$comment_char" in
-'' | auto)
-	comment_char="#"
-	;;
-?)
-	;;
-*)
-	comment_char=$(echo "$comment_char" | cut -c1)
-	;;
-esac
-
-warn () {
-	printf '%s\n' "$*" >&2
-}
-
-# Output the commit message for the specified commit.
-commit_message () {
-	git cat-file commit "$1" | sed "1,/^$/d"
-}
-
-orig_reflog_action="$GIT_REFLOG_ACTION"
-
-comment_for_reflog () {
-	case "$orig_reflog_action" in
-	''|rebase*)
-		GIT_REFLOG_ACTION="rebase -i ($1)"
-		export GIT_REFLOG_ACTION
-		;;
-	esac
-}
-
-last_count=
-mark_action_done () {
-	sed -e 1q < "$todo" >> "$done"
-	sed -e 1d < "$todo" >> "$todo".new
-	mv -f "$todo".new "$todo"
-	new_count=$(( $(git stripspace --strip-comments <"$done" | wc -l) ))
-	echo $new_count >"$msgnum"
-	total=$(($new_count + $(git stripspace --strip-comments <"$todo" | wc -l)))
-	echo $total >"$end"
-	if test "$last_count" != "$new_count"
-	then
-		last_count=$new_count
-		eval_gettext "Rebasing (\$new_count/\$total)"; printf "\r"
-		test -z "$verbose" || echo
-	fi
-}
-
-# Put the last action marked done at the beginning of the todo list
-# again. If there has not been an action marked done yet, leave the list of
-# items on the todo list unchanged.
-reschedule_last_action () {
-	tail -n 1 "$done" | cat - "$todo" >"$todo".new
-	sed -e \$d <"$done" >"$done".new
-	mv -f "$todo".new "$todo"
-	mv -f "$done".new "$done"
-}
-
-append_todo_help () {
-	gettext "
-Commands:
-p, pick = use commit
-r, reword = use commit, but edit the commit message
-e, edit = use commit, but stop for amending
-s, squash = use commit, but meld into previous commit
-f, fixup = like \"squash\", but discard this commit's log message
-x, exec = run command (the rest of the line) using shell
-d, drop = remove commit
-
-These lines can be re-ordered; they are executed from top to bottom.
-" | git stripspace --comment-lines >>"$todo"
-
-	if test $(get_missing_commit_check_level) = error
-	then
-		gettext "
-Do not remove any line. Use 'drop' explicitly to remove a commit.
-" | git stripspace --comment-lines >>"$todo"
-	else
-		gettext "
-If you remove a line here THAT COMMIT WILL BE LOST.
-" | git stripspace --comment-lines >>"$todo"
-	fi
-}
-
-make_patch () {
-	sha1_and_parents="$(git rev-list --parents -1 "$1")"
-	case "$sha1_and_parents" in
-	?*' '?*' '?*)
-		git diff --cc $sha1_and_parents
-		;;
-	?*' '?*)
-		git diff-tree -p "$1^!"
-		;;
-	*)
-		echo "Root commit"
-		;;
-	esac > "$state_dir"/patch
-	test -f "$msg" ||
-		commit_message "$1" > "$msg"
-	test -f "$author_script" ||
-		get_author_ident_from_commit "$1" > "$author_script"
-}
-
-die_with_patch () {
-	echo "$1" > "$state_dir"/stopped-sha
-	git update-ref REBASE_HEAD "$1"
-	make_patch "$1"
-	die "$2"
-}
-
-exit_with_patch () {
-	echo "$1" > "$state_dir"/stopped-sha
-	git update-ref REBASE_HEAD "$1"
-	make_patch $1
-	git rev-parse --verify HEAD > "$amend"
-	gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")}
-	warn "$(eval_gettext "\
-You can amend the commit now, with
-
-	git commit --amend \$gpg_sign_opt_quoted
-
-Once you are satisfied with your changes, run
 
-	git rebase --continue")"
-	warn
-	exit $2
-}
-
-die_abort () {
-	apply_autostash
-	rm -rf "$state_dir"
-	die "$1"
-}
-
-has_action () {
-	test -n "$(git stripspace --strip-comments <"$1")"
-}
-
-is_empty_commit() {
-	tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null) || {
-		sha1=$1
-		die "$(eval_gettext "\$sha1: not a commit that can be picked")"
-	}
-	ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null) ||
-		ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
-	test "$tree" = "$ptree"
-}
-
-is_merge_commit()
-{
-	git rev-parse --verify --quiet "$1"^2 >/dev/null 2>&1
-}
-
-# Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
-# GIT_AUTHOR_DATE exported from the current environment.
-do_with_author () {
-	(
-		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE
-		"$@"
-	)
-}
-
-git_sequence_editor () {
-	if test -z "$GIT_SEQUENCE_EDITOR"
-	then
-		GIT_SEQUENCE_EDITOR="$(git config sequence.editor)"
-		if [ -z "$GIT_SEQUENCE_EDITOR" ]
-		then
-			GIT_SEQUENCE_EDITOR="$(git var GIT_EDITOR)" || return $?
-		fi
-	fi
-
-	eval "$GIT_SEQUENCE_EDITOR" '"$@"'
-}
-
-pick_one () {
-	ff=--ff
-
-	case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
-	case "$force_rebase" in '') ;; ?*) ff= ;; esac
-	output git rev-parse --verify $sha1 || die "$(eval_gettext "Invalid commit name: \$sha1")"
-
-	if is_empty_commit "$sha1"
-	then
-		empty_args="--allow-empty"
-	fi
-
-	test -d "$rewritten" &&
-		pick_one_preserving_merges "$@" && return
-	output eval git cherry-pick $allow_rerere_autoupdate $allow_empty_message \
-			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
-			"$strategy_args" $empty_args $ff "$@"
-
-	# If cherry-pick dies it leaves the to-be-picked commit unrecorded. Reschedule
-	# previous task so this commit is not lost.
-	ret=$?
-	case "$ret" in [01]) ;; *) reschedule_last_action ;; esac
-	return $ret
-}
-
-pick_one_preserving_merges () {
-	fast_forward=t
-	case "$1" in
-	-n)
-		fast_forward=f
-		sha1=$2
-		;;
-	*)
-		sha1=$1
-		;;
-	esac
-	sha1=$(git rev-parse $sha1)
-
-	if test -f "$state_dir"/current-commit
-	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
-	fi
-
-	echo $sha1 >> "$state_dir"/current-commit
-
-	# rewrite parents; if none were rewritten, we can fast-forward.
-	new_parents=
-	pend=" $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)"
-	if test "$pend" = " "
-	then
-		pend=" root"
-	fi
-	while [ "$pend" != "" ]
-	do
-		p=$(expr "$pend" : ' \([^ ]*\)')
-		pend="${pend# $p}"
-
-		if test -f "$rewritten"/$p
-		then
-			new_p=$(cat "$rewritten"/$p)
-
-			# If the todo reordered commits, and our parent is marked for
-			# rewriting, but hasn't been gotten to yet, assume the user meant to
-			# drop it on top of the current HEAD
-			if test -z "$new_p"
-			then
-				new_p=$(git rev-parse HEAD)
-			fi
-
-			test $p != $new_p && fast_forward=f
-			case "$new_parents" in
-			*$new_p*)
-				;; # do nothing; that parent is already there
-			*)
-				new_parents="$new_parents $new_p"
-				;;
-			esac
-		else
-			if test -f "$dropped"/$p
-			then
-				fast_forward=f
-				replacement="$(cat "$dropped"/$p)"
-				test -z "$replacement" && replacement=root
-				pend=" $replacement$pend"
-			else
-				new_parents="$new_parents $p"
-			fi
-		fi
-	done
-	case $fast_forward in
-	t)
-		output warn "$(eval_gettext "Fast-forward to \$sha1")"
-		output git reset --hard $sha1 ||
-			die "$(eval_gettext "Cannot fast-forward to \$sha1")"
-		;;
-	f)
-		first_parent=$(expr "$new_parents" : ' \([^ ]*\)')
-
-		if [ "$1" != "-n" ]
-		then
-			# detach HEAD to current parent
-			output git checkout $first_parent 2> /dev/null ||
-				die "$(eval_gettext "Cannot move HEAD to \$first_parent")"
-		fi
-
-		case "$new_parents" in
-		' '*' '*)
-			test "a$1" = a-n && die "$(eval_gettext "Refusing to squash a merge: \$sha1")"
-
-			# redo merge
-			author_script_content=$(get_author_ident_from_commit $sha1)
-			eval "$author_script_content"
-			msg_content="$(commit_message $sha1)"
-			# No point in merging the first parent, that's HEAD
-			new_parents=${new_parents# $first_parent}
-			merge_args="--no-log --no-ff"
-			if ! do_with_author output eval \
-				git merge ${gpg_sign_opt:+$(git rev-parse \
-					--sq-quote "$gpg_sign_opt")} \
-				$allow_rerere_autoupdate "$merge_args" \
-				"$strategy_args" \
-				-m "$(git rev-parse --sq-quote "$msg_content")" \
-				"$new_parents"
-			then
-				printf "%s\n" "$msg_content" > "$GIT_DIR"/MERGE_MSG
-				die_with_patch $sha1 "$(eval_gettext "Error redoing merge \$sha1")"
-			fi
-			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
-			;;
-		*)
-			output eval git cherry-pick $allow_rerere_autoupdate \
-				$allow_empty_message \
-				${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
-				"$strategy_args" "$@" ||
-				die_with_patch $sha1 "$(eval_gettext "Could not pick \$sha1")"
-			;;
-		esac
-		;;
-	esac
-}
-
-this_nth_commit_message () {
-	n=$1
-	eval_gettext "This is the commit message #\${n}:"
-}
-
-skip_nth_commit_message () {
-	n=$1
-	eval_gettext "The commit message #\${n} will be skipped:"
-}
-
-update_squash_messages () {
-	if test -f "$squash_msg"; then
-		mv "$squash_msg" "$squash_msg".bak || exit
-		count=$(($(sed -n \
-			-e "1s/^$comment_char[^0-9]*\([0-9][0-9]*\).*/\1/p" \
-			-e "q" < "$squash_msg".bak)+1))
-		{
-			printf '%s\n' "$comment_char $(eval_ngettext \
-				"This is a combination of \$count commit." \
-				"This is a combination of \$count commits." \
-				$count)"
-			sed -e 1d -e '2,/^./{
-				/^$/d
-			}' <"$squash_msg".bak
-		} >"$squash_msg"
-	else
-		commit_message HEAD >"$fixup_msg" ||
-		die "$(eval_gettext "Cannot write \$fixup_msg")"
-		count=2
-		{
-			printf '%s\n' "$comment_char $(gettext "This is a combination of 2 commits.")"
-			printf '%s\n' "$comment_char $(gettext "This is the 1st commit message:")"
-			echo
-			cat "$fixup_msg"
-		} >"$squash_msg"
-	fi
-	case $1 in
-	squash)
-		rm -f "$fixup_msg"
-		echo
-		printf '%s\n' "$comment_char $(this_nth_commit_message $count)"
-		echo
-		commit_message $2
-		;;
-	fixup)
-		echo
-		printf '%s\n' "$comment_char $(skip_nth_commit_message $count)"
-		echo
-		# Change the space after the comment character to TAB:
-		commit_message $2 | git stripspace --comment-lines | sed -e 's/ /	/'
-		;;
-	esac >>"$squash_msg"
-}
-
-peek_next_command () {
-	git stripspace --strip-comments <"$todo" | sed -n -e 's/ .*//p' -e q
-}
-
-# A squash/fixup has failed.  Prepare the long version of the squash
-# commit message, then die_with_patch.  This code path requires the
-# user to edit the combined commit message for all commits that have
-# been squashed/fixedup so far.  So also erase the old squash
-# messages, effectively causing the combined commit to be used as the
-# new basis for any further squash/fixups.  Args: sha1 rest
-die_failed_squash() {
-	sha1=$1
-	rest=$2
-	mv "$squash_msg" "$msg" || exit
-	rm -f "$fixup_msg"
-	cp "$msg" "$GIT_DIR"/MERGE_MSG || exit
-	warn
-	warn "$(eval_gettext "Could not apply \$sha1... \$rest")"
-	die_with_patch $sha1 ""
-}
-
-flush_rewritten_pending() {
-	test -s "$rewritten_pending" || return
-	newsha1="$(git rev-parse HEAD^0)"
-	sed "s/$/ $newsha1/" < "$rewritten_pending" >> "$rewritten_list"
-	rm -f "$rewritten_pending"
-}
-
-record_in_rewritten() {
-	oldsha1="$(git rev-parse $1)"
-	echo "$oldsha1" >> "$rewritten_pending"
-
-	case "$(peek_next_command)" in
-	squash|s|fixup|f)
-		;;
-	*)
-		flush_rewritten_pending
-		;;
-	esac
-}
-
-do_pick () {
-	sha1=$1
-	rest=$2
-	if test "$(git rev-parse HEAD)" = "$squash_onto"
-	then
-		# Set the correct commit message and author info on the
-		# sentinel root before cherry-picking the original changes
-		# without committing (-n).  Finally, update the sentinel again
-		# to include these changes.  If the cherry-pick results in a
-		# conflict, this means our behaviour is similar to a standard
-		# failed cherry-pick during rebase, with a dirty index to
-		# resolve before manually running git commit --amend then git
-		# rebase --continue.
-		git commit --allow-empty --allow-empty-message --amend \
-			   --no-post-rewrite -n -q -C $sha1 &&
-			pick_one -n $sha1 &&
-			git commit --allow-empty --allow-empty-message \
-				   --amend --no-post-rewrite -n -q -C $sha1 \
-				   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
-				   die_with_patch $sha1 "$(eval_gettext "Could not apply \$sha1... \$rest")"
-	else
-		pick_one $sha1 ||
-			die_with_patch $sha1 "$(eval_gettext "Could not apply \$sha1... \$rest")"
-	fi
-}
-
-do_next () {
-	rm -f "$msg" "$author_script" "$amend" "$state_dir"/stopped-sha || exit
-	read -r command sha1 rest < "$todo"
-	case "$command" in
-	"$comment_char"*|''|noop|drop|d)
-		mark_action_done
-		;;
-	"$cr")
-		# Work around CR left by "read" (e.g. with Git for Windows' Bash).
-		mark_action_done
-		;;
-	pick|p)
-		comment_for_reflog pick
-
-		mark_action_done
-		do_pick $sha1 "$rest"
-		record_in_rewritten $sha1
-		;;
-	reword|r)
-		comment_for_reflog reword
-
-		mark_action_done
-		do_pick $sha1 "$rest"
-		git commit --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} \
-			$allow_empty_message || {
-			warn "$(eval_gettext "\
-Could not amend commit after successfully picking \$sha1... \$rest
-This is most likely due to an empty commit message, or the pre-commit hook
-failed. If the pre-commit hook failed, you may need to resolve the issue before
-you are able to reword the commit.")"
-			exit_with_patch $sha1 1
-		}
-		record_in_rewritten $sha1
-		;;
-	edit|e)
-		comment_for_reflog edit
-
-		mark_action_done
-		do_pick $sha1 "$rest"
-		sha1_abbrev=$(git rev-parse --short $sha1)
-		warn "$(eval_gettext "Stopped at \$sha1_abbrev... \$rest")"
-		exit_with_patch $sha1 0
-		;;
-	squash|s|fixup|f)
-		case "$command" in
-		squash|s)
-			squash_style=squash
-			;;
-		fixup|f)
-			squash_style=fixup
-			;;
-		esac
-		comment_for_reflog $squash_style
-
-		test -f "$done" && has_action "$done" ||
-			die "$(eval_gettext "Cannot '\$squash_style' without a previous commit")"
-
-		mark_action_done
-		update_squash_messages $squash_style $sha1
-		author_script_content=$(get_author_ident_from_commit HEAD)
-		echo "$author_script_content" > "$author_script"
-		eval "$author_script_content"
-		if ! pick_one -n $sha1
-		then
-			git rev-parse --verify HEAD >"$amend"
-			die_failed_squash $sha1 "$rest"
-		fi
-		case "$(peek_next_command)" in
-		squash|s|fixup|f)
-			# This is an intermediate commit; its message will only be
-			# used in case of trouble.  So use the long version:
-			do_with_author output git commit --amend --no-verify -F "$squash_msg" \
-				${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
-				die_failed_squash $sha1 "$rest"
-			;;
-		*)
-			# This is the final command of this squash/fixup group
-			if test -f "$fixup_msg"
-			then
-				do_with_author git commit --amend --no-verify -F "$fixup_msg" \
-					${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
-					die_failed_squash $sha1 "$rest"
-			else
-				cp "$squash_msg" "$GIT_DIR"/SQUASH_MSG || exit
-				rm -f "$GIT_DIR"/MERGE_MSG
-				do_with_author git commit --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e \
-					${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message ||
-					die_failed_squash $sha1 "$rest"
-			fi
-			rm -f "$squash_msg" "$fixup_msg"
-			;;
-		esac
-		record_in_rewritten $sha1
-		;;
-	x|"exec")
-		read -r command rest < "$todo"
-		mark_action_done
-		eval_gettextln "Executing: \$rest"
-		"${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
-		status=$?
-		# Run in subshell because require_clean_work_tree can die.
-		dirty=f
-		(require_clean_work_tree "rebase" 2>/dev/null) || dirty=t
-		if test "$status" -ne 0
-		then
-			warn "$(eval_gettext "Execution failed: \$rest")"
-			test "$dirty" = f ||
-				warn "$(gettext "and made changes to the index and/or the working tree")"
-
-			warn "$(gettext "\
-You can fix the problem, and then run
-
-	git rebase --continue")"
-			warn
-			if test $status -eq 127		# command not found
-			then
-				status=1
-			fi
-			exit "$status"
-		elif test "$dirty" = t
-		then
-			# TRANSLATORS: after these lines is a command to be issued by the user
-			warn "$(eval_gettext "\
-Execution succeeded: \$rest
-but left changes to the index and/or the working tree
-Commit or stash your changes, and then run
-
-	git rebase --continue")"
-			warn
-			exit 1
-		fi
-		;;
-	*)
-		warn "$(eval_gettext "Unknown command: \$command \$sha1 \$rest")"
-		fixtodo="$(gettext "Please fix this using 'git rebase --edit-todo'.")"
-		if git rev-parse --verify -q "$sha1" >/dev/null
-		then
-			die_with_patch $sha1 "$fixtodo"
-		else
-			die "$fixtodo"
-		fi
-		;;
-	esac
-	test -s "$todo" && return
-
-	comment_for_reflog finish &&
-	newhead=$(git rev-parse HEAD) &&
-	case $head_name in
-	refs/*)
-		message="$GIT_REFLOG_ACTION: $head_name onto $onto" &&
-		git update-ref -m "$message" $head_name $newhead $orig_head &&
-		git symbolic-ref \
-		  -m "$GIT_REFLOG_ACTION: returning to $head_name" \
-		  HEAD $head_name
-		;;
-	esac && {
-		test ! -f "$state_dir"/verbose ||
-			git diff-tree --stat $orig_head..HEAD
-	} &&
-	{
-		test -s "$rewritten_list" &&
-		git notes copy --for-rewrite=rebase < "$rewritten_list" ||
-		true # we don't care if this copying failed
-	} &&
-	hook="$(git rev-parse --git-path hooks/post-rewrite)"
-	if test -x "$hook" && test -s "$rewritten_list"; then
-		"$hook" rebase < "$rewritten_list"
-		true # we don't care if this hook failed
-	fi &&
-		warn "$(eval_gettext "Successfully rebased and updated \$head_name.")"
-
-	return 1 # not failure; just to break the do_rest loop
-}
-
-# can only return 0, when the infinite loop breaks
-do_rest () {
-	while :
-	do
-		do_next || break
-	done
-}
-
-expand_todo_ids() {
-	git rebase--helper --expand-ids
-}
-
-collapse_todo_ids() {
-	git rebase--helper --shorten-ids
-}
-
-# Switch to the branch in $into and notify it in the reflog
-checkout_onto () {
-	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
-	output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
-	git update-ref ORIG_HEAD $orig_head
-}
-
-get_missing_commit_check_level () {
-	check_level=$(git config --get rebase.missingCommitsCheck)
-	check_level=${check_level:-ignore}
-	# Don't be case sensitive
-	printf '%s' "$check_level" | tr 'A-Z' 'a-z'
-}
+. git-rebase--interactive--lib
 
 # 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
@@ -754,283 +21,35 @@ get_missing_commit_check_level () {
 # here, and immediately call it after defining it.
 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
+	initiate_action "$action"
+	ret=$?
+	if test $ret = 0; then
+		return 0
 	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
-
-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
+	setup_reflog_action
+	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)"
+	merges_option="--no-merges --cherry-pick"
 
-: > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
-write_basic_state
-if test t = "$preserve_merges"
-then
+	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
-		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
+		shortupstream=$(git rev-parse --short $upstream)
+		revisions=$upstream...$orig_head
+		shortrevisions=$shortupstream..$shorthead
 	else
-		mkdir "$rewritten" &&
-		echo $onto > "$rewritten"/root ||
-			die "$(gettext "Could not init rewritten commits")"
+		revisions=$onto...$orig_head
+		shortrevisions=$shorthead
 	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"
-
-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
+	    die "$(gettext "Could not generate todo list")"
 
+	complete_action
 }
 # ... and then we call the whole thing.
 git_rebase__interactive
-- 
2.16.2

Re: [RFC PATCH 0/3] rebase-interactive

[Eric Sunshine]

On Tue, Mar 20, 2018 at 4:45 PM, Wink Saville <wink@saville.com> wrote:
> Patch 0001 creates a library of functions which can be
> used by git-rebase--interactive and
> git-rebase--interactive--preserve-merges. The functions are
> those that exist in git-rebase--interactive.sh plus new
> functions created from the body of git_rebase_interactive
> that will be used git_rebase_interactive in the third patch
> and git_rebase_interactive_preserve_merges in the second
> patch. None of the functions are invoked so there is no
> logic changes and the system builds and passes all tests
> on travis-ci.org.
>
> Patch 0002 creates git-rebase--interactive--preserve-merges.sh
> with the function git_rebase_interactive_preserve_merges. The contents
> of the function are refactored from git_rebase_interactive and
> uses existing and new functions in the library. A small modification
> of git-rebase is also done to invoke the new function when the -p
> switch is used with git-rebase. When this is applied on top of
> 0001 the system builds and passes all tests on travis-ci.org.
>
> The final patch, 0003, removes all unused code from
> git_rebase_interactive and uses the functions from the library
> where appropriate. And, of course, when applied on top of 0002
> the system builds and passes all tests on travis-ci.org.

A problem with this approach is that it loses "blame" information. A
git-blame of git-rebase--interactive--lib.sh shows all code in that
file as having arisen spontaneously from thin air; it is unable to
trace its real history. It would be much better to actually _move_
code to the new file (and update callers if necessary), which would
preserve provenance.

Ideally, patches which move code around should do so verbatim (or at
least as close to verbatim as possible) to ease review burden.
Sometimes changes to code are needed to make it relocatable before
movement, in which case those changes should be made as separate
preparatory patches, again to ease review.

As it is, without detailed spelunking, it is not immediately clear to
a reviewer which functions in git-rebase--interactive--lib.sh are
newly written, and which are merely moved (or moved and edited) from
git-rebase--interactive.sh. This shortcoming suggests that the patch
series could be re-worked to do the refactoring in a more piecemeal
fashion which more clearly holds the hands of those trying to
understand the changes. (For instance, one or more preparatory patches
may be needed to make the code relocatable, followed by verbatim code
relocation, possibly iterating these steps if some changes depend upon
earlier changes, etc.)

Thanks.

Re: [RFC PATCH 0/3] rebase-interactive

[Wink Saville]

On Tue, Mar 20, 2018 at 2:11 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Mar 20, 2018 at 4:45 PM, Wink Saville <wink@saville.com> wrote:
>> Patch 0001 creates a library of functions which can be
>> used by git-rebase--interactive and
>> git-rebase--interactive--preserve-merges. The functions are
>> those that exist in git-rebase--interactive.sh plus new
>> functions created from the body of git_rebase_interactive
>> that will be used git_rebase_interactive in the third patch
>> and git_rebase_interactive_preserve_merges in the second
>> patch. None of the functions are invoked so there is no
>> logic changes and the system builds and passes all tests
>> on travis-ci.org.
>>
>> Patch 0002 creates git-rebase--interactive--preserve-merges.sh
>> with the function git_rebase_interactive_preserve_merges. The contents
>> of the function are refactored from git_rebase_interactive and
>> uses existing and new functions in the library. A small modification
>> of git-rebase is also done to invoke the new function when the -p
>> switch is used with git-rebase. When this is applied on top of
>> 0001 the system builds and passes all tests on travis-ci.org.
>>
>> The final patch, 0003, removes all unused code from
>> git_rebase_interactive and uses the functions from the library
>> where appropriate. And, of course, when applied on top of 0002
>> the system builds and passes all tests on travis-ci.org.
>
> A problem with this approach is that it loses "blame" information. A
> git-blame of git-rebase--interactive--lib.sh shows all code in that
> file as having arisen spontaneously from thin air; it is unable to
> trace its real history. It would be much better to actually _move_
> code to the new file (and update callers if necessary), which would
> preserve provenance.
>
> Ideally, patches which move code around should do so verbatim (or at
> least as close to verbatim as possible) to ease review burden.
> Sometimes changes to code are needed to make it relocatable before
> movement, in which case those changes should be made as separate
> preparatory patches, again to ease review.
>
> As it is, without detailed spelunking, it is not immediately clear to
> a reviewer which functions in git-rebase--interactive--lib.sh are
> newly written, and which are merely moved (or moved and edited) from
> git-rebase--interactive.sh. This shortcoming suggests that the patch
> series could be re-worked to do the refactoring in a more piecemeal
> fashion which more clearly holds the hands of those trying to
> understand the changes. (For instance, one or more preparatory patches
> may be needed to make the code relocatable, followed by verbatim code
> relocation, possibly iterating these steps if some changes depend upon
> earlier changes, etc.)
>
> Thanks.

Must all intermediate commits continue build and pass tests?

Re: [RFC PATCH 0/3] rebase-interactive

[Eric Sunshine]

On Tue, Mar 20, 2018 at 5:23 PM, Wink Saville <wink@saville.com> wrote:
> On Tue, Mar 20, 2018 at 2:11 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> A problem with this approach is that it loses "blame" information. A
>> git-blame of git-rebase--interactive--lib.sh shows all code in that
>> file as having arisen spontaneously from thin air; it is unable to
>> trace its real history. It would be much better to actually _move_
>> code to the new file (and update callers if necessary), which would
>> preserve provenance.
>>
>> Ideally, patches which move code around should do so verbatim (or at
>> least as close to verbatim as possible) to ease review burden.
>> Sometimes changes to code are needed to make it relocatable before
>> movement, in which case those changes should be made as separate
>> preparatory patches, again to ease review.
>>
>> As it is, without detailed spelunking, it is not immediately clear to
>> a reviewer which functions in git-rebase--interactive--lib.sh are
>> newly written, and which are merely moved (or moved and edited) from
>> git-rebase--interactive.sh. This shortcoming suggests that the patch
>> series could be re-worked to do the refactoring in a more piecemeal
>> fashion which more clearly holds the hands of those trying to
>> understand the changes. (For instance, one or more preparatory patches
>> may be needed to make the code relocatable, followed by verbatim code
>> relocation, possibly iterating these steps if some changes depend upon
>> earlier changes, etc.)
>
> Must all intermediate commits continue build and pass tests?

Yes, not just because it is good hygiene, but also because it
preserves git-bisect'ability.

Re: [RFC PATCH 0/3] rebase-interactive

[Wink Saville]

On Tue, Mar 20, 2018 at 2:47 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Mar 20, 2018 at 5:23 PM, Wink Saville <wink@saville.com> wrote:
>> On Tue, Mar 20, 2018 at 2:11 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> A problem with this approach is that it loses "blame" information. A
>>> git-blame of git-rebase--interactive--lib.sh shows all code in that
>>> file as having arisen spontaneously from thin air; it is unable to
>>> trace its real history. It would be much better to actually _move_
>>> code to the new file (and update callers if necessary), which would
>>> preserve provenance.
>>>
>>> Ideally, patches which move code around should do so verbatim (or at
>>> least as close to verbatim as possible) to ease review burden.
>>> Sometimes changes to code are needed to make it relocatable before
>>> movement, in which case those changes should be made as separate
>>> preparatory patches, again to ease review.
>>>
>>> As it is, without detailed spelunking, it is not immediately clear to
>>> a reviewer which functions in git-rebase--interactive--lib.sh are
>>> newly written, and which are merely moved (or moved and edited) from
>>> git-rebase--interactive.sh. This shortcoming suggests that the patch
>>> series could be re-worked to do the refactoring in a more piecemeal
>>> fashion which more clearly holds the hands of those trying to
>>> understand the changes. (For instance, one or more preparatory patches
>>> may be needed to make the code relocatable, followed by verbatim code
>>> relocation, possibly iterating these steps if some changes depend upon
>>> earlier changes, etc.)
>>
>> Must all intermediate commits continue build and pass tests?
>
> Yes, not just because it is good hygiene, but also because it
> preserves git-bisect'ability.

That's what I figured.

Anyway, I've played around and my current thoughts are to not create
any new files and
keep git_rebase__interactive and the new
git_rebase__interactive__preserve_merges
functions in git-rebase--interactive.sh.

Doing that will preserve the blame for the existing functions. But if
I do indentation
reformating as I extract functions that will be shared between
git_rebase__interactive
and git_rebase__interactive__preserve_merges then we still lose the blame
information unless the "-w" parameter is passed to blame. I could choose to
not do the indentation, but that doesn't seem like a good idea.

An alternative is that we don't accept the refactoring. What I'd
probably do is use
the refactored code to figure out a fix for the bug and then back port
the fix to the
existing code.

My opinion is that to not accept "improved" code because we lose blame
information
is not a good trade off. Of course what I might think is "improved"
others may rightfully
say the refactoring is gratuitous. If that is the case than not doing
the refactoring is the
right solution. I don't see a right or wong here, just a typical
engineering trade off.

Thoughts or other ideas?

Re: [RFC PATCH 0/3] rebase-interactive

[Eric Sunshine]

{cc:+junio}

On Tue, Mar 20, 2018 at 11:31 PM, Wink Saville <wink@saville.com> wrote:
> Anyway, I've played around and my current thoughts are to not create
> any new files and keep git_rebase__interactive and the new
> git_rebase__interactive__preserve_merges functions in
> git-rebase--interactive.sh. Doing that will preserve the blame for
> the existing functions.

"blame -C" detects code movement across files, so you needn't feel
constrained in that sense. (In fact, "blame -C -C" should detect the
copied -- not moved -- code in your patch 1/2, so my objection is not
entirely valid, although "-C -C" is potentially much slower.)

Nevertheless, leaving all the code in git-rebase--interactive.sh
sounds sensible if there is not a compelling reason to split it out,
especially since each refactoring (especially a big one) has the
potential to collide with in-flight or planned topics.

> But if I do indentation reformating as I extract functions that will
> be shared between git_rebase__interactive and
> git_rebase__interactive__preserve_merges then we still lose the
> blame information unless the "-w" parameter is passed to blame. I
> could choose to not do the indentation, but that doesn't seem like a
> good idea.

Don't worry about indentation changes during refactoring and code
movement; it's frequently necessary, and "blame" still works (with
"-w", as you point out).

> Thoughts or other ideas?

A few...

Although you may have the entire refactoring in your head -- you know
what you moved where and why and what changes were needed to make the
move possible -- reviewers don't have that luxury. A nearly 1000-line
patch, such as 1/3, which copies code (possibly verbatim or possibly
with edits -- reviewers don't know which) and which contains
newly-written code is a significant burden on reviewers. Crafting a
patch series such that it holds the hands of reviewers by laying out
each change in easily digested chunks helps significantly, both with
attracting more and better reviews, and with potential buy-in that the
changes are worthwhile.

However, I'm just one (potential) reviewer, and I don't want you
wasting your time embarking on large-scale changes based upon my
comments before hearing from Dscho (Johannes) and Junio if such
refactoring is even welcome.

[RFC PATCH v2 0/1] rebase-interactive: Add git_rebase__interactive__preserve_merges

[Wink Saville]

Version 2 keeps all changes in git-rebase--interactive.sh this should
make it easier to use blame. But there is quite a bit of refactoring
and reformatting so using "git blame -w" or "git blame -w -C -C" is
needed to improve the results of blame.

I can break this into several patches to have the history show the code
movement more directly if that is desired.

Wink Saville (1):
  rebase-interactive: Add git_rebase__interactive__preserve_merges

 git-rebase--interactive.sh | 469 +++++++++++++++++++++++++--------------------
 git-rebase.sh              |  16 +-
 2 files changed, 273 insertions(+), 212 deletions(-)

-- 
2.16.2

[RFC PATCH v2 1/1] rebase-interactive: Add git_rebase__interactive__preserve_merges

[Wink Saville]

Refactor git_rebase__interactive__preserve_merges out of
git_rebase__interactive resulting in fewer conditionals making
both routines are simpler.

Changed run_specific_rebase in git-rebase to dispatch to the appropriate
function after sourcing git-rebase--interactive.
---
 git-rebase--interactive.sh | 469 +++++++++++++++++++++++++--------------------
 git-rebase.sh              |  16 +-
 2 files changed, 273 insertions(+), 212 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 331c8dfea..65ff75654 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1,5 +1,7 @@
-# This shell script fragment is sourced by git-rebase to implement
-# its interactive mode.  "git rebase --interactive" makes it easy
+#!/bin/sh
+# This shell script fragment is sourced by git-rebase--interactive
+# and git-rebase--interactive--preserve-merges in support of the
+# interactive mode.  "git rebase --interactive" makes it easy
 # to fix up commits in the middle of a series and rearrange commits.
 #
 # Copyright (c) 2006 Johannes E. Schindelin
@@ -7,6 +9,7 @@
 # The original idea comes from Eric W. Biederman, in
 # https://public-inbox.org/git/m1odwkyuf5.fsf_-_@ebiederm.dsl.xmission.com/
 #
+
 # The file containing rebase commands, comments, and empty lines.
 # This file is created by "git rebase -i" then edited by the user.  As
 # the lines are processed, they are removed from the front of this
@@ -125,6 +128,19 @@ comment_for_reflog () {
 	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
+}
+
 last_count=
 mark_action_done () {
 	sed -e 1q < "$todo" >> "$done"
@@ -274,7 +290,8 @@ pick_one () {
 
 	case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
 	case "$force_rebase" in '') ;; ?*) ff= ;; esac
-	output git rev-parse --verify $sha1 || die "$(eval_gettext "Invalid commit name: \$sha1")"
+	output git rev-parse --verify $sha1 ||
+		die "$(eval_gettext "Invalid commit name: \$sha1")"
 
 	if is_empty_commit "$sha1"
 	then
@@ -287,8 +304,8 @@ pick_one () {
 			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
 			"$strategy_args" $empty_args $ff "$@"
 
-	# If cherry-pick dies it leaves the to-be-picked commit unrecorded. Reschedule
-	# previous task so this commit is not lost.
+	# If cherry-pick dies it leaves the to-be-picked commit unrecorded.
+	# Reschedule previous task so this commit is not lost.
 	ret=$?
 	case "$ret" in [01]) ;; *) reschedule_last_action ;; esac
 	return $ret
@@ -307,17 +324,15 @@ 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
@@ -337,10 +352,10 @@ pick_one_preserving_merges () {
 		if test -f "$rewritten"/$p
 		then
 			new_p=$(cat "$rewritten"/$p)
-
-			# If the todo reordered commits, and our parent is marked for
-			# rewriting, but hasn't been gotten to yet, assume the user meant to
-			# drop it on top of the current HEAD
+			# If the todo reordered commits, and our parent is
+			# marked for rewriting, but hasn't been gotten to yet,
+			# assume the user meant to drop it on top of the
+			# current HEAD
 			if test -z "$new_p"
 			then
 				new_p=$(git rev-parse HEAD)
@@ -379,7 +394,7 @@ pick_one_preserving_merges () {
 		then
 			# detach HEAD to current parent
 			output git checkout $first_parent 2> /dev/null ||
-				die "$(eval_gettext "Cannot move HEAD to \$first_parent")"
+			   die "$(eval_gettext "Cannot move HEAD to \$first_parent")"
 		fi
 
 		case "$new_parents" in
@@ -553,7 +568,7 @@ do_next () {
 	pick|p)
 		comment_for_reflog pick
 
-		mark_action_done
+		mark_action_done $sha1 "$rest"
 		do_pick $sha1 "$rest"
 		record_in_rewritten $sha1
 		;;
@@ -637,7 +652,7 @@ you are able to reword the commit.")"
 		read -r command rest < "$todo"
 		mark_action_done
 		eval_gettextln "Executing: \$rest"
-		"${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
+		"${SHELL:-/bin/sh}" -c "$rest" # Actual execution
 		status=$?
 		# Run in subshell because require_clean_work_tree can die.
 		dirty=f
@@ -743,37 +758,38 @@ 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 () {
+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)"
 
-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
+	: > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
+	write_basic_state
+}
 
-		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"
+# Initiate an action which is first parameter.
+# Returns 0 if the action was able to complete
+# or 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
-			gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")}
-			die "$(eval_gettext "\
+			# 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:
@@ -788,137 +804,231 @@ 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 ||
+			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 ||
+			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 # done
+		;;
+	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 # done
+		;;
+	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
+}
+
+# Complete the action
+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 "$(gettext "Could not execute editor")"
+		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
 
-	exit
-	;;
-show-current-patch)
-	exec git show REBASE_HEAD --
-	;;
-esac
+	test -d "$rewritten" || test -n "$force_rebase" ||
+	onto="$(git rebase--helper --skip-unnecessary-picks)" ||
+	die "Could not skip unnecessary pick commands"
 
-comment_for_reflog start
+	checkout_onto
 
-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")"
+	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
+	else
+		do_rest
+	fi
+}
 
-	comment_for_reflog start
-fi
+git_rebase__interactive () {
+	initiate_action "$action"
+	ret=$?
+	if test $ret = 0; then
+		return 0
+	fi
+
+	setup_reflog_action
+	init_basic_state
+
+	merges_option="--no-merges --cherry-pick"
+
+	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 rebase--helper --make-script ${keep_empty:+--keep-empty} \
+		$revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
+	    die "$(gettext "Could not generate todo list")"
+
+	complete_action
+}
+
+git_rebase__interactive__preserve_merges () {
+	initiate_action "$action"
+	ret=$?
+	if test $ret == 0; then
+		return 0
+	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"
 	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")"
+			    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
-
-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
+
+	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
+
+	# The 'rev-list .. | sed'
+	#   requires %m to parse; where as the the instruction
+	#   requires %H to parse
 	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
+		if test -z "$keep_empty" \
+			&& is_empty_commit $sha1 \
+			&& ! is_merge_commit $sha1
 		then
 			comment_out="$comment_char "
 		else
@@ -928,7 +1038,8 @@ else
 		if test -z "$rebase_root"
 		then
 			preserve=t
-			for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)
+			for p in $(git rev-list --parents -1 $sha1 | \
+				cut -d' ' -s -f2-)
 			do
 				if test -f "$rewritten"/$p
 				then
@@ -944,11 +1055,7 @@ else
 			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 | \
@@ -961,76 +1068,18 @@ then
 		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
+			# 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"
+			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"
-
-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
 }
-# ... and then we call the whole thing.
-git_rebase__interactive
diff --git a/git-rebase.sh b/git-rebase.sh
index a1f6e5de6..b32282f4c 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -196,8 +196,20 @@ run_specific_rebase () {
 		export GIT_EDITOR
 		autosquash=
 	fi
-	. git-rebase--$type
-	ret=$?
+	if test "$type" = interactive
+	then
+		. git-rebase--interactive
+		if test "$preserve_merges" = t
+		then
+			git_rebase__interactive__preserve_merges
+		else
+			git_rebase__interactive
+		fi
+		ret=$?
+	else
+		. git-rebase--$type
+		ret=$?
+	fi
 	if test $ret -eq 0
 	then
 		finish_rebase
-- 
2.16.2

Re: [RFC PATCH v2 1/1] rebase-interactive: Add git_rebase__interactive__preserve_merges

[Junio C Hamano]

Wink Saville <wink@saville.com> writes:

> Refactor git_rebase__interactive__preserve_merges out of
> git_rebase__interactive resulting in fewer conditionals making
> both routines are simpler.
>
> Changed run_specific_rebase in git-rebase to dispatch to the appropriate
> function after sourcing git-rebase--interactive.
> ---

I think this will become (more) reviewable if it is split into two
patches (at least).  A preliminary patch that does the style changes
and line wrapping (left below) as step #1, and all the rest on top
as step #2.

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 331c8dfea..65ff75654 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -1,5 +1,7 @@
> -# This shell script fragment is sourced by git-rebase to implement
> -# its interactive mode.  "git rebase --interactive" makes it easy
> +#!/bin/sh

Addition of #! implies that this might be invoked as the top-level
script; is that the case now?  I did not get such an impression from
the proposed log message and it is still always dot-sourced (in
which case #! gives a wrong signal to the readers).

> +# This shell script fragment is sourced by git-rebase--interactive
> +# and git-rebase--interactive--preserve-merges in support of the
> +# interactive mode.  "git rebase --interactive" makes it easy
>  # to fix up commits in the middle of a series and rearrange commits.
>  #
>  # Copyright (c) 2006 Johannes E. Schindelin
> @@ -7,6 +9,7 @@
>  # The original idea comes from Eric W. Biederman, in
>  # https://public-inbox.org/git/m1odwkyuf5.fsf_-_@ebiederm.dsl.xmission.com/
>  #
> +
>  # The file containing rebase commands, comments, and empty lines.

Why?

> @@ -274,7 +290,8 @@ pick_one () {
>  
>  	case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
>  	case "$force_rebase" in '') ;; ?*) ff= ;; esac
> -	output git rev-parse --verify $sha1 || die "$(eval_gettext "Invalid commit name: \$sha1")"
> +	output git rev-parse --verify $sha1 ||
> +		die "$(eval_gettext "Invalid commit name: \$sha1")"

Just linewrapping.

> @@ -287,8 +304,8 @@ pick_one () {
>  			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
>  			"$strategy_args" $empty_args $ff "$@"
>  
> -	# If cherry-pick dies it leaves the to-be-picked commit unrecorded. Reschedule
> -	# previous task so this commit is not lost.
> +	# If cherry-pick dies it leaves the to-be-picked commit unrecorded.
> +	# Reschedule previous task so this commit is not lost.

Ditto.

> @@ -307,17 +324,15 @@ 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

Good code simplification that turns

	if A
		if B
			do this thing
		fi
	fi

into

	if A & B
		do this thing
	fi

> @@ -337,10 +352,10 @@ pick_one_preserving_merges () {
>  		if test -f "$rewritten"/$p
>  		then
>  			new_p=$(cat "$rewritten"/$p)
> -
> -			# If the todo reordered commits, and our parent is marked for
> -			# rewriting, but hasn't been gotten to yet, assume the user meant to
> -			# drop it on top of the current HEAD
> +			# If the todo reordered commits, and our parent is
> +			# marked for rewriting, but hasn't been gotten to yet,
> +			# assume the user meant to drop it on top of the
> +			# current HEAD

Just line wrapping.

> @@ -379,7 +394,7 @@ pick_one_preserving_merges () {
>  		then
>  			# detach HEAD to current parent
>  			output git checkout $first_parent 2> /dev/null ||
> -				die "$(eval_gettext "Cannot move HEAD to \$first_parent")"
> +			   die "$(eval_gettext "Cannot move HEAD to \$first_parent")"
>  		fi

Ditto.

> @@ -553,7 +568,7 @@ do_next () {
>  	pick|p)
>  		comment_for_reflog pick
>  
> -		mark_action_done
> +		mark_action_done $sha1 "$rest"

This demands more attention from the readers than all the changes we
have seen so far which were just line wrapping and whitespace
changes.  That is why it is better to leave these changes to a
separate patch after preliminary clean-up.  It allows reviewers'
eyes coast over the clean-up step, and then lets them focus on the
more "meaningful" changes  like this one.

> @@ -637,7 +652,7 @@ you are able to reword the commit.")"
>  		read -r command rest < "$todo"
>  		mark_action_done
>  		eval_gettextln "Executing: \$rest"
> -		"${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
> +		"${SHELL:-/bin/sh}" -c "$rest" # Actual execution

Why?  This change is not justified in the proposed log message.

Re: [RFC PATCH v2 1/1] rebase-interactive: Add git_rebase__interactive__preserve_merges

[Wink Saville]

On Wed, Mar 21, 2018 at 12:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Wink Saville <wink@saville.com> writes:
>
>> Refactor git_rebase__interactive__preserve_merges out of
>> git_rebase__interactive resulting in fewer conditionals making
>> both routines are simpler.
>>
>> Changed run_specific_rebase in git-rebase to dispatch to the appropriate
>> function after sourcing git-rebase--interactive.
>> ---
>
> I think this will become (more) reviewable if it is split into two
> patches (at least).  A preliminary patch that does the style changes
> and line wrapping (left below) as step #1, and all the rest on top
> as step #2.

Yes, I will break this into several commits

>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index 331c8dfea..65ff75654 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -1,5 +1,7 @@
>> -# This shell script fragment is sourced by git-rebase to implement
>> -# its interactive mode.  "git rebase --interactive" makes it easy
>> +#!/bin/sh
>
> Addition of #! implies that this might be invoked as the top-level
> script; is that the case now?  I did not get such an impression from
> the proposed log message and it is still always dot-sourced (in
> which case #! gives a wrong signal to the readers).

Will remove adding the shebang

>> +# This shell script fragment is sourced by git-rebase--interactive
>> +# and git-rebase--interactive--preserve-merges in support of the
>> +# interactive mode.  "git rebase --interactive" makes it easy
>>  # to fix up commits in the middle of a series and rearrange commits.
>>  #
>>  # Copyright (c) 2006 Johannes E. Schindelin
>> @@ -7,6 +9,7 @@
>>  # The original idea comes from Eric W. Biederman, in
>>  # https://public-inbox.org/git/m1odwkyuf5.fsf_-_@ebiederm.dsl.xmission.com/
>>  #
>> +
>>  # The file containing rebase commands, comments, and empty lines.
>
> Why?

Will remove the blank line.

>> @@ -274,7 +290,8 @@ pick_one () {
>>
>>       case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
>>       case "$force_rebase" in '') ;; ?*) ff= ;; esac
>> -     output git rev-parse --verify $sha1 || die "$(eval_gettext "Invalid commit name: \$sha1")"
>> +     output git rev-parse --verify $sha1 ||
>> +             die "$(eval_gettext "Invalid commit name: \$sha1")"
>
> Just linewrapping.

Will be leaving for now

>
>> @@ -287,8 +304,8 @@ pick_one () {
>>                       ${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
>>                       "$strategy_args" $empty_args $ff "$@"
>>
>> -     # If cherry-pick dies it leaves the to-be-picked commit unrecorded. Reschedule
>> -     # previous task so this commit is not lost.
>> +     # If cherry-pick dies it leaves the to-be-picked commit unrecorded.
>> +     # Reschedule previous task so this commit is not lost.
>
> Ditto.

Will be leaving for now

>
>> @@ -307,17 +324,15 @@ 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
>
> Good code simplification that turns
>
>         if A
>                 if B
>                         do this thing
>                 fi
>         fi
>
> into
>
>         if A & B
>                 do this thing
>         fi

Will be keeping this in a future commit

>
>> @@ -337,10 +352,10 @@ pick_one_preserving_merges () {
>>               if test -f "$rewritten"/$p
>>               then
>>                       new_p=$(cat "$rewritten"/$p)
>> -
>> -                     # If the todo reordered commits, and our parent is marked for
>> -                     # rewriting, but hasn't been gotten to yet, assume the user meant to
>> -                     # drop it on top of the current HEAD
>> +                     # If the todo reordered commits, and our parent is
>> +                     # marked for rewriting, but hasn't been gotten to yet,
>> +                     # assume the user meant to drop it on top of the
>> +                     # current HEAD
>
> Just line wrapping.

Will be leaving for now

>
>> @@ -379,7 +394,7 @@ pick_one_preserving_merges () {
>>               then
>>                       # detach HEAD to current parent
>>                       output git checkout $first_parent 2> /dev/null ||
>> -                             die "$(eval_gettext "Cannot move HEAD to \$first_parent")"
>> +                        die "$(eval_gettext "Cannot move HEAD to \$first_parent")"
>>               fi
>
> Ditto.

Will be leaving for now

>
>> @@ -553,7 +568,7 @@ do_next () {
>>       pick|p)
>>               comment_for_reflog pick
>>
>> -             mark_action_done
>> +             mark_action_done $sha1 "$rest"
>
> This demands more attention from the readers than all the changes we
> have seen so far which were just line wrapping and whitespace
> changes.  That is why it is better to leave these changes to a
> separate patch after preliminary clean-up.  It allows reviewers'
> eyes coast over the clean-up step, and then lets them focus on the
> more "meaningful" changes  like this one.
>
>> @@ -637,7 +652,7 @@ you are able to reword the commit.")"
>>               read -r command rest < "$todo"
>>               mark_action_done
>>               eval_gettextln "Executing: \$rest"
>> -             "${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
>> +             "${SHELL:-/bin/sh}" -c "$rest" # Actual execution
> Why?  This change is not justified in the proposed log message.

Left over from testing, will be removed.

>

Junio, thanks for the review.

As you suggest in v3 I'm going to break the change into several commits
but leave everything in one file as it is here in v2. If desired we can break
it into several files as a last step.

I'm currently contemplating not touching long lines. In this file long lines
were acceptable in the past and rather than introduce many changes to
fix them I think it might be best to leave them. Also, there is a proposal
that a GSOC might convert one or more of the git-rebase--* files into C,
in which case these files could be removed.

But if that doesn't come to past, I believe my goal of simplication and fixing:
  "not ok 24 - exchange two commits with -p # TODO known breakage"
In t3404-rebase-interactive.sh is worth while.

Re: [RFC PATCH v2 1/1] rebase-interactive: Add git_rebase__interactive__preserve_merges

[Junio C Hamano]

Wink Saville <wink@saville.com> writes:

>> Good code simplification that turns
>>
>>         if A
>>                 if B
>>                         do this thing
>>                 fi
>>         fi
>>
>> into
>>
>>         if A & B
>>                 do this thing
>>         fi
>
> Will be keeping this in a future commit

I think this one is simple enough to be in the "prelim clean-up"
change, so that you can reduce the size of the remaining stuff that
really needs focused review.

> But if that doesn't come to past, I believe my goal of simplication and fixing:
>   "not ok 24 - exchange two commits with -p # TODO known breakage"
> In t3404-rebase-interactive.sh is worth while.

Oh, thanks for working on this.