Skip to contentSkip to conversation

rebase-interactive

RFC PATCH Superseded Wink Saville 3 participants 14 messages Mar 20, 2018 – Mar 21, 2018 Kbuild

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

cover rebase-interactive

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
Signature · 1 line
2.16.2
#1 lore raw

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.

---
 .gitignore                      |   1 +
 Makefile                        |   1 +
 git-rebase--interactive--lib.sh | 944 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 946 insertions(+)
 create mode 100644 git-rebase--interactive--lib.sh
.gitignore +1 -0
index 833ef3b0b..4ea246780 100644
@@ -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
Makefile +1 -0
index de4b8f0c0..f13540da6 100644
@@ -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
git-rebase--interactive--lib.sh new file +944 -0

944 lines not on this page · raw message

Signature · 1 line
2.16.2
#2 lore raw

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

In reply to Wink Saville

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

---
 .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
.gitignore +1 -0
index 4ea246780..c57a6b563 100644
@@ -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
Makefile +1 -0
index f13540da6..543e0a659 100644
@@ -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
git-rebase--interactive--preserve-merges.sh new file +134 -0

134 lines not on this page · raw message

git-rebase.sh +6 -1
index a1f6e5de6..3c2fc35f7 100755
@@ -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
Signature · 1 line
2.16.2
#3 lore raw

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

In reply to Wink Saville

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

---
 git-rebase--interactive.sh | 1019 +-------------------------------------------
 1 file changed, 19 insertions(+), 1000 deletions(-)
git-rebase--interactive.sh +19 -1000
index 331c8dfea..3d2b89e1c 100644
@@ -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/[email protected]/
-#
-# 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
Signature · 1 line
2.16.2
#4 lore raw
In reply to Wink Saville
Quoted 19 lines · Wink Saville

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.

Quoted 4 lines

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.

#5 lore raw
Quoted 49 lines · Eric Sunshine
Quoted 19 lines · Wink Saville

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.

Quoted 4 lines

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

Quoted 1 line

Thanks.

Must all intermediate commits continue build and pass tests?

#6 lore raw
Quoted 25 lines · Wink Saville
Quoted 13 lines · Eric Sunshine

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.

Quoted 10 lines

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

Quoted 1 line

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.

#7 lore raw
Quoted 26 lines · Eric Sunshine
Quoted 25 lines · Wink Saville
Quoted 23 lines · Eric Sunshine

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

Quoted 4 lines
Quoted 1 line

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?

#8 lore raw

{cc:+junio}

Quoted 5 lines · Wink Saville

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.

Quoted 6 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).

Quoted 1 line

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.

#9 lore raw

v2 cover 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(-)
Signature · 1 line
2.16.2
#10 lore raw

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(-)
git-rebase--interactive.sh +259 -210
index 331c8dfea..65ff75654 100644
@@ -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/[email protected]/
 #
+
 # 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
git-rebase.sh +14 -2
index a1f6e5de6..b32282f4c 100755
@@ -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
Signature · 1 line
2.16.2
#11 lore raw

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

Quoted 7 lines · Wink Saville

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

Changed run_specific_rebase in git-rebase to dispatch to the appropriate function after sourcing git-rebase--interactive.
---

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.

git-rebase--interactive.sh line 1

Quoted 8 lines
git-rebase--interactive.sh +1 -2
index 331c8dfea..65ff75654 100644
@@ -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).

git-rebase--interactive.sh line 13

Quoted 12 lines
git-rebase--interactive.sh +4 -0
+# 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/[email protected]/
 #
+
 # The file containing rebase commands, comments, and empty lines.

Why?

git-rebase--interactive.sh pick_one () { · line 294

Quoted 7 lines
git-rebase--interactive.sh +2 -1
@@ -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.

git-rebase--interactive.sh pick_one () { · line 308

Quoted 8 lines
git-rebase--interactive.sh +2 -2
@@ -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.

Quoted 14 lines
git-rebase--interactive.sh +1 -8
@@ -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 ||

git-rebase--interactive.sh pick_one_preserving_merges () { · line 336

Quoted 10 lines
git-rebase--interactive.sh +7 -2
-				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

git-rebase--interactive.sh pick_one_preserving_merges () { · line 358

Quoted 12 lines
git-rebase--interactive.sh +4 -4
@@ -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.

git-rebase--interactive.sh pick_one_preserving_merges () { · line 398

Quoted 7 lines
git-rebase--interactive.sh +1 -1
@@ -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.

git-rebase--interactive.sh do_next () { · line 571

Quoted 6 lines
git-rebase--interactive.sh +1 -1
@@ -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.

git-rebase--interactive.sh you are able to reword the commit.")" · line 655

Quoted 6 lines
git-rebase--interactive.sh +1 -1
@@ -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.

#12 lore raw

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

Quoted 6 lines · Junio C Hamano
Quoted 4 lines · Wink Saville

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

Quoted 8 lines
Quoted 3 lines

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

Quoted 14 lines

git-rebase--interactive.sh line 1

Quoted 9 lines
git-rebase--interactive.sh +1 -2
index 331c8dfea..65ff75654 100644
@@ -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

Quoted 6 lines
Quoted 6 lines
git-rebase--interactive.sh +3 -0
+# 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
Quoted 8 lines

git-rebase--interactive.sh line 13

Quoted 6 lines
git-rebase--interactive.sh +1 -0
@@ -7,6 +9,7 @@
 # The original idea comes from Eric W. Biederman, in
 # https://public-inbox.org/git/[email protected]/
 #
+
 # The file containing rebase commands, comments, and empty lines.

Why?

Will remove the blank line.

Quoted 9 lines

git-rebase--interactive.sh pick_one () { · line 294

Quoted 7 lines
git-rebase--interactive.sh +2 -1
@@ -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

Quoted 11 lines

git-rebase--interactive.sh pick_one () { · line 308

Quoted 9 lines
git-rebase--interactive.sh +2 -2
@@ -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

Quoted 34 lines
Quoted 15 lines
git-rebase--interactive.sh +1 -8
@@ -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 ||

git-rebase--interactive.sh pick_one_preserving_merges () { · line 336

Quoted 10 lines
git-rebase--interactive.sh +7 -2
-                             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

Quoted 5 lines

into

if A & B
do this thing
fi

Will be keeping this in a future commit

Quoted 15 lines

git-rebase--interactive.sh pick_one_preserving_merges () { · line 358

Quoted 13 lines
git-rebase--interactive.sh +4 -4
@@ -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

Quoted 10 lines

git-rebase--interactive.sh pick_one_preserving_merges () { · line 398

Quoted 8 lines
git-rebase--interactive.sh +1 -1
@@ -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

Quoted 15 lines

git-rebase--interactive.sh do_next () { · line 571

Quoted 7 lines
git-rebase--interactive.sh +1 -1
@@ -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.

Quoted 7 lines

git-rebase--interactive.sh you are able to reword the commit.")" · line 655

Quoted 6 lines
git-rebase--interactive.sh +1 -1
@@ -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.

Quoted 1 line

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.

#13 lore raw

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

Quoted 8 lines · Wink Saville
Quoted 8 lines

Good code simplification that turns

if A
if B
do this thing
fi
fi

Quoted 7 lines
Quoted 5 lines

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.

Quoted 3 lines

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.