threads / rfc / 48102

RFC patch, 3 partsrebase-interactive

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

## tl;dr

14 messages between Mar 20, 2018 and Mar 21, 2018. Diffs are folded; open one to read it.

replies: 13people: 3as markdown or json

Wink Saville· Mar 20, 2018, 20:45 UTC · lore

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
Wink Saville· Mar 20, 2018, 20:45 UTC · re: Wink Saville · lore

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

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
Show changes to 3 files +946 −0

.gitignore, Makefile, 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
Wink Saville· Mar 20, 2018, 20:45 UTC · re: Wink Saville · lore

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

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
Show changes to 4 files +142 −1

.gitignore, Makefile, git-rebase--interactive--preserve-merges.sh, git-rebase.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
Wink Saville· Mar 20, 2018, 20:45 UTC · re: Wink Saville · lore

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

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(-)
Show changes to git-rebase--interactive.sh +19 −1000
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
Eric Sunshine· Mar 20, 2018, 21:11 UTC · re: Wink Saville · lore

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

On Tue, Mar 20, 2018 at 4:45 PM, Wink Saville <wink@saville.com> wrote:
Show 23 quoted lines
> 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.
Wink Saville· Mar 20, 2018, 21:23 UTC · re: Eric Sunshine · lore

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

On Tue, Mar 20, 2018 at 2:11 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 50 quoted lines
> 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?
Eric Sunshine· Mar 20, 2018, 21:47 UTC · re: Wink Saville · lore

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

On Tue, Mar 20, 2018 at 5:23 PM, Wink Saville <wink@saville.com> wrote:
Show 26 quoted lines
> 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.

Wink Saville· Mar 21, 2018, 03:31 UTC · re: Eric Sunshine · lore

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

On Tue, Mar 20, 2018 at 2:47 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 30 quoted lines
> 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?
Eric Sunshine· Mar 21, 2018, 08:54 UTC · re: Wink Saville · lore

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

{cc:+junio}
On Tue, Mar 20, 2018 at 11:31 PM, Wink Saville <wink@saville.com> wrote:
Show 5 quoted lines
> 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.

Show 6 quoted lines
> 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.

Wink Saville· Mar 21, 2018, 17:44 UTC · re: Eric Sunshine · lore

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

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
Wink Saville· Mar 21, 2018, 17:44 UTC · re: Wink Saville · lore

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

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(-)
Show changes to 2 files +273 −212

git-rebase--interactive.sh, git-rebase.sh

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
Junio C Hamano· Mar 21, 2018, 19:42 UTC · re: Wink Saville · lore

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

Wink Saville <wink@saville.com> writes:
Show 7 quoted lines
> 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.

Show 8 quoted lines
> 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).

Show 12 quoted lines
> +# 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?
Show 7 quoted lines
> @@ -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.
Show 8 quoted lines
> @@ -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.
Show 24 quoted lines
> @@ -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
Show 12 quoted lines
> @@ -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.
Show 7 quoted lines
> @@ -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.
Show 6 quoted lines
> @@ -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.

Show 6 quoted lines
> @@ -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.
Wink Saville· Mar 21, 2018, 21:49 UTC · re: Junio C Hamano · lore

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

On Wed, Mar 21, 2018 at 12:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> 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
Show 14 quoted lines
>
>> 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
Show 14 quoted lines
>> +# 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.
Show 9 quoted lines
>> @@ -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
Show 11 quoted lines
>
>> @@ -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
Show 39 quoted lines
>
>> @@ -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
Show 15 quoted lines
>
>> @@ -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
Show 10 quoted lines
>
>> @@ -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
Show 22 quoted lines
>
>> @@ -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.
Junio C Hamano· Mar 21, 2018, 22:43 UTC · re: Wink Saville · lore

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

Wink Saville <wink@saville.com> writes:
Show 15 quoted lines
>> 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.  

← back to recent threads