threads / patch / 48795

patch, 5 partsrebase: rewrite rebase in C

Subject: [GSoC] [PATCH 0/5] rebase: rewrite rebase in C

## tl;dr

61 messages between Jun 28, 2018 and Aug 16, 2018. Diffs are folded; open one to read it.

replies: 60people: 9as markdown or json

Pratik Karki· Jun 28, 2018, 07:46 UTC · lore
As a GSoC project, I have been working on the builtin rebase.

The motivation behind the rewrite of rebase i.e. from shell script to C are for following reasons:

1.  Writing shell scripts and getting it to production is much faster
    than doing the equivalent in C but lacks in performance and extra
    workarounds are needed for non-POSIX platforms.
2.  Git for Windows is at loss as the installer size increases due to
    addition of extra dependencies for the shell scripts which are usually
    available in POSIX compliant platforms.

This series of patches serves to demonstrate a minimal builtin rebase which supports running `git rebase <upstream>` and also serves to ask for reviews.

Pratik Karki (5):
  Start TODO-rebase.sh
  rebase: start implementing it as a builtin
  rebase: refactor common shell functions into their own file
  sequencer: refactor the code to detach HEAD to checkout.c
  builtin/rebase: support running "git rebase <upstream>"
 .gitignore                      |   2 +
 Makefile                        |   4 +-
 git-rebase.sh => TODO-rebase.sh |   0
 builtin.h                       |   1 +
 builtin/rebase.c                | 282 +++++++++++++
 checkout.c                      |  64 +++
 checkout.h                      |   3 +
 git-legacy-rebase.sh            | 678 ++++++++++++++++++++++++++++++++
 git-rebase--common.sh           |  61 +++
 git.c                           |   6 +
 sequencer.c                     |  58 +--
 11 files changed, 1105 insertions(+), 54 deletions(-)
 rename git-rebase.sh => TODO-rebase.sh (100%)
 create mode 100644 builtin/rebase.c
 create mode 100755 git-legacy-rebase.sh
 create mode 100644 git-rebase--common.sh
-- 
2.18.0
Pratik Karki· Jun 28, 2018, 07:46 UTC · re: Pratik Karki · lore

[PATCH 1/5] Start TODO-rebase.sh

This is a verbatim copy of `git-rebase.sh`. This acts as a tracker for components to be converted and to find the progress of current conversion of `git-rebase.sh` to `builtin/rebase.c`. The commented parts denote the converted parts.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 TODO-rebase.sh | 738 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 738 insertions(+)
 create mode 100755 TODO-rebase.sh
Show changes to TODO-rebase.sh +738 −0
diff --git a/TODO-rebase.sh b/TODO-rebase.sh
new file mode 100755
index 000000000..19bdebb48
--- /dev/null
+++ b/TODO-rebase.sh
@@ -0,0 +1,738 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano.
+#
+
+SUBDIRECTORY_OK=Yes
+OPTIONS_KEEPDASHDASH=
+OPTIONS_STUCKLONG=t
+OPTIONS_SPEC="\
+git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] [<upstream>] [<branch>]
+git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] --root [<branch>]
+git rebase --continue | --abort | --skip | --edit-todo
+--
+ Available options are
+v,verbose!         display a diffstat of what changed upstream
+q,quiet!           be quiet. implies --no-stat
+autostash          automatically stash/stash pop before and after
+fork-point         use 'merge-base --fork-point' to refine upstream
+onto=!             rebase onto given branch instead of upstream
+r,rebase-merges?   try to rebase merges instead of skipping them
+p,preserve-merges! try to recreate merges instead of ignoring them
+s,strategy=!       use the given merge strategy
+no-ff!             cherry-pick all commits, even if unchanged
+m,merge!           use merging strategies to rebase
+i,interactive!     let the user edit the list of commits to rebase
+x,exec=!           add exec lines after each commit of the editable list
+k,keep-empty	   preserve empty commits during rebase
+allow-empty-message allow rebasing commits with empty messages
+f,force-rebase!    force rebase even if branch is up to date
+X,strategy-option=! pass the argument through to the merge strategy
+stat!              display a diffstat of what changed upstream
+n,no-stat!         do not show diffstat of what changed upstream
+verify             allow pre-rebase hook to run
+rerere-autoupdate  allow rerere to update index with resolved conflicts
+root!              rebase all reachable commits up to the root(s)
+autosquash         move commits that begin with squash!/fixup! under -i
+committer-date-is-author-date! passed to 'git am'
+ignore-date!       passed to 'git am'
+signoff            passed to 'git am'
+whitespace=!       passed to 'git apply'
+ignore-whitespace! passed to 'git apply'
+C=!                passed to 'git apply'
+S,gpg-sign?        GPG-sign commits
+ Actions:
+continue!          continue
+abort!             abort and check out the original branch
+skip!              skip current patch and continue
+edit-todo!         edit the todo list during an interactive rebase
+quit!              abort but keep HEAD where it is
+show-current-patch! show the patch file being applied or merged
+"
+. git-sh-setup
+set_reflog_action rebase
+require_work_tree_exists
+cd_to_toplevel
+
+LF='
+'
+ok_to_skip_pre_rebase=
+resolvemsg="
+$(gettext 'Resolve all conflicts manually, mark them as resolved with
+"git add/rm <conflicted_files>", then run "git rebase --continue".
+You can instead skip this commit: run "git rebase --skip".
+To abort and get back to the state before "git rebase", run "git rebase --abort".')
+"
+squash_onto=
+unset onto
+unset restrict_revision
+cmd=
+strategy=
+strategy_opts=
+do_merge=
+merge_dir="$GIT_DIR"/rebase-merge
+apply_dir="$GIT_DIR"/rebase-apply
+verbose=
+diffstat=
+test "$(git config --bool rebase.stat)" = true && diffstat=t
+autostash="$(git config --bool rebase.autostash || echo false)"
+fork_point=auto
+git_am_opt=
+git_format_patch_opt=
+rebase_root=
+force_rebase=
+allow_rerere_autoupdate=
+# Non-empty if a rebase was in progress when 'git rebase' was invoked
+in_progress=
+# One of {am, merge, interactive}
+type=
+# One of {"$GIT_DIR"/rebase-apply, "$GIT_DIR"/rebase-merge}
+state_dir=
+# One of {'', continue, skip, abort}, as parsed from command line
+action=
+rebase_merges=
+rebase_cousins=
+preserve_merges=
+autosquash=
+keep_empty=
+allow_empty_message=
+signoff=
+test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
+case "$(git config --bool commit.gpgsign)" in
+true)	gpg_sign_opt=-S ;;
+*)	gpg_sign_opt= ;;
+esac
+
+read_basic_state () {
+	test -f "$state_dir/head-name" &&
+	test -f "$state_dir/onto" &&
+	head_name=$(cat "$state_dir"/head-name) &&
+	onto=$(cat "$state_dir"/onto) &&
+	# We always write to orig-head, but interactive rebase used to write to
+	# head. Fall back to reading from head to cover for the case that the
+	# user upgraded git with an ongoing interactive rebase.
+	if test -f "$state_dir"/orig-head
+	then
+		orig_head=$(cat "$state_dir"/orig-head)
+	else
+		orig_head=$(cat "$state_dir"/head)
+	fi &&
+	GIT_QUIET=$(cat "$state_dir"/quiet) &&
+	test -f "$state_dir"/verbose && verbose=t
+	test -f "$state_dir"/strategy && strategy="$(cat "$state_dir"/strategy)"
+	test -f "$state_dir"/strategy_opts &&
+		strategy_opts="$(cat "$state_dir"/strategy_opts)"
+	test -f "$state_dir"/allow_rerere_autoupdate &&
+		allow_rerere_autoupdate="$(cat "$state_dir"/allow_rerere_autoupdate)"
+	test -f "$state_dir"/gpg_sign_opt &&
+		gpg_sign_opt="$(cat "$state_dir"/gpg_sign_opt)"
+	test -f "$state_dir"/signoff && {
+		signoff="$(cat "$state_dir"/signoff)"
+		force_rebase=t
+	}
+}
+
+write_basic_state () {
+	echo "$head_name" > "$state_dir"/head-name &&
+	echo "$onto" > "$state_dir"/onto &&
+	echo "$orig_head" > "$state_dir"/orig-head &&
+	echo "$GIT_QUIET" > "$state_dir"/quiet &&
+	test t = "$verbose" && : > "$state_dir"/verbose
+	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
+	test -n "$strategy_opts" && echo "$strategy_opts" > \
+		"$state_dir"/strategy_opts
+	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
+		"$state_dir"/allow_rerere_autoupdate
+	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
+	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
+}
+
+output () {
+	case "$verbose" in
+	'')
+		output=$("$@" 2>&1 )
+		status=$?
+		test $status != 0 && printf "%s\n" "$output"
+		return $status
+		;;
+	*)
+		"$@"
+		;;
+	esac
+}
+
+move_to_original_branch () {
+	case "$head_name" in
+	refs/*)
+		message="rebase finished: $head_name onto $onto"
+		git update-ref -m "$message" \
+			$head_name $(git rev-parse HEAD) $orig_head &&
+		git symbolic-ref \
+			-m "rebase finished: returning to $head_name" \
+			HEAD $head_name ||
+		die "$(eval_gettext "Could not move back to \$head_name")"
+		;;
+	esac
+}
+
+apply_autostash () {
+	if test -f "$state_dir/autostash"
+	then
+		stash_sha1=$(cat "$state_dir/autostash")
+		if git stash apply $stash_sha1 >/dev/null 2>&1
+		then
+			echo "$(gettext 'Applied autostash.')" >&2
+		else
+			git stash store -m "autostash" -q $stash_sha1 ||
+			die "$(eval_gettext "Cannot store \$stash_sha1")"
+			gettext 'Applying autostash resulted in conflicts.
+Your changes are safe in the stash.
+You can run "git stash pop" or "git stash drop" at any time.
+' >&2
+		fi
+	fi
+}
+
+finish_rebase () {
+	rm -f "$(git rev-parse --git-path REBASE_HEAD)"
+	apply_autostash &&
+	{ git gc --auto || true; } &&
+	rm -rf "$state_dir"
+}
+
+run_specific_rebase () {
+	if [ "$interactive_rebase" = implied ]; then
+		GIT_EDITOR=:
+		export GIT_EDITOR
+		autosquash=
+	fi
+	. git-rebase--$type
+
+	if test -z "$preserve_merges"
+	then
+		git_rebase__$type
+	else
+		git_rebase__preserve_merges
+	fi
+
+	ret=$?
+	if test $ret -eq 0
+	then
+		finish_rebase
+	elif test $ret -eq 2 # special exit status for rebase -i
+	then
+		apply_autostash &&
+		rm -rf "$state_dir" &&
+		die "Nothing to do"
+	fi
+	exit $ret
+}
+
+run_pre_rebase_hook () {
+	if test -z "$ok_to_skip_pre_rebase" &&
+	   test -x "$(git rev-parse --git-path hooks/pre-rebase)"
+	then
+		"$(git rev-parse --git-path hooks/pre-rebase)" ${1+"$@"} ||
+		die "$(gettext "The pre-rebase hook refused to rebase.")"
+	fi
+}
+
+test -f "$apply_dir"/applying &&
+	die "$(gettext "It looks like 'git am' is in progress. Cannot rebase.")"
+
+if test -d "$apply_dir"
+then
+	type=am
+	state_dir="$apply_dir"
+elif test -d "$merge_dir"
+then
+	if test -d "$merge_dir"/rewritten
+	then
+		type=preserve-merges
+		interactive_rebase=explicit
+		preserve_merges=t
+	elif test -f "$merge_dir"/interactive
+	then
+		type=interactive
+		interactive_rebase=explicit
+	else
+		type=merge
+	fi
+	state_dir="$merge_dir"
+fi
+test -n "$type" && in_progress=t
+
+total_argc=$#
+while test $# != 0
+do
+	case "$1" in
+	--no-verify)
+		ok_to_skip_pre_rebase=yes
+		;;
+	--verify)
+		ok_to_skip_pre_rebase=
+		;;
+	--continue|--skip|--abort|--quit|--edit-todo|--show-current-patch)
+		test $total_argc -eq 2 || usage
+		action=${1##--}
+		;;
+	--onto=*)
+		onto="${1#--onto=}"
+		;;
+	--exec=*)
+		cmd="${cmd}exec ${1#--exec=}${LF}"
+		test -z "$interactive_rebase" && interactive_rebase=implied
+		;;
+	--interactive)
+		interactive_rebase=explicit
+		;;
+	--keep-empty)
+		keep_empty=yes
+		;;
+	--allow-empty-message)
+		allow_empty_message=--allow-empty-message
+		;;
+	--no-keep-empty)
+		keep_empty=
+		;;
+	--rebase-merges)
+		rebase_merges=t
+		test -z "$interactive_rebase" && interactive_rebase=implied
+		;;
+	--rebase-merges=*)
+		rebase_merges=t
+		case "${1#*=}" in
+		rebase-cousins) rebase_cousins=t;;
+		no-rebase-cousins) rebase_cousins=;;
+		*) die "Unknown mode: $1";;
+		esac
+		test -z "$interactive_rebase" && interactive_rebase=implied
+		;;
+	--preserve-merges)
+		preserve_merges=t
+		test -z "$interactive_rebase" && interactive_rebase=implied
+		;;
+	--autosquash)
+		autosquash=t
+		;;
+	--no-autosquash)
+		autosquash=
+		;;
+	--fork-point)
+		fork_point=t
+		;;
+	--no-fork-point)
+		fork_point=
+		;;
+	--merge)
+		do_merge=t
+		;;
+	--strategy-option=*)
+		strategy_opts="$strategy_opts $(git rev-parse --sq-quote "--${1#--strategy-option=}")"
+		do_merge=t
+		test -z "$strategy" && strategy=recursive
+		;;
+	--strategy=*)
+		strategy="${1#--strategy=}"
+		do_merge=t
+		;;
+	--no-stat)
+		diffstat=
+		;;
+	--stat)
+		diffstat=t
+		;;
+	--autostash)
+		autostash=true
+		;;
+	--no-autostash)
+		autostash=false
+		;;
+	--verbose)
+		verbose=t
+		diffstat=t
+		GIT_QUIET=
+		;;
+	--quiet)
+		GIT_QUIET=t
+		git_am_opt="$git_am_opt -q"
+		verbose=
+		diffstat=
+		;;
+	--whitespace=*)
+		git_am_opt="$git_am_opt --whitespace=${1#--whitespace=}"
+		case "${1#--whitespace=}" in
+		fix|strip)
+			force_rebase=t
+			;;
+		esac
+		;;
+	--ignore-whitespace)
+		git_am_opt="$git_am_opt $1"
+		;;
+	--signoff)
+		signoff=--signoff
+		;;
+	--no-signoff)
+		signoff=
+		;;
+	--committer-date-is-author-date|--ignore-date)
+		git_am_opt="$git_am_opt $1"
+		force_rebase=t
+		;;
+	-C*)
+		git_am_opt="$git_am_opt $1"
+		;;
+	--root)
+		rebase_root=t
+		;;
+	--force-rebase|--no-ff)
+		force_rebase=t
+		;;
+	--rerere-autoupdate|--no-rerere-autoupdate)
+		allow_rerere_autoupdate="$1"
+		;;
+	--gpg-sign)
+		gpg_sign_opt=-S
+		;;
+	--gpg-sign=*)
+		gpg_sign_opt="-S${1#--gpg-sign=}"
+		;;
+	--)
+		shift
+		break
+		;;
+	*)
+		usage
+		;;
+	esac
+	shift
+done
+test $# -gt 2 && usage
+
+if test -n "$action"
+then
+	test -z "$in_progress" && die "$(gettext "No rebase in progress?")"
+	# Only interactive rebase uses detailed reflog messages
+	if test -n "$interactive_rebase" && test "$GIT_REFLOG_ACTION" = rebase
+	then
+		GIT_REFLOG_ACTION="rebase -i ($action)"
+		export GIT_REFLOG_ACTION
+	fi
+fi
+
+if test "$action" = "edit-todo" && test -z "$interactive_rebase"
+then
+	die "$(gettext "The --edit-todo action can only be used during interactive rebase.")"
+fi
+
+case "$action" in
+continue)
+	# Sanity check
+	git rev-parse --verify HEAD >/dev/null ||
+		die "$(gettext "Cannot read HEAD")"
+	git update-index --ignore-submodules --refresh &&
+	git diff-files --quiet --ignore-submodules || {
+		echo "$(gettext "You must edit all merge conflicts and then
+mark them as resolved using git add")"
+		exit 1
+	}
+	read_basic_state
+	run_specific_rebase
+	;;
+skip)
+	output git reset --hard HEAD || exit $?
+	read_basic_state
+	run_specific_rebase
+	;;
+abort)
+	git rerere clear
+	read_basic_state
+	case "$head_name" in
+	refs/*)
+		git symbolic-ref -m "rebase: aborting" HEAD $head_name ||
+		die "$(eval_gettext "Could not move back to \$head_name")"
+		;;
+	esac
+	output git reset --hard $orig_head
+	finish_rebase
+	exit
+	;;
+quit)
+	exec rm -rf "$state_dir"
+	;;
+edit-todo)
+	run_specific_rebase
+	;;
+show-current-patch)
+	run_specific_rebase
+	die "BUG: run_specific_rebase is not supposed to return here"
+	;;
+esac
+
+# Make sure no rebase is in progress
+if test -n "$in_progress"
+then
+	state_dir_base=${state_dir##*/}
+	cmd_live_rebase="git rebase (--continue | --abort | --skip)"
+	cmd_clear_stale_rebase="rm -fr \"$state_dir\""
+	die "
+$(eval_gettext 'It seems that there is already a $state_dir_base directory, and
+I wonder if you are in the middle of another rebase.  If that is the
+case, please try
+	$cmd_live_rebase
+If that is not the case, please
+	$cmd_clear_stale_rebase
+and run me again.  I am stopping in case you still have something
+valuable there.')"
+fi
+
+if test -n "$rebase_root" && test -z "$onto"
+then
+	test -z "$interactive_rebase" && interactive_rebase=implied
+fi
+
+if test -n "$keep_empty"
+then
+	test -z "$interactive_rebase" && interactive_rebase=implied
+fi
+
+if test -n "$interactive_rebase"
+then
+	if test -z "$preserve_merges"
+	then
+		type=interactive
+	else
+		type=preserve-merges
+	fi
+
+	state_dir="$merge_dir"
+elif test -n "$do_merge"
+then
+	type=merge
+	state_dir="$merge_dir"
+else
+	type=am
+	state_dir="$apply_dir"
+fi
+
+if test -t 2 && test -z "$GIT_QUIET"
+then
+	git_format_patch_opt="$git_format_patch_opt --progress"
+fi
+
+if test -n "$signoff"
+then
+	test -n "$preserve_merges" &&
+		die "$(gettext "error: cannot combine '--signoff' with '--preserve-merges'")"
+	git_am_opt="$git_am_opt $signoff"
+	force_rebase=t
+fi
+
+if test -z "$rebase_root"
+then
+	case "$#" in
+	0)
+		if ! upstream_name=$(git rev-parse --symbolic-full-name \
+			--verify -q @{upstream} 2>/dev/null)
+		then
+			. git-parse-remote
+			error_on_missing_default_upstream "rebase" "rebase" \
+				"against" "git rebase $(gettext '<branch>')"
+		fi
+
+		test "$fork_point" = auto && fork_point=t
+		;;
+	*)	upstream_name="$1"
+		if test "$upstream_name" = "-"
+		then
+			upstream_name="@{-1}"
+		fi
+		shift
+		;;
+	esac
+	upstream=$(peel_committish "${upstream_name}") ||
+	die "$(eval_gettext "invalid upstream '\$upstream_name'")"
+	upstream_arg="$upstream_name"
+else
+	if test -z "$onto"
+	then
+		empty_tree=$(git hash-object -t tree /dev/null)
+		onto=$(git commit-tree $empty_tree </dev/null)
+		squash_onto="$onto"
+	fi
+	unset upstream_name
+	unset upstream
+	test $# -gt 1 && usage
+	upstream_arg=--root
+fi
+
+# Make sure the branch to rebase onto is valid.
+onto_name=${onto-"$upstream_name"}
+case "$onto_name" in
+*...*)
+	if	left=${onto_name%...*} right=${onto_name#*...} &&
+		onto=$(git merge-base --all ${left:-HEAD} ${right:-HEAD})
+	then
+		case "$onto" in
+		?*"$LF"?*)
+			die "$(eval_gettext "\$onto_name: there are more than one merge bases")"
+			;;
+		'')
+			die "$(eval_gettext "\$onto_name: there is no merge base")"
+			;;
+		esac
+	else
+		die "$(eval_gettext "\$onto_name: there is no merge base")"
+	fi
+	;;
+*)
+	onto=$(peel_committish "$onto_name") ||
+	die "$(eval_gettext "Does not point to a valid commit: \$onto_name")"
+	;;
+esac
+
+# If the branch to rebase is given, that is the branch we will rebase
+# $branch_name -- branch/commit being rebased, or HEAD (already detached)
+# $orig_head -- commit object name of tip of the branch before rebasing
+# $head_name -- refs/heads/<that-branch> or "detached HEAD"
+switch_to=
+case "$#" in
+1)
+	# Is it "rebase other $branchname" or "rebase other $commit"?
+	branch_name="$1"
+	switch_to="$1"
+
+	# Is it a local branch?
+	if git show-ref --verify --quiet -- "refs/heads/$branch_name" &&
+	   orig_head=$(git rev-parse -q --verify "refs/heads/$branch_name")
+	then
+		head_name="refs/heads/$branch_name"
+	# If not is it a valid ref (branch or commit)?
+	elif orig_head=$(git rev-parse -q --verify "$branch_name")
+	then
+		head_name="detached HEAD"
+
+	else
+		die "$(eval_gettext "fatal: no such branch/commit '\$branch_name'")"
+	fi
+	;;
+0)
+	# Do not need to switch branches, we are already on it.
+	if branch_name=$(git symbolic-ref -q HEAD)
+	then
+		head_name=$branch_name
+		branch_name=$(expr "z$branch_name" : 'zrefs/heads/\(.*\)')
+	else
+		head_name="detached HEAD"
+		branch_name=HEAD
+	fi
+	orig_head=$(git rev-parse --verify HEAD) || exit
+	;;
+*)
+	die "BUG: unexpected number of arguments left to parse"
+	;;
+esac
+
+if test "$fork_point" = t
+then
+	new_upstream=$(git merge-base --fork-point "$upstream_name" \
+			"${switch_to:-HEAD}")
+	if test -n "$new_upstream"
+	then
+		restrict_revision=$new_upstream
+	fi
+fi
+
+if test "$autostash" = true && ! (require_clean_work_tree) 2>/dev/null
+then
+	stash_sha1=$(git stash create "autostash") ||
+	die "$(gettext 'Cannot autostash')"
+
+	mkdir -p "$state_dir" &&
+	echo $stash_sha1 >"$state_dir/autostash" &&
+	stash_abbrev=$(git rev-parse --short $stash_sha1) &&
+	echo "$(eval_gettext 'Created autostash: $stash_abbrev')" &&
+	git reset --hard
+fi
+
+require_clean_work_tree "rebase" "$(gettext "Please commit or stash them.")"
+
+# Now we are rebasing commits $upstream..$orig_head (or with --root,
+# everything leading up to $orig_head) on top of $onto
+
+# Check if we are already based on $onto with linear history,
+# but this should be done only when upstream and onto are the same
+# and if this is not an interactive rebase.
+mb=$(git merge-base "$onto" "$orig_head")
+if test -z "$interactive_rebase" && test "$upstream" = "$onto" &&
+	test "$mb" = "$onto" && test -z "$restrict_revision" &&
+	# linear history?
+	! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > /dev/null
+then
+	if test -z "$force_rebase"
+	then
+		# Lazily switch to the target branch if needed...
+		test -z "$switch_to" ||
+		GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" \
+			git checkout -q "$switch_to" --
+		if test "$branch_name" = "HEAD" &&
+			 ! git symbolic-ref -q HEAD
+		then
+			say "$(eval_gettext "HEAD is up to date.")"
+		else
+			say "$(eval_gettext "Current branch \$branch_name is up to date.")"
+		fi
+		finish_rebase
+		exit 0
+	else
+		if test "$branch_name" = "HEAD" &&
+			 ! git symbolic-ref -q HEAD
+		then
+			say "$(eval_gettext "HEAD is up to date, rebase forced.")"
+		else
+			say "$(eval_gettext "Current branch \$branch_name is up to date, rebase forced.")"
+		fi
+	fi
+fi
+
+# If a hook exists, give it a chance to interrupt
+run_pre_rebase_hook "$upstream_arg" "$@"
+
+if test -n "$diffstat"
+then
+	if test -n "$verbose"
+	then
+		echo "$(eval_gettext "Changes from \$mb to \$onto:")"
+	fi
+	# We want color (if set), but no pager
+	GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
+fi
+
+test -n "$interactive_rebase" && run_specific_rebase
+
+# Detach HEAD and reset the tree
+say "$(gettext "First, rewinding head to replay your work on top of it...")"
+
+GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
+	git checkout -q "$onto^0" || die "could not detach HEAD"
+git update-ref ORIG_HEAD $orig_head
+
+# If the $onto is a proper descendant of the tip of the branch, then
+# we just fast-forwarded.
+if test "$mb" = "$orig_head"
+then
+	say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
+	move_to_original_branch
+	finish_rebase
+	exit 0
+fi
+
+if test -n "$rebase_root"
+then
+	revisions="$onto..$orig_head"
+else
+	revisions="${restrict_revision-$upstream}..$orig_head"
+fi
+
+run_specific_rebase
-- 
2.18.0
Pratik Karki· Jun 28, 2018, 09:17 UTC · re: Pratik Karki · lore

Re: [PATCH 1/5] Start TODO-rebase.sh

Hi,

This is a patch I keep in wip-rebase branch[1][2]. It shouldn't be applied to `pu`.

This main objective of this file in the branch is to keep track of the converted shell scripts of `git-rebase.sh` and plan on the conversion of other remaining parts.

[1]: https://github.com/prertik/git/tree/wip-rebase [2]: https://github.com/git/git/pull/505

Thanks, Pratik

Pratik Karki· Jun 28, 2018, 07:46 UTC · re: Pratik Karki · lore

[PATCH 2/5] rebase: start implementing it as a builtin

This commit imitates the strategy that was used to convert the difftool to a builtin, see be8a90e (difftool: add a skeleton for the upcoming builtin, 2017-01-17) for details: This commit renames the shell script `git-rebase.sh` to `git-legacy-rebase.sh` and hands off to it by default.

The current version of the builtin rebase does not, however, make full use of the internals but instead chooses to spawn a couple of Git processes to find out if we run the builtin or legacy rebase as that keeps the directory that we are in correct. There remains a lot of room for improvement, left for a later date. The following commits will recreate the functionality of the shell script, in pure C.

We intentionally avoid reading the config directly to avoid messing up the GIT_* environment variables when we need to fall back to exec()ing the shell script. The test of builtin rebase can be done by `git -c rebase.useBuiltin=true rebase ...`

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 .gitignore                            |  1 +
 Makefile                              |  3 +-
 builtin.h                             |  1 +
 builtin/rebase.c                      | 55 +++++++++++++++++++++++++++
 git-rebase.sh => git-legacy-rebase.sh |  0
 git.c                                 |  6 +++
 6 files changed, 65 insertions(+), 1 deletion(-)
 create mode 100644 builtin/rebase.c
 rename git-rebase.sh => git-legacy-rebase.sh (100%)
Show changes to 6 files +65 −1

.gitignore, Makefile, builtin.h, builtin/rebase.c, git-rebase.sh, git.c

diff --git a/.gitignore b/.gitignore
index 3284a1e9b..ec2395901 100644
--- a/.gitignore
+++ b/.gitignore
@@ -78,6 +78,7 @@
 /git-init-db
 /git-interpret-trailers
 /git-instaweb
+/git-legacy-rebase
 /git-log
 /git-ls-files
 /git-ls-remote
diff --git a/Makefile b/Makefile
index 0cb6590f2..e88fe2e5f 100644
--- a/Makefile
+++ b/Makefile
@@ -609,7 +609,7 @@ SCRIPT_SH += git-merge-one-file.sh
 SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
 SCRIPT_SH += git-quiltimport.sh
-SCRIPT_SH += git-rebase.sh
+SCRIPT_SH += git-legacy-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
 SCRIPT_SH += git-request-pull.sh
 SCRIPT_SH += git-stash.sh
@@ -1059,6 +1059,7 @@ BUILTIN_OBJS += builtin/prune.o
 BUILTIN_OBJS += builtin/pull.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase.o
 BUILTIN_OBJS += builtin/rebase--helper.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
diff --git a/builtin.h b/builtin.h
index 0362f1ce2..44651a447 100644
--- a/builtin.h
+++ b/builtin.h
@@ -202,6 +202,7 @@ extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
 extern int cmd_pull(int argc, const char **argv, const char *prefix);
 extern int cmd_push(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase(int argc, const char **argv, const char *prefix);
 extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase.c b/builtin/rebase.c
new file mode 100644
index 000000000..1152b7229
--- /dev/null
+++ b/builtin/rebase.c
@@ -0,0 +1,55 @@
+/*
+ * "git rebase" builtin command
+ *
+ * Copyright (c) 2018 Pratik Karki
+ */
+
+#include "builtin.h"
+#include "run-command.h"
+#include "exec-cmd.h"
+#include "argv-array.h"
+#include "dir.h"
+
+static int use_builtin_rebase(void)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf out = STRBUF_INIT;
+	int ret;
+
+	argv_array_pushl(&cp.args,
+			 "config", "--bool", "rebase.usebuiltin", NULL);
+	cp.git_cmd = 1;
+	if (capture_command(&cp, &out, 6))
+		return 0;
+
+	strbuf_trim(&out);
+	ret = !strcmp("true", out.buf);
+	strbuf_release(&out);
+	return ret;
+}
+
+int cmd_rebase(int argc, const char **argv, const char *prefix)
+{
+	/*
+	* NEEDSWORK: Once the builtin rebase has been tested enough
+	* and git-legacy-rebase.sh is retired to contrib/, this preamble
+	* can be removed.
+	*/
+	if (!use_builtin_rebase()) {
+		const char *path = mkpath("%s/git-legacy-rebase",
+					  git_exec_path());
+
+		if (sane_execvp(path, (char **)argv) < 0)
+			die_errno("could not exec %s", path);
+
+		return 0;
+	}
+
+	if (argc != 2)
+		die("Usage: %s <base>", argv[0]);
+	prefix = setup_git_directory();
+	trace_repo_setup(prefix);
+	setup_work_tree();
+
+	die("TODO");
+}
\ No newline at end of file
diff --git a/git-rebase.sh b/git-legacy-rebase.sh
similarity index 100%
rename from git-rebase.sh
rename to git-legacy-rebase.sh
diff --git a/git.c b/git.c
index 9dbe6ffaa..bacfefb2d 100644
--- a/git.c
+++ b/git.c
@@ -518,6 +518,12 @@ static struct cmd_struct commands[] = {
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
 	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
+	/*
+	 *NEEDSWORK: Until the rebase is independent and needs no redirection
+	 to rebase shell script this is kept as is, then should be changed to
+	 RUN_SETUP | NEED_WORK_TREE
+	*/
+	{ "rebase", cmd_rebase },
 	{ "rebase--helper", cmd_rebase__helper, RUN_SETUP | NEED_WORK_TREE },
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
-- 
2.18.0
Christian Couder· Jun 28, 2018, 08:08 UTC · re: Pratik Karki · lore

Re: [PATCH 2/5] rebase: start implementing it as a builtin

On Thu, Jun 28, 2018 at 9:46 AM, Pratik Karki <predatoramigo@gmail.com> wrote:
Show 11 quoted lines
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> new file mode 100644
> index 000000000..1152b7229
> --- /dev/null
> +++ b/builtin/rebase.c
> @@ -0,0 +1,55 @@
> +/*
> + * "git rebase" builtin command
> + *
> + * Copyright (c) 2018 Pratik Karki
> + */
[...]
> +       die("TODO");
> +}
> \ No newline at end of file
Please add a newline at the end of the files you create.
Stefan Beller· Jun 28, 2018, 18:49 UTC · re: Pratik Karki · lore

Re: [PATCH 2/5] rebase: start implementing it as a builtin

On Thu, Jun 28, 2018 at 12:48 AM Pratik Karki <predatoramigo@gmail.com> wrote:
Show 6 quoted lines
>
> This commit imitates the strategy that was used to convert the
> difftool to a builtin, see be8a90e (difftool: add a skeleton for the
> upcoming builtin, 2017-01-17) for details: This commit renames the
> shell script `git-rebase.sh` to `git-legacy-rebase.sh` and hands off to
> it by default.

That is a good way to start, imitating Johannes approach on rewriting the difftool. Thanks for pointing this out.

Show 10 quoted lines
> The current version of the builtin rebase does not, however, make full
> use of the internals but instead chooses to spawn a couple of Git
> processes to find out if we run the builtin or legacy rebase as that
> keeps the directory that we are in correct. There remains a lot
> of room for improvement, left for a later date. The following commits
> will recreate the functionality of the shell script, in pure C.
>
> We intentionally avoid reading the config directly to avoid
> messing up the GIT_* environment variables when we need to fall back to
> exec()ing the shell script.
Thanks for calling this out!
The test of builtin rebase can be done by
Show 85 quoted lines
> `git -c rebase.useBuiltin=true rebase ...`
>
> Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> ---
>  .gitignore                            |  1 +
>  Makefile                              |  3 +-
>  builtin.h                             |  1 +
>  builtin/rebase.c                      | 55 +++++++++++++++++++++++++++
>  git-rebase.sh => git-legacy-rebase.sh |  0
>  git.c                                 |  6 +++
>  6 files changed, 65 insertions(+), 1 deletion(-)
>  create mode 100644 builtin/rebase.c
>  rename git-rebase.sh => git-legacy-rebase.sh (100%)
>
> diff --git a/.gitignore b/.gitignore
> index 3284a1e9b..ec2395901 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -78,6 +78,7 @@
>  /git-init-db
>  /git-interpret-trailers
>  /git-instaweb
> +/git-legacy-rebase
>  /git-log
>  /git-ls-files
>  /git-ls-remote
> diff --git a/Makefile b/Makefile
> index 0cb6590f2..e88fe2e5f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -609,7 +609,7 @@ SCRIPT_SH += git-merge-one-file.sh
>  SCRIPT_SH += git-merge-resolve.sh
>  SCRIPT_SH += git-mergetool.sh
>  SCRIPT_SH += git-quiltimport.sh
> -SCRIPT_SH += git-rebase.sh
> +SCRIPT_SH += git-legacy-rebase.sh
>  SCRIPT_SH += git-remote-testgit.sh
>  SCRIPT_SH += git-request-pull.sh
>  SCRIPT_SH += git-stash.sh
> @@ -1059,6 +1059,7 @@ BUILTIN_OBJS += builtin/prune.o
>  BUILTIN_OBJS += builtin/pull.o
>  BUILTIN_OBJS += builtin/push.o
>  BUILTIN_OBJS += builtin/read-tree.o
> +BUILTIN_OBJS += builtin/rebase.o
>  BUILTIN_OBJS += builtin/rebase--helper.o
>  BUILTIN_OBJS += builtin/receive-pack.o
>  BUILTIN_OBJS += builtin/reflog.o
> diff --git a/builtin.h b/builtin.h
> index 0362f1ce2..44651a447 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -202,6 +202,7 @@ extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
>  extern int cmd_pull(int argc, const char **argv, const char *prefix);
>  extern int cmd_push(int argc, const char **argv, const char *prefix);
>  extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
> +extern int cmd_rebase(int argc, const char **argv, const char *prefix);
>  extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
>  extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
>  extern int cmd_reflog(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> new file mode 100644
> index 000000000..1152b7229
> --- /dev/null
> +++ b/builtin/rebase.c
> @@ -0,0 +1,55 @@
> +/*
> + * "git rebase" builtin command
> + *
> + * Copyright (c) 2018 Pratik Karki
> + */
> +
> +#include "builtin.h"
> +#include "run-command.h"
> +#include "exec-cmd.h"
> +#include "argv-array.h"
> +#include "dir.h"
> +
> +static int use_builtin_rebase(void)
> +{
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +       struct strbuf out = STRBUF_INIT;
> +       int ret;
> +
> +       argv_array_pushl(&cp.args,
> +                        "config", "--bool", "rebase.usebuiltin", NULL);

--bool is documented as "Historical options for selecting a type specifier. Prefer instead --type, (see: above)." in the man page of git-config. But as this code will go away once the conversion is done, this is not kept around for long. So we should be fine using the --bool option.

Show 6 quoted lines
> +       cp.git_cmd = 1;
> +       if (capture_command(&cp, &out, 6))
> +               return 0;
> +
> +       strbuf_trim(&out);
> +       ret = !strcmp("true", out.buf);

As --bool will make sure that the config command prints "true" or "false", even when the user configured 0 or 1 instead, this is fine.

Show 7 quoted lines
> +       if (argc != 2)
> +               die("Usage: %s <base>", argv[0]);
> +       prefix = setup_git_directory();
> +       trace_repo_setup(prefix);
> +       setup_work_tree();
> +
> +       die("TODO");

When reading the last sentence in the commit message ("This can be tested ...") I shortly wondered how we adapt the tests but as this is really just the skeleton, there is no need to adapt any tests.

This patch looks fine to me except for the nit that Christian points out.

Thanks! Stefan

Pratik Karki· Jun 28, 2018, 07:46 UTC · re: Pratik Karki · lore

[PATCH 3/5] rebase: refactor common shell functions into their own file

The function present in `git-legacy-rebase.sh` are used by backends so, this refactor tries to extract the functions out so that, the `git-legacy-rebase.sh` can be retired easily as the `git-rebase--common.sh` will provide the functions for now.

The motivation behind this is to call the backend functions *directly* from C, bypassing `git-rebase.sh`. Therefore those functions need to live in a separate file: we need to be able to call `.git-rebase--common` in that script snippet so that those functions are defined.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 .gitignore            |  1 +
 Makefile              |  1 +
 git-legacy-rebase.sh  | 62 +------------------------------------------
 git-rebase--common.sh | 61 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 64 insertions(+), 61 deletions(-)
 create mode 100644 git-rebase--common.sh
Show changes to 4 files +64 −61

.gitignore, Makefile, git-legacy-rebase.sh, git-rebase--common.sh

diff --git a/.gitignore b/.gitignore
index ec2395901..824141cba 100644
--- a/.gitignore
+++ b/.gitignore
@@ -117,6 +117,7 @@
 /git-read-tree
 /git-rebase
 /git-rebase--am
+/git-rebase--common
 /git-rebase--helper
 /git-rebase--interactive
 /git-rebase--merge
diff --git a/Makefile b/Makefile
index e88fe2e5f..163e52ad1 100644
--- a/Makefile
+++ b/Makefile
@@ -619,6 +619,7 @@ SCRIPT_SH += git-web--browse.sh
 SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
+SCRIPT_LIB += git-rebase--common
 SCRIPT_LIB += git-rebase--interactive
 SCRIPT_LIB += git-rebase--preserve-merges
 SCRIPT_LIB += git-rebase--merge
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 19bdebb48..240aac507 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -102,6 +102,7 @@ case "$(git config --bool commit.gpgsign)" in
 true)	gpg_sign_opt=-S ;;
 *)	gpg_sign_opt= ;;
 esac
+. git-rebase--common
 
 read_basic_state () {
 	test -f "$state_dir/head-name" &&
@@ -132,67 +133,6 @@ read_basic_state () {
 	}
 }
 
-write_basic_state () {
-	echo "$head_name" > "$state_dir"/head-name &&
-	echo "$onto" > "$state_dir"/onto &&
-	echo "$orig_head" > "$state_dir"/orig-head &&
-	echo "$GIT_QUIET" > "$state_dir"/quiet &&
-	test t = "$verbose" && : > "$state_dir"/verbose
-	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
-	test -n "$strategy_opts" && echo "$strategy_opts" > \
-		"$state_dir"/strategy_opts
-	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
-		"$state_dir"/allow_rerere_autoupdate
-	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
-	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
-}
-
-output () {
-	case "$verbose" in
-	'')
-		output=$("$@" 2>&1 )
-		status=$?
-		test $status != 0 && printf "%s\n" "$output"
-		return $status
-		;;
-	*)
-		"$@"
-		;;
-	esac
-}
-
-move_to_original_branch () {
-	case "$head_name" in
-	refs/*)
-		message="rebase finished: $head_name onto $onto"
-		git update-ref -m "$message" \
-			$head_name $(git rev-parse HEAD) $orig_head &&
-		git symbolic-ref \
-			-m "rebase finished: returning to $head_name" \
-			HEAD $head_name ||
-		die "$(eval_gettext "Could not move back to \$head_name")"
-		;;
-	esac
-}
-
-apply_autostash () {
-	if test -f "$state_dir/autostash"
-	then
-		stash_sha1=$(cat "$state_dir/autostash")
-		if git stash apply $stash_sha1 >/dev/null 2>&1
-		then
-			echo "$(gettext 'Applied autostash.')" >&2
-		else
-			git stash store -m "autostash" -q $stash_sha1 ||
-			die "$(eval_gettext "Cannot store \$stash_sha1")"
-			gettext 'Applying autostash resulted in conflicts.
-Your changes are safe in the stash.
-You can run "git stash pop" or "git stash drop" at any time.
-' >&2
-		fi
-	fi
-}
-
 finish_rebase () {
 	rm -f "$(git rev-parse --git-path REBASE_HEAD)"
 	apply_autostash &&
diff --git a/git-rebase--common.sh b/git-rebase--common.sh
new file mode 100644
index 000000000..5ba060392
--- /dev/null
+++ b/git-rebase--common.sh
@@ -0,0 +1,61 @@
+
+write_basic_state () {
+	echo "$head_name" > "$state_dir"/head-name &&
+	echo "$onto" > "$state_dir"/onto &&
+	echo "$orig_head" > "$state_dir"/orig-head &&
+	echo "$GIT_QUIET" > "$state_dir"/quiet &&
+	test t = "$verbose" && : > "$state_dir"/verbose
+	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
+	test -n "$strategy_opts" && echo "$strategy_opts" > \
+		"$state_dir"/strategy_opts
+	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
+		"$state_dir"/allow_rerere_autoupdate
+	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
+	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
+}
+
+apply_autostash () {
+	if test -f "$state_dir/autostash"
+	then
+		stash_sha1=$(cat "$state_dir/autostash")
+		if git stash apply $stash_sha1 >/dev/null 2>&1
+		then
+			echo "$(gettext 'Applied autostash.')" >&2
+		else
+			git stash store -m "autostash" -q $stash_sha1 ||
+			die "$(eval_gettext "Cannot store \$stash_sha1")"
+			gettext 'Applying autostash resulted in conflicts.
+Your changes are safe in the stash.
+You can run "git stash pop" or "git stash drop" at any time.
+' >&2
+		fi
+	fi
+}
+
+move_to_original_branch () {
+	case "$head_name" in
+	refs/*)
+		message="rebase finished: $head_name onto $onto"
+		git update-ref -m "$message" \
+			$head_name $(git rev-parse HEAD) $orig_head &&
+		git symbolic-ref \
+			-m "rebase finished: returning to $head_name" \
+			HEAD $head_name ||
+		die "$(eval_gettext "Could not move back to \$head_name")"
+		;;
+	esac
+}
+
+output () {
+	case "$verbose" in
+	'')
+		output=$("$@" 2>&1 )
+		status=$?
+		test $status != 0 && printf "%s\n" "$output"
+		return $status
+		;;
+	*)
+		"$@"
+		;;
+	esac
+}
-- 
2.18.0
Christian Couder· Jun 28, 2018, 08:02 UTC · re: Pratik Karki · lore

Re: [PATCH 3/5] rebase: refactor common shell functions into their own file

On Thu, Jun 28, 2018 at 9:46 AM, Pratik Karki <predatoramigo@gmail.com> wrote:
> The motivation behind this is to call the backend functions
> *directly* from C, bypassing `git-rebase.sh`. Therefore those functions
> need to live in a separate file: we need to be able to call
> `.git-rebase--common` in that script snippet so that those functions

I think it should be `. git-rebase--common` (space missing between . and git-rebase--common).

> are defined.
Stefan Beller· Jun 28, 2018, 19:17 UTC · re: Pratik Karki · lore

Re: [PATCH 3/5] rebase: refactor common shell functions into their own file

On Thu, Jun 28, 2018 at 12:48 AM Pratik Karki <predatoramigo@gmail.com> wrote:
>
> The function present in `git-legacy-rebase.sh` are used by backends
> so, this refactor tries to extract the functions out so that, the
it not only tries to, it actually does. :)
Show 8 quoted lines
> `git-legacy-rebase.sh` can be retired easily as the
> `git-rebase--common.sh` will provide the functions for now.
>
> The motivation behind this is to call the backend functions
> *directly* from C, bypassing `git-rebase.sh`. Therefore those functions
> need to live in a separate file: we need to be able to call
> `.git-rebase--common` in that script snippet so that those functions
> are defined.
Makes sense.

I applied the patch (and checked the move via the --color-moved option to see if there are discrepancies that slip in easily via rebases as there is more work currently going on in the rebase area) and the found the functions were moved as-is, just reordered. Can you give a hint on why you choose a different order for the moved functions (not as an email reply, but as part of the commit message, later on people may ask the same question only to find this commit via git-blame or git-log for example)

Thanks, Stefan

Pratik Karki· Jun 28, 2018, 07:46 UTC · re: Pratik Karki · lore

[PATCH 4/5] sequencer: refactor the code to detach HEAD to checkout.c

The motivation behind this commit is to extract the core part of do_reset() from sequencer.c and move it to a new detach_head_to() function in checkout.c.

Here the index only gets locked after performing the first part of `do_reset()` rather than before which essentially derives the `oid` from the specified label/name passed to the `do_reset()` function. It also fixes two bugs: there were two `return error()` statements in the `[new root]` case that would have failed to unlock the index.

The new function will be used in the next commit by the builtin rebase, to perform the initial checkout.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 checkout.c  | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 checkout.h  |  3 +++
 sequencer.c | 58 +++++-------------------------------------------
 3 files changed, 72 insertions(+), 53 deletions(-)
Show changes to 3 files +72 −53

checkout.c, checkout.h, sequencer.c

diff --git a/checkout.c b/checkout.c
index bdefc888b..da68915fd 100644
--- a/checkout.c
+++ b/checkout.c
@@ -2,6 +2,11 @@
 #include "remote.h"
 #include "refspec.h"
 #include "checkout.h"
+#include "unpack-trees.h"
+#include "lockfile.h"
+#include "refs.h"
+#include "tree.h"
+#include "cache-tree.h"
 
 struct tracking_name_data {
 	/* const */ char *src_ref;
@@ -42,3 +47,62 @@ const char *unique_tracking_name(const char *name, struct object_id *oid)
 	free(cb_data.dst_ref);
 	return NULL;
 }
+
+int detach_head_to(struct object_id *oid, const char *action,
+		   const char *reflog_message)
+{
+	struct strbuf ref_name = STRBUF_INIT;
+	struct tree_desc desc;
+	struct lock_file lock = LOCK_INIT;
+	struct unpack_trees_options unpack_tree_opts;
+	struct tree *tree;
+	int ret = 0;
+
+	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
+		return -1;
+
+	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
+	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
+	unpack_tree_opts.head_idx = 1;
+	unpack_tree_opts.src_index = &the_index;
+	unpack_tree_opts.dst_index = &the_index;
+	unpack_tree_opts.fn = oneway_merge;
+	unpack_tree_opts.merge = 1;
+	unpack_tree_opts.update = 1;
+
+	if (read_cache_unmerged()) {
+		rollback_lock_file(&lock);
+		strbuf_release(&ref_name);
+		return error_resolve_conflict(_(action));
+	}
+
+	if (!fill_tree_descriptor(&desc, oid)) {
+		error(_("failed to find tree of %s"), oid_to_hex(oid));
+		rollback_lock_file(&lock);
+		free((void *)desc.buffer);
+		strbuf_release(&ref_name);
+		return -1;
+	}
+
+	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
+		rollback_lock_file(&lock);
+		free((void *)desc.buffer);
+		strbuf_release(&ref_name);
+		return -1;
+	}
+
+	tree = parse_tree_indirect(oid);
+	prime_cache_tree(&the_index, tree);
+
+	if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
+		ret = error(_("could not write index"));
+	free((void *)desc.buffer);
+
+	if (!ret)
+		ret = update_ref(reflog_message, "HEAD", oid,
+				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+
+	strbuf_release(&ref_name);
+	return ret;
+
+}
diff --git a/checkout.h b/checkout.h
index 998071117..6ce46cf4e 100644
--- a/checkout.h
+++ b/checkout.h
@@ -10,4 +10,7 @@
  */
 extern const char *unique_tracking_name(const char *name, struct object_id *oid);
 
+int detach_head_to(struct object_id *oid, const char *action,
+		   const char *reflog_message);
+
 #endif /* CHECKOUT_H */
diff --git a/sequencer.c b/sequencer.c
index 5354d4d51..106d518f4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -29,6 +29,7 @@
 #include "oidset.h"
 #include "commit-slab.h"
 #include "alias.h"
+#include "checkout.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -2756,14 +2757,7 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
 {
 	struct strbuf ref_name = STRBUF_INIT;
 	struct object_id oid;
-	struct lock_file lock = LOCK_INIT;
-	struct tree_desc desc;
-	struct tree *tree;
-	struct unpack_trees_options unpack_tree_opts;
-	int ret = 0, i;
-
-	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
-		return -1;
+	int i;
 
 	if (len == 10 && !strncmp("[new root]", name, len)) {
 		if (!opts->have_squash_onto) {
@@ -2789,56 +2783,14 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
 		if (get_oid(ref_name.buf, &oid) &&
 		    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
 			error(_("could not read '%s'"), ref_name.buf);
-			rollback_lock_file(&lock);
 			strbuf_release(&ref_name);
 			return -1;
 		}
 	}
 
-	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
-	setup_unpack_trees_porcelain(&unpack_tree_opts, "reset");
-	unpack_tree_opts.head_idx = 1;
-	unpack_tree_opts.src_index = &the_index;
-	unpack_tree_opts.dst_index = &the_index;
-	unpack_tree_opts.fn = oneway_merge;
-	unpack_tree_opts.merge = 1;
-	unpack_tree_opts.update = 1;
-
-	if (read_cache_unmerged()) {
-		rollback_lock_file(&lock);
-		strbuf_release(&ref_name);
-		return error_resolve_conflict(_(action_name(opts)));
-	}
-
-	if (!fill_tree_descriptor(&desc, &oid)) {
-		error(_("failed to find tree of %s"), oid_to_hex(&oid));
-		rollback_lock_file(&lock);
-		free((void *)desc.buffer);
-		strbuf_release(&ref_name);
-		return -1;
-	}
-
-	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
-		rollback_lock_file(&lock);
-		free((void *)desc.buffer);
-		strbuf_release(&ref_name);
-		return -1;
-	}
-
-	tree = parse_tree_indirect(&oid);
-	prime_cache_tree(&the_index, tree);
-
-	if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
-		ret = error(_("could not write index"));
-	free((void *)desc.buffer);
-
-	if (!ret)
-		ret = update_ref(reflog_message(opts, "reset", "'%.*s'",
-						len, name), "HEAD", &oid,
-				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
-
-	strbuf_release(&ref_name);
-	return ret;
+	return detach_head_to(&oid, action_name(opts),
+			      reflog_message(opts, "reset", "'%.*s'",
+					     len, name));
 }
 
 static int do_merge(struct commit *commit, const char *arg, int arg_len,
-- 
2.18.0
Christian Couder· Jun 28, 2018, 08:14 UTC · re: Pratik Karki · lore

Re: [PATCH 4/5] sequencer: refactor the code to detach HEAD to checkout.c

On Thu, Jun 28, 2018 at 9:46 AM, Pratik Karki <predatoramigo@gmail.com> wrote:
> The motivation behind this commit is to extract the core part of
> do_reset() from sequencer.c and move it to a new detach_head_to()
> function in checkout.c.

If this is independent from your other patches and if this can be used by Alban's work, then it might be a good idea to send this patch separately (and then to state in this patch series that it depends on the separate patch) or at least to move this patch to the beginning of the patch series.

Stefan Beller· Jun 28, 2018, 21:19 UTC · re: Pratik Karki · lore

Re: [PATCH 4/5] sequencer: refactor the code to detach HEAD to checkout.c

Hi Pratik, On Thu, Jun 28, 2018 at 12:48 AM Pratik Karki <predatoramigo@gmail.com> wrote:

Show 5 quoted lines
>
> The motivation behind this commit is to extract the core part of
> do_reset() from sequencer.c and move it to a new detach_head_to()
> function in checkout.c.
>
[...]
>
> The new function will be used in the next commit by the builtin rebase,
> to perform the initial checkout.
This sounds like the actual motivation, which is fine.
Show 5 quoted lines
> Here the index only gets locked after performing the first part of
> `do_reset()` rather than before which essentially derives the `oid`
> from the specified label/name passed to the `do_reset()` function.
> It also fixes two bugs: there were two `return error()` statements in
> the `[new root]` case that would have failed to unlock the index.

This sounds as if this fixes a problem? If so it would be nice to have a test that demonstrates that these specific problems go away. (but I think we could just argue based on the motivation above that this is a good change on its own, with or without demonstrating these additional issues)

Show 61 quoted lines
> Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> ---
>  checkout.c  | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  checkout.h  |  3 +++
>  sequencer.c | 58 +++++-------------------------------------------
>  3 files changed, 72 insertions(+), 53 deletions(-)
>
> diff --git a/checkout.c b/checkout.c
> index bdefc888b..da68915fd 100644
> --- a/checkout.c
> +++ b/checkout.c
> @@ -2,6 +2,11 @@
>  #include "remote.h"
>  #include "refspec.h"
>  #include "checkout.h"
> +#include "unpack-trees.h"
> +#include "lockfile.h"
> +#include "refs.h"
> +#include "tree.h"
> +#include "cache-tree.h"
>
>  struct tracking_name_data {
>         /* const */ char *src_ref;
> @@ -42,3 +47,62 @@ const char *unique_tracking_name(const char *name, struct object_id *oid)
>         free(cb_data.dst_ref);
>         return NULL;
>  }
> +
> +int detach_head_to(struct object_id *oid, const char *action,
> +                  const char *reflog_message)
> +{
> +       struct strbuf ref_name = STRBUF_INIT;
> +       struct tree_desc desc;
> +       struct lock_file lock = LOCK_INIT;
> +       struct unpack_trees_options unpack_tree_opts;
> +       struct tree *tree;
> +       int ret = 0;
> +
> +       if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> +               return -1;
> +
> +       memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
> +       setup_unpack_trees_porcelain(&unpack_tree_opts, action);
> +       unpack_tree_opts.head_idx = 1;
> +       unpack_tree_opts.src_index = &the_index;
> +       unpack_tree_opts.dst_index = &the_index;
> +       unpack_tree_opts.fn = oneway_merge;
> +       unpack_tree_opts.merge = 1;
> +       unpack_tree_opts.update = 1;
> +
> +       if (read_cache_unmerged()) {
> +               rollback_lock_file(&lock);
> +               strbuf_release(&ref_name);
> +               return error_resolve_conflict(_(action));
> +       }
> +
> +       if (!fill_tree_descriptor(&desc, oid)) {
> +               error(_("failed to find tree of %s"), oid_to_hex(oid));
> +               rollback_lock_file(&lock);
> +               free((void *)desc.buffer);
> +               strbuf_release(&ref_name);

These lines are repeated as a very similar pattern after each failing function. Maybe we can make it more readable by moving all these to the end and then using goto to jump there.

For example see "write_pseudoref" in refs.c, that has some interesting patterns to learn from, e.g. how the return code is constructed (start off with setting it -1 and only if we go through the whole function, just before the jump label, we'd set it to 0) and how all the free/strbuf_releases are at the end (no need to repeat them).

Show 11 quoted lines
> +               return -1;
> +       }
> +
> +       if (unpack_trees(1, &desc, &unpack_tree_opts)) {
> +               rollback_lock_file(&lock);
> +               free((void *)desc.buffer);
> +               strbuf_release(&ref_name);
> +               return -1;
> +       }
> +
> +       tree = parse_tree_indirect(oid);
Awesome, the _indirect function can take commits/tags or trees.
> +       prime_cache_tree(&the_index, tree);

As there is a larger movement to get rid of globals, and the_index is one of them[1]. So maybe just use the_repository->index already (the_repository suffers a similar problem, but I think that is more futureproof for the time being as we'd want to kill off the_repository in library code eventually as well and pass through a repository struct. But for now I'd just use the_repository instead of having a repository argument)

[1] c.f. https://public-inbox.org/git/20180616054157.32433-1-pclouds@gmail.com/
Show 5 quoted lines
> -       struct lock_file lock = LOCK_INIT;
> -       struct tree_desc desc;
> -       struct tree *tree;
> -       struct unpack_trees_options unpack_tree_opts;
> -       int ret = 0, i;
[...]

Oh I misspoke above, this is moving code (I should have understood the hint with 'extracting' by the commit message), so in this case we'd rather want to move code most verbatim to make review easier, which it is. So the idea with a goto cleanup could be an optional extra step.

Thanks, Stefan

Pratik Karki· Jun 28, 2018, 07:46 UTC · re: Pratik Karki · lore

[PATCH 5/5] builtin/rebase: support running "git rebase <upstream>"

This patch gives life to the skeleton added in the previous patch. This patch makes real operation happen i.e. by using `git -c rebase.usebuiltin=true rebase <upstream>`. With this patch, the basic operation of rebase can be done.

The current version of the builtin rebase does not, however, make full use of the internals but instead chooses to spawn a couple of Git processes, still, to make for an easier conversion. There remains a lot of room for improvement, left later.

These backends use Unix shell functions defined both by git-sh-setup.sh and git-rebase.sh (we move the latter's into git-rebase--common.sh to accommodate for that), so we not only have to source the backend file before calling the respective Unix shell script function, but we have to source git-sh-setup and git-rebase--common before that. And since this is all done in a Unix shell script snippet, all of this is in argv[0]. There never will be a non-NULL argv[1].

This patch does the *bare* minimum to get `git rebase <upstream>` to work: there is still no option parsing, and only the bare minimum set of environment variables are set (in particular, the current revision would be susceptible to bugs where e.g. `rebase_root` could be set by mistake before running `git rebase` and the `git-rebase--am` backend would pick up that variable and use it).

It still calls original `git-legacy-rebase.sh` unless the config setting rebase.useBuiltin is set to true. This patch uses the detach_head_to() function from checkout.c introduced by a previous commit to perform initial checkout.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 builtin/rebase.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 229 insertions(+), 2 deletions(-)
Show changes to builtin/rebase.c +229 −2
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1152b7229..2f90389c2 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -9,6 +9,19 @@
 #include "exec-cmd.h"
 #include "argv-array.h"
 #include "dir.h"
+#include "packfile.h"
+#include "checkout.h"
+#include "refs.h"
+
+static GIT_PATH_FUNC(apply_dir, "rebase-apply");
+static GIT_PATH_FUNC(merge_dir, "rebase-merge");
+
+enum rebase_type {
+	REBASE_AM,
+	REBASE_MERGE,
+	REBASE_INTERACTIVE,
+	REBASE_PRESERVE_MERGES
+};
 
 static int use_builtin_rebase(void)
 {
@@ -28,8 +41,129 @@ static int use_builtin_rebase(void)
 	return ret;
 }
 
+static int apply_autostash(void)
+{
+	warning("TODO");
+	return 0;
+}
+
+struct rebase_options {
+	enum rebase_type type;
+	const char *state_dir;
+	struct commit *upstream;
+	const char *upstream_name;
+	char *head_name;
+	struct object_id orig_head;
+	struct commit *onto;
+	const char *onto_name;
+	const char *revisions;
+	const char *root;
+};
+
+static int finish_rebase(struct rebase_options *opts)
+{
+	struct strbuf dir = STRBUF_INIT;
+	const char *argv_gc_auto[] = { "gc", "--auto", NULL };
+
+	delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
+	apply_autostash();
+	close_all_packs(the_repository->objects);
+	/*
+	 * We ignore errors in 'gc --auto', since the
+	 * user should see them.
+	 */
+	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
+	strbuf_addstr(&dir, opts->state_dir);
+	remove_dir_recursively(&dir, 0);
+	strbuf_release(&dir);
+
+	return 0;
+}
+
+static struct commit *peel_committish(const char *name)
+{
+	struct object *obj;
+	struct object_id oid;
+	if (get_oid(name, &oid))
+		return NULL;
+	obj = parse_object(&oid);
+	return (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
+}
+
+static int run_specific_rebase(struct rebase_options *opts)
+{
+	const char *argv[] = { NULL, NULL };
+	struct strbuf script_snippet = STRBUF_INIT;
+	struct argv_array env = ARGV_ARRAY_INIT;
+	int status;
+	const char *backend, *backend_func;
+
+	argv_array_pushf(&env, "upstream_name=%s", opts->upstream_name);
+	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
+	argv_array_pushf(&env, "upstream=%s",
+				oid_to_hex(&opts->upstream->object.oid));
+	argv_array_pushf(&env, "orig_head=%s", oid_to_hex(&opts->orig_head));
+	argv_array_pushf(&env, "onto=%s",
+				oid_to_hex(&opts->onto->object.oid));
+	argv_array_pushf(&env, "onto_name=%s", opts->onto_name);
+	argv_array_pushf(&env, "revisions=%s", opts->revisions);
+
+	switch (opts->type) {
+		case REBASE_AM:
+			backend = "git-rebase--am";
+			backend_func = "git_rebase__am";
+			break;
+		case REBASE_INTERACTIVE:
+			backend = "git-rebase--interactive";
+			backend_func = "git_rebase__interactive";
+			break;
+		case REBASE_MERGE:
+			backend = "git-rebase--merge";
+			backend_func = "git_rebase__merge";
+			break;
+		case REBASE_PRESERVE_MERGES:
+			backend = "git-rebase--preserve-merges";
+			backend_func = "git_rebase__preserve_merges";
+			break;
+		default:
+			BUG("Unhandled rebase type %d", opts->type);
+			break;
+	}
+
+	strbuf_addf(&script_snippet,
+		    ". git-rebase--common && . %s && %s",
+		    backend, backend_func);
+	argv[0] = script_snippet.buf;
+
+	status = run_command_v_opt_cd_env(argv, RUN_USING_SHELL, NULL,
+					  env.argv);
+	if (status == 0)
+		finish_rebase(opts);
+	else if (status == 2) {
+		struct strbuf dir = STRBUF_INIT;
+
+		apply_autostash();
+		strbuf_addstr(&dir, opts->state_dir);
+		remove_dir_recursively(&dir, 0);
+		strbuf_release(&dir);
+		die("Nothing to do");
+	}
+
+	strbuf_release(&script_snippet);
+	argv_array_clear(&env);
+
+	return status ? -1 : 0;
+}
+
 int cmd_rebase(int argc, const char **argv, const char *prefix)
 {
+	struct rebase_options options = { -1 };
+	const char *branch_name;
+	int ret, flags, quiet = 0;
+	struct strbuf msg = STRBUF_INIT;
+	struct strbuf revisions = STRBUF_INIT;
+	const char *restrict_revision = NULL;
+
 	/*
 	* NEEDSWORK: Once the builtin rebase has been tested enough
 	* and git-legacy-rebase.sh is retired to contrib/, this preamble
@@ -51,5 +185,98 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	trace_repo_setup(prefix);
 	setup_work_tree();
 
-	die("TODO");
-}
\ No newline at end of file
+	options.type = REBASE_AM;
+
+	switch (options.type) {
+	case REBASE_AM:
+		options.state_dir = apply_dir();
+		break;
+	case REBASE_MERGE:
+	case REBASE_INTERACTIVE:
+	case REBASE_PRESERVE_MERGES:
+		options.state_dir = merge_dir();
+		break;
+	}
+	if (!options.root) {
+		if(!argc)
+			die("TODO: handle @{upstream}");
+		else {
+			options.upstream_name = argv[1];
+			argc--;
+			argv++;
+			if (!strcmp(options.upstream_name, "-"))
+				options.upstream_name = "@{-1}";
+		}
+		options.upstream = peel_committish(options.upstream_name);
+		if (!options.upstream)
+			die(_("invalid upstream '%s'"), options.upstream_name);
+	} else
+		die("TODO: upstream for --root");
+
+	/* Make sure the branch to rebase onto is valid. */
+	options.onto_name = (const char *) options.onto ?
+		(const char *) options.onto : options.upstream_name;
+	if (strstr(options.onto_name, "...")) {
+		die("TODO");
+	} else {
+		options.onto = peel_committish(options.onto_name);
+		if (!options.onto)
+			die(_("Does not point to a valid commit '%s'"),
+				options.onto_name);
+	}
+
+	/*
+	* If the branch to rebase is given, that is the branch we will rebase
+	* branch_name -- branch/commit being rebased, or HEAD (already detached)
+	* orig_head -- commit object name of tip of the branch before rebasing
+	* head_name -- refs/heads/<that-branch> or "detached HEAD"
+	*/
+	if (argc > 1)
+		 die ("TODO: handle switch_to");
+	else {
+		/* Do not need to switch branches, we are already on it. */
+		options.head_name =
+			xstrdup_or_null(resolve_ref_unsafe("HEAD", 0, NULL,
+					 &flags));
+		if (!options.head_name)
+			die(_("No such ref: %s"), "HEAD");
+		if (flags & REF_ISSYMREF) {
+			if (!skip_prefix(options.head_name,
+					 "refs/heads/", &branch_name))
+				branch_name = options.head_name;
+
+		} else {
+			options.head_name = xstrdup("detached HEAD");
+			branch_name = "HEAD";
+		}
+		if (get_oid("HEAD", &options.orig_head))
+			die(_("Could not resolve HEAD to a revision"));
+	}
+
+	/* Detach HEAD and reset the tree */
+	if (!quiet)
+		printf("First, rewinding head to replay your work on top of it...");
+
+	strbuf_addf(&msg, "rebase: checkout %s", options.onto_name);
+	if (detach_head_to(&options.onto->object.oid, "checkout", msg.buf))
+		die(_("Could not detach HEAD"));
+	strbuf_release(&msg);
+	if (update_ref("rebase", "ORIG_HEAD", &options.orig_head, NULL, 0,
+		       UPDATE_REFS_MSG_ON_ERR) < 0)
+		die(_("Could not update ORIG_HEAD to '%s'"),
+		    oid_to_hex(&options.orig_head));
+
+	strbuf_addf(&revisions, "%s..%s",
+		!options.root ? oid_to_hex(&options.onto->object.oid) :
+		    (restrict_revision ? restrict_revision :
+			    oid_to_hex(&options.upstream->object.oid)),
+			    oid_to_hex(&options.orig_head));
+
+	options.revisions = revisions.buf;
+
+	ret = !!run_specific_rebase(&options);
+
+	strbuf_release(&revisions);
+	free(options.head_name);
+	return ret;
+}
-- 
2.18.0
Stefan Beller· Jun 28, 2018, 21:58 UTC · re: Pratik Karki · lore

Re: [PATCH 5/5] builtin/rebase: support running "git rebase <upstream>"

On Thu, Jun 28, 2018 at 12:48 AM Pratik Karki <predatoramigo@gmail.com> wrote:
Show 5 quoted lines
>
> This patch gives life to the skeleton added in the previous patch.
> This patch makes real operation happen i.e. by using
> `git -c rebase.usebuiltin=true rebase <upstream>`.
> With this patch, the basic operation of rebase can be done.

Would it make sense to add this config option to some basic test in the test suite to show off one case in there? (Otherwise it is hard to keep this code correct for the future (even if it is just a few days/weeks) as other series on the list may collide with it in subtle ways, so a test would be fast signal to catch these subtleties).

Maybe setting this in one of the early tests in t3400 would be good?
> These backends use Unix shell functions defined both by git-sh-setup.sh
> and git-rebase.sh (we move the latter's into git-rebase--common.sh to

s/move/moved in a previous patch/ ? But then again we already know about the earlier patch, I am on the fence whether this is worth mentioning. But it sure is fine to leave it here.

Show 5 quoted lines
> accommodate for that), so we not only have to source the backend file
> before calling the respective Unix shell script function, but we have
> to source git-sh-setup and git-rebase--common before that.
> And since this is all done in a Unix shell script snippet, all of this
> is in argv[0]. There never will be a non-NULL argv[1].

No double negatives are never harder to read than simple forms. ;) So you are saying, there are no further arguments to that shell invocation?

Show 48 quoted lines
> This patch does the *bare* minimum to get `git rebase <upstream>` to
> work: there is still no option parsing, and only the bare minimum set
> of environment variables are set (in particular, the current revision
> would be susceptible to bugs where e.g. `rebase_root` could be set by
> mistake before running `git rebase` and the `git-rebase--am` backend
> would pick up that variable and use it).
>
> It still calls original `git-legacy-rebase.sh` unless the config
> setting rebase.useBuiltin is set to true. This patch uses the
> detach_head_to() function from checkout.c introduced by a previous
> commit to perform initial checkout.
>
> Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> ---
>  builtin/rebase.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 229 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 1152b7229..2f90389c2 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -9,6 +9,19 @@
>  #include "exec-cmd.h"
>  #include "argv-array.h"
>  #include "dir.h"
> +#include "packfile.h"
> +#include "checkout.h"
> +#include "refs.h"
> +
> +static GIT_PATH_FUNC(apply_dir, "rebase-apply");
> +static GIT_PATH_FUNC(merge_dir, "rebase-merge");
> +
> +enum rebase_type {
> +       REBASE_AM,
> +       REBASE_MERGE,
> +       REBASE_INTERACTIVE,
> +       REBASE_PRESERVE_MERGES
> +};
>
>  static int use_builtin_rebase(void)
>  {
> @@ -28,8 +41,129 @@ static int use_builtin_rebase(void)
>         return ret;
>  }
>
> +static int apply_autostash(void)
> +{
> +       warning("TODO");

This comes up unconditionally here, so the automated testing idea from above might not be as good as I thought after all.

> +static struct commit *peel_committish(const char *name)

The -ish suffix is to indicate that a wide range of notations that describe commits are accepted. Another way of naming this function would be by its output, i.e. peel_to_commit, the name similar to peel_to_type. But I guess emphasizing the input to be anything that describes a commit is also important here, as we pass in the arguments eventually provided by users (e.g. "master^^") so this name sounds fine; I cannot think of a better suggestion for now.

Pratik Karki· Jul 2, 2018, 09:15 UTC · re: Pratik Karki · lore

[GSoC] [PATCH v2 0/4] rebase: rewrite rebase in C

As a GSoC project, I have been working on the builtin rebase.

The motivation behind the rewrite of rebase i.e. from shell script to C are for following reasons:

1.  Writing shell scripts and getting it to production is much faster
    than doing the equivalent in C but lacks in performance and extra
    workarounds are needed for non-POSIX platforms.
2.  Git for Windows is at loss as the installer size increases due to
    addition of extra dependencies for the shell scripts which are usually
    available in POSIX compliant platforms.

This series of patches serves to demonstrate a minimal builtin rebase which supports running `git rebase <upstream>` and also serves to ask for reviews.

Changes since v1:
  -  Remove the TODO-rebase.sh which shouldn't be merged into `pu`.
  -  Add newline at the end of the file `builtin/rebase.c` in
     `rebase: start implementing it as a builtin`.
  -  Fix unintentional ordering in `git-rebase--common.sh` and fix the commit
     message in `rebase: refactor common shell functions into their own file`.
  -  Fix wrong expression of `argc` in the handle upstream loop, fix wrong
     type casting code, make the condition simple and fix commit message in
     `builtin/rebase: support running "git-rebase <upstream>".
Pratik Karki (4):
  rebase: start implementing it as a builtin
  rebase: refactor common shell functions into their own file
  sequencer: refactor the code to detach HEAD to checkout.c
  builtin/rebase: support running "git rebase <upstream>"
 .gitignore                            |   2 +
 Makefile                              |   4 +-
 builtin.h                             |   1 +
 builtin/rebase.c                      | 282 ++++++++++++++++++++++++++
 checkout.c                            |  64 ++++++
 checkout.h                            |   3 +
 git-rebase.sh => git-legacy-rebase.sh |  62 +-----
 git-rebase--common.sh                 |  61 ++++++
 git.c                                 |   6 +
 sequencer.c                           |  58 +-----
 10 files changed, 428 insertions(+), 115 deletions(-)
 create mode 100644 builtin/rebase.c
 rename git-rebase.sh => git-legacy-rebase.sh (91%)
 create mode 100644 git-rebase--common.sh
-- 
2.18.0
Pratik Karki· Jul 2, 2018, 09:15 UTC · re: Pratik Karki · lore

[PATCH v2 1/4] rebase: start implementing it as a builtin

This commit imitates the strategy that was used to convert the difftool to a builtin, see be8a90e (difftool: add a skeleton for the upcoming builtin, 2017-01-17) for details: This commit renames the shell script `git-rebase.sh` to `git-legacy-rebase.sh` and hands off to it by default.

The current version of the builtin rebase does not, however, make full use of the internals but instead chooses to spawn a couple of Git processes to find out if we run the builtin or legacy rebase as that keeps the directory that we are in correct. There remains a lot of room for improvement, left for a later date. The following commits will recreate the functionality of the shell script, in pure C.

We intentionally avoid reading the config directly to avoid messing up the GIT_* environment variables when we need to fall back to exec()ing the shell script. The test of builtin rebase can be done by `git -c rebase.useBuiltin=true rebase ...`

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 .gitignore                            |  1 +
 Makefile                              |  3 +-
 builtin.h                             |  1 +
 builtin/rebase.c                      | 55 +++++++++++++++++++++++++++
 git-rebase.sh => git-legacy-rebase.sh |  0
 git.c                                 |  6 +++
 6 files changed, 65 insertions(+), 1 deletion(-)
 create mode 100644 builtin/rebase.c
 rename git-rebase.sh => git-legacy-rebase.sh (100%)
Show changes to 6 files +65 −1

.gitignore, Makefile, builtin.h, builtin/rebase.c, git-rebase.sh, git.c

diff --git a/.gitignore b/.gitignore
index 3284a1e9b..ec2395901 100644
--- a/.gitignore
+++ b/.gitignore
@@ -78,6 +78,7 @@
 /git-init-db
 /git-interpret-trailers
 /git-instaweb
+/git-legacy-rebase
 /git-log
 /git-ls-files
 /git-ls-remote
diff --git a/Makefile b/Makefile
index 0cb6590f2..e88fe2e5f 100644
--- a/Makefile
+++ b/Makefile
@@ -609,7 +609,7 @@ SCRIPT_SH += git-merge-one-file.sh
 SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
 SCRIPT_SH += git-quiltimport.sh
-SCRIPT_SH += git-rebase.sh
+SCRIPT_SH += git-legacy-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
 SCRIPT_SH += git-request-pull.sh
 SCRIPT_SH += git-stash.sh
@@ -1059,6 +1059,7 @@ BUILTIN_OBJS += builtin/prune.o
 BUILTIN_OBJS += builtin/pull.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase.o
 BUILTIN_OBJS += builtin/rebase--helper.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
diff --git a/builtin.h b/builtin.h
index 0362f1ce2..44651a447 100644
--- a/builtin.h
+++ b/builtin.h
@@ -202,6 +202,7 @@ extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
 extern int cmd_pull(int argc, const char **argv, const char *prefix);
 extern int cmd_push(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase(int argc, const char **argv, const char *prefix);
 extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase.c b/builtin/rebase.c
new file mode 100644
index 000000000..c0c9d6cd2
--- /dev/null
+++ b/builtin/rebase.c
@@ -0,0 +1,55 @@
+/*
+ * "git rebase" builtin command
+ *
+ * Copyright (c) 2018 Pratik Karki
+ */
+
+#include "builtin.h"
+#include "run-command.h"
+#include "exec-cmd.h"
+#include "argv-array.h"
+#include "dir.h"
+
+static int use_builtin_rebase(void)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf out = STRBUF_INIT;
+	int ret;
+
+	argv_array_pushl(&cp.args,
+			 "config", "--bool", "rebase.usebuiltin", NULL);
+	cp.git_cmd = 1;
+	if (capture_command(&cp, &out, 6))
+		return 0;
+
+	strbuf_trim(&out);
+	ret = !strcmp("true", out.buf);
+	strbuf_release(&out);
+	return ret;
+}
+
+int cmd_rebase(int argc, const char **argv, const char *prefix)
+{
+	/*
+	* NEEDSWORK: Once the builtin rebase has been tested enough
+	* and git-legacy-rebase.sh is retired to contrib/, this preamble
+	* can be removed.
+	*/
+	if (!use_builtin_rebase()) {
+		const char *path = mkpath("%s/git-legacy-rebase",
+					  git_exec_path());
+
+		if (sane_execvp(path, (char **)argv) < 0)
+			die_errno("could not exec %s", path);
+
+		return 0;
+	}
+
+	if (argc != 2)
+		die("Usage: %s <base>", argv[0]);
+	prefix = setup_git_directory();
+	trace_repo_setup(prefix);
+	setup_work_tree();
+
+	die("TODO");
+}
diff --git a/git-rebase.sh b/git-legacy-rebase.sh
similarity index 100%
rename from git-rebase.sh
rename to git-legacy-rebase.sh
diff --git a/git.c b/git.c
index 9dbe6ffaa..bacfefb2d 100644
--- a/git.c
+++ b/git.c
@@ -518,6 +518,12 @@ static struct cmd_struct commands[] = {
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
 	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
+	/*
+	 *NEEDSWORK: Until the rebase is independent and needs no redirection
+	 to rebase shell script this is kept as is, then should be changed to
+	 RUN_SETUP | NEED_WORK_TREE
+	*/
+	{ "rebase", cmd_rebase },
 	{ "rebase--helper", cmd_rebase__helper, RUN_SETUP | NEED_WORK_TREE },
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
-- 
2.18.0
Junio C Hamano· Jul 3, 2018, 21:09 UTC · re: Pratik Karki · lore

Re: [PATCH v2 1/4] rebase: start implementing it as a builtin

Pratik Karki <predatoramigo@gmail.com> writes:
Show 5 quoted lines
> This commit imitates the strategy that was used to convert the
> difftool to a builtin, see be8a90e (difftool: add a skeleton for the
> upcoming builtin, 2017-01-17) for details: This commit renames the
> shell script `git-rebase.sh` to `git-legacy-rebase.sh` and hands off to
> it by default.

I'd appreciate it if mentors can teach a bit more log message writing to students, thanks.

Show 8 quoted lines
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> new file mode 100644
> index 000000000..c0c9d6cd2
> --- /dev/null
> +++ b/builtin/rebase.c
> @@ -0,0 +1,55 @@
> +/*
> + * "git rebase" builtin command
Nice ;-)
Show 26 quoted lines
> + * Copyright (c) 2018 Pratik Karki
> + */
> +
> +#include "builtin.h"
> +#include "run-command.h"
> +#include "exec-cmd.h"
> +#include "argv-array.h"
> +#include "dir.h"
> +
> +static int use_builtin_rebase(void)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	int ret;
> +
> +	argv_array_pushl(&cp.args,
> +			 "config", "--bool", "rebase.usebuiltin", NULL);
> +	cp.git_cmd = 1;
> +	if (capture_command(&cp, &out, 6))
> +		return 0;
> +
> +	strbuf_trim(&out);
> +	ret = !strcmp("true", out.buf);
> +	strbuf_release(&out);
> +	return ret;
> +}

OK. As we give "--bool", it is perfectly OK to check with "true" and no other forms of positive setting.

Show 5 quoted lines
> +int cmd_rebase(int argc, const char **argv, const char *prefix)
> +{
> +	/*
> +	* NEEDSWORK: Once the builtin rebase has been tested enough
> +	* and git-legacy-rebase.sh is retired to contrib/, this preamble
Align these asterisks, i.e.
	/*
	 * NEEDSWORK: ...
	 */
Show 8 quoted lines
> +	if (!use_builtin_rebase()) {
> +		const char *path = mkpath("%s/git-legacy-rebase",
> +					  git_exec_path());
> +
> +		if (sane_execvp(path, (char **)argv) < 0)
> +			die_errno("could not exec %s", path);
> +
> +		return 0;

Hmph, I know this was inherited from the old commit you modelled this patch after, but can sane_execvp() ever give us control back with non-negative value returned? IOW I am wondering if this should be more like

	if (sane_execvp(...) < 0)
		die_errno(...);
	else
		die("sane_execvp() returned???");
Not worth a reroll, but something to think about.
Show 17 quoted lines
> diff --git a/git-rebase.sh b/git-legacy-rebase.sh
> similarity index 100%
> rename from git-rebase.sh
> rename to git-legacy-rebase.sh
> diff --git a/git.c b/git.c
> index 9dbe6ffaa..bacfefb2d 100644
> --- a/git.c
> +++ b/git.c
> @@ -518,6 +518,12 @@ static struct cmd_struct commands[] = {
>  	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
>  	{ "push", cmd_push, RUN_SETUP },
>  	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
> +	/*
> +	 *NEEDSWORK: Until the rebase is independent and needs no redirection
> +	 to rebase shell script this is kept as is, then should be changed to
> +	 RUN_SETUP | NEED_WORK_TREE
> +	*/
Likewise.
> +	{ "rebase", cmd_rebase },
>  	{ "rebase--helper", cmd_rebase__helper, RUN_SETUP | NEED_WORK_TREE },
>  	{ "receive-pack", cmd_receive_pack },
>  	{ "reflog", cmd_reflog, RUN_SETUP },
Pratik Karki· Jul 2, 2018, 09:15 UTC · re: Pratik Karki · lore

[PATCH v2 2/4] rebase: refactor common shell functions into their own file

The function present in `git-legacy-rebase.sh` are used by backends so, this refactor tries to extract the functions out so that, the `git-legacy-rebase.sh` can be retired easily as the `git-rebase--common.sh` will provide the functions for now.

The motivation behind this is to call the backend functions *directly* from C, bypassing `git-rebase.sh`. Therefore those functions need to live in a separate file: we need to be able to call `. git-rebase--common` in that script snippet so that those functions are defined.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 .gitignore            |  1 +
 Makefile              |  1 +
 git-legacy-rebase.sh  | 62 +------------------------------------------
 git-rebase--common.sh | 61 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 64 insertions(+), 61 deletions(-)
 create mode 100644 git-rebase--common.sh
Show changes to 4 files +64 −61

.gitignore, Makefile, git-legacy-rebase.sh, git-rebase--common.sh

diff --git a/.gitignore b/.gitignore
index ec2395901..824141cba 100644
--- a/.gitignore
+++ b/.gitignore
@@ -117,6 +117,7 @@
 /git-read-tree
 /git-rebase
 /git-rebase--am
+/git-rebase--common
 /git-rebase--helper
 /git-rebase--interactive
 /git-rebase--merge
diff --git a/Makefile b/Makefile
index e88fe2e5f..163e52ad1 100644
--- a/Makefile
+++ b/Makefile
@@ -619,6 +619,7 @@ SCRIPT_SH += git-web--browse.sh
 SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
+SCRIPT_LIB += git-rebase--common
 SCRIPT_LIB += git-rebase--interactive
 SCRIPT_LIB += git-rebase--preserve-merges
 SCRIPT_LIB += git-rebase--merge
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 19bdebb48..240aac507 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -102,6 +102,7 @@ case "$(git config --bool commit.gpgsign)" in
 true)	gpg_sign_opt=-S ;;
 *)	gpg_sign_opt= ;;
 esac
+. git-rebase--common
 
 read_basic_state () {
 	test -f "$state_dir/head-name" &&
@@ -132,67 +133,6 @@ read_basic_state () {
 	}
 }
 
-write_basic_state () {
-	echo "$head_name" > "$state_dir"/head-name &&
-	echo "$onto" > "$state_dir"/onto &&
-	echo "$orig_head" > "$state_dir"/orig-head &&
-	echo "$GIT_QUIET" > "$state_dir"/quiet &&
-	test t = "$verbose" && : > "$state_dir"/verbose
-	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
-	test -n "$strategy_opts" && echo "$strategy_opts" > \
-		"$state_dir"/strategy_opts
-	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
-		"$state_dir"/allow_rerere_autoupdate
-	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
-	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
-}
-
-output () {
-	case "$verbose" in
-	'')
-		output=$("$@" 2>&1 )
-		status=$?
-		test $status != 0 && printf "%s\n" "$output"
-		return $status
-		;;
-	*)
-		"$@"
-		;;
-	esac
-}
-
-move_to_original_branch () {
-	case "$head_name" in
-	refs/*)
-		message="rebase finished: $head_name onto $onto"
-		git update-ref -m "$message" \
-			$head_name $(git rev-parse HEAD) $orig_head &&
-		git symbolic-ref \
-			-m "rebase finished: returning to $head_name" \
-			HEAD $head_name ||
-		die "$(eval_gettext "Could not move back to \$head_name")"
-		;;
-	esac
-}
-
-apply_autostash () {
-	if test -f "$state_dir/autostash"
-	then
-		stash_sha1=$(cat "$state_dir/autostash")
-		if git stash apply $stash_sha1 >/dev/null 2>&1
-		then
-			echo "$(gettext 'Applied autostash.')" >&2
-		else
-			git stash store -m "autostash" -q $stash_sha1 ||
-			die "$(eval_gettext "Cannot store \$stash_sha1")"
-			gettext 'Applying autostash resulted in conflicts.
-Your changes are safe in the stash.
-You can run "git stash pop" or "git stash drop" at any time.
-' >&2
-		fi
-	fi
-}
-
 finish_rebase () {
 	rm -f "$(git rev-parse --git-path REBASE_HEAD)"
 	apply_autostash &&
diff --git a/git-rebase--common.sh b/git-rebase--common.sh
new file mode 100644
index 000000000..fdce7bba5
--- /dev/null
+++ b/git-rebase--common.sh
@@ -0,0 +1,61 @@
+
+write_basic_state () {
+	echo "$head_name" > "$state_dir"/head-name &&
+	echo "$onto" > "$state_dir"/onto &&
+	echo "$orig_head" > "$state_dir"/orig-head &&
+	echo "$GIT_QUIET" > "$state_dir"/quiet &&
+	test t = "$verbose" && : > "$state_dir"/verbose
+	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
+	test -n "$strategy_opts" && echo "$strategy_opts" > \
+		"$state_dir"/strategy_opts
+	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
+		"$state_dir"/allow_rerere_autoupdate
+	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
+	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
+}
+
+output () {
+	case "$verbose" in
+	'')
+		output=$("$@" 2>&1 )
+		status=$?
+		test $status != 0 && printf "%s\n" "$output"
+		return $status
+		;;
+	*)
+		"$@"
+		;;
+	esac
+}
+
+move_to_original_branch () {
+	case "$head_name" in
+	refs/*)
+		message="rebase finished: $head_name onto $onto"
+		git update-ref -m "$message" \
+			$head_name $(git rev-parse HEAD) $orig_head &&
+		git symbolic-ref \
+			-m "rebase finished: returning to $head_name" \
+			HEAD $head_name ||
+		die "$(eval_gettext "Could not move back to \$head_name")"
+		;;
+	esac
+}
+
+apply_autostash () {
+	if test -f "$state_dir/autostash"
+	then
+		stash_sha1=$(cat "$state_dir/autostash")
+		if git stash apply $stash_sha1 >/dev/null 2>&1
+		then
+			echo "$(gettext 'Applied autostash.')" >&2
+		else
+			git stash store -m "autostash" -q $stash_sha1 ||
+			die "$(eval_gettext "Cannot store \$stash_sha1")"
+			gettext 'Applying autostash resulted in conflicts.
+Your changes are safe in the stash.
+You can run "git stash pop" or "git stash drop" at any time.
+' >&2
+		fi
+	fi
+}
-- 
2.18.0
Junio C Hamano· Jul 3, 2018, 21:13 UTC · re: Pratik Karki · lore

Re: [PATCH v2 2/4] rebase: refactor common shell functions into their own file

Pratik Karki <predatoramigo@gmail.com> writes:
> The function present in `git-legacy-rebase.sh` are used by backends
> so, this refactor tries to extract the functions out so that, the
> `git-legacy-rebase.sh` can be retired easily as the
> `git-rebase--common.sh` will provide the functions for now.
Makes sense.
Pratik Karki· Jul 2, 2018, 09:15 UTC · re: Pratik Karki · lore

[PATCH v2 3/4] sequencer: refactor the code to detach HEAD to checkout.c

The motivation behind this commit is to extract the core part of do_reset() from sequencer.c and move it to a new detach_head_to() function in checkout.c.

Here the index only gets locked after performing the first part of `do_reset()` rather than before which essentially derives the `oid` from the specified label/name passed to the `do_reset()` function. It also fixes two bugs: there were two `return error()` statements in the `[new root]` case that would have failed to unlock the index.

The new function will be used in the next commit by the builtin rebase, to perform the initial checkout.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 checkout.c  | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 checkout.h  |  3 +++
 sequencer.c | 58 +++++-------------------------------------------
 3 files changed, 72 insertions(+), 53 deletions(-)
Show changes to 3 files +72 −53

checkout.c, checkout.h, sequencer.c

diff --git a/checkout.c b/checkout.c
index bdefc888b..da68915fd 100644
--- a/checkout.c
+++ b/checkout.c
@@ -2,6 +2,11 @@
 #include "remote.h"
 #include "refspec.h"
 #include "checkout.h"
+#include "unpack-trees.h"
+#include "lockfile.h"
+#include "refs.h"
+#include "tree.h"
+#include "cache-tree.h"
 
 struct tracking_name_data {
 	/* const */ char *src_ref;
@@ -42,3 +47,62 @@ const char *unique_tracking_name(const char *name, struct object_id *oid)
 	free(cb_data.dst_ref);
 	return NULL;
 }
+
+int detach_head_to(struct object_id *oid, const char *action,
+		   const char *reflog_message)
+{
+	struct strbuf ref_name = STRBUF_INIT;
+	struct tree_desc desc;
+	struct lock_file lock = LOCK_INIT;
+	struct unpack_trees_options unpack_tree_opts;
+	struct tree *tree;
+	int ret = 0;
+
+	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
+		return -1;
+
+	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
+	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
+	unpack_tree_opts.head_idx = 1;
+	unpack_tree_opts.src_index = &the_index;
+	unpack_tree_opts.dst_index = &the_index;
+	unpack_tree_opts.fn = oneway_merge;
+	unpack_tree_opts.merge = 1;
+	unpack_tree_opts.update = 1;
+
+	if (read_cache_unmerged()) {
+		rollback_lock_file(&lock);
+		strbuf_release(&ref_name);
+		return error_resolve_conflict(_(action));
+	}
+
+	if (!fill_tree_descriptor(&desc, oid)) {
+		error(_("failed to find tree of %s"), oid_to_hex(oid));
+		rollback_lock_file(&lock);
+		free((void *)desc.buffer);
+		strbuf_release(&ref_name);
+		return -1;
+	}
+
+	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
+		rollback_lock_file(&lock);
+		free((void *)desc.buffer);
+		strbuf_release(&ref_name);
+		return -1;
+	}
+
+	tree = parse_tree_indirect(oid);
+	prime_cache_tree(&the_index, tree);
+
+	if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
+		ret = error(_("could not write index"));
+	free((void *)desc.buffer);
+
+	if (!ret)
+		ret = update_ref(reflog_message, "HEAD", oid,
+				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+
+	strbuf_release(&ref_name);
+	return ret;
+
+}
diff --git a/checkout.h b/checkout.h
index 998071117..6ce46cf4e 100644
--- a/checkout.h
+++ b/checkout.h
@@ -10,4 +10,7 @@
  */
 extern const char *unique_tracking_name(const char *name, struct object_id *oid);
 
+int detach_head_to(struct object_id *oid, const char *action,
+		   const char *reflog_message);
+
 #endif /* CHECKOUT_H */
diff --git a/sequencer.c b/sequencer.c
index 5354d4d51..106d518f4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -29,6 +29,7 @@
 #include "oidset.h"
 #include "commit-slab.h"
 #include "alias.h"
+#include "checkout.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -2756,14 +2757,7 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
 {
 	struct strbuf ref_name = STRBUF_INIT;
 	struct object_id oid;
-	struct lock_file lock = LOCK_INIT;
-	struct tree_desc desc;
-	struct tree *tree;
-	struct unpack_trees_options unpack_tree_opts;
-	int ret = 0, i;
-
-	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
-		return -1;
+	int i;
 
 	if (len == 10 && !strncmp("[new root]", name, len)) {
 		if (!opts->have_squash_onto) {
@@ -2789,56 +2783,14 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
 		if (get_oid(ref_name.buf, &oid) &&
 		    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
 			error(_("could not read '%s'"), ref_name.buf);
-			rollback_lock_file(&lock);
 			strbuf_release(&ref_name);
 			return -1;
 		}
 	}
 
-	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
-	setup_unpack_trees_porcelain(&unpack_tree_opts, "reset");
-	unpack_tree_opts.head_idx = 1;
-	unpack_tree_opts.src_index = &the_index;
-	unpack_tree_opts.dst_index = &the_index;
-	unpack_tree_opts.fn = oneway_merge;
-	unpack_tree_opts.merge = 1;
-	unpack_tree_opts.update = 1;
-
-	if (read_cache_unmerged()) {
-		rollback_lock_file(&lock);
-		strbuf_release(&ref_name);
-		return error_resolve_conflict(_(action_name(opts)));
-	}
-
-	if (!fill_tree_descriptor(&desc, &oid)) {
-		error(_("failed to find tree of %s"), oid_to_hex(&oid));
-		rollback_lock_file(&lock);
-		free((void *)desc.buffer);
-		strbuf_release(&ref_name);
-		return -1;
-	}
-
-	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
-		rollback_lock_file(&lock);
-		free((void *)desc.buffer);
-		strbuf_release(&ref_name);
-		return -1;
-	}
-
-	tree = parse_tree_indirect(&oid);
-	prime_cache_tree(&the_index, tree);
-
-	if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
-		ret = error(_("could not write index"));
-	free((void *)desc.buffer);
-
-	if (!ret)
-		ret = update_ref(reflog_message(opts, "reset", "'%.*s'",
-						len, name), "HEAD", &oid,
-				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
-
-	strbuf_release(&ref_name);
-	return ret;
+	return detach_head_to(&oid, action_name(opts),
+			      reflog_message(opts, "reset", "'%.*s'",
+					     len, name));
 }
 
 static int do_merge(struct commit *commit, const char *arg, int arg_len,
-- 
2.18.0
Junio C Hamano· Jul 3, 2018, 21:26 UTC · re: Pratik Karki · lore

Re: [PATCH v2 3/4] sequencer: refactor the code to detach HEAD to checkout.c

Pratik Karki <predatoramigo@gmail.com> writes:
Show 12 quoted lines
> The motivation behind this commit is to extract the core part of
> do_reset() from sequencer.c and move it to a new detach_head_to()
> function in checkout.c.
>
> Here the index only gets locked after performing the first part of
> `do_reset()` rather than before which essentially derives the `oid`
> from the specified label/name passed to the `do_reset()` function.
> It also fixes two bugs: there were two `return error()` statements in
> the `[new root]` case that would have failed to unlock the index.
>
> The new function will be used in the next commit by the builtin rebase,
> to perform the initial checkout.

I like the split of do_reset() into this "detach-to" part that is less specific to replaying an existing commit and the rest, which as you said incidentally corrects the locking issue.

It is not clear to me why checkout.[ch] is a better place to house this function, though.

Pratik Karki· Jul 2, 2018, 09:15 UTC · re: Pratik Karki · lore

[PATCH v2 4/4] builtin/rebase: support running "git rebase <upstream>"

This patch gives life to the skeleton added in the previous patch. This patch makes real operation happen i.e. by using `git -c rebase.usebuiltin=true rebase <upstream>`. With this patch, the basic operation of rebase can be done.

The current version of the builtin rebase does not, however, make full use of the internals but instead chooses to spawn a couple of Git processes, still, to make for an easier conversion. There remains a lot of room for improvement, left later.

These backends use Unix shell functions defined both by git-sh-setup.sh and git-rebase.sh (we move the latter's into git-rebase--common.sh to accommodate for that), so we not only have to source the backend file before calling the respective Unix shell script function, but we have to source git-sh-setup and git-rebase--common before that. And since this is all done in a Unix shell script snippet, all of this is in argv[0]. There will be NULL argv[1] i.e. there are no further arguments.

This patch does the *bare* minimum to get `git rebase <upstream>` to work: there is still no option parsing, and only the bare minimum set of environment variables are set (in particular, the current revision would be susceptible to bugs where e.g. `rebase_root` could be set by mistake before running `git rebase` and the `git-rebase--am` backend would pick up that variable and use it).

It still calls original `git-legacy-rebase.sh` unless the config setting rebase.useBuiltin is set to true. This patch uses the detach_head_to() function from checkout.c introduced by a previous commit to perform initial checkout.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 builtin/rebase.c | 229 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 228 insertions(+), 1 deletion(-)
Show changes to builtin/rebase.c +228 −1
diff --git a/builtin/rebase.c b/builtin/rebase.c
index c0c9d6cd2..dc6816e34 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -9,6 +9,19 @@
 #include "exec-cmd.h"
 #include "argv-array.h"
 #include "dir.h"
+#include "packfile.h"
+#include "checkout.h"
+#include "refs.h"
+
+static GIT_PATH_FUNC(apply_dir, "rebase-apply");
+static GIT_PATH_FUNC(merge_dir, "rebase-merge");
+
+enum rebase_type {
+	REBASE_AM,
+	REBASE_MERGE,
+	REBASE_INTERACTIVE,
+	REBASE_PRESERVE_MERGES
+};
 
 static int use_builtin_rebase(void)
 {
@@ -28,8 +41,129 @@ static int use_builtin_rebase(void)
 	return ret;
 }
 
+static int apply_autostash(void)
+{
+	warning("TODO");
+	return 0;
+}
+
+struct rebase_options {
+	enum rebase_type type;
+	const char *state_dir;
+	struct commit *upstream;
+	const char *upstream_name;
+	char *head_name;
+	struct object_id orig_head;
+	struct commit *onto;
+	const char *onto_name;
+	const char *revisions;
+	const char *root;
+};
+
+static int finish_rebase(struct rebase_options *opts)
+{
+	struct strbuf dir = STRBUF_INIT;
+	const char *argv_gc_auto[] = { "gc", "--auto", NULL };
+
+	delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
+	apply_autostash();
+	close_all_packs(the_repository->objects);
+	/*
+	 * We ignore errors in 'gc --auto', since the
+	 * user should see them.
+	 */
+	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
+	strbuf_addstr(&dir, opts->state_dir);
+	remove_dir_recursively(&dir, 0);
+	strbuf_release(&dir);
+
+	return 0;
+}
+
+static struct commit *peel_committish(const char *name)
+{
+	struct object *obj;
+	struct object_id oid;
+	if (get_oid(name, &oid))
+		return NULL;
+	obj = parse_object(&oid);
+	return (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
+}
+
+static int run_specific_rebase(struct rebase_options *opts)
+{
+	const char *argv[] = { NULL, NULL };
+	struct strbuf script_snippet = STRBUF_INIT;
+	struct argv_array env = ARGV_ARRAY_INIT;
+	int status;
+	const char *backend, *backend_func;
+
+	argv_array_pushf(&env, "upstream_name=%s", opts->upstream_name);
+	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
+	argv_array_pushf(&env, "upstream=%s",
+				oid_to_hex(&opts->upstream->object.oid));
+	argv_array_pushf(&env, "orig_head=%s", oid_to_hex(&opts->orig_head));
+	argv_array_pushf(&env, "onto=%s",
+				oid_to_hex(&opts->onto->object.oid));
+	argv_array_pushf(&env, "onto_name=%s", opts->onto_name);
+	argv_array_pushf(&env, "revisions=%s", opts->revisions);
+
+	switch (opts->type) {
+		case REBASE_AM:
+			backend = "git-rebase--am";
+			backend_func = "git_rebase__am";
+			break;
+		case REBASE_INTERACTIVE:
+			backend = "git-rebase--interactive";
+			backend_func = "git_rebase__interactive";
+			break;
+		case REBASE_MERGE:
+			backend = "git-rebase--merge";
+			backend_func = "git_rebase__merge";
+			break;
+		case REBASE_PRESERVE_MERGES:
+			backend = "git-rebase--preserve-merges";
+			backend_func = "git_rebase__preserve_merges";
+			break;
+		default:
+			BUG("Unhandled rebase type %d", opts->type);
+			break;
+	}
+
+	strbuf_addf(&script_snippet,
+		    ". git-rebase--common && . %s && %s",
+		    backend, backend_func);
+	argv[0] = script_snippet.buf;
+
+	status = run_command_v_opt_cd_env(argv, RUN_USING_SHELL, NULL,
+					  env.argv);
+	if (status == 0)
+		finish_rebase(opts);
+	else if (status == 2) {
+		struct strbuf dir = STRBUF_INIT;
+
+		apply_autostash();
+		strbuf_addstr(&dir, opts->state_dir);
+		remove_dir_recursively(&dir, 0);
+		strbuf_release(&dir);
+		die("Nothing to do");
+	}
+
+	strbuf_release(&script_snippet);
+	argv_array_clear(&env);
+
+	return status ? -1 : 0;
+}
+
 int cmd_rebase(int argc, const char **argv, const char *prefix)
 {
+	struct rebase_options options = { -1 };
+	const char *branch_name;
+	int ret, flags, quiet = 0;
+	struct strbuf msg = STRBUF_INIT;
+	struct strbuf revisions = STRBUF_INIT;
+	const char *restrict_revision = NULL;
+
 	/*
 	* NEEDSWORK: Once the builtin rebase has been tested enough
 	* and git-legacy-rebase.sh is retired to contrib/, this preamble
@@ -51,5 +185,98 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	trace_repo_setup(prefix);
 	setup_work_tree();
 
-	die("TODO");
+	options.type = REBASE_AM;
+
+	switch (options.type) {
+	case REBASE_AM:
+		options.state_dir = apply_dir();
+		break;
+	case REBASE_MERGE:
+	case REBASE_INTERACTIVE:
+	case REBASE_PRESERVE_MERGES:
+		options.state_dir = merge_dir();
+		break;
+	}
+	if (!options.root) {
+		if (argc != 2)
+			die("TODO: handle @{upstream}");
+		else {
+			options.upstream_name = argv[1];
+			argc--;
+			argv++;
+			if (!strcmp(options.upstream_name, "-"))
+				options.upstream_name = "@{-1}";
+		}
+		options.upstream = peel_committish(options.upstream_name);
+		if (!options.upstream)
+			die(_("invalid upstream '%s'"), options.upstream_name);
+	} else
+		die("TODO: upstream for --root");
+
+	/* Make sure the branch to rebase onto is valid. */
+	if (!options.onto_name)
+		options.onto_name = options.upstream_name;
+	if (strstr(options.onto_name, "...")) {
+		die("TODO");
+	} else {
+		options.onto = peel_committish(options.onto_name);
+		if (!options.onto)
+			die(_("Does not point to a valid commit '%s'"),
+				options.onto_name);
+	}
+
+	/*
+	* If the branch to rebase is given, that is the branch we will rebase
+	* branch_name -- branch/commit being rebased, or HEAD (already detached)
+	* orig_head -- commit object name of tip of the branch before rebasing
+	* head_name -- refs/heads/<that-branch> or "detached HEAD"
+	*/
+	if (argc > 1)
+		 die ("TODO: handle switch_to");
+	else {
+		/* Do not need to switch branches, we are already on it. */
+		options.head_name =
+			xstrdup_or_null(resolve_ref_unsafe("HEAD", 0, NULL,
+					 &flags));
+		if (!options.head_name)
+			die(_("No such ref: %s"), "HEAD");
+		if (flags & REF_ISSYMREF) {
+			if (!skip_prefix(options.head_name,
+					 "refs/heads/", &branch_name))
+				branch_name = options.head_name;
+
+		} else {
+			options.head_name = xstrdup("detached HEAD");
+			branch_name = "HEAD";
+		}
+		if (get_oid("HEAD", &options.orig_head))
+			die(_("Could not resolve HEAD to a revision"));
+	}
+
+	/* Detach HEAD and reset the tree */
+	if (!quiet)
+		printf("First, rewinding head to replay your work on top of it...");
+
+	strbuf_addf(&msg, "rebase: checkout %s", options.onto_name);
+	if (detach_head_to(&options.onto->object.oid, "checkout", msg.buf))
+		die(_("Could not detach HEAD"));
+	strbuf_release(&msg);
+	if (update_ref("rebase", "ORIG_HEAD", &options.orig_head, NULL, 0,
+		       UPDATE_REFS_MSG_ON_ERR) < 0)
+		die(_("Could not update ORIG_HEAD to '%s'"),
+		    oid_to_hex(&options.orig_head));
+
+	strbuf_addf(&revisions, "%s..%s",
+		!options.root ? oid_to_hex(&options.onto->object.oid) :
+		    (restrict_revision ? restrict_revision :
+			    oid_to_hex(&options.upstream->object.oid)),
+			    oid_to_hex(&options.orig_head));
+
+	options.revisions = revisions.buf;
+
+	ret = !!run_specific_rebase(&options);
+
+	strbuf_release(&revisions);
+	free(options.head_name);
+	return ret;
 }
-- 
2.18.0
Junio C Hamano· Jul 3, 2018, 21:42 UTC · re: Pratik Karki · lore

Re: [PATCH v2 4/4] builtin/rebase: support running "git rebase <upstream>"

Pratik Karki <predatoramigo@gmail.com> writes:
> +static struct commit *peel_committish(const char *name)
> +{
> +	struct object *obj;
> +	struct object_id oid;

It by itself is not a big enough deal to warrant a reroll, but please make it a habit to leave a blank line between the declarations and the first statement (i.e. here), to help readers.

Show 6 quoted lines
> +	if (get_oid(name, &oid))
> +		return NULL;
> +	obj = parse_object(&oid);
> +	return (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
> +}
> +
Show 37 quoted lines
> +{
> +	const char *argv[] = { NULL, NULL };
> +	struct strbuf script_snippet = STRBUF_INIT;
> +	struct argv_array env = ARGV_ARRAY_INIT;
> +	int status;
> +	const char *backend, *backend_func;
> +
> +	argv_array_pushf(&env, "upstream_name=%s", opts->upstream_name);
> +	argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
> +	argv_array_pushf(&env, "upstream=%s",
> +				oid_to_hex(&opts->upstream->object.oid));
> +	argv_array_pushf(&env, "orig_head=%s", oid_to_hex(&opts->orig_head));
> +	argv_array_pushf(&env, "onto=%s",
> +				oid_to_hex(&opts->onto->object.oid));
> +	argv_array_pushf(&env, "onto_name=%s", opts->onto_name);
> +	argv_array_pushf(&env, "revisions=%s", opts->revisions);
> +
> +	switch (opts->type) {
> +		case REBASE_AM:
> +			backend = "git-rebase--am";
> +			backend_func = "git_rebase__am";
> +			break;
> +		case REBASE_INTERACTIVE:
> +			backend = "git-rebase--interactive";
> +			backend_func = "git_rebase__interactive";
> +			break;
> +		case REBASE_MERGE:
> +			backend = "git-rebase--merge";
> +			backend_func = "git_rebase__merge";
> +			break;
> +		case REBASE_PRESERVE_MERGES:
> +			backend = "git-rebase--preserve-merges";
> +			backend_func = "git_rebase__preserve_merges";
> +			break;
> +		default:
> +			BUG("Unhandled rebase type %d", opts->type);
> +			break;

De-dent these lines so that case/default and switch all sit on the same column, and the body of each case arm is indented one level deeper than the case labels.

Show 9 quoted lines
> +	}
> +
> +	strbuf_addf(&script_snippet,
> +		    ". git-rebase--common && . %s && %s",
> +		    backend, backend_func);
> +	argv[0] = script_snippet.buf;
> +
> +	status = run_command_v_opt_cd_env(argv, RUN_USING_SHELL, NULL,
> +					  env.argv);

Hmph. These shell variables that tell rebase backend scriptlets what rebase frontend figured out about the request and current state used to be just plain shell variables that are not exported. Now they are exported and visible even to subprocesses these scriptlets spawn. That does not sound safe at all, especially GIT_DIR being among these variables (git-sh-setup sets GIT_DIR but does not export it and that has been very much deliberate).

You should actually be able to avoid exporthing them by building these variable assignment into script_snippet (you need to quote the values properly, using quote.c::sq_quote_buf() etc.), I think, and that would be a more faithful-to-the-original safe covnersion.

Thanks for a pleasant read. This step smells more like WIP, but I can see it is already moving in the right direction. The previous ones (except for the issues I noticed and sent responses separately) looked more-or-less good, too.

Pratik Karki· Jul 6, 2018, 12:08 UTC · re: Pratik Karki · lore

[GSoC] [PATCH v3 0/4] rebase: rewrite rebase in C

As a GSoC project, I have been working on the builtin rebase.

The motivation behind the rewrite of rebase i.e. from shell script to C are for following reasons:

1.  Writing shell scripts and getting it to production is much faster
    than doing the equivalent in C but lacks in performance and extra
    workarounds are needed for non-POSIX platforms.
2.  Git for Windows is at loss as the installer size increases due to
    addition of extra dependencies for the shell scripts which are usually
    available in POSIX compliant platforms.

This series of patches serves to demonstrate a minimal builtin rebase which supports running `git rebase <upstream>` and also serves to ask for reviews.

Changes since v2:
  -  Fix commit messages of patches.
  -  Acknowledge Junio's reviews.
  -  Demonstrate an alternative approach to setting of environment variables
     via `run_command_v_opt_cd_env()` which would be visible by spawned
     process from rebase backends as this is not safe.
Pratik Karki (4):
  rebase: start implementing it as a builtin
  rebase: refactor common shell functions into their own file
  sequencer: refactor the code to detach HEAD to checkout.c
  builtin/rebase: support running "git rebase <upstream>"
 .gitignore                            |   2 +
 Makefile                              |   4 +-
 builtin.h                             |   1 +
 builtin/rebase.c                      | 291 ++++++++++++++++++++++++++
 checkout.c                            |  64 ++++++
 checkout.h                            |   3 +
 git-rebase.sh => git-legacy-rebase.sh |  62 +-----
 git-rebase--common.sh                 |  61 ++++++
 git.c                                 |   6 +
 sequencer.c                           |  58 +----
 10 files changed, 437 insertions(+), 115 deletions(-)
 create mode 100644 builtin/rebase.c
 rename git-rebase.sh => git-legacy-rebase.sh (91%)
 create mode 100644 git-rebase--common.sh
-- 
2.18.0
Pratik Karki· Jul 6, 2018, 12:08 UTC · re: Pratik Karki · lore

[PATCH v3 1/4] rebase: start implementing it as a builtin

This commit imitates the strategy that was used to convert the difftool to a builtin. We start by renaming the shell script `git-rebase.sh` to `git-legacy-rebase.sh` and introduce a `builtin/rebase.c` that simply executes the shell script version, unless the config setting `rebase.useBuiltin` is set to `true`.

The motivation behind this is to rewrite all the functionality of the shell script version in the aforementioned `rebase.c`, one by one and be able to conveniently test new features by configuring `rebase.useBuiltin`.

We intentionally avoid reading the config directly to avoid messing up the GIT_* environment variables when we need to fall back to exec()ing the shell script. The test of builtin rebase can be done by `git -c rebase.useBuiltin=true rebase ...`

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 .gitignore                            |  1 +
 Makefile                              |  3 +-
 builtin.h                             |  1 +
 builtin/rebase.c                      | 56 +++++++++++++++++++++++++++
 git-rebase.sh => git-legacy-rebase.sh |  0
 git.c                                 |  6 +++
 6 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 builtin/rebase.c
 rename git-rebase.sh => git-legacy-rebase.sh (100%)
Show changes to 6 files +66 −1

.gitignore, Makefile, builtin.h, builtin/rebase.c, git-rebase.sh, git.c

diff --git a/.gitignore b/.gitignore
index 3284a1e9b1..ec23959014 100644
--- a/.gitignore
+++ b/.gitignore
@@ -78,6 +78,7 @@
 /git-init-db
 /git-interpret-trailers
 /git-instaweb
+/git-legacy-rebase
 /git-log
 /git-ls-files
 /git-ls-remote
diff --git a/Makefile b/Makefile
index 0cb6590f24..e88fe2e5fb 100644
--- a/Makefile
+++ b/Makefile
@@ -609,7 +609,7 @@ SCRIPT_SH += git-merge-one-file.sh
 SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
 SCRIPT_SH += git-quiltimport.sh
-SCRIPT_SH += git-rebase.sh
+SCRIPT_SH += git-legacy-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
 SCRIPT_SH += git-request-pull.sh
 SCRIPT_SH += git-stash.sh
@@ -1059,6 +1059,7 @@ BUILTIN_OBJS += builtin/prune.o
 BUILTIN_OBJS += builtin/pull.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase.o
 BUILTIN_OBJS += builtin/rebase--helper.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
diff --git a/builtin.h b/builtin.h
index 0362f1ce25..44651a447f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -202,6 +202,7 @@ extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
 extern int cmd_pull(int argc, const char **argv, const char *prefix);
 extern int cmd_push(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase(int argc, const char **argv, const char *prefix);
 extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase.c b/builtin/rebase.c
new file mode 100644
index 0000000000..fab7fca37e
--- /dev/null
+++ b/builtin/rebase.c
@@ -0,0 +1,56 @@
+/*
+ * "git rebase" builtin command
+ *
+ * Copyright (c) 2018 Pratik Karki
+ */
+
+#include "builtin.h"
+#include "run-command.h"
+#include "exec-cmd.h"
+#include "argv-array.h"
+#include "dir.h"
+
+static int use_builtin_rebase(void)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf out = STRBUF_INIT;
+	int ret;
+
+	argv_array_pushl(&cp.args,
+			 "config", "--bool", "rebase.usebuiltin", NULL);
+	cp.git_cmd = 1;
+	if (capture_command(&cp, &out, 6))
+		return 0;
+
+	strbuf_trim(&out);
+	ret = !strcmp("true", out.buf);
+	strbuf_release(&out);
+	return ret;
+}
+
+int cmd_rebase(int argc, const char **argv, const char *prefix)
+{
+	/*
+	 * NEEDSWORK: Once the builtin rebase has been tested enough
+	 * and git-legacy-rebase.sh is retired to contrib/, this preamble
+	 * can be removed.
+	 */
+
+	if (!use_builtin_rebase()) {
+		const char *path = mkpath("%s/git-legacy-rebase",
+					  git_exec_path());
+
+		if (sane_execvp(path, (char **)argv) < 0)
+			die_errno("could not exec %s", path);
+		else
+			die("sane_execvp() returned???");
+	}
+
+	if (argc != 2)
+		die("Usage: %s <base>", argv[0]);
+	prefix = setup_git_directory();
+	trace_repo_setup(prefix);
+	setup_work_tree();
+
+	die("TODO");
+}
diff --git a/git-rebase.sh b/git-legacy-rebase.sh
similarity index 100%
rename from git-rebase.sh
rename to git-legacy-rebase.sh
diff --git a/git.c b/git.c
index 9dbe6ffaa7..0e35632464 100644
--- a/git.c
+++ b/git.c
@@ -518,6 +518,12 @@ static struct cmd_struct commands[] = {
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
 	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
+	/*
+	 * NEEDSWORK: Until the rebase is independent and needs no redirection
+	 * to rebase shell script this is kept as is, then should be changed to
+	 * RUN_SETUP | NEED_WORK_TREE
+	 */
+	{ "rebase", cmd_rebase },
 	{ "rebase--helper", cmd_rebase__helper, RUN_SETUP | NEED_WORK_TREE },
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
-- 
2.18.0
Junio C Hamano· Jul 6, 2018, 21:09 UTC · re: Pratik Karki · lore

Re: [PATCH v3 1/4] rebase: start implementing it as a builtin

Pratik Karki <predatoramigo@gmail.com> writes:
Show 34 quoted lines
> +static int use_builtin_rebase(void)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	int ret;
> +
> +	argv_array_pushl(&cp.args,
> +			 "config", "--bool", "rebase.usebuiltin", NULL);
> +	cp.git_cmd = 1;
> +	if (capture_command(&cp, &out, 6))
> +		return 0;
> +
> +	strbuf_trim(&out);
> +	ret = !strcmp("true", out.buf);
> +	strbuf_release(&out);
> +	return ret;
> +}
> +
> +int cmd_rebase(int argc, const char **argv, const char *prefix)
> +{
> +	/*
> +	 * NEEDSWORK: Once the builtin rebase has been tested enough
> +	 * and git-legacy-rebase.sh is retired to contrib/, this preamble
> +	 * can be removed.
> +	 */
> +
> +	if (!use_builtin_rebase()) {
> +		const char *path = mkpath("%s/git-legacy-rebase",
> +					  git_exec_path());
> +
> +		if (sane_execvp(path, (char **)argv) < 0)
> +			die_errno("could not exec %s", path);
> +		else
> +			die("sane_execvp() returned???");

This part got changed from returning 0 since the previous round, which makes sense, but if we are making this change relative to the original in be8a90e, just because we mention explicitly that we imitate that old commit, we must mention how and why we "improved" on it, in the log message.

	In the old original used while converting difftool, if
	sane_execvp() that attempts to run the legacy scripted
	version returned with non-negative status, the command
	silently exited without doing anything with success, but
	sane_execvp() should not retun with non-negative status in
	the first place, so make sure we die() to notice such an
	abnormal case.

or something like that, perhaps (but you should be able to shorten it further).

Between die() and BUG(), I am not sure which is more appropriate. It certainly is not _our_ bug if platform execvp() returns success, so BUG() would not help our developers all that much. But end-users won't be helped by being told that sane_execvp() returned, so die() is not all that useful, either. I guess it does not matter that much bewteen the two, as this is something that is not supposed to happen anyway ;-)

In any case, this one looks good.  Will queue.
Thanks.
Pratik Karki· Jul 6, 2018, 12:08 UTC · re: Pratik Karki · lore

[PATCH v3 2/4] rebase: refactor common shell functions into their own file

The functions present in `git-legacy-rebase.sh` are used by the rebase backends as they are implemented as shell script functions in the `git-rebase--<backend>` files.

To make the `builtin/rebase.c` work, we have to provide support via a Unix shell script snippet that uses these functions and so, we want to use the rebase backends *directly* from the builtin rebase without going through `git-legacy-rebase.sh`.

This commit extracts the functions to a separate file, `git-rebase--common`, that will be read by `git-legacy-rebase.sh` and by the shell script snippets which will be used extensively in the following commits.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 .gitignore            |  1 +
 Makefile              |  1 +
 git-legacy-rebase.sh  | 62 +------------------------------------------
 git-rebase--common.sh | 61 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 64 insertions(+), 61 deletions(-)
 create mode 100644 git-rebase--common.sh
Show changes to 4 files +64 −61

.gitignore, Makefile, git-legacy-rebase.sh, git-rebase--common.sh

diff --git a/.gitignore b/.gitignore
index ec23959014..824141cba1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -117,6 +117,7 @@
 /git-read-tree
 /git-rebase
 /git-rebase--am
+/git-rebase--common
 /git-rebase--helper
 /git-rebase--interactive
 /git-rebase--merge
diff --git a/Makefile b/Makefile
index e88fe2e5fb..163e52ad1a 100644
--- a/Makefile
+++ b/Makefile
@@ -619,6 +619,7 @@ SCRIPT_SH += git-web--browse.sh
 SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
+SCRIPT_LIB += git-rebase--common
 SCRIPT_LIB += git-rebase--interactive
 SCRIPT_LIB += git-rebase--preserve-merges
 SCRIPT_LIB += git-rebase--merge
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 19bdebb480..240aac507c 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -102,6 +102,7 @@ case "$(git config --bool commit.gpgsign)" in
 true)	gpg_sign_opt=-S ;;
 *)	gpg_sign_opt= ;;
 esac
+. git-rebase--common
 
 read_basic_state () {
 	test -f "$state_dir/head-name" &&
@@ -132,67 +133,6 @@ read_basic_state () {
 	}
 }
 
-write_basic_state () {
-	echo "$head_name" > "$state_dir"/head-name &&
-	echo "$onto" > "$state_dir"/onto &&
-	echo "$orig_head" > "$state_dir"/orig-head &&
-	echo "$GIT_QUIET" > "$state_dir"/quiet &&
-	test t = "$verbose" && : > "$state_dir"/verbose
-	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
-	test -n "$strategy_opts" && echo "$strategy_opts" > \
-		"$state_dir"/strategy_opts
-	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
-		"$state_dir"/allow_rerere_autoupdate
-	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
-	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
-}
-
-output () {
-	case "$verbose" in
-	'')
-		output=$("$@" 2>&1 )
-		status=$?
-		test $status != 0 && printf "%s\n" "$output"
-		return $status
-		;;
-	*)
-		"$@"
-		;;
-	esac
-}
-
-move_to_original_branch () {
-	case "$head_name" in
-	refs/*)
-		message="rebase finished: $head_name onto $onto"
-		git update-ref -m "$message" \
-			$head_name $(git rev-parse HEAD) $orig_head &&
-		git symbolic-ref \
-			-m "rebase finished: returning to $head_name" \
-			HEAD $head_name ||
-		die "$(eval_gettext "Could not move back to \$head_name")"
-		;;
-	esac
-}
-
-apply_autostash () {
-	if test -f "$state_dir/autostash"
-	then
-		stash_sha1=$(cat "$state_dir/autostash")
-		if git stash apply $stash_sha1 >/dev/null 2>&1
-		then
-			echo "$(gettext 'Applied autostash.')" >&2
-		else
-			git stash store -m "autostash" -q $stash_sha1 ||
-			die "$(eval_gettext "Cannot store \$stash_sha1")"
-			gettext 'Applying autostash resulted in conflicts.
-Your changes are safe in the stash.
-You can run "git stash pop" or "git stash drop" at any time.
-' >&2
-		fi
-	fi
-}
-
 finish_rebase () {
 	rm -f "$(git rev-parse --git-path REBASE_HEAD)"
 	apply_autostash &&
diff --git a/git-rebase--common.sh b/git-rebase--common.sh
new file mode 100644
index 0000000000..5ba0603924
--- /dev/null
+++ b/git-rebase--common.sh
@@ -0,0 +1,61 @@
+
+write_basic_state () {
+	echo "$head_name" > "$state_dir"/head-name &&
+	echo "$onto" > "$state_dir"/onto &&
+	echo "$orig_head" > "$state_dir"/orig-head &&
+	echo "$GIT_QUIET" > "$state_dir"/quiet &&
+	test t = "$verbose" && : > "$state_dir"/verbose
+	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
+	test -n "$strategy_opts" && echo "$strategy_opts" > \
+		"$state_dir"/strategy_opts
+	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
+		"$state_dir"/allow_rerere_autoupdate
+	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
+	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
+}
+
+apply_autostash () {
+	if test -f "$state_dir/autostash"
+	then
+		stash_sha1=$(cat "$state_dir/autostash")
+		if git stash apply $stash_sha1 >/dev/null 2>&1
+		then
+			echo "$(gettext 'Applied autostash.')" >&2
+		else
+			git stash store -m "autostash" -q $stash_sha1 ||
+			die "$(eval_gettext "Cannot store \$stash_sha1")"
+			gettext 'Applying autostash resulted in conflicts.
+Your changes are safe in the stash.
+You can run "git stash pop" or "git stash drop" at any time.
+' >&2
+		fi
+	fi
+}
+
+move_to_original_branch () {
+	case "$head_name" in
+	refs/*)
+		message="rebase finished: $head_name onto $onto"
+		git update-ref -m "$message" \
+			$head_name $(git rev-parse HEAD) $orig_head &&
+		git symbolic-ref \
+			-m "rebase finished: returning to $head_name" \
+			HEAD $head_name ||
+		die "$(eval_gettext "Could not move back to \$head_name")"
+		;;
+	esac
+}
+
+output () {
+	case "$verbose" in
+	'')
+		output=$("$@" 2>&1 )
+		status=$?
+		test $status != 0 && printf "%s\n" "$output"
+		return $status
+		;;
+	*)
+		"$@"
+		;;
+	esac
+}
-- 
2.18.0
Johannes Schindelin· Jul 6, 2018, 12:36 UTC · re: Pratik Karki · lore

Re: [PATCH v3 2/4] rebase: refactor common shell functions into their own file

Hi Pratik,
On Fri, 6 Jul 2018, Pratik Karki wrote:
Show 13 quoted lines
> The functions present in `git-legacy-rebase.sh` are used by the rebase
> backends as they are implemented as shell script functions in the
> `git-rebase--<backend>` files.
> 
> To make the `builtin/rebase.c` work, we have to provide support via
> a Unix shell script snippet that uses these functions and so, we
> want to use the rebase backends *directly* from the builtin rebase
> without going through `git-legacy-rebase.sh`.
> 
> This commit extracts the functions to a separate file,
> `git-rebase--common`, that will be read by `git-legacy-rebase.sh` and
> by the shell script snippets which will be used extensively in the
> following commits.
Good.

While this seems to catch all the functions required by the backends, I am fairly certain that the `resolvemsg` variable is used exclusively by the backends, and it should therefore also moved into `git-rebase--common`. See my comments on your https://github.com/git/git/pull/505 for more details.

Ciao, Dscho

Pratik Karki· Jul 6, 2018, 12:08 UTC · re: Pratik Karki · lore

[PATCH v3 3/4] sequencer: refactor the code to detach HEAD to checkout.c

In the upcoming builtin rebase, we will have to start by detaching the HEAD, just like shell script version does. Essentially, we have to do the same thing as `git checkout -q <revision>^0 --`, in pure C.

The aforementioned functionality was already present in `sequencer.c` in `do_reset()` function. But `do_reset()` performs more than detaching the HEAD, and performs action specific to `sequencer.c`.

So this commit refactors out that part from `do_reset()`, and moves it to a new function called `detach_head_to()`. As this function has nothing to do with the sequencer, and everything to do with what `git checkout -q <revision>^0 --` does, we move that function to checkout.c.

This refactoring actually introduces a slight change in behavior: previously, the index was locked before parsing the argument to the todo command `reset`, while it now gets locked *after* that, in the `detach_head_to()` function.

It does not make a huge difference, and the upside is that this closes a few (unlikely) code paths where the index would not be unlocked upon error.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 checkout.c  | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 checkout.h  |  3 +++
 sequencer.c | 58 +++++-------------------------------------------
 3 files changed, 72 insertions(+), 53 deletions(-)
Show changes to 3 files +72 −53

checkout.c, checkout.h, sequencer.c

diff --git a/checkout.c b/checkout.c
index bdefc888ba..da68915fd7 100644
--- a/checkout.c
+++ b/checkout.c
@@ -2,6 +2,11 @@
 #include "remote.h"
 #include "refspec.h"
 #include "checkout.h"
+#include "unpack-trees.h"
+#include "lockfile.h"
+#include "refs.h"
+#include "tree.h"
+#include "cache-tree.h"
 
 struct tracking_name_data {
 	/* const */ char *src_ref;
@@ -42,3 +47,62 @@ const char *unique_tracking_name(const char *name, struct object_id *oid)
 	free(cb_data.dst_ref);
 	return NULL;
 }
+
+int detach_head_to(struct object_id *oid, const char *action,
+		   const char *reflog_message)
+{
+	struct strbuf ref_name = STRBUF_INIT;
+	struct tree_desc desc;
+	struct lock_file lock = LOCK_INIT;
+	struct unpack_trees_options unpack_tree_opts;
+	struct tree *tree;
+	int ret = 0;
+
+	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
+		return -1;
+
+	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
+	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
+	unpack_tree_opts.head_idx = 1;
+	unpack_tree_opts.src_index = &the_index;
+	unpack_tree_opts.dst_index = &the_index;
+	unpack_tree_opts.fn = oneway_merge;
+	unpack_tree_opts.merge = 1;
+	unpack_tree_opts.update = 1;
+
+	if (read_cache_unmerged()) {
+		rollback_lock_file(&lock);
+		strbuf_release(&ref_name);
+		return error_resolve_conflict(_(action));
+	}
+
+	if (!fill_tree_descriptor(&desc, oid)) {
+		error(_("failed to find tree of %s"), oid_to_hex(oid));
+		rollback_lock_file(&lock);
+		free((void *)desc.buffer);
+		strbuf_release(&ref_name);
+		return -1;
+	}
+
+	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
+		rollback_lock_file(&lock);
+		free((void *)desc.buffer);
+		strbuf_release(&ref_name);
+		return -1;
+	}
+
+	tree = parse_tree_indirect(oid);
+	prime_cache_tree(&the_index, tree);
+
+	if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
+		ret = error(_("could not write index"));
+	free((void *)desc.buffer);
+
+	if (!ret)
+		ret = update_ref(reflog_message, "HEAD", oid,
+				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+
+	strbuf_release(&ref_name);
+	return ret;
+
+}
diff --git a/checkout.h b/checkout.h
index 9980711179..6ce46cf4e8 100644
--- a/checkout.h
+++ b/checkout.h
@@ -10,4 +10,7 @@
  */
 extern const char *unique_tracking_name(const char *name, struct object_id *oid);
 
+int detach_head_to(struct object_id *oid, const char *action,
+		   const char *reflog_message);
+
 #endif /* CHECKOUT_H */
diff --git a/sequencer.c b/sequencer.c
index 5354d4d51e..106d518f4d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -29,6 +29,7 @@
 #include "oidset.h"
 #include "commit-slab.h"
 #include "alias.h"
+#include "checkout.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -2756,14 +2757,7 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
 {
 	struct strbuf ref_name = STRBUF_INIT;
 	struct object_id oid;
-	struct lock_file lock = LOCK_INIT;
-	struct tree_desc desc;
-	struct tree *tree;
-	struct unpack_trees_options unpack_tree_opts;
-	int ret = 0, i;
-
-	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
-		return -1;
+	int i;
 
 	if (len == 10 && !strncmp("[new root]", name, len)) {
 		if (!opts->have_squash_onto) {
@@ -2789,56 +2783,14 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
 		if (get_oid(ref_name.buf, &oid) &&
 		    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
 			error(_("could not read '%s'"), ref_name.buf);
-			rollback_lock_file(&lock);
 			strbuf_release(&ref_name);
 			return -1;
 		}
 	}
 
-	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
-	setup_unpack_trees_porcelain(&unpack_tree_opts, "reset");
-	unpack_tree_opts.head_idx = 1;
-	unpack_tree_opts.src_index = &the_index;
-	unpack_tree_opts.dst_index = &the_index;
-	unpack_tree_opts.fn = oneway_merge;
-	unpack_tree_opts.merge = 1;
-	unpack_tree_opts.update = 1;
-
-	if (read_cache_unmerged()) {
-		rollback_lock_file(&lock);
-		strbuf_release(&ref_name);
-		return error_resolve_conflict(_(action_name(opts)));
-	}
-
-	if (!fill_tree_descriptor(&desc, &oid)) {
-		error(_("failed to find tree of %s"), oid_to_hex(&oid));
-		rollback_lock_file(&lock);
-		free((void *)desc.buffer);
-		strbuf_release(&ref_name);
-		return -1;
-	}
-
-	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
-		rollback_lock_file(&lock);
-		free((void *)desc.buffer);
-		strbuf_release(&ref_name);
-		return -1;
-	}
-
-	tree = parse_tree_indirect(&oid);
-	prime_cache_tree(&the_index, tree);
-
-	if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
-		ret = error(_("could not write index"));
-	free((void *)desc.buffer);
-
-	if (!ret)
-		ret = update_ref(reflog_message(opts, "reset", "'%.*s'",
-						len, name), "HEAD", &oid,
-				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
-
-	strbuf_release(&ref_name);
-	return ret;
+	return detach_head_to(&oid, action_name(opts),
+			      reflog_message(opts, "reset", "'%.*s'",
+					     len, name));
 }
 
 static int do_merge(struct commit *commit, const char *arg, int arg_len,
-- 
2.18.0
Pratik Karki· Jul 6, 2018, 12:08 UTC · re: Pratik Karki · lore

[PATCH v3 4/4] builtin/rebase: support running "git rebase <upstream>"

This patch gives life to the skeleton added in the previous patches: With this change, we can perform a elementary rebase (without any options).

It can be tested thusly by:
git -c rebase.usebuiltin=true rebase HEAD~2

The rebase backends (i.e. the shell script functions defined in `git-rebase--<backend>`) are still at work here and the "builtin rebase"'s purpose is simply to parse the options and set everything up so that those rebase backends can do their work.

Note: We take an alternative approach here which is *not* to set the
environment variables via `run_command_v_opt_cd_env()` because those
variables would then be visible by processes spawned from the rebase
backends. Instead, we work hard to set them in the shell script snippet.

The next commits will handle and support all the wonderful rebase options.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 builtin/rebase.c | 237 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 236 insertions(+), 1 deletion(-)
Show changes to builtin/rebase.c +236 −1
diff --git a/builtin/rebase.c b/builtin/rebase.c
index fab7fca37e..71f06f0789 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -9,6 +9,20 @@
 #include "exec-cmd.h"
 #include "argv-array.h"
 #include "dir.h"
+#include "packfile.h"
+#include "checkout.h"
+#include "refs.h"
+#include "quote.h"
+
+static GIT_PATH_FUNC(apply_dir, "rebase-apply");
+static GIT_PATH_FUNC(merge_dir, "rebase-merge");
+
+enum rebase_type {
+	REBASE_AM,
+	REBASE_MERGE,
+	REBASE_INTERACTIVE,
+	REBASE_PRESERVE_MERGES
+};
 
 static int use_builtin_rebase(void)
 {
@@ -28,8 +42,136 @@ static int use_builtin_rebase(void)
 	return ret;
 }
 
+static int apply_autostash(void)
+{
+	warning("TODO");
+	return 0;
+}
+
+struct rebase_options {
+	enum rebase_type type;
+	const char *state_dir;
+	struct commit *upstream;
+	const char *upstream_name;
+	char *head_name;
+	struct object_id orig_head;
+	struct commit *onto;
+	const char *onto_name;
+	const char *revisions;
+	const char *root;
+};
+
+static int finish_rebase(struct rebase_options *opts)
+{
+	struct strbuf dir = STRBUF_INIT;
+	const char *argv_gc_auto[] = { "gc", "--auto", NULL };
+
+	delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
+	apply_autostash();
+	close_all_packs(the_repository->objects);
+	/*
+	 * We ignore errors in 'gc --auto', since the
+	 * user should see them.
+	 */
+	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
+	strbuf_addstr(&dir, opts->state_dir);
+	remove_dir_recursively(&dir, 0);
+	strbuf_release(&dir);
+
+	return 0;
+}
+
+static struct commit *peel_committish(const char *name)
+{
+	struct object *obj;
+	struct object_id oid;
+
+	if (get_oid(name, &oid))
+		return NULL;
+	obj = parse_object(&oid);
+	return (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
+}
+
+static void add_var(struct strbuf *buf, const char *name, const char *value)
+{
+	strbuf_addstr(buf, name);
+	strbuf_addstr(buf, "=");
+	sq_quote_buf(buf, value);
+	strbuf_addstr(buf, "; ");
+}
+
+static int run_specific_rebase(struct rebase_options *opts)
+{
+	const char *argv[] = { NULL, NULL };
+	struct strbuf script_snippet = STRBUF_INIT;
+	int status;
+	const char *backend, *backend_func;
+
+	add_var(&script_snippet, "GIT_DIR", absolute_path(get_git_dir()));
+
+	add_var(&script_snippet, "upstream_name", opts->upstream_name);
+	add_var(&script_snippet, "upstream",
+				 oid_to_hex(&opts->upstream->object.oid));
+	add_var(&script_snippet, "head_name", opts->head_name);
+	add_var(&script_snippet, "orig_head", oid_to_hex(&opts->orig_head));
+	add_var(&script_snippet, "onto", oid_to_hex(&opts->onto->object.oid));
+	add_var(&script_snippet, "onto_name", opts->onto_name);
+	add_var(&script_snippet, "revisions", opts->revisions);
+
+	switch (opts->type) {
+	case REBASE_AM:
+		backend = "git-rebase--am";
+		backend_func = "git_rebase__am";
+		break;
+	case REBASE_INTERACTIVE:
+		backend = "git-rebase--interactive";
+		backend_func = "git_rebase__interactive";
+		break;
+	case REBASE_MERGE:
+		backend = "git-rebase--merge";
+		backend_func = "git_rebase__merge";
+		break;
+	case REBASE_PRESERVE_MERGES:
+		backend = "git-rebase--preserve-merges";
+		backend_func = "git_rebase__preserve_merges";
+		break;
+	default:
+		BUG("Unhandled rebase type %d", opts->type);
+		break;
+	}
+
+	strbuf_addf(&script_snippet,
+		    ". git-rebase--common && . %s && %s",
+		    backend, backend_func);
+	argv[0] = script_snippet.buf;
+
+	status = run_command_v_opt(argv, RUN_USING_SHELL);
+	if (status == 0)
+		finish_rebase(opts);
+	else if (status == 2) {
+		struct strbuf dir = STRBUF_INIT;
+
+		apply_autostash();
+		strbuf_addstr(&dir, opts->state_dir);
+		remove_dir_recursively(&dir, 0);
+		strbuf_release(&dir);
+		die("Nothing to do");
+	}
+
+	strbuf_release(&script_snippet);
+
+	return status ? -1 : 0;
+}
+
 int cmd_rebase(int argc, const char **argv, const char *prefix)
 {
+	struct rebase_options options = { -1 };
+	const char *branch_name;
+	int ret, flags, quiet = 0;
+	struct strbuf msg = STRBUF_INIT;
+	struct strbuf revisions = STRBUF_INIT;
+	const char *restrict_revision = NULL;
+
 	/*
 	 * NEEDSWORK: Once the builtin rebase has been tested enough
 	 * and git-legacy-rebase.sh is retired to contrib/, this preamble
@@ -52,5 +194,98 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	trace_repo_setup(prefix);
 	setup_work_tree();
 
-	die("TODO");
+	options.type = REBASE_AM;
+
+	switch (options.type) {
+	case REBASE_AM:
+		options.state_dir = apply_dir();
+		break;
+	case REBASE_MERGE:
+	case REBASE_INTERACTIVE:
+	case REBASE_PRESERVE_MERGES:
+		options.state_dir = merge_dir();
+		break;
+	}
+	if (!options.root) {
+		if (argc != 2)
+			die("TODO: handle @{upstream}");
+		else {
+			options.upstream_name = argv[1];
+			argc--;
+			argv++;
+			if (!strcmp(options.upstream_name, "-"))
+				options.upstream_name = "@{-1}";
+		}
+		options.upstream = peel_committish(options.upstream_name);
+		if (!options.upstream)
+			die(_("invalid upstream '%s'"), options.upstream_name);
+	} else
+		die("TODO: upstream for --root");
+
+	/* Make sure the branch to rebase onto is valid. */
+	if (!options.onto_name)
+		options.onto_name = options.upstream_name;
+	if (strstr(options.onto_name, "...")) {
+		die("TODO");
+	} else {
+		options.onto = peel_committish(options.onto_name);
+		if (!options.onto)
+			die(_("Does not point to a valid commit '%s'"),
+				options.onto_name);
+	}
+
+	/*
+	* If the branch to rebase is given, that is the branch we will rebase
+	* branch_name -- branch/commit being rebased, or HEAD (already detached)
+	* orig_head -- commit object name of tip of the branch before rebasing
+	* head_name -- refs/heads/<that-branch> or "detached HEAD"
+	*/
+	if (argc > 1)
+		 die ("TODO: handle switch_to");
+	else {
+		/* Do not need to switch branches, we are already on it. */
+		options.head_name =
+			xstrdup_or_null(resolve_ref_unsafe("HEAD", 0, NULL,
+					 &flags));
+		if (!options.head_name)
+			die(_("No such ref: %s"), "HEAD");
+		if (flags & REF_ISSYMREF) {
+			if (!skip_prefix(options.head_name,
+					 "refs/heads/", &branch_name))
+				branch_name = options.head_name;
+
+		} else {
+			options.head_name = xstrdup("detached HEAD");
+			branch_name = "HEAD";
+		}
+		if (get_oid("HEAD", &options.orig_head))
+			die(_("Could not resolve HEAD to a revision"));
+	}
+
+	/* Detach HEAD and reset the tree */
+	if (!quiet)
+		printf("First, rewinding head to replay your work on top of it...");
+
+	strbuf_addf(&msg, "rebase: checkout %s", options.onto_name);
+	if (detach_head_to(&options.onto->object.oid, "checkout", msg.buf))
+		die(_("Could not detach HEAD"));
+	strbuf_release(&msg);
+	if (update_ref("rebase", "ORIG_HEAD", &options.orig_head, NULL, 0,
+		       UPDATE_REFS_MSG_ON_ERR) < 0)
+		die(_("Could not update ORIG_HEAD to '%s'"),
+		    oid_to_hex(&options.orig_head));
+
+	strbuf_addf(&revisions, "%s..%s",
+		!options.root ? oid_to_hex(&options.onto->object.oid) :
+		    (restrict_revision ? restrict_revision :
+			    oid_to_hex(&options.upstream->object.oid)),
+			    oid_to_hex(&options.orig_head));
+
+	options.revisions = revisions.buf;
+
+	ret = !!run_specific_rebase(&options);
+
+	strbuf_release(&revisions);
+	free(options.head_name);
+	return ret;
 }
-- 
2.18.0
Junio C Hamano· Jul 6, 2018, 21:30 UTC · re: Pratik Karki · lore

Re: [PATCH v3 4/4] builtin/rebase: support running "git rebase <upstream>"

Pratik Karki <predatoramigo@gmail.com> writes:
Show 25 quoted lines
> +static void add_var(struct strbuf *buf, const char *name, const char *value)
> +{
> +	strbuf_addstr(buf, name);
> +	strbuf_addstr(buf, "=");
> +	sq_quote_buf(buf, value);
> +	strbuf_addstr(buf, "; ");
> +}
> +
> +static int run_specific_rebase(struct rebase_options *opts)
> +{
> +	const char *argv[] = { NULL, NULL };
> +	struct strbuf script_snippet = STRBUF_INIT;
> +	int status;
> +	const char *backend, *backend_func;
> +
> +	add_var(&script_snippet, "GIT_DIR", absolute_path(get_git_dir()));
> +
> +	add_var(&script_snippet, "upstream_name", opts->upstream_name);
> +	add_var(&script_snippet, "upstream",
> +				 oid_to_hex(&opts->upstream->object.oid));
> +	add_var(&script_snippet, "head_name", opts->head_name);
> +	add_var(&script_snippet, "orig_head", oid_to_hex(&opts->orig_head));
> +	add_var(&script_snippet, "onto", oid_to_hex(&opts->onto->object.oid));
> +	add_var(&script_snippet, "onto_name", opts->onto_name);
> +	add_var(&script_snippet, "revisions", opts->revisions);
Looks alright.
Show 7 quoted lines
> +	switch (opts->type) {
> +	case REBASE_AM:
> +...
> +	default:
> +		BUG("Unhandled rebase type %d", opts->type);
> +		break;
> +	}
Better.
Show 6 quoted lines
> +	strbuf_addf(&script_snippet,
> +		    ". git-rebase--common && . %s && %s",
> +		    backend, backend_func);
> +	argv[0] = script_snippet.buf;
> +
> +	status = run_command_v_opt(argv, RUN_USING_SHELL);

This used to use run_command_v_opt_cd_env() to pass extra environment but now we can do a simpler one. As cmd_rebase() has called setup_git_directory() to chdir to the top level of the working tree, we just want to .-source the backend within the current working directory, so loss of cd is also good ;-)

Show 20 quoted lines
> +	if (status == 0)
> +		finish_rebase(opts);
> +	else if (status == 2) {
> +		struct strbuf dir = STRBUF_INIT;
> +
> +		apply_autostash();
> +		strbuf_addstr(&dir, opts->state_dir);
> +		remove_dir_recursively(&dir, 0);
> +		strbuf_release(&dir);
> +		die("Nothing to do");
> +	}
> +
> +	strbuf_release(&script_snippet);
> +
> +	return status ? -1 : 0;
> +}
> +
>  int cmd_rebase(int argc, const char **argv, const char *prefix)
>  {
> +	struct rebase_options options = { -1 };

The first field of this struct is "enum rebase_type" defined without "REBASE_TYPE_UNSPECIFIED = -1" in it. It probably makes sense to add that constant there so that you can spell this value out.

Show 16 quoted lines
> @@ -52,5 +194,98 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	trace_repo_setup(prefix);
>  	setup_work_tree();
>  
> -	die("TODO");
> +	options.type = REBASE_AM;
> +
> +	switch (options.type) {
> +	case REBASE_AM:
> +		options.state_dir = apply_dir();
> +		break;
> +	case REBASE_MERGE:
> +	case REBASE_INTERACTIVE:
> +	case REBASE_PRESERVE_MERGES:
> +		options.state_dir = merge_dir();
> +		break;

Have "default:" here that barfs with BUG() to help future introduction of bugs as the code to set options.type grows complexity over time.

Show 31 quoted lines
> +	}
> +	if (!options.root) {
> +		if (argc != 2)
> +			die("TODO: handle @{upstream}");
> +		else {
> +			options.upstream_name = argv[1];
> +			argc--;
> +			argv++;
> +			if (!strcmp(options.upstream_name, "-"))
> +				options.upstream_name = "@{-1}";
> +		}
> +		options.upstream = peel_committish(options.upstream_name);
> +		if (!options.upstream)
> +			die(_("invalid upstream '%s'"), options.upstream_name);
> +	} else
> +		die("TODO: upstream for --root");
> +
> +	/* Make sure the branch to rebase onto is valid. */
> +	if (!options.onto_name)
> +		options.onto_name = options.upstream_name;
> +	if (strstr(options.onto_name, "...")) {
> +		die("TODO");
> +	} else {
> +		options.onto = peel_committish(options.onto_name);
> +		if (!options.onto)
> +			die(_("Does not point to a valid commit '%s'"),
> +				options.onto_name);
> +	}
> +
> +	/*
> +	* If the branch to rebase is given, that is the branch we will rebase
Style: align asterisks.
Show 6 quoted lines
> +	* branch_name -- branch/commit being rebased, or HEAD (already detached)
> +	* orig_head -- commit object name of tip of the branch before rebasing
> +	* head_name -- refs/heads/<that-branch> or "detached HEAD"
> +	*/
> +	if (argc > 1)
> +		 die ("TODO: handle switch_to");
Style: no SP between "die" and "("
Christian Couder· Jul 7, 2018, 06:45 UTC · re: Pratik Karki · lore

Re: [PATCH v3 4/4] builtin/rebase: support running "git rebase <upstream>"

On Fri, Jul 6, 2018 at 2:08 PM, Pratik Karki <predatoramigo@gmail.com> wrote:
Show 20 quoted lines
> +       switch (opts->type) {
> +       case REBASE_AM:
> +               backend = "git-rebase--am";
> +               backend_func = "git_rebase__am";
> +               break;
> +       case REBASE_INTERACTIVE:
> +               backend = "git-rebase--interactive";
> +               backend_func = "git_rebase__interactive";
> +               break;
> +       case REBASE_MERGE:
> +               backend = "git-rebase--merge";
> +               backend_func = "git_rebase__merge";
> +               break;
> +       case REBASE_PRESERVE_MERGES:
> +               backend = "git-rebase--preserve-merges";
> +               backend_func = "git_rebase__preserve_merges";
> +               break;
> +       default:
> +               BUG("Unhandled rebase type %d", opts->type);
> +               break;
Nit: I think the "break;" line could be removed as the BUG() should always exit.

A quick grep shows that there are other places where there is a "break;" line after a BUG() though. Maybe one of the #leftoverbits could be about removing those "break;" lines.

> +       }
Johannes Schindelin· Jul 7, 2018, 11:59 UTC · re: Christian Couder · lore

Re: [PATCH v3 4/4] builtin/rebase: support running "git rebase <upstream>"

Hi Christian,
On Sat, 7 Jul 2018, Christian Couder wrote:
Show 28 quoted lines
> On Fri, Jul 6, 2018 at 2:08 PM, Pratik Karki <predatoramigo@gmail.com> wrote:
> 
> > +       switch (opts->type) {
> > +       case REBASE_AM:
> > +               backend = "git-rebase--am";
> > +               backend_func = "git_rebase__am";
> > +               break;
> > +       case REBASE_INTERACTIVE:
> > +               backend = "git-rebase--interactive";
> > +               backend_func = "git_rebase__interactive";
> > +               break;
> > +       case REBASE_MERGE:
> > +               backend = "git-rebase--merge";
> > +               backend_func = "git_rebase__merge";
> > +               break;
> > +       case REBASE_PRESERVE_MERGES:
> > +               backend = "git-rebase--preserve-merges";
> > +               backend_func = "git_rebase__preserve_merges";
> > +               break;
> > +       default:
> > +               BUG("Unhandled rebase type %d", opts->type);
> > +               break;
> 
> Nit: I think the "break;" line could be removed as the BUG() should always exit.
> 
> A quick grep shows that there are other places where there is a
> "break;" line after a BUG() though. Maybe one of the #leftoverbits
> could be about removing those "break;" lines.

But what if there is a bug in the BUG() function? Shouldn't we then not call `die()` directly after the `BUG()`?

Okay, sorry, I let myself loose a little, as I think that we are still safely in the territory where the code needs to be made correct. We can nitpick when there are no "biggies" left to comment about. Maybe focus a little more on whether the code does what it should do, rather than whether some stylistic guidelines are violated?

I mean, we can argue back and forth about white-space, indentation, superfluous break statements, etc. But that way, we won't get anywhere with the builtin rebase.

Let's help Pratik instead to complete his project, okay?

Ciao, Dscho

Junio C Hamano· Jul 7, 2018, 16:24 UTC · re: Christian Couder · lore

Re: [PATCH v3 4/4] builtin/rebase: support running "git rebase <upstream>"

Christian Couder <christian.couder@gmail.com> writes:
Show 9 quoted lines
>> +       default:
>> +               BUG("Unhandled rebase type %d", opts->type);
>> +               break;
>
> Nit: I think the "break;" line could be removed as the BUG() should always exit.
>
> A quick grep shows that there are other places where there is a
> "break;" line after a BUG() though. Maybe one of the #leftoverbits
> could be about removing those "break;" lines.

I do not see the above as nit, though. "could" is often quite a different thing from "should", and in this case (and all the other cases you incorrectly mark with %leftoverbits) removal of these lines does not improve readability. In fact, if there were another case arm after this one, having "break" there would help those who scan the code, as the person who wants to ensure what that next case arm does is correct is given a strong hint by "break;" immediately before it that nothing in the previous case arm matters and does not have to even be looked at. By removing it you force such a reader to notice BUG() and reason that it does not matter because it does not return control to us (hence no fallthru).

Beat Bolli· Jul 17, 2018, 21:49 UTC · re: Pratik Karki · lore

Re: [PATCH v3 4/4] builtin/rebase: support running "git rebase <upstream>"

On 06.07.18 14:08, Pratik Karki wrote:
> +static GIT_PATH_FUNC(apply_dir, "rebase-apply");
> +static GIT_PATH_FUNC(merge_dir, "rebase-merge");
Maybe fix this up with

-static GIT_PATH_FUNC(apply_dir, "rebase-apply"); -static GIT_PATH_FUNC(merge_dir, "rebase-merge"); +static GIT_PATH_FUNC(apply_dir, "rebase-apply") +static GIT_PATH_FUNC(merge_dir, "rebase-merge")

?

(See https://public-inbox.org/git/20180709192537.18564-5-dev+git@drbeat.li/#t)

Cheers, Beat
Beat Bolli· Jul 17, 2018, 21:55 UTC · re: Beat Bolli · lore

Re: [PATCH v3 4/4] builtin/rebase: support running "git rebase <upstream>"

On 17.07.18 23:49, Beat Bolli wrote:
Show 12 quoted lines
> On 06.07.18 14:08, Pratik Karki wrote:
>> +static GIT_PATH_FUNC(apply_dir, "rebase-apply");
>> +static GIT_PATH_FUNC(merge_dir, "rebase-merge");
> 
> Maybe fix this up with
> 
> -static GIT_PATH_FUNC(apply_dir, "rebase-apply");
> -static GIT_PATH_FUNC(merge_dir, "rebase-merge");
> +static GIT_PATH_FUNC(apply_dir, "rebase-apply")
> +static GIT_PATH_FUNC(merge_dir, "rebase-merge")
> 
> ?

Sorry, this should have been a reply to [PATCH v4 4/4]. The remark still applies, though.

> (See https://public-inbox.org/git/20180709192537.18564-5-dev+git@drbeat.li/#t)
Cheers, Beat
Pratik Karki· Jul 8, 2018, 18:01 UTC · re: Pratik Karki · lore

[GSoC] [PATCH v4 0/4] rebase: rewrite rebase in C

As a GSoC project, I have been working on the builtin rebase.

The motivation behind the rewrite of rebase i.e. from shell script to C are for following reasons:

1.  Writing shell scripts and getting it to production is much faster
    than doing the equivalent in C but lacks in performance and extra
    workarounds are needed for non-POSIX platforms.
2.  Git for Windows is at loss as the installer size increases due to
    addition of extra dependencies for the shell scripts which are usually
    available in POSIX compliant platforms.

This series of patches serves to demonstrate a minimal builtin rebase which supports running `git rebase <upstream>` and also serves to ask for reviews.

Changes since v3:
  -  Fix commit message of `rebase: start implementing it as a builtin`.
  -  Acknowledge Junio's style reviews.
  -  Acknowledge Johannes Schindelin's review.
Pratik Karki (4):
  rebase: start implementing it as a builtin
  rebase: refactor common shell functions into their own file
  sequencer: refactor the code to detach HEAD to checkout.c
  builtin/rebase: support running "git rebase <upstream>"
 .gitignore                            |   2 +
 Makefile                              |   4 +-
 builtin.h                             |   1 +
 builtin/rebase.c                      | 292 ++++++++++++++++++++++++++
 checkout.c                            |  64 ++++++
 checkout.h                            |   3 +
 git-rebase.sh => git-legacy-rebase.sh |  69 +-----
 git-rebase--common.sh                 |  68 ++++++
 git.c                                 |   6 +
 sequencer.c                           |  58 +----
 10 files changed, 446 insertions(+), 121 deletions(-)
 create mode 100644 builtin/rebase.c
 rename git-rebase.sh => git-legacy-rebase.sh (89%)
 create mode 100644 git-rebase--common.sh
-- 
2.18.0
Pratik Karki· Jul 8, 2018, 18:01 UTC · re: Pratik Karki · lore

[PATCH v4 1/4] rebase: start implementing it as a builtin

This commit imitates the strategy that was used to convert the difftool to a builtin. We start by renaming the shell script `git-rebase.sh` to `git-legacy-rebase.sh` and introduce a `builtin/rebase.c` that simply executes the shell script version, unless the config setting `rebase.useBuiltin` is set to `true`.

The motivation behind this is to rewrite all the functionality of the shell script version in the aforementioned `rebase.c`, one by one and be able to conveniently test new features by configuring `rebase.useBuiltin`.

In the original difftool conversion, if sane_execvp() that attempts to run the legacy scripted version returned with non-negative status, the command silently exited without doing anything with success, but sane_execvp() should not retun with non-negative status in the first place, so we use die() to notice such an abnormal case.

We intentionally avoid reading the config directly to avoid messing up the GIT_* environment variables when we need to fall back to exec()ing the shell script. The test of builtin rebase can be done by `git -c rebase.useBuiltin=true rebase ...`

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 .gitignore                            |  1 +
 Makefile                              |  3 +-
 builtin.h                             |  1 +
 builtin/rebase.c                      | 56 +++++++++++++++++++++++++++
 git-rebase.sh => git-legacy-rebase.sh |  0
 git.c                                 |  6 +++
 6 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 builtin/rebase.c
 rename git-rebase.sh => git-legacy-rebase.sh (100%)
Show changes to 6 files +66 −1

.gitignore, Makefile, builtin.h, builtin/rebase.c, git-rebase.sh, git.c

diff --git a/.gitignore b/.gitignore
index 3284a1e9b1..ec23959014 100644
--- a/.gitignore
+++ b/.gitignore
@@ -78,6 +78,7 @@
 /git-init-db
 /git-interpret-trailers
 /git-instaweb
+/git-legacy-rebase
 /git-log
 /git-ls-files
 /git-ls-remote
diff --git a/Makefile b/Makefile
index 0cb6590f24..e88fe2e5fb 100644
--- a/Makefile
+++ b/Makefile
@@ -609,7 +609,7 @@ SCRIPT_SH += git-merge-one-file.sh
 SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
 SCRIPT_SH += git-quiltimport.sh
-SCRIPT_SH += git-rebase.sh
+SCRIPT_SH += git-legacy-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
 SCRIPT_SH += git-request-pull.sh
 SCRIPT_SH += git-stash.sh
@@ -1059,6 +1059,7 @@ BUILTIN_OBJS += builtin/prune.o
 BUILTIN_OBJS += builtin/pull.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase.o
 BUILTIN_OBJS += builtin/rebase--helper.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
diff --git a/builtin.h b/builtin.h
index 0362f1ce25..44651a447f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -202,6 +202,7 @@ extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
 extern int cmd_pull(int argc, const char **argv, const char *prefix);
 extern int cmd_push(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase(int argc, const char **argv, const char *prefix);
 extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase.c b/builtin/rebase.c
new file mode 100644
index 0000000000..fab7fca37e
--- /dev/null
+++ b/builtin/rebase.c
@@ -0,0 +1,56 @@
+/*
+ * "git rebase" builtin command
+ *
+ * Copyright (c) 2018 Pratik Karki
+ */
+
+#include "builtin.h"
+#include "run-command.h"
+#include "exec-cmd.h"
+#include "argv-array.h"
+#include "dir.h"
+
+static int use_builtin_rebase(void)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf out = STRBUF_INIT;
+	int ret;
+
+	argv_array_pushl(&cp.args,
+			 "config", "--bool", "rebase.usebuiltin", NULL);
+	cp.git_cmd = 1;
+	if (capture_command(&cp, &out, 6))
+		return 0;
+
+	strbuf_trim(&out);
+	ret = !strcmp("true", out.buf);
+	strbuf_release(&out);
+	return ret;
+}
+
+int cmd_rebase(int argc, const char **argv, const char *prefix)
+{
+	/*
+	 * NEEDSWORK: Once the builtin rebase has been tested enough
+	 * and git-legacy-rebase.sh is retired to contrib/, this preamble
+	 * can be removed.
+	 */
+
+	if (!use_builtin_rebase()) {
+		const char *path = mkpath("%s/git-legacy-rebase",
+					  git_exec_path());
+
+		if (sane_execvp(path, (char **)argv) < 0)
+			die_errno("could not exec %s", path);
+		else
+			die("sane_execvp() returned???");
+	}
+
+	if (argc != 2)
+		die("Usage: %s <base>", argv[0]);
+	prefix = setup_git_directory();
+	trace_repo_setup(prefix);
+	setup_work_tree();
+
+	die("TODO");
+}
diff --git a/git-rebase.sh b/git-legacy-rebase.sh
similarity index 100%
rename from git-rebase.sh
rename to git-legacy-rebase.sh
diff --git a/git.c b/git.c
index 9dbe6ffaa7..0e35632464 100644
--- a/git.c
+++ b/git.c
@@ -518,6 +518,12 @@ static struct cmd_struct commands[] = {
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
 	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
+	/*
+	 * NEEDSWORK: Until the rebase is independent and needs no redirection
+	 * to rebase shell script this is kept as is, then should be changed to
+	 * RUN_SETUP | NEED_WORK_TREE
+	 */
+	{ "rebase", cmd_rebase },
 	{ "rebase--helper", cmd_rebase__helper, RUN_SETUP | NEED_WORK_TREE },
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
-- 
2.18.0
Andrei Rybak· Jul 9, 2018, 07:59 UTC · re: Pratik Karki · lore

Re: [PATCH v4 1/4] rebase: start implementing it as a builtin

On 2018-07-08 20:01, Pratik Karki wrote:
Show 12 quoted lines
> +
> +static int use_builtin_rebase(void)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	int ret;
> +
> +	argv_array_pushl(&cp.args,
> +			 "config", "--bool", "rebase.usebuiltin", NULL);
> +	cp.git_cmd = 1;
> +	if (capture_command(&cp, &out, 6))
> +		return 0;
Does strbuf out leak on return here?
Show 6 quoted lines
> +
> +	strbuf_trim(&out);
> +	ret = !strcmp("true", out.buf);
> +	strbuf_release(&out);
> +	return ret;
> +}
Eric Sunshine· Jul 9, 2018, 08:36 UTC · re: Andrei Rybak · lore

Re: [PATCH v4 1/4] rebase: start implementing it as a builtin

On Mon, Jul 9, 2018 at 3:59 AM Andrei Rybak <rybak.a.v@gmail.com> wrote:
Show 14 quoted lines
> On 2018-07-08 20:01, Pratik Karki wrote:
> > +static int use_builtin_rebase(void)
> > +{
> > +     struct child_process cp = CHILD_PROCESS_INIT;
> > +     struct strbuf out = STRBUF_INIT;
> > +     int ret;
> > +
> > +     argv_array_pushl(&cp.args,
> > +                      "config", "--bool", "rebase.usebuiltin", NULL);
> > +     cp.git_cmd = 1;
> > +     if (capture_command(&cp, &out, 6))
> > +             return 0;
>
> Does strbuf out leak on return here?

Good catch. This _is_ a potential leak. Here is an excerpt from the documentation of pipe_command(), which is called by capture_command():

    Any output collected in the buffers is kept even if the
    command returns a non-zero exit.
So, yes, this needs a strbuf_release() before returning.
Show 5 quoted lines
> > +     strbuf_trim(&out);
> > +     ret = !strcmp("true", out.buf);
> > +     strbuf_release(&out);
> > +     return ret;
> > +}
Pratik Karki· Jul 9, 2018, 17:18 UTC · re: Eric Sunshine · lore

Re: [PATCH v4 1/4] rebase: start implementing it as a builtin

Hi,
On Mon, Jul 9, 2018 at 2:21 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 24 quoted lines
>
> On Mon, Jul 9, 2018 at 3:59 AM Andrei Rybak <rybak.a.v@gmail.com> wrote:
> > On 2018-07-08 20:01, Pratik Karki wrote:
> > > +static int use_builtin_rebase(void)
> > > +{
> > > +     struct child_process cp = CHILD_PROCESS_INIT;
> > > +     struct strbuf out = STRBUF_INIT;
> > > +     int ret;
> > > +
> > > +     argv_array_pushl(&cp.args,
> > > +                      "config", "--bool", "rebase.usebuiltin", NULL);
> > > +     cp.git_cmd = 1;
> > > +     if (capture_command(&cp, &out, 6))
> > > +             return 0;
> >
> > Does strbuf out leak on return here?
>
> Good catch. This _is_ a potential leak. Here is an excerpt from the
> documentation of pipe_command(), which is called by capture_command():
>
>     Any output collected in the buffers is kept even if the
>     command returns a non-zero exit.
>
> So, yes, this needs a strbuf_release() before returning.
Hmm. This seems to be a problem. Thanks for reviewing.
Duy Nguyen· Jul 22, 2018, 09:14 UTC · re: Pratik Karki · lore

Re: [PATCH v4 1/4] rebase: start implementing it as a builtin

On Sun, Jul 8, 2018 at 8:03 PM Pratik Karki <predatoramigo@gmail.com> wrote:
Show 14 quoted lines
> +int cmd_rebase(int argc, const char **argv, const char *prefix)
> +{
> +       /*
> +        * NEEDSWORK: Once the builtin rebase has been tested enough
> +        * and git-legacy-rebase.sh is retired to contrib/, this preamble
> +        * can be removed.
> +        */
> +
> +       if (!use_builtin_rebase()) {
> +               const char *path = mkpath("%s/git-legacy-rebase",
> +                                         git_exec_path());
> +
> +               if (sane_execvp(path, (char **)argv) < 0)
> +                       die_errno("could not exec %s", path);
Please wrap all user visible strings in thi series in _().
> +               else
> +                       die("sane_execvp() returned???");
or if it's definitely a bug in the code, go with BUG()
-- 
Duy
Pratik Karki· Jul 8, 2018, 18:01 UTC · re: Pratik Karki · lore

[PATCH v4 2/4] rebase: refactor common shell functions into their own file

The functions present in `git-legacy-rebase.sh` are used by the rebase backends as they are implemented as shell script functions in the `git-rebase--<backend>` files.

To make the `builtin/rebase.c` work, we have to provide support via a Unix shell script snippet that uses these functions and so, we want to use the rebase backends *directly* from the builtin rebase without going through `git-legacy-rebase.sh`.

This commit extracts the functions to a separate file, `git-rebase--common`, that will be read by `git-legacy-rebase.sh` and by the shell script snippets which will be used extensively in the following commits.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 .gitignore            |  1 +
 Makefile              |  1 +
 git-legacy-rebase.sh  | 69 ++-----------------------------------------
 git-rebase--common.sh | 68 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 67 deletions(-)
 create mode 100644 git-rebase--common.sh
Show changes to 4 files +72 −67

.gitignore, Makefile, git-legacy-rebase.sh, git-rebase--common.sh

diff --git a/.gitignore b/.gitignore
index ec23959014..824141cba1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -117,6 +117,7 @@
 /git-read-tree
 /git-rebase
 /git-rebase--am
+/git-rebase--common
 /git-rebase--helper
 /git-rebase--interactive
 /git-rebase--merge
diff --git a/Makefile b/Makefile
index e88fe2e5fb..163e52ad1a 100644
--- a/Makefile
+++ b/Makefile
@@ -619,6 +619,7 @@ SCRIPT_SH += git-web--browse.sh
 SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
+SCRIPT_LIB += git-rebase--common
 SCRIPT_LIB += git-rebase--interactive
 SCRIPT_LIB += git-rebase--preserve-merges
 SCRIPT_LIB += git-rebase--merge
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 19bdebb480..fbaf16420d 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -57,12 +57,7 @@ cd_to_toplevel
 LF='
 '
 ok_to_skip_pre_rebase=
-resolvemsg="
-$(gettext 'Resolve all conflicts manually, mark them as resolved with
-"git add/rm <conflicted_files>", then run "git rebase --continue".
-You can instead skip this commit: run "git rebase --skip".
-To abort and get back to the state before "git rebase", run "git rebase --abort".')
-"
+
 squash_onto=
 unset onto
 unset restrict_revision
@@ -102,6 +97,7 @@ case "$(git config --bool commit.gpgsign)" in
 true)	gpg_sign_opt=-S ;;
 *)	gpg_sign_opt= ;;
 esac
+. git-rebase--common
 
 read_basic_state () {
 	test -f "$state_dir/head-name" &&
@@ -132,67 +128,6 @@ read_basic_state () {
 	}
 }
 
-write_basic_state () {
-	echo "$head_name" > "$state_dir"/head-name &&
-	echo "$onto" > "$state_dir"/onto &&
-	echo "$orig_head" > "$state_dir"/orig-head &&
-	echo "$GIT_QUIET" > "$state_dir"/quiet &&
-	test t = "$verbose" && : > "$state_dir"/verbose
-	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
-	test -n "$strategy_opts" && echo "$strategy_opts" > \
-		"$state_dir"/strategy_opts
-	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
-		"$state_dir"/allow_rerere_autoupdate
-	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
-	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
-}
-
-output () {
-	case "$verbose" in
-	'')
-		output=$("$@" 2>&1 )
-		status=$?
-		test $status != 0 && printf "%s\n" "$output"
-		return $status
-		;;
-	*)
-		"$@"
-		;;
-	esac
-}
-
-move_to_original_branch () {
-	case "$head_name" in
-	refs/*)
-		message="rebase finished: $head_name onto $onto"
-		git update-ref -m "$message" \
-			$head_name $(git rev-parse HEAD) $orig_head &&
-		git symbolic-ref \
-			-m "rebase finished: returning to $head_name" \
-			HEAD $head_name ||
-		die "$(eval_gettext "Could not move back to \$head_name")"
-		;;
-	esac
-}
-
-apply_autostash () {
-	if test -f "$state_dir/autostash"
-	then
-		stash_sha1=$(cat "$state_dir/autostash")
-		if git stash apply $stash_sha1 >/dev/null 2>&1
-		then
-			echo "$(gettext 'Applied autostash.')" >&2
-		else
-			git stash store -m "autostash" -q $stash_sha1 ||
-			die "$(eval_gettext "Cannot store \$stash_sha1")"
-			gettext 'Applying autostash resulted in conflicts.
-Your changes are safe in the stash.
-You can run "git stash pop" or "git stash drop" at any time.
-' >&2
-		fi
-	fi
-}
-
 finish_rebase () {
 	rm -f "$(git rev-parse --git-path REBASE_HEAD)"
 	apply_autostash &&
diff --git a/git-rebase--common.sh b/git-rebase--common.sh
new file mode 100644
index 0000000000..7e39d22871
--- /dev/null
+++ b/git-rebase--common.sh
@@ -0,0 +1,68 @@
+
+resolvemsg="
+$(gettext 'Resolve all conflicts manually, mark them as resolved with
+"git add/rm <conflicted_files>", then run "git rebase --continue".
+You can instead skip this commit: run "git rebase --skip".
+To abort and get back to the state before "git rebase", run "git rebase --abort".')
+"
+
+write_basic_state () {
+	echo "$head_name" > "$state_dir"/head-name &&
+	echo "$onto" > "$state_dir"/onto &&
+	echo "$orig_head" > "$state_dir"/orig-head &&
+	echo "$GIT_QUIET" > "$state_dir"/quiet &&
+	test t = "$verbose" && : > "$state_dir"/verbose
+	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
+	test -n "$strategy_opts" && echo "$strategy_opts" > \
+		"$state_dir"/strategy_opts
+	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
+		"$state_dir"/allow_rerere_autoupdate
+	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
+	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
+}
+
+apply_autostash () {
+	if test -f "$state_dir/autostash"
+	then
+		stash_sha1=$(cat "$state_dir/autostash")
+		if git stash apply $stash_sha1 >/dev/null 2>&1
+		then
+			echo "$(gettext 'Applied autostash.')" >&2
+		else
+			git stash store -m "autostash" -q $stash_sha1 ||
+			die "$(eval_gettext "Cannot store \$stash_sha1")"
+			gettext 'Applying autostash resulted in conflicts.
+Your changes are safe in the stash.
+You can run "git stash pop" or "git stash drop" at any time.
+' >&2
+		fi
+	fi
+}
+
+move_to_original_branch () {
+	case "$head_name" in
+	refs/*)
+		message="rebase finished: $head_name onto $onto"
+		git update-ref -m "$message" \
+			$head_name $(git rev-parse HEAD) $orig_head &&
+		git symbolic-ref \
+			-m "rebase finished: returning to $head_name" \
+			HEAD $head_name ||
+		die "$(eval_gettext "Could not move back to \$head_name")"
+		;;
+	esac
+}
+
+output () {
+	case "$verbose" in
+	'')
+		output=$("$@" 2>&1 )
+		status=$?
+		test $status != 0 && printf "%s\n" "$output"
+		return $status
+		;;
+	*)
+		"$@"
+		;;
+	esac
+}
-- 
2.18.0
Pratik Karki· Jul 8, 2018, 18:01 UTC · re: Pratik Karki · lore

[PATCH v4 3/4] sequencer: refactor the code to detach HEAD to checkout.c

In the upcoming builtin rebase, we will have to start by detaching the HEAD, just like shell script version does. Essentially, we have to do the same thing as `git checkout -q <revision>^0 --`, in pure C.

The aforementioned functionality was already present in `sequencer.c` in `do_reset()` function. But `do_reset()` performs more than detaching the HEAD, and performs action specific to `sequencer.c`.

So this commit refactors out that part from `do_reset()`, and moves it to a new function called `detach_head_to()`. As this function has nothing to do with the sequencer, and everything to do with what `git checkout -q <revision>^0 --` does, we move that function to checkout.c.

This refactoring actually introduces a slight change in behavior: previously, the index was locked before parsing the argument to the todo command `reset`, while it now gets locked *after* that, in the `detach_head_to()` function.

It does not make a huge difference, and the upside is that this closes a few (unlikely) code paths where the index would not be unlocked upon error.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 checkout.c  | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 checkout.h  |  3 +++
 sequencer.c | 58 +++++-------------------------------------------
 3 files changed, 72 insertions(+), 53 deletions(-)
Show changes to 3 files +72 −53

checkout.c, checkout.h, sequencer.c

diff --git a/checkout.c b/checkout.c
index bdefc888ba..da68915fd7 100644
--- a/checkout.c
+++ b/checkout.c
@@ -2,6 +2,11 @@
 #include "remote.h"
 #include "refspec.h"
 #include "checkout.h"
+#include "unpack-trees.h"
+#include "lockfile.h"
+#include "refs.h"
+#include "tree.h"
+#include "cache-tree.h"
 
 struct tracking_name_data {
 	/* const */ char *src_ref;
@@ -42,3 +47,62 @@ const char *unique_tracking_name(const char *name, struct object_id *oid)
 	free(cb_data.dst_ref);
 	return NULL;
 }
+
+int detach_head_to(struct object_id *oid, const char *action,
+		   const char *reflog_message)
+{
+	struct strbuf ref_name = STRBUF_INIT;
+	struct tree_desc desc;
+	struct lock_file lock = LOCK_INIT;
+	struct unpack_trees_options unpack_tree_opts;
+	struct tree *tree;
+	int ret = 0;
+
+	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
+		return -1;
+
+	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
+	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
+	unpack_tree_opts.head_idx = 1;
+	unpack_tree_opts.src_index = &the_index;
+	unpack_tree_opts.dst_index = &the_index;
+	unpack_tree_opts.fn = oneway_merge;
+	unpack_tree_opts.merge = 1;
+	unpack_tree_opts.update = 1;
+
+	if (read_cache_unmerged()) {
+		rollback_lock_file(&lock);
+		strbuf_release(&ref_name);
+		return error_resolve_conflict(_(action));
+	}
+
+	if (!fill_tree_descriptor(&desc, oid)) {
+		error(_("failed to find tree of %s"), oid_to_hex(oid));
+		rollback_lock_file(&lock);
+		free((void *)desc.buffer);
+		strbuf_release(&ref_name);
+		return -1;
+	}
+
+	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
+		rollback_lock_file(&lock);
+		free((void *)desc.buffer);
+		strbuf_release(&ref_name);
+		return -1;
+	}
+
+	tree = parse_tree_indirect(oid);
+	prime_cache_tree(&the_index, tree);
+
+	if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
+		ret = error(_("could not write index"));
+	free((void *)desc.buffer);
+
+	if (!ret)
+		ret = update_ref(reflog_message, "HEAD", oid,
+				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+
+	strbuf_release(&ref_name);
+	return ret;
+
+}
diff --git a/checkout.h b/checkout.h
index 9980711179..6ce46cf4e8 100644
--- a/checkout.h
+++ b/checkout.h
@@ -10,4 +10,7 @@
  */
 extern const char *unique_tracking_name(const char *name, struct object_id *oid);
 
+int detach_head_to(struct object_id *oid, const char *action,
+		   const char *reflog_message);
+
 #endif /* CHECKOUT_H */
diff --git a/sequencer.c b/sequencer.c
index 5354d4d51e..106d518f4d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -29,6 +29,7 @@
 #include "oidset.h"
 #include "commit-slab.h"
 #include "alias.h"
+#include "checkout.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -2756,14 +2757,7 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
 {
 	struct strbuf ref_name = STRBUF_INIT;
 	struct object_id oid;
-	struct lock_file lock = LOCK_INIT;
-	struct tree_desc desc;
-	struct tree *tree;
-	struct unpack_trees_options unpack_tree_opts;
-	int ret = 0, i;
-
-	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
-		return -1;
+	int i;
 
 	if (len == 10 && !strncmp("[new root]", name, len)) {
 		if (!opts->have_squash_onto) {
@@ -2789,56 +2783,14 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
 		if (get_oid(ref_name.buf, &oid) &&
 		    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
 			error(_("could not read '%s'"), ref_name.buf);
-			rollback_lock_file(&lock);
 			strbuf_release(&ref_name);
 			return -1;
 		}
 	}
 
-	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
-	setup_unpack_trees_porcelain(&unpack_tree_opts, "reset");
-	unpack_tree_opts.head_idx = 1;
-	unpack_tree_opts.src_index = &the_index;
-	unpack_tree_opts.dst_index = &the_index;
-	unpack_tree_opts.fn = oneway_merge;
-	unpack_tree_opts.merge = 1;
-	unpack_tree_opts.update = 1;
-
-	if (read_cache_unmerged()) {
-		rollback_lock_file(&lock);
-		strbuf_release(&ref_name);
-		return error_resolve_conflict(_(action_name(opts)));
-	}
-
-	if (!fill_tree_descriptor(&desc, &oid)) {
-		error(_("failed to find tree of %s"), oid_to_hex(&oid));
-		rollback_lock_file(&lock);
-		free((void *)desc.buffer);
-		strbuf_release(&ref_name);
-		return -1;
-	}
-
-	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
-		rollback_lock_file(&lock);
-		free((void *)desc.buffer);
-		strbuf_release(&ref_name);
-		return -1;
-	}
-
-	tree = parse_tree_indirect(&oid);
-	prime_cache_tree(&the_index, tree);
-
-	if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
-		ret = error(_("could not write index"));
-	free((void *)desc.buffer);
-
-	if (!ret)
-		ret = update_ref(reflog_message(opts, "reset", "'%.*s'",
-						len, name), "HEAD", &oid,
-				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
-
-	strbuf_release(&ref_name);
-	return ret;
+	return detach_head_to(&oid, action_name(opts),
+			      reflog_message(opts, "reset", "'%.*s'",
+					     len, name));
 }
 
 static int do_merge(struct commit *commit, const char *arg, int arg_len,
-- 
2.18.0
Johannes Schindelin· Jul 8, 2018, 21:31 UTC · re: Pratik Karki · lore

Re: [PATCH v4 3/4] sequencer: refactor the code to detach HEAD to checkout.c

Hi Pratik,
On Sun, 8 Jul 2018, Pratik Karki wrote:
Show 74 quoted lines
> diff --git a/checkout.c b/checkout.c
> index bdefc888ba..da68915fd7 100644
> --- a/checkout.c
> +++ b/checkout.c
> @@ -2,6 +2,11 @@
>  #include "remote.h"
>  #include "refspec.h"
>  #include "checkout.h"
> +#include "unpack-trees.h"
> +#include "lockfile.h"
> +#include "refs.h"
> +#include "tree.h"
> +#include "cache-tree.h"
>  
>  struct tracking_name_data {
>  	/* const */ char *src_ref;
> @@ -42,3 +47,62 @@ const char *unique_tracking_name(const char *name, struct object_id *oid)
>  	free(cb_data.dst_ref);
>  	return NULL;
>  }
> +
> +int detach_head_to(struct object_id *oid, const char *action,
> +		   const char *reflog_message)
> +{
> +	struct strbuf ref_name = STRBUF_INIT;
> +	struct tree_desc desc;
> +	struct lock_file lock = LOCK_INIT;
> +	struct unpack_trees_options unpack_tree_opts;
> +	struct tree *tree;
> +	int ret = 0;
> +
> +	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> +		return -1;
> +
> +	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
> +	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
> +	unpack_tree_opts.head_idx = 1;
> +	unpack_tree_opts.src_index = &the_index;
> +	unpack_tree_opts.dst_index = &the_index;
> +	unpack_tree_opts.fn = oneway_merge;
> +	unpack_tree_opts.merge = 1;
> +	unpack_tree_opts.update = 1;
> +
> +	if (read_cache_unmerged()) {
> +		rollback_lock_file(&lock);
> +		strbuf_release(&ref_name);
> +		return error_resolve_conflict(_(action));
> +	}
> +
> +	if (!fill_tree_descriptor(&desc, oid)) {
> +		error(_("failed to find tree of %s"), oid_to_hex(oid));
> +		rollback_lock_file(&lock);
> +		free((void *)desc.buffer);
> +		strbuf_release(&ref_name);
> +		return -1;
> +	}
> +
> +	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
> +		rollback_lock_file(&lock);
> +		free((void *)desc.buffer);
> +		strbuf_release(&ref_name);
> +		return -1;
> +	}
> +
> +	tree = parse_tree_indirect(oid);
> +	prime_cache_tree(&the_index, tree);
> +
> +	if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
> +		ret = error(_("could not write index"));
> +	free((void *)desc.buffer);
> +
> +	if (!ret)
> +		ret = update_ref(reflog_message, "HEAD", oid,
> +				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);

I noticed that this does not actually detach the HEAD. That is my fault, of course, as I should have not only suggested refactoring the `do_reset()` function from `sequencer.c`, but I should also have remembered that that function has the benefit of *always* acting on a detached HEAD (because it runs during an interactive rebase), and therefore does not need to detach it explicitly.

In light of the `reset_hard()` function that you added in a `wip` (see https://github.com/git/git/pull/505/files#diff-c7361e406139e8cd3a300b80b8f8cc8dR296), I could imagine that it might be better, after all, to leave `do_reset()` alone and implement a `reset_hard()` function that also optionally detaches the `HEAD` (I *think* that the flag `REF_NO_DEREF` would do that for you).

Alternatively, just update the code in `do_reset()` to use that flag first, and only *then* extract the code to `checkout.c`.

(I could not resist, and made this quick change on top of your `wip-rebase`, and together with a couple more, obvious fixups, this lets t3403 pass. It still needs some things that you have not yet sent to the mailing list, such as support for `--skip`.)

Ciao, Dscho

Pratik Karki· Jul 9, 2018, 17:23 UTC · re: Johannes Schindelin · lore

Re: [PATCH v4 3/4] sequencer: refactor the code to detach HEAD to checkout.c

Hi, On Mon, Jul 9, 2018 at 3:16 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 93 quoted lines
>
> Hi Pratik,
>
> On Sun, 8 Jul 2018, Pratik Karki wrote:
>
> > diff --git a/checkout.c b/checkout.c
> > index bdefc888ba..da68915fd7 100644
> > --- a/checkout.c
> > +++ b/checkout.c
> > @@ -2,6 +2,11 @@
> >  #include "remote.h"
> >  #include "refspec.h"
> >  #include "checkout.h"
> > +#include "unpack-trees.h"
> > +#include "lockfile.h"
> > +#include "refs.h"
> > +#include "tree.h"
> > +#include "cache-tree.h"
> >
> >  struct tracking_name_data {
> >       /* const */ char *src_ref;
> > @@ -42,3 +47,62 @@ const char *unique_tracking_name(const char *name, struct object_id *oid)
> >       free(cb_data.dst_ref);
> >       return NULL;
> >  }
> > +
> > +int detach_head_to(struct object_id *oid, const char *action,
> > +                const char *reflog_message)
> > +{
> > +     struct strbuf ref_name = STRBUF_INIT;
> > +     struct tree_desc desc;
> > +     struct lock_file lock = LOCK_INIT;
> > +     struct unpack_trees_options unpack_tree_opts;
> > +     struct tree *tree;
> > +     int ret = 0;
> > +
> > +     if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> > +             return -1;
> > +
> > +     memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
> > +     setup_unpack_trees_porcelain(&unpack_tree_opts, action);
> > +     unpack_tree_opts.head_idx = 1;
> > +     unpack_tree_opts.src_index = &the_index;
> > +     unpack_tree_opts.dst_index = &the_index;
> > +     unpack_tree_opts.fn = oneway_merge;
> > +     unpack_tree_opts.merge = 1;
> > +     unpack_tree_opts.update = 1;
> > +
> > +     if (read_cache_unmerged()) {
> > +             rollback_lock_file(&lock);
> > +             strbuf_release(&ref_name);
> > +             return error_resolve_conflict(_(action));
> > +     }
> > +
> > +     if (!fill_tree_descriptor(&desc, oid)) {
> > +             error(_("failed to find tree of %s"), oid_to_hex(oid));
> > +             rollback_lock_file(&lock);
> > +             free((void *)desc.buffer);
> > +             strbuf_release(&ref_name);
> > +             return -1;
> > +     }
> > +
> > +     if (unpack_trees(1, &desc, &unpack_tree_opts)) {
> > +             rollback_lock_file(&lock);
> > +             free((void *)desc.buffer);
> > +             strbuf_release(&ref_name);
> > +             return -1;
> > +     }
> > +
> > +     tree = parse_tree_indirect(oid);
> > +     prime_cache_tree(&the_index, tree);
> > +
> > +     if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
> > +             ret = error(_("could not write index"));
> > +     free((void *)desc.buffer);
> > +
> > +     if (!ret)
> > +             ret = update_ref(reflog_message, "HEAD", oid,
> > +                              NULL, 0, UPDATE_REFS_MSG_ON_ERR);
>
> I noticed that this does not actually detach the HEAD. That is my fault,
> of course, as I should have not only suggested refactoring the
> `do_reset()` function from `sequencer.c`, but I should also have
> remembered that that function has the benefit of *always* acting on a
> detached HEAD (because it runs during an interactive rebase), and
> therefore does not need to detach it explicitly.
>
> In light of the `reset_hard()` function that you added in a `wip` (see
> https://github.com/git/git/pull/505/files#diff-c7361e406139e8cd3a300b80b8f8cc8dR296),
> I could imagine that it might be better, after all, to leave `do_reset()`
> alone and implement a `reset_hard()` function that also optionally
> detaches the `HEAD` (I *think* that the flag `REF_NO_DEREF` would do that
> for you).
Yes. I think this will be better. Thanks.
Show 7 quoted lines
> Alternatively, just update the code in `do_reset()` to use that flag
> first, and only *then* extract the code to `checkout.c`.
>
> (I could not resist, and made this quick change on top of your
> `wip-rebase`, and together with a couple more, obvious fixups, this lets
> t3403 pass. It still needs some things that you have not yet sent to the
> mailing list, such as support for `--skip`.)
Thank you for taking the time to review.
Junio C Hamano· Jul 9, 2018, 16:42 UTC · re: Pratik Karki · lore

Re: [PATCH v4 3/4] sequencer: refactor the code to detach HEAD to checkout.c

Pratik Karki <predatoramigo@gmail.com> writes:
Show 24 quoted lines
> In the upcoming builtin rebase, we will have to start by detaching
> the HEAD, just like shell script version does. Essentially, we have
> to do the same thing as `git checkout -q <revision>^0 --`, in pure C.
>
> The aforementioned functionality was already present in `sequencer.c`
> in `do_reset()` function. But `do_reset()` performs more than detaching
> the HEAD, and performs action specific to `sequencer.c`.
>
> So this commit refactors out that part from `do_reset()`, and moves it
> to a new function called `detach_head_to()`. As this function has
> nothing to do with the sequencer, and everything to do with what `git
> checkout -q <revision>^0 --` does, we move that function to checkout.c.
>
> This refactoring actually introduces a slight change in behavior:
> previously, the index was locked before parsing the argument to the
> todo command `reset`, while it now gets locked *after* that, in the
> `detach_head_to()` function.
>
> It does not make a huge difference, and the upside is that this closes
> a few (unlikely) code paths where the index would not be unlocked upon
> error.
>
> Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> ---

Here is a place to say "unchanged since v3" for a change like this one that changes neither the proposed log message above nor the patch below to help reviewers who have seen the previous round (they can stop reading here). Hopefully there are more reviewers than you who write new code, so spending a bit more time to help them spend less would be an overall win for the project.

Thanks.
Show 184 quoted lines
>  checkout.c  | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  checkout.h  |  3 +++
>  sequencer.c | 58 +++++-------------------------------------------
>  3 files changed, 72 insertions(+), 53 deletions(-)
>
> diff --git a/checkout.c b/checkout.c
> index bdefc888ba..da68915fd7 100644
> --- a/checkout.c
> +++ b/checkout.c
> @@ -2,6 +2,11 @@
>  #include "remote.h"
>  #include "refspec.h"
>  #include "checkout.h"
> +#include "unpack-trees.h"
> +#include "lockfile.h"
> +#include "refs.h"
> +#include "tree.h"
> +#include "cache-tree.h"
>  
>  struct tracking_name_data {
>  	/* const */ char *src_ref;
> @@ -42,3 +47,62 @@ const char *unique_tracking_name(const char *name, struct object_id *oid)
>  	free(cb_data.dst_ref);
>  	return NULL;
>  }
> +
> +int detach_head_to(struct object_id *oid, const char *action,
> +		   const char *reflog_message)
> +{
> +	struct strbuf ref_name = STRBUF_INIT;
> +	struct tree_desc desc;
> +	struct lock_file lock = LOCK_INIT;
> +	struct unpack_trees_options unpack_tree_opts;
> +	struct tree *tree;
> +	int ret = 0;
> +
> +	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> +		return -1;
> +
> +	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
> +	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
> +	unpack_tree_opts.head_idx = 1;
> +	unpack_tree_opts.src_index = &the_index;
> +	unpack_tree_opts.dst_index = &the_index;
> +	unpack_tree_opts.fn = oneway_merge;
> +	unpack_tree_opts.merge = 1;
> +	unpack_tree_opts.update = 1;
> +
> +	if (read_cache_unmerged()) {
> +		rollback_lock_file(&lock);
> +		strbuf_release(&ref_name);
> +		return error_resolve_conflict(_(action));
> +	}
> +
> +	if (!fill_tree_descriptor(&desc, oid)) {
> +		error(_("failed to find tree of %s"), oid_to_hex(oid));
> +		rollback_lock_file(&lock);
> +		free((void *)desc.buffer);
> +		strbuf_release(&ref_name);
> +		return -1;
> +	}
> +
> +	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
> +		rollback_lock_file(&lock);
> +		free((void *)desc.buffer);
> +		strbuf_release(&ref_name);
> +		return -1;
> +	}
> +
> +	tree = parse_tree_indirect(oid);
> +	prime_cache_tree(&the_index, tree);
> +
> +	if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
> +		ret = error(_("could not write index"));
> +	free((void *)desc.buffer);
> +
> +	if (!ret)
> +		ret = update_ref(reflog_message, "HEAD", oid,
> +				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> +
> +	strbuf_release(&ref_name);
> +	return ret;
> +
> +}
> diff --git a/checkout.h b/checkout.h
> index 9980711179..6ce46cf4e8 100644
> --- a/checkout.h
> +++ b/checkout.h
> @@ -10,4 +10,7 @@
>   */
>  extern const char *unique_tracking_name(const char *name, struct object_id *oid);
>  
> +int detach_head_to(struct object_id *oid, const char *action,
> +		   const char *reflog_message);
> +
>  #endif /* CHECKOUT_H */
> diff --git a/sequencer.c b/sequencer.c
> index 5354d4d51e..106d518f4d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -29,6 +29,7 @@
>  #include "oidset.h"
>  #include "commit-slab.h"
>  #include "alias.h"
> +#include "checkout.h"
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
> @@ -2756,14 +2757,7 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
>  {
>  	struct strbuf ref_name = STRBUF_INIT;
>  	struct object_id oid;
> -	struct lock_file lock = LOCK_INIT;
> -	struct tree_desc desc;
> -	struct tree *tree;
> -	struct unpack_trees_options unpack_tree_opts;
> -	int ret = 0, i;
> -
> -	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> -		return -1;
> +	int i;
>  
>  	if (len == 10 && !strncmp("[new root]", name, len)) {
>  		if (!opts->have_squash_onto) {
> @@ -2789,56 +2783,14 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
>  		if (get_oid(ref_name.buf, &oid) &&
>  		    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
>  			error(_("could not read '%s'"), ref_name.buf);
> -			rollback_lock_file(&lock);
>  			strbuf_release(&ref_name);
>  			return -1;
>  		}
>  	}
>  
> -	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
> -	setup_unpack_trees_porcelain(&unpack_tree_opts, "reset");
> -	unpack_tree_opts.head_idx = 1;
> -	unpack_tree_opts.src_index = &the_index;
> -	unpack_tree_opts.dst_index = &the_index;
> -	unpack_tree_opts.fn = oneway_merge;
> -	unpack_tree_opts.merge = 1;
> -	unpack_tree_opts.update = 1;
> -
> -	if (read_cache_unmerged()) {
> -		rollback_lock_file(&lock);
> -		strbuf_release(&ref_name);
> -		return error_resolve_conflict(_(action_name(opts)));
> -	}
> -
> -	if (!fill_tree_descriptor(&desc, &oid)) {
> -		error(_("failed to find tree of %s"), oid_to_hex(&oid));
> -		rollback_lock_file(&lock);
> -		free((void *)desc.buffer);
> -		strbuf_release(&ref_name);
> -		return -1;
> -	}
> -
> -	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
> -		rollback_lock_file(&lock);
> -		free((void *)desc.buffer);
> -		strbuf_release(&ref_name);
> -		return -1;
> -	}
> -
> -	tree = parse_tree_indirect(&oid);
> -	prime_cache_tree(&the_index, tree);
> -
> -	if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
> -		ret = error(_("could not write index"));
> -	free((void *)desc.buffer);
> -
> -	if (!ret)
> -		ret = update_ref(reflog_message(opts, "reset", "'%.*s'",
> -						len, name), "HEAD", &oid,
> -				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> -
> -	strbuf_release(&ref_name);
> -	return ret;
> +	return detach_head_to(&oid, action_name(opts),
> +			      reflog_message(opts, "reset", "'%.*s'",
> +					     len, name));
>  }
>  
>  static int do_merge(struct commit *commit, const char *arg, int arg_len,
Pratik Karki· Jul 9, 2018, 17:20 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 3/4] sequencer: refactor the code to detach HEAD to checkout.c

On Mon, Jul 9, 2018 at 10:27 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 34 quoted lines
>
> Pratik Karki <predatoramigo@gmail.com> writes:
>
> > In the upcoming builtin rebase, we will have to start by detaching
> > the HEAD, just like shell script version does. Essentially, we have
> > to do the same thing as `git checkout -q <revision>^0 --`, in pure C.
> >
> > The aforementioned functionality was already present in `sequencer.c`
> > in `do_reset()` function. But `do_reset()` performs more than detaching
> > the HEAD, and performs action specific to `sequencer.c`.
> >
> > So this commit refactors out that part from `do_reset()`, and moves it
> > to a new function called `detach_head_to()`. As this function has
> > nothing to do with the sequencer, and everything to do with what `git
> > checkout -q <revision>^0 --` does, we move that function to checkout.c.
> >
> > This refactoring actually introduces a slight change in behavior:
> > previously, the index was locked before parsing the argument to the
> > todo command `reset`, while it now gets locked *after* that, in the
> > `detach_head_to()` function.
> >
> > It does not make a huge difference, and the upside is that this closes
> > a few (unlikely) code paths where the index would not be unlocked upon
> > error.
> >
> > Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> > ---
>
> Here is a place to say "unchanged since v3" for a change like this
> one that changes neither the proposed log message above nor the
> patch below to help reviewers who have seen the previous round (they
> can stop reading here).  Hopefully there are more reviewers than you
> who write new code, so spending a bit more time to help them spend
> less would be an overall win for the project.
Thanks for the review. I'll keep this in mind, the next time.
Pratik Karki· Jul 8, 2018, 18:01 UTC · re: Pratik Karki · lore

[PATCH v4 4/4] builtin/rebase: support running "git rebase <upstream>"

This patch gives life to the skeleton added in the previous patches: With this change, we can perform a elementary rebase (without any options).

It can be tested thusly by:
git -c rebase.usebuiltin=true rebase HEAD~2

The rebase backends (i.e. the shell script functions defined in `git-rebase--<backend>`) are still at work here and the "builtin rebase"'s purpose is simply to parse the options and set everything up so that those rebase backends can do their work.

Note: We take an alternative approach here which is *not* to set the
environment variables via `run_command_v_opt_cd_env()` because those
variables would then be visible by processes spawned from the rebase
backends. Instead, we work hard to set them in the shell script snippet.

The next commits will handle and support all the wonderful rebase options.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 builtin/rebase.c | 238 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 237 insertions(+), 1 deletion(-)
Show changes to builtin/rebase.c +237 −1
diff --git a/builtin/rebase.c b/builtin/rebase.c
index fab7fca37e..e38ea80874 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -9,6 +9,20 @@
 #include "exec-cmd.h"
 #include "argv-array.h"
 #include "dir.h"
+#include "packfile.h"
+#include "checkout.h"
+#include "refs.h"
+#include "quote.h"
+
+static GIT_PATH_FUNC(apply_dir, "rebase-apply");
+static GIT_PATH_FUNC(merge_dir, "rebase-merge");
+
+enum rebase_type {
+	REBASE_AM,
+	REBASE_MERGE,
+	REBASE_INTERACTIVE,
+	REBASE_PRESERVE_MERGES
+};
 
 static int use_builtin_rebase(void)
 {
@@ -28,8 +42,136 @@ static int use_builtin_rebase(void)
 	return ret;
 }
 
+static int apply_autostash(void)
+{
+	warning("TODO");
+	return 0;
+}
+
+struct rebase_options {
+	enum rebase_type type;
+	const char *state_dir;
+	struct commit *upstream;
+	const char *upstream_name;
+	char *head_name;
+	struct object_id orig_head;
+	struct commit *onto;
+	const char *onto_name;
+	const char *revisions;
+	const char *root;
+};
+
+static int finish_rebase(struct rebase_options *opts)
+{
+	struct strbuf dir = STRBUF_INIT;
+	const char *argv_gc_auto[] = { "gc", "--auto", NULL };
+
+	delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
+	apply_autostash();
+	close_all_packs(the_repository->objects);
+	/*
+	 * We ignore errors in 'gc --auto', since the
+	 * user should see them.
+	 */
+	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
+	strbuf_addstr(&dir, opts->state_dir);
+	remove_dir_recursively(&dir, 0);
+	strbuf_release(&dir);
+
+	return 0;
+}
+
+static struct commit *peel_committish(const char *name)
+{
+	struct object *obj;
+	struct object_id oid;
+
+	if (get_oid(name, &oid))
+		return NULL;
+	obj = parse_object(&oid);
+	return (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
+}
+
+static void add_var(struct strbuf *buf, const char *name, const char *value)
+{
+	strbuf_addstr(buf, name);
+	strbuf_addstr(buf, "=");
+	sq_quote_buf(buf, value);
+	strbuf_addstr(buf, "; ");
+}
+
+static int run_specific_rebase(struct rebase_options *opts)
+{
+	const char *argv[] = { NULL, NULL };
+	struct strbuf script_snippet = STRBUF_INIT;
+	int status;
+	const char *backend, *backend_func;
+
+	add_var(&script_snippet, "GIT_DIR", absolute_path(get_git_dir()));
+
+	add_var(&script_snippet, "upstream_name", opts->upstream_name);
+	add_var(&script_snippet, "upstream",
+				 oid_to_hex(&opts->upstream->object.oid));
+	add_var(&script_snippet, "head_name", opts->head_name);
+	add_var(&script_snippet, "orig_head", oid_to_hex(&opts->orig_head));
+	add_var(&script_snippet, "onto", oid_to_hex(&opts->onto->object.oid));
+	add_var(&script_snippet, "onto_name", opts->onto_name);
+	add_var(&script_snippet, "revisions", opts->revisions);
+
+	switch (opts->type) {
+	case REBASE_AM:
+		backend = "git-rebase--am";
+		backend_func = "git_rebase__am";
+		break;
+	case REBASE_INTERACTIVE:
+		backend = "git-rebase--interactive";
+		backend_func = "git_rebase__interactive";
+		break;
+	case REBASE_MERGE:
+		backend = "git-rebase--merge";
+		backend_func = "git_rebase__merge";
+		break;
+	case REBASE_PRESERVE_MERGES:
+		backend = "git-rebase--preserve-merges";
+		backend_func = "git_rebase__preserve_merges";
+		break;
+	default:
+		BUG("Unhandled rebase type %d", opts->type);
+		break;
+	}
+
+	strbuf_addf(&script_snippet,
+		    ". git-rebase--common && . %s && %s",
+		    backend, backend_func);
+	argv[0] = script_snippet.buf;
+
+	status = run_command_v_opt(argv, RUN_USING_SHELL);
+	if (status == 0)
+		finish_rebase(opts);
+	else if (status == 2) {
+		struct strbuf dir = STRBUF_INIT;
+
+		apply_autostash();
+		strbuf_addstr(&dir, opts->state_dir);
+		remove_dir_recursively(&dir, 0);
+		strbuf_release(&dir);
+		die("Nothing to do");
+	}
+
+	strbuf_release(&script_snippet);
+
+	return status ? -1 : 0;
+}
+
 int cmd_rebase(int argc, const char **argv, const char *prefix)
 {
+	struct rebase_options options = { -1 };
+	const char *branch_name;
+	int ret, flags, quiet = 0;
+	struct strbuf msg = STRBUF_INIT;
+	struct strbuf revisions = STRBUF_INIT;
+	const char *restrict_revision = NULL;
+
 	/*
 	 * NEEDSWORK: Once the builtin rebase has been tested enough
 	 * and git-legacy-rebase.sh is retired to contrib/, this preamble
@@ -52,5 +194,99 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	trace_repo_setup(prefix);
 	setup_work_tree();
 
-	die("TODO");
+	options.type = REBASE_AM;
+
+	switch (options.type) {
+	case REBASE_AM:
+		options.state_dir = apply_dir();
+		break;
+	case REBASE_MERGE:
+	case REBASE_INTERACTIVE:
+	case REBASE_PRESERVE_MERGES:
+		options.state_dir = merge_dir();
+		break;
+	}
+	if (!options.root) {
+		if (argc != 2)
+			die("TODO: handle @{upstream}");
+		else {
+			options.upstream_name = argv[1];
+			argc--;
+			argv++;
+			if (!strcmp(options.upstream_name, "-"))
+				options.upstream_name = "@{-1}";
+		}
+		options.upstream = peel_committish(options.upstream_name);
+		if (!options.upstream)
+			die(_("invalid upstream '%s'"), options.upstream_name);
+	} else
+		die("TODO: upstream for --root");
+
+	/* Make sure the branch to rebase onto is valid. */
+	if (!options.onto_name)
+		options.onto_name = options.upstream_name;
+	if (strstr(options.onto_name, "...")) {
+		die("TODO");
+	} else {
+		options.onto = peel_committish(options.onto_name);
+		if (!options.onto)
+			die(_("Does not point to a valid commit '%s'"),
+				options.onto_name);
+	}
+
+	/*
+	 * If the branch to rebase is given, that is the branch we will rebase
+	 * branch_name -- branch/commit being rebased, or
+	 * 		  HEAD (already detached)
+	 * orig_head -- commit object name of tip of the branch before rebasing
+	 * head_name -- refs/heads/<that-branch> or "detached HEAD"
+	 */
+	if (argc > 1)
+		 die("TODO: handle switch_to");
+	else {
+		/* Do not need to switch branches, we are already on it. */
+		options.head_name =
+			xstrdup_or_null(resolve_ref_unsafe("HEAD", 0, NULL,
+					 &flags));
+		if (!options.head_name)
+			die(_("No such ref: %s"), "HEAD");
+		if (flags & REF_ISSYMREF) {
+			if (!skip_prefix(options.head_name,
+					 "refs/heads/", &branch_name))
+				branch_name = options.head_name;
+
+		} else {
+			options.head_name = xstrdup("detached HEAD");
+			branch_name = "HEAD";
+		}
+		if (get_oid("HEAD", &options.orig_head))
+			die(_("Could not resolve HEAD to a revision"));
+	}
+
+	/* Detach HEAD and reset the tree */
+	if (!quiet)
+		printf("First, rewinding head to replay your work on top of it...");
+
+	strbuf_addf(&msg, "rebase: checkout %s", options.onto_name);
+	if (detach_head_to(&options.onto->object.oid, "checkout", msg.buf))
+		die(_("Could not detach HEAD"));
+	strbuf_release(&msg);
+	if (update_ref("rebase", "ORIG_HEAD", &options.orig_head, NULL, 0,
+		       UPDATE_REFS_MSG_ON_ERR) < 0)
+		die(_("Could not update ORIG_HEAD to '%s'"),
+		    oid_to_hex(&options.orig_head));
+
+	strbuf_addf(&revisions, "%s..%s",
+		!options.root ? oid_to_hex(&options.onto->object.oid) :
+		    (restrict_revision ? restrict_revision :
+			    oid_to_hex(&options.upstream->object.oid)),
+			    oid_to_hex(&options.orig_head));
+
+	options.revisions = revisions.buf;
+
+	ret = !!run_specific_rebase(&options);
+
+	strbuf_release(&revisions);
+	free(options.head_name);
+	return ret;
 }
-- 
2.18.0
Johannes Schindelin· Jul 8, 2018, 21:44 UTC · re: Pratik Karki · lore

Re: [PATCH v4 4/4] builtin/rebase: support running "git rebase <upstream>"

Hi Pratik,
On Sun, 8 Jul 2018, Pratik Karki wrote:
> +	strbuf_addf(&script_snippet,
> +		    ". git-rebase--common && . %s && %s",
> +		    backend, backend_func);
In my quick test (on top of `wip-rebase`), this line needed this change:
-- snip --
Show changes to diff +2 −2
@@ -269,7 +279,8 @@ static int run_specific_rebase(struct rebase_options *opts)
        }

        strbuf_addf(&script_snippet,
-                   "set -x && . git-rebase--common && . %s && %s",
+                   "set -x && "
+                   ". git-sh-setup && . git-rebase--common && . %s && %s",
                    backend, backend_func);
        argv[0] = script_snippet.buf;
-- snap --

Obviously, the `set -x && ` part was not part of the patches you sent to
the Git mailing list, so please do not let that distract you from the fact
that I had to source also `git-sh-setup` (it implies `git-sh-i18n`, and
the `eval_gettext` function is defined there and used in the
`move_to_original_branch` fnuction).

With this (and the REF_NO_DEREF change), t3404-rebase-skip.sh passes,
which is pretty cool.

Ciao,
Dscho
Johannes Schindelin· Jul 8, 2018, 21:14 UTC · re: Pratik Karki · lore

Re: [GSoC] [PATCH v4 0/4] rebase: rewrite rebase in C

Hi Pratik,
On Sun, 8 Jul 2018, Pratik Karki wrote:
Show 24 quoted lines
> As a GSoC project, I have been working on the builtin rebase.
> 
> The motivation behind the rewrite of rebase i.e. from shell script to C
> are for following reasons:
> 
> 1.  Writing shell scripts and getting it to production is much faster
>     than doing the equivalent in C but lacks in performance and extra
>     workarounds are needed for non-POSIX platforms.
> 
> 2.  Git for Windows is at loss as the installer size increases due to
>     addition of extra dependencies for the shell scripts which are usually
>     available in POSIX compliant platforms.
> 
> This series of patches serves to demonstrate a minimal builtin rebase
> which supports running `git rebase <upstream>` and also serves to ask for
> reviews.
> 
> Changes since v3:
> 
>   -  Fix commit message of `rebase: start implementing it as a builtin`.
> 
>   -  Acknowledge Junio's style reviews.
> 
>   -  Acknowledge Johannes Schindelin's review.

The range-diff looks like this (and makes sense to me; you might want to fix the typo s/retun/return/, but that's all for now):

-- snipsnap --
 1:  7baec70f219 !  1:  42778b20edf rebase: start implementing it as a builtin
    @@ -13,6 +13,12 @@
         be able to conveniently test new features by configuring
         `rebase.useBuiltin`.
    +    In the original difftool conversion, if sane_execvp() that attempts to
    +    run the legacy scripted version returned with non-negative status, the
    +    command silently exited without doing anything with success, but
    +    sane_execvp() should not retun with non-negative status in the first
    +    place, so we use die() to notice such an abnormal case.
    +
         We intentionally avoid reading the config directly to avoid
         messing up the GIT_* environment variables when we need to fall back to
         exec()ing the shell script. The test of builtin rebase can be done by
 2:  f385f42dc56 !  2:  a28be7308e6 rebase: refactor common shell functions into their own file
    @@ -45,6 +45,20 @@
     diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
     --- a/git-legacy-rebase.sh
     +++ b/git-legacy-rebase.sh
    +@@
    + LF='
    + '
    + ok_to_skip_pre_rebase=
    +-resolvemsg="
    +-$(gettext 'Resolve all conflicts manually, mark them as resolved with
    +-"git add/rm <conflicted_files>", then run "git rebase --continue".
    +-You can instead skip this commit: run "git rebase --skip".
    +-To abort and get back to the state before "git rebase", run "git rebase --abort".')
    +-"
    ++
    + squash_onto=
    + unset onto
    + unset restrict_revision
     @@
      true)     gpg_sign_opt=-S ;;
      *)        gpg_sign_opt= ;;
    @@ -128,6 +142,13 @@
     +++ b/git-rebase--common.sh
     @@
     +
    ++resolvemsg="
    ++$(gettext 'Resolve all conflicts manually, mark them as resolved with
    ++"git add/rm <conflicted_files>", then run "git rebase --continue".
    ++You can instead skip this commit: run "git rebase --skip".
    ++To abort and get back to the state before "git rebase", run "git rebase --abort".')
    ++"
    ++
     +write_basic_state () {
     +  echo "$head_name" > "$state_dir"/head-name &&
     +  echo "$onto" > "$state_dir"/onto &&
 3:  147699bd195 =  3:  7591098c4d1 sequencer: refactor the code to detach HEAD to checkout.c
 4:  bbaa4264caa !  4:  f8429e950a4 builtin/rebase: support running "git rebase <upstream>"
    @@ -232,13 +232,14 @@
     +  }
     +
     +  /*
    -+  * If the branch to rebase is given, that is the branch we will rebase
    -+  * branch_name -- branch/commit being rebased, or HEAD (already detached)
    -+  * orig_head -- commit object name of tip of the branch before rebasing
    -+  * head_name -- refs/heads/<that-branch> or "detached HEAD"
    -+  */
    ++   * If the branch to rebase is given, that is the branch we will rebase
    ++   * branch_name -- branch/commit being rebased, or
    ++   *                HEAD (already detached)
    ++   * orig_head -- commit object name of tip of the branch before rebasing
    ++   * head_name -- refs/heads/<that-branch> or "detached HEAD"
    ++   */
     +  if (argc > 1)
    -+           die ("TODO: handle switch_to");
    ++           die("TODO: handle switch_to");
     +  else {
     +          /* Do not need to switch branches, we are already on it.  */
     +          options.head_name =
Pratik Karki· Jul 30, 2018, 16:29 UTC · re: Pratik Karki · lore

[GSoC] [PATCH v5 0/3] rebase: rewrite rebase in C

As a GSoC project, I have been working on the builtin rebase.

The motivation behind the rewrite of rebase i.e. from shell script to C are for following reasons:

1.  Writing shell scripts and getting it to production is much faster
    than doing the equivalent in C but lacks in performance and extra
    workarounds are needed for non-POSIX platforms.
2.  Git for Windows is at loss as the installer size increases due to
    addition of extra dependencies for the shell scripts which are usually
    available in POSIX compliant platforms.

This series of patches serves to demonstrate a minimal builtin rebase which supports running `git rebase <upstream>` and also serves to ask for reviews.

Changes since v4:
  -  Remove the `do_reset()` refactored function from sequencer.
     In other words `sequencer: refactor the code to detach HEAD to checkout.c`
     patch was dropped to introduce a new function `reset_hard()` for `rebase.c`
     (as suggested by Johannes).
  -  Fix a case of leak in `rebase: start implementing it as a builtin`.
     (as pointed out by Andrei Rybak and Eric Sunshine).
  -  Wrap the user visible comments in `_()` and used `BUG()` depending on the
     scenarios (as pointed out by Duy Nguyen).
  -  Fix the macro `GIT_PATH_FUNC` which expands to function definition and
     doesn't require semicolons (as pointed out by Beat Bolli).
Pratik Karki (3):
  rebase: start implementing it as a builtin
  rebase: refactor common shell functions into their own file
  builtin/rebase: support running "git rebase <upstream>"
 .gitignore                            |   2 +
 Makefile                              |   4 +-
 builtin.h                             |   1 +
 builtin/rebase.c                      | 406 ++++++++++++++++++++++++++
 git-rebase.sh => git-legacy-rebase.sh |  69 +----
 git-rebase--common.sh                 |  68 +++++
 git.c                                 |   6 +
 7 files changed, 488 insertions(+), 68 deletions(-)
 create mode 100644 builtin/rebase.c
 rename git-rebase.sh => git-legacy-rebase.sh (90%)
 create mode 100644 git-rebase--common.sh
-- 
2.18.0
Pratik Karki· Jul 30, 2018, 16:29 UTC · re: Pratik Karki · lore

[PATCH v5 1/3] rebase: start implementing it as a builtin

This commit imitates the strategy that was used to convert the difftool to a builtin. We start by renaming the shell script `git-rebase.sh` to `git-legacy-rebase.sh` and introduce a `builtin/rebase.c` that simply executes the shell script version, unless the config setting `rebase.useBuiltin` is set to `true`.

The motivation behind this is to rewrite all the functionality of the shell script version in the aforementioned `rebase.c`, one by one and be able to conveniently test new features by configuring `rebase.useBuiltin`.

In the original difftool conversion, if sane_execvp() that attempts to run the legacy scripted version returned with non-negative status, the command silently exited without doing anything with success, but sane_execvp() should not return with non-negative status in the first place, so we use die() to notice such an abnormal case.

We intentionally avoid reading the config directly to avoid messing up the GIT_* environment variables when we need to fall back to exec()ing the shell script. The test of builtin rebase can be done by `git -c rebase.useBuiltin=true rebase ...`

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 .gitignore                            |  1 +
 Makefile                              |  3 +-
 builtin.h                             |  1 +
 builtin/rebase.c                      | 58 +++++++++++++++++++++++++++
 git-rebase.sh => git-legacy-rebase.sh |  0
 git.c                                 |  6 +++
 6 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 builtin/rebase.c
 rename git-rebase.sh => git-legacy-rebase.sh (100%)
Show changes to 6 files +68 −1

.gitignore, Makefile, builtin.h, builtin/rebase.c, git-rebase.sh, git.c

diff --git a/.gitignore b/.gitignore
index 3284a1e9b1..ec23959014 100644
--- a/.gitignore
+++ b/.gitignore
@@ -78,6 +78,7 @@
 /git-init-db
 /git-interpret-trailers
 /git-instaweb
+/git-legacy-rebase
 /git-log
 /git-ls-files
 /git-ls-remote
diff --git a/Makefile b/Makefile
index 08e5c54549..a0c410b7d6 100644
--- a/Makefile
+++ b/Makefile
@@ -609,7 +609,7 @@ SCRIPT_SH += git-merge-one-file.sh
 SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
 SCRIPT_SH += git-quiltimport.sh
-SCRIPT_SH += git-rebase.sh
+SCRIPT_SH += git-legacy-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
 SCRIPT_SH += git-request-pull.sh
 SCRIPT_SH += git-stash.sh
@@ -1059,6 +1059,7 @@ BUILTIN_OBJS += builtin/prune.o
 BUILTIN_OBJS += builtin/pull.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase.o
 BUILTIN_OBJS += builtin/rebase--helper.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
diff --git a/builtin.h b/builtin.h
index 0362f1ce25..44651a447f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -202,6 +202,7 @@ extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
 extern int cmd_pull(int argc, const char **argv, const char *prefix);
 extern int cmd_push(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase(int argc, const char **argv, const char *prefix);
 extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase.c b/builtin/rebase.c
new file mode 100644
index 0000000000..c44addb2a4
--- /dev/null
+++ b/builtin/rebase.c
@@ -0,0 +1,58 @@
+/*
+ * "git rebase" builtin command
+ *
+ * Copyright (c) 2018 Pratik Karki
+ */
+
+#include "builtin.h"
+#include "run-command.h"
+#include "exec-cmd.h"
+#include "argv-array.h"
+#include "dir.h"
+
+static int use_builtin_rebase(void)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf out = STRBUF_INIT;
+	int ret;
+
+	argv_array_pushl(&cp.args,
+			 "config", "--bool", "rebase.usebuiltin", NULL);
+	cp.git_cmd = 1;
+	if (capture_command(&cp, &out, 6)) {
+		strbuf_release(&out);
+		return 0;
+	}
+
+	strbuf_trim(&out);
+	ret = !strcmp("true", out.buf);
+	strbuf_release(&out);
+	return ret;
+}
+
+int cmd_rebase(int argc, const char **argv, const char *prefix)
+{
+	/*
+	 * NEEDSWORK: Once the builtin rebase has been tested enough
+	 * and git-legacy-rebase.sh is retired to contrib/, this preamble
+	 * can be removed.
+	 */
+
+	if (!use_builtin_rebase()) {
+		const char *path = mkpath("%s/git-legacy-rebase",
+					  git_exec_path());
+
+		if (sane_execvp(path, (char **)argv) < 0)
+			die_errno(_("could not exec %s"), path);
+		else
+			BUG("sane_execvp() returned???");
+	}
+
+	if (argc != 2)
+		die(_("Usage: %s <base>"), argv[0]);
+	prefix = setup_git_directory();
+	trace_repo_setup(prefix);
+	setup_work_tree();
+
+	die("TODO");
+}
diff --git a/git-rebase.sh b/git-legacy-rebase.sh
similarity index 100%
rename from git-rebase.sh
rename to git-legacy-rebase.sh
diff --git a/git.c b/git.c
index 3fded74519..d28ad90e31 100644
--- a/git.c
+++ b/git.c
@@ -518,6 +518,12 @@ static struct cmd_struct commands[] = {
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
 	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
+	/*
+	 * NEEDSWORK: Until the rebase is independent and needs no redirection
+	 * to rebase shell script this is kept as is, then should be changed to
+	 * RUN_SETUP | NEED_WORK_TREE
+	 */
+	{ "rebase", cmd_rebase },
 	{ "rebase--helper", cmd_rebase__helper, RUN_SETUP | NEED_WORK_TREE },
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
-- 
2.18.0
Pratik Karki· Jul 30, 2018, 16:29 UTC · re: Pratik Karki · lore

[PATCH v5 2/3] rebase: refactor common shell functions into their own file

The functions present in `git-legacy-rebase.sh` are used by the rebase backends as they are implemented as shell script functions in the `git-rebase--<backend>` files.

To make the `builtin/rebase.c` work, we have to provide support via a Unix shell script snippet that uses these functions and so, we want to use the rebase backends *directly* from the builtin rebase without going through `git-legacy-rebase.sh`.

This commit extracts the functions to a separate file, `git-rebase--common`, that will be read by `git-legacy-rebase.sh` and by the shell script snippets which will be used extensively in the following commits.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
Unchanged since v4.
 .gitignore            |  1 +
 Makefile              |  1 +
 git-legacy-rebase.sh  | 69 ++-----------------------------------------
 git-rebase--common.sh | 68 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 67 deletions(-)
 create mode 100644 git-rebase--common.sh
Show changes to 4 files +72 −67

.gitignore, Makefile, git-legacy-rebase.sh, git-rebase--common.sh

diff --git a/.gitignore b/.gitignore
index ec23959014..824141cba1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -117,6 +117,7 @@
 /git-read-tree
 /git-rebase
 /git-rebase--am
+/git-rebase--common
 /git-rebase--helper
 /git-rebase--interactive
 /git-rebase--merge
diff --git a/Makefile b/Makefile
index a0c410b7d6..c59e2f64a1 100644
--- a/Makefile
+++ b/Makefile
@@ -619,6 +619,7 @@ SCRIPT_SH += git-web--browse.sh
 SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
+SCRIPT_LIB += git-rebase--common
 SCRIPT_LIB += git-rebase--interactive
 SCRIPT_LIB += git-rebase--preserve-merges
 SCRIPT_LIB += git-rebase--merge
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 7973447645..af2cdfef03 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -57,12 +57,7 @@ cd_to_toplevel
 LF='
 '
 ok_to_skip_pre_rebase=
-resolvemsg="
-$(gettext 'Resolve all conflicts manually, mark them as resolved with
-"git add/rm <conflicted_files>", then run "git rebase --continue".
-You can instead skip this commit: run "git rebase --skip".
-To abort and get back to the state before "git rebase", run "git rebase --abort".')
-"
+
 squash_onto=
 unset onto
 unset restrict_revision
@@ -102,6 +97,7 @@ case "$(git config --bool commit.gpgsign)" in
 true)	gpg_sign_opt=-S ;;
 *)	gpg_sign_opt= ;;
 esac
+. git-rebase--common
 
 read_basic_state () {
 	test -f "$state_dir/head-name" &&
@@ -132,67 +128,6 @@ read_basic_state () {
 	}
 }
 
-write_basic_state () {
-	echo "$head_name" > "$state_dir"/head-name &&
-	echo "$onto" > "$state_dir"/onto &&
-	echo "$orig_head" > "$state_dir"/orig-head &&
-	echo "$GIT_QUIET" > "$state_dir"/quiet &&
-	test t = "$verbose" && : > "$state_dir"/verbose
-	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
-	test -n "$strategy_opts" && echo "$strategy_opts" > \
-		"$state_dir"/strategy_opts
-	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
-		"$state_dir"/allow_rerere_autoupdate
-	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
-	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
-}
-
-output () {
-	case "$verbose" in
-	'')
-		output=$("$@" 2>&1 )
-		status=$?
-		test $status != 0 && printf "%s\n" "$output"
-		return $status
-		;;
-	*)
-		"$@"
-		;;
-	esac
-}
-
-move_to_original_branch () {
-	case "$head_name" in
-	refs/*)
-		message="rebase finished: $head_name onto $onto"
-		git update-ref -m "$message" \
-			$head_name $(git rev-parse HEAD) $orig_head &&
-		git symbolic-ref \
-			-m "rebase finished: returning to $head_name" \
-			HEAD $head_name ||
-		die "$(eval_gettext "Could not move back to \$head_name")"
-		;;
-	esac
-}
-
-apply_autostash () {
-	if test -f "$state_dir/autostash"
-	then
-		stash_sha1=$(cat "$state_dir/autostash")
-		if git stash apply $stash_sha1 >/dev/null 2>&1
-		then
-			echo "$(gettext 'Applied autostash.')" >&2
-		else
-			git stash store -m "autostash" -q $stash_sha1 ||
-			die "$(eval_gettext "Cannot store \$stash_sha1")"
-			gettext 'Applying autostash resulted in conflicts.
-Your changes are safe in the stash.
-You can run "git stash pop" or "git stash drop" at any time.
-' >&2
-		fi
-	fi
-}
-
 finish_rebase () {
 	rm -f "$(git rev-parse --git-path REBASE_HEAD)"
 	apply_autostash &&
diff --git a/git-rebase--common.sh b/git-rebase--common.sh
new file mode 100644
index 0000000000..7e39d22871
--- /dev/null
+++ b/git-rebase--common.sh
@@ -0,0 +1,68 @@
+
+resolvemsg="
+$(gettext 'Resolve all conflicts manually, mark them as resolved with
+"git add/rm <conflicted_files>", then run "git rebase --continue".
+You can instead skip this commit: run "git rebase --skip".
+To abort and get back to the state before "git rebase", run "git rebase --abort".')
+"
+
+write_basic_state () {
+	echo "$head_name" > "$state_dir"/head-name &&
+	echo "$onto" > "$state_dir"/onto &&
+	echo "$orig_head" > "$state_dir"/orig-head &&
+	echo "$GIT_QUIET" > "$state_dir"/quiet &&
+	test t = "$verbose" && : > "$state_dir"/verbose
+	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
+	test -n "$strategy_opts" && echo "$strategy_opts" > \
+		"$state_dir"/strategy_opts
+	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
+		"$state_dir"/allow_rerere_autoupdate
+	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
+	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
+}
+
+apply_autostash () {
+	if test -f "$state_dir/autostash"
+	then
+		stash_sha1=$(cat "$state_dir/autostash")
+		if git stash apply $stash_sha1 >/dev/null 2>&1
+		then
+			echo "$(gettext 'Applied autostash.')" >&2
+		else
+			git stash store -m "autostash" -q $stash_sha1 ||
+			die "$(eval_gettext "Cannot store \$stash_sha1")"
+			gettext 'Applying autostash resulted in conflicts.
+Your changes are safe in the stash.
+You can run "git stash pop" or "git stash drop" at any time.
+' >&2
+		fi
+	fi
+}
+
+move_to_original_branch () {
+	case "$head_name" in
+	refs/*)
+		message="rebase finished: $head_name onto $onto"
+		git update-ref -m "$message" \
+			$head_name $(git rev-parse HEAD) $orig_head &&
+		git symbolic-ref \
+			-m "rebase finished: returning to $head_name" \
+			HEAD $head_name ||
+		die "$(eval_gettext "Could not move back to \$head_name")"
+		;;
+	esac
+}
+
+output () {
+	case "$verbose" in
+	'')
+		output=$("$@" 2>&1 )
+		status=$?
+		test $status != 0 && printf "%s\n" "$output"
+		return $status
+		;;
+	*)
+		"$@"
+		;;
+	esac
+}
-- 
2.18.0
Pratik Karki· Jul 30, 2018, 16:29 UTC · re: Pratik Karki · lore

[PATCH v5 3/3] builtin/rebase: support running "git rebase <upstream>"

This patch gives life to the skeleton added in the previous patches: With this change, we can perform a elementary rebase (without any options).

It can be tested thusly by:
git -c rebase.usebuiltin=true rebase HEAD~2

The rebase backends (i.e. the shell script functions defined in `git-rebase--<backend>`) are still at work here and the "builtin rebase"'s purpose is simply to parse the options and set everything up so that those rebase backends can do their work.

Note: We take an alternative approach here which is *not* to set the
environment variables via `run_command_v_opt_cd_env()` because those
variables would then be visible by processes spawned from the rebase
backends. Instead, we work hard to set them in the shell script snippet.
On Windows, some of the tests fail merely due to core.fileMode not
being heeded that's why the core.*config variables is parsed here.

The next commits will handle and support all the wonderful rebase options.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 builtin/rebase.c | 350 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 349 insertions(+), 1 deletion(-)
Show changes to builtin/rebase.c +349 −1
diff --git a/builtin/rebase.c b/builtin/rebase.c
index c44addb2a4..5863139204 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -9,6 +9,24 @@
 #include "exec-cmd.h"
 #include "argv-array.h"
 #include "dir.h"
+#include "packfile.h"
+#include "refs.h"
+#include "quote.h"
+#include "config.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "lockfile.h"
+
+static GIT_PATH_FUNC(apply_dir, "rebase-apply")
+static GIT_PATH_FUNC(merge_dir, "rebase-merge")
+
+enum rebase_type {
+	REBASE_UNSPECIFIED = -1,
+	REBASE_AM,
+	REBASE_MERGE,
+	REBASE_INTERACTIVE,
+	REBASE_PRESERVE_MERGES
+};
 
 static int use_builtin_rebase(void)
 {
@@ -30,8 +48,243 @@ static int use_builtin_rebase(void)
 	return ret;
 }
 
+static int apply_autostash(void)
+{
+	warning("TODO");
+	return 0;
+}
+
+struct rebase_options {
+	enum rebase_type type;
+	const char *state_dir;
+	struct commit *upstream;
+	const char *upstream_name;
+	char *head_name;
+	struct object_id orig_head;
+	struct commit *onto;
+	const char *onto_name;
+	const char *revisions;
+	const char *root;
+	const char *restrict_revision;
+};
+
+static int finish_rebase(struct rebase_options *opts)
+{
+	struct strbuf dir = STRBUF_INIT;
+	const char *argv_gc_auto[] = { "gc", "--auto", NULL };
+
+	delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
+	apply_autostash();
+	close_all_packs(the_repository->objects);
+	/*
+	 * We ignore errors in 'gc --auto', since the
+	 * user should see them.
+	 */
+	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
+	strbuf_addstr(&dir, opts->state_dir);
+	remove_dir_recursively(&dir, 0);
+	strbuf_release(&dir);
+
+	return 0;
+}
+
+static struct commit *peel_committish(const char *name)
+{
+	struct object *obj;
+	struct object_id oid;
+
+	if (get_oid(name, &oid))
+		return NULL;
+	obj = parse_object(&oid);
+	return (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
+}
+
+static void add_var(struct strbuf *buf, const char *name, const char *value)
+{
+	strbuf_addstr(buf, name);
+	if (value) {
+		strbuf_addstr(buf, "=");
+		sq_quote_buf(buf, value);
+	}
+	strbuf_addstr(buf, "; ");
+}
+
+static int run_specific_rebase(struct rebase_options *opts)
+{
+	const char *argv[] = { NULL, NULL };
+	struct strbuf script_snippet = STRBUF_INIT;
+	int status;
+	const char *backend, *backend_func;
+
+	add_var(&script_snippet, "GIT_DIR", absolute_path(get_git_dir()));
+	add_var(&script_snippet, "state_dir", opts->state_dir);
+
+	add_var(&script_snippet, "upstream_name", opts->upstream_name);
+	add_var(&script_snippet, "upstream",
+				 oid_to_hex(&opts->upstream->object.oid));
+	add_var(&script_snippet, "head_name", opts->head_name);
+	add_var(&script_snippet, "orig_head", oid_to_hex(&opts->orig_head));
+	add_var(&script_snippet, "onto", oid_to_hex(&opts->onto->object.oid));
+	add_var(&script_snippet, "onto_name", opts->onto_name);
+	add_var(&script_snippet, "revisions", opts->revisions);
+	if (opts->restrict_revision == NULL)
+		add_var(&script_snippet, "restrict_revision", "");
+	else
+		add_var(&script_snippet, "restrict_revision",
+			opts->restrict_revision);
+
+	switch (opts->type) {
+	case REBASE_AM:
+		backend = "git-rebase--am";
+		backend_func = "git_rebase__am";
+		break;
+	case REBASE_INTERACTIVE:
+		backend = "git-rebase--interactive";
+		backend_func = "git_rebase__interactive";
+		break;
+	case REBASE_MERGE:
+		backend = "git-rebase--merge";
+		backend_func = "git_rebase__merge";
+		break;
+	case REBASE_PRESERVE_MERGES:
+		backend = "git-rebase--preserve-merges";
+		backend_func = "git_rebase__preserve_merges";
+		break;
+	default:
+		BUG("Unhandled rebase type %d", opts->type);
+		break;
+	}
+
+	strbuf_addf(&script_snippet,
+		    ". git-sh-setup && . git-rebase--common && . %s && %s",
+		    backend, backend_func);
+	argv[0] = script_snippet.buf;
+
+	status = run_command_v_opt(argv, RUN_USING_SHELL);
+	if (status == 0)
+		finish_rebase(opts);
+	else if (status == 2) {
+		struct strbuf dir = STRBUF_INIT;
+
+		apply_autostash();
+		strbuf_addstr(&dir, opts->state_dir);
+		remove_dir_recursively(&dir, 0);
+		strbuf_release(&dir);
+		die("Nothing to do");
+	}
+
+	strbuf_release(&script_snippet);
+
+	return status ? -1 : 0;
+}
+
+#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
+
+static int reset_head(struct object_id *oid, const char *action,
+		    const char *switch_to_branch, int detach_head)
+{
+	struct object_id head_oid;
+	struct tree_desc desc;
+	struct lock_file lock = LOCK_INIT;
+	struct unpack_trees_options unpack_tree_opts;
+	struct tree *tree;
+	const char *reflog_action;
+	struct strbuf msg = STRBUF_INIT;
+	size_t prefix_len;
+	struct object_id *orig = NULL, oid_orig,
+		*old_orig = NULL, oid_old_orig;
+	int ret = 0;
+
+	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
+		return -1;
+
+	if (!oid) {
+		if (get_oid("HEAD", &head_oid)) {
+			rollback_lock_file(&lock);
+			return error(_("could not determine HEAD revision"));
+		}
+		oid = &head_oid;
+	}
+
+	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
+	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
+	unpack_tree_opts.head_idx = 1;
+	unpack_tree_opts.src_index = the_repository->index;
+	unpack_tree_opts.dst_index = the_repository->index;
+	unpack_tree_opts.fn = oneway_merge;
+	unpack_tree_opts.update = 1;
+	if (!detach_head)
+		unpack_tree_opts.reset = 1;
+	else
+		unpack_tree_opts.merge = 1;
+
+	if (read_index(the_repository->index) < 0) {
+		rollback_lock_file(&lock);
+		return error(_("could not read index"));
+	}
+
+	if (!fill_tree_descriptor(&desc, oid)) {
+		error(_("failed to find tree of %s"), oid_to_hex(oid));
+		rollback_lock_file(&lock);
+		free((void *)desc.buffer);
+		return -1;
+	}
+
+	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
+		rollback_lock_file(&lock);
+		free((void *)desc.buffer);
+		return -1;
+	}
+
+	tree = parse_tree_indirect(oid);
+	prime_cache_tree(the_repository->index, tree);
+
+	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)
+		ret = error(_("could not write index"));
+	free((void *)desc.buffer);
+
+	if (ret)
+		return ret;
+
+	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
+	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
+	prefix_len = msg.len;
+
+	if (!get_oid("ORIG_HEAD", &oid_old_orig))
+		old_orig = &oid_old_orig;
+	if (!get_oid("HEAD", &oid_orig)) {
+		orig = &oid_orig;
+		strbuf_addstr(&msg, "updating ORIG_HEAD");
+		update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0,
+			   UPDATE_REFS_MSG_ON_ERR);
+	} else if (old_orig)
+		delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
+	strbuf_setlen(&msg, prefix_len);
+	strbuf_addstr(&msg, "updating HEAD");
+	if (!switch_to_branch)
+		ret = update_ref(msg.buf, "HEAD", oid, orig, REF_NO_DEREF,
+				 UPDATE_REFS_MSG_ON_ERR);
+	else {
+		ret = create_symref("HEAD", switch_to_branch, msg.buf);
+		if (!ret)
+			ret = update_ref(msg.buf, "HEAD", oid, orig, 0,
+					 UPDATE_REFS_MSG_ON_ERR);
+	}
+
+	strbuf_release(&msg);
+	return ret;
+}
+
 int cmd_rebase(int argc, const char **argv, const char *prefix)
 {
+	struct rebase_options options = {
+		.type = REBASE_UNSPECIFIED,
+	};
+	const char *branch_name;
+	int ret, flags, quiet = 0;
+	struct strbuf msg = STRBUF_INIT;
+	struct strbuf revisions = STRBUF_INIT;
+
 	/*
 	 * NEEDSWORK: Once the builtin rebase has been tested enough
 	 * and git-legacy-rebase.sh is retired to contrib/, this preamble
@@ -54,5 +307,100 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	trace_repo_setup(prefix);
 	setup_work_tree();
 
-	die("TODO");
+	git_config(git_default_config, NULL);
+
+	switch (options.type) {
+	case REBASE_MERGE:
+	case REBASE_INTERACTIVE:
+	case REBASE_PRESERVE_MERGES:
+		options.state_dir = merge_dir();
+		break;
+	case REBASE_AM:
+		options.state_dir = apply_dir();
+		break;
+	default:
+		options.type = REBASE_AM;
+		options.state_dir = apply_dir();
+		break;
+	}
+
+	if (!options.root) {
+		if (argc < 2)
+			die("TODO: handle @{upstream}");
+		else {
+			options.upstream_name = argv[1];
+			argc--;
+			argv++;
+			if (!strcmp(options.upstream_name, "-"))
+				options.upstream_name = "@{-1}";
+		}
+		options.upstream = peel_committish(options.upstream_name);
+		if (!options.upstream)
+			die(_("invalid upstream '%s'"), options.upstream_name);
+	} else
+		die("TODO: upstream for --root");
+
+	/* Make sure the branch to rebase onto is valid. */
+	if (!options.onto_name)
+		options.onto_name = options.upstream_name;
+	if (strstr(options.onto_name, "...")) {
+		die("TODO");
+	} else {
+		options.onto = peel_committish(options.onto_name);
+		if (!options.onto)
+			die(_("Does not point to a valid commit '%s'"),
+				options.onto_name);
+	}
+
+	/*
+	 * If the branch to rebase is given, that is the branch we will rebase
+	 * branch_name -- branch/commit being rebased, or
+	 * 		  HEAD (already detached)
+	 * orig_head -- commit object name of tip of the branch before rebasing
+	 * head_name -- refs/heads/<that-branch> or "detached HEAD"
+	 */
+	if (argc > 1)
+		 die("TODO: handle switch_to");
+	else {
+		/* Do not need to switch branches, we are already on it. */
+		options.head_name =
+			xstrdup_or_null(resolve_ref_unsafe("HEAD", 0, NULL,
+					 &flags));
+		if (!options.head_name)
+			die(_("No such ref: %s"), "HEAD");
+		if (flags & REF_ISSYMREF) {
+			if (!skip_prefix(options.head_name,
+					 "refs/heads/", &branch_name))
+				branch_name = options.head_name;
+
+		} else {
+			options.head_name = xstrdup("detached HEAD");
+			branch_name = "HEAD";
+		}
+		if (get_oid("HEAD", &options.orig_head))
+			die(_("Could not resolve HEAD to a revision"));
+	}
+
+	/* Detach HEAD and reset the tree */
+	if (!quiet)
+		printf("First, rewinding head to replay your work on top of it...");
+
+	strbuf_addf(&msg, "rebase: checkout %s", options.onto_name);
+	if (reset_head(&options.onto->object.oid, "checkout", NULL, 1))
+		die(_("Could not detach HEAD"));
+	strbuf_release(&msg);
+
+	strbuf_addf(&revisions, "%s..%s",
+		!options.root ? oid_to_hex(&options.onto->object.oid) :
+		    (options.restrict_revision ? options.restrict_revision :
+			    oid_to_hex(&options.upstream->object.oid)),
+			    oid_to_hex(&options.orig_head));
+
+	options.revisions = revisions.buf;
+
+	ret = !!run_specific_rebase(&options);
+
+	strbuf_release(&revisions);
+	free(options.head_name);
+	return ret;
 }
-- 
2.18.0
Pratik Karki· Aug 1, 2018, 00:17 UTC · re: Pratik Karki · lore

Re: [GSoC] [PATCH v5 0/3] rebase: rewrite rebase in C

Hi Junio,

During recent development, I found out that `v5` has some issues and shouldn't be merged into `next`. I implemented more options and ran a couple of regression tests from which I figured out that certain choices I made in those commits need to be reconsidered.

During recent development, my working branch `wip-rebase` has passing `t3400` and for which I have made some changes to the code already in v5.

Cheers, Pratik

Pratik Karki· Aug 6, 2018, 19:31 UTC · re: Pratik Karki · lore

[GSoC] [PATCH v6 0/3] rebase: rewrite rebase in C

As a GSoC project, I have been working on the builtin rebase.

The motivation behind the rewrite of rebase i.e. from shell script to C are for following reasons:

1.  Writing shell scripts and getting it to production is much faster
    than doing the equivalent in C but lacks in performance and extra
    workarounds are needed for non-POSIX platforms.
2.  Git for Windows is at loss as the installer size increases due to
    addition of extra dependencies for the shell scripts which are usually
    available in POSIX compliant platforms.

This series of patches serves to demonstrate a minimal builtin rebase which supports running `git rebase <upstream>` and also serves to ask for reviews.

This is only the first patch series, with many more to come. I have finished the conversion, but I want to organize the patches in a neat patch series to make review process more pleasant, and you can see the progress at <https://github.com/git/git/pull/505>.

The organization of patches is also almost done. After the follow-up patches, the rebase operation will be handled completely by this `builtin/rebase.c`.

Changes since v5:
  -  Fix `builtin/rebase: support running "git rebase <upstream>"` as it
     does not need to switch to another branch and only needs to detach the
     `HEAD` for now.
  -  The `reset_head()` function is introduced in
     `builtin/rebase: support running "git rebase <upstream>"`
     which is only used to detach the `HEAD` to `onto` for starting the rebase,
     but upcoming patches will add more functionality like moving or updating to
     original branch.
  -  Lots of changes that fix bugs discovered while getting the test suite to pass
     with the patch series.
Pratik Karki (3):
  rebase: start implementing it as a builtin
  rebase: refactor common shell functions into their own file
  builtin/rebase: support running "git rebase <upstream>"
 .gitignore                            |   2 +
 Makefile                              |   4 +-
 builtin.h                             |   1 +
 builtin/rebase.c                      | 421 ++++++++++++++++++++++++++
 git-rebase.sh => git-legacy-rebase.sh |  69 +----
 git-rebase--common.sh                 |  68 +++++
 git.c                                 |   6 +
 7 files changed, 503 insertions(+), 68 deletions(-)
 create mode 100644 builtin/rebase.c
 rename git-rebase.sh => git-legacy-rebase.sh (90%)
 create mode 100644 git-rebase--common.sh
-- 
2.18.0
Pratik Karki· Aug 6, 2018, 19:31 UTC · re: Pratik Karki · lore

[GSoC] [PATCH v6 3/3] builtin/rebase: support running "git rebase <upstream>"

This patch gives life to the skeleton added in the previous patches: With this change, we can perform a elementary rebase (without any options).

It can be tested thusly by:
git -c rebase.usebuiltin=true rebase HEAD~2

The rebase backends (i.e. the shell script functions defined in `git-rebase--<backend>`) are still at work here and the "builtin rebase"'s purpose is simply to parse the options and set everything up so that those rebase backends can do their work.

Note: We take an alternative approach here which is *not* to set the
environment variables via `run_command_v_opt_cd_env()` because those
variables would then be visible by processes spawned from the rebase
backends. Instead, we work hard to set them in the shell script snippet.
On Windows, some of the tests fail merely due to core.fileMode not
being heeded that's why the core.*config variables is parsed here.

The `reset_head()` function is currently only used to detach the HEAD to onto by way of starting the rebase, but it offers additional functionality that subsequent patches will need like moving to the original branch (possibly updating it) and also to do the equivalent of `git reset --hard`.

The next commits will handle and support all the wonderful rebase options.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
 builtin/rebase.c | 365 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 364 insertions(+), 1 deletion(-)
Show changes to builtin/rebase.c +364 −1
diff --git a/builtin/rebase.c b/builtin/rebase.c
index c44addb2a4..e695d8a430 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -9,6 +9,24 @@
 #include "exec-cmd.h"
 #include "argv-array.h"
 #include "dir.h"
+#include "packfile.h"
+#include "refs.h"
+#include "quote.h"
+#include "config.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "lockfile.h"
+
+static GIT_PATH_FUNC(apply_dir, "rebase-apply")
+static GIT_PATH_FUNC(merge_dir, "rebase-merge")
+
+enum rebase_type {
+	REBASE_UNSPECIFIED = -1,
+	REBASE_AM,
+	REBASE_MERGE,
+	REBASE_INTERACTIVE,
+	REBASE_PRESERVE_MERGES
+};
 
 static int use_builtin_rebase(void)
 {
@@ -30,8 +48,260 @@ static int use_builtin_rebase(void)
 	return ret;
 }
 
+static int apply_autostash(void)
+{
+	warning("TODO");
+	return 0;
+}
+
+struct rebase_options {
+	enum rebase_type type;
+	const char *state_dir;
+	struct commit *upstream;
+	const char *upstream_name;
+	char *head_name;
+	struct object_id orig_head;
+	struct commit *onto;
+	const char *onto_name;
+	const char *revisions;
+	int root;
+	struct commit *restrict_revision;
+	int dont_finish_rebase;
+};
+
+/* Returns the filename prefixed by the state_dir */
+static const char *state_dir_path(const char *filename, struct rebase_options *opts)
+{
+	static struct strbuf path = STRBUF_INIT;
+	static size_t prefix_len;
+
+	if (!prefix_len) {
+		strbuf_addf(&path, "%s/", opts->state_dir);
+		prefix_len = path.len;
+	}
+
+	strbuf_setlen(&path, prefix_len);
+	strbuf_addstr(&path, filename);
+	return path.buf;
+}
+
+static int finish_rebase(struct rebase_options *opts)
+{
+	struct strbuf dir = STRBUF_INIT;
+	const char *argv_gc_auto[] = { "gc", "--auto", NULL };
+
+	delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
+	apply_autostash();
+	close_all_packs(the_repository->objects);
+	/*
+	 * We ignore errors in 'gc --auto', since the
+	 * user should see them.
+	 */
+	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
+	strbuf_addstr(&dir, opts->state_dir);
+	remove_dir_recursively(&dir, 0);
+	strbuf_release(&dir);
+
+	return 0;
+}
+
+static struct commit *peel_committish(const char *name)
+{
+	struct object *obj;
+	struct object_id oid;
+
+	if (get_oid(name, &oid))
+		return NULL;
+	obj = parse_object(the_repository, &oid);
+	return (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
+}
+
+static void add_var(struct strbuf *buf, const char *name, const char *value)
+{
+	if (!value)
+		strbuf_addf(buf, "unset %s; ", name);
+	else {
+		strbuf_addf(buf, "%s=", name);
+		sq_quote_buf(buf, value);
+		strbuf_addstr(buf, "; ");
+	}
+}
+
+static int run_specific_rebase(struct rebase_options *opts)
+{
+	const char *argv[] = { NULL, NULL };
+	struct strbuf script_snippet = STRBUF_INIT;
+	int status;
+	const char *backend, *backend_func;
+
+	add_var(&script_snippet, "GIT_DIR", absolute_path(get_git_dir()));
+	add_var(&script_snippet, "state_dir", opts->state_dir);
+
+	add_var(&script_snippet, "upstream_name", opts->upstream_name);
+	add_var(&script_snippet, "upstream",
+				 oid_to_hex(&opts->upstream->object.oid));
+	add_var(&script_snippet, "head_name", opts->head_name);
+	add_var(&script_snippet, "orig_head", oid_to_hex(&opts->orig_head));
+	add_var(&script_snippet, "onto", oid_to_hex(&opts->onto->object.oid));
+	add_var(&script_snippet, "onto_name", opts->onto_name);
+	add_var(&script_snippet, "revisions", opts->revisions);
+	add_var(&script_snippet, "restrict_revision", opts->restrict_revision ?
+		oid_to_hex(&opts->restrict_revision->object.oid) : NULL);
+
+	switch (opts->type) {
+	case REBASE_AM:
+		backend = "git-rebase--am";
+		backend_func = "git_rebase__am";
+		break;
+	case REBASE_INTERACTIVE:
+		backend = "git-rebase--interactive";
+		backend_func = "git_rebase__interactive";
+		break;
+	case REBASE_MERGE:
+		backend = "git-rebase--merge";
+		backend_func = "git_rebase__merge";
+		break;
+	case REBASE_PRESERVE_MERGES:
+		backend = "git-rebase--preserve-merges";
+		backend_func = "git_rebase__preserve_merges";
+		break;
+	default:
+		BUG("Unhandled rebase type %d", opts->type);
+		break;
+	}
+
+	strbuf_addf(&script_snippet,
+		    ". git-sh-setup && . git-rebase--common &&"
+		    " . %s && %s", backend, backend_func);
+	argv[0] = script_snippet.buf;
+
+	status = run_command_v_opt(argv, RUN_USING_SHELL);
+	if (opts->dont_finish_rebase)
+		; /* do nothing */
+	else if (status == 0) {
+		if (!file_exists(state_dir_path("stopped-sha", opts)))
+			finish_rebase(opts);
+	} else if (status == 2) {
+		struct strbuf dir = STRBUF_INIT;
+
+		apply_autostash();
+		strbuf_addstr(&dir, opts->state_dir);
+		remove_dir_recursively(&dir, 0);
+		strbuf_release(&dir);
+		die("Nothing to do");
+	}
+
+	strbuf_release(&script_snippet);
+
+	return status ? -1 : 0;
+}
+
+#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
+
+static int reset_head(struct object_id *oid, const char *action,
+		      const char *switch_to_branch, int detach_head)
+{
+	struct object_id head_oid;
+	struct tree_desc desc;
+	struct lock_file lock = LOCK_INIT;
+	struct unpack_trees_options unpack_tree_opts;
+	struct tree *tree;
+	const char *reflog_action;
+	struct strbuf msg = STRBUF_INIT;
+	size_t prefix_len;
+	struct object_id *orig = NULL, oid_orig,
+		*old_orig = NULL, oid_old_orig;
+	int ret = 0;
+
+	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
+		return -1;
+
+	if (!oid) {
+		if (get_oid("HEAD", &head_oid)) {
+			rollback_lock_file(&lock);
+			return error(_("could not determine HEAD revision"));
+		}
+		oid = &head_oid;
+	}
+
+	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
+	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
+	unpack_tree_opts.head_idx = 1;
+	unpack_tree_opts.src_index = the_repository->index;
+	unpack_tree_opts.dst_index = the_repository->index;
+	unpack_tree_opts.fn = oneway_merge;
+	unpack_tree_opts.update = 1;
+	unpack_tree_opts.merge = 1;
+	if (!detach_head)
+		unpack_tree_opts.reset = 1;
+
+	if (read_index_unmerged(the_repository->index) < 0) {
+		rollback_lock_file(&lock);
+		return error(_("could not read index"));
+	}
+
+	if (!fill_tree_descriptor(&desc, oid)) {
+		error(_("failed to find tree of %s"), oid_to_hex(oid));
+		rollback_lock_file(&lock);
+		free((void *)desc.buffer);
+		return -1;
+	}
+
+	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
+		rollback_lock_file(&lock);
+		free((void *)desc.buffer);
+		return -1;
+	}
+
+	tree = parse_tree_indirect(oid);
+	prime_cache_tree(the_repository->index, tree);
+
+	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)
+		ret = error(_("could not write index"));
+	free((void *)desc.buffer);
+
+	if (ret)
+		return ret;
+
+	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
+	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
+	prefix_len = msg.len;
+
+	if (!get_oid("ORIG_HEAD", &oid_old_orig))
+		old_orig = &oid_old_orig;
+	if (!get_oid("HEAD", &oid_orig)) {
+		orig = &oid_orig;
+		strbuf_addstr(&msg, "updating ORIG_HEAD");
+		update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0,
+			   UPDATE_REFS_MSG_ON_ERR);
+	} else if (old_orig)
+		delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
+	strbuf_setlen(&msg, prefix_len);
+	strbuf_addstr(&msg, "updating HEAD");
+	if (!switch_to_branch)
+		ret = update_ref(msg.buf, "HEAD", oid, orig, REF_NO_DEREF,
+				 UPDATE_REFS_MSG_ON_ERR);
+	else {
+		ret = create_symref("HEAD", switch_to_branch, msg.buf);
+		if (!ret)
+			ret = update_ref(msg.buf, "HEAD", oid, NULL, 0,
+					 UPDATE_REFS_MSG_ON_ERR);
+	}
+
+	strbuf_release(&msg);
+	return ret;
+}
+
 int cmd_rebase(int argc, const char **argv, const char *prefix)
 {
+	struct rebase_options options = {
+		.type = REBASE_UNSPECIFIED,
+	};
+	const char *branch_name;
+	int ret, flags;
+	struct strbuf msg = STRBUF_INIT;
+	struct strbuf revisions = STRBUF_INIT;
+
 	/*
 	 * NEEDSWORK: Once the builtin rebase has been tested enough
 	 * and git-legacy-rebase.sh is retired to contrib/, this preamble
@@ -54,5 +324,98 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	trace_repo_setup(prefix);
 	setup_work_tree();
 
-	die("TODO");
+	git_config(git_default_config, NULL);
+
+	switch (options.type) {
+	case REBASE_MERGE:
+	case REBASE_INTERACTIVE:
+	case REBASE_PRESERVE_MERGES:
+		options.state_dir = merge_dir();
+		break;
+	case REBASE_AM:
+		options.state_dir = apply_dir();
+		break;
+	default:
+		/* the default rebase backend is `--am` */
+		options.type = REBASE_AM;
+		options.state_dir = apply_dir();
+		break;
+	}
+
+	if (!options.root) {
+		if (argc < 2)
+			die("TODO: handle @{upstream}");
+		else {
+			options.upstream_name = argv[1];
+			argc--;
+			argv++;
+			if (!strcmp(options.upstream_name, "-"))
+				options.upstream_name = "@{-1}";
+		}
+		options.upstream = peel_committish(options.upstream_name);
+		if (!options.upstream)
+			die(_("invalid upstream '%s'"), options.upstream_name);
+	} else
+		die("TODO: upstream for --root");
+
+	/* Make sure the branch to rebase onto is valid. */
+	if (!options.onto_name)
+		options.onto_name = options.upstream_name;
+	if (strstr(options.onto_name, "...")) {
+		die("TODO");
+	} else {
+		options.onto = peel_committish(options.onto_name);
+		if (!options.onto)
+			die(_("Does not point to a valid commit '%s'"),
+				options.onto_name);
+	}
+
+	/*
+	 * If the branch to rebase is given, that is the branch we will rebase
+	 * branch_name -- branch/commit being rebased, or
+	 * 		  HEAD (already detached)
+	 * orig_head -- commit object name of tip of the branch before rebasing
+	 * head_name -- refs/heads/<that-branch> or "detached HEAD"
+	 */
+	if (argc > 1)
+		 die("TODO: handle switch_to");
+	else {
+		/* Do not need to switch branches, we are already on it. */
+		options.head_name =
+			xstrdup_or_null(resolve_ref_unsafe("HEAD", 0, NULL,
+					 &flags));
+		if (!options.head_name)
+			die(_("No such ref: %s"), "HEAD");
+		if (flags & REF_ISSYMREF) {
+			if (!skip_prefix(options.head_name,
+					 "refs/heads/", &branch_name))
+				branch_name = options.head_name;
+
+		} else {
+			options.head_name = xstrdup("detached HEAD");
+			branch_name = "HEAD";
+		}
+		if (get_oid("HEAD", &options.orig_head))
+			die(_("Could not resolve HEAD to a revision"));
+	}
+
+	strbuf_addf(&msg, "rebase: checkout %s", options.onto_name);
+	if (reset_head(&options.onto->object.oid, "checkout", NULL, 1))
+		die(_("Could not detach HEAD"));
+	strbuf_release(&msg);
+
+	strbuf_addf(&revisions, "%s..%s",
+		    options.root ? oid_to_hex(&options.onto->object.oid) :
+		    (options.restrict_revision ?
+		     oid_to_hex(&options.restrict_revision->object.oid) :
+		     oid_to_hex(&options.upstream->object.oid)),
+		    oid_to_hex(&options.orig_head));
+
+	options.revisions = revisions.buf;
+
+	ret = !!run_specific_rebase(&options);
+
+	strbuf_release(&revisions);
+	free(options.head_name);
+	return ret;
 }
-- 
2.18.0
Junio C Hamano· Aug 16, 2018, 21:43 UTC · re: Pratik Karki · lore

Re: [GSoC] [PATCH v6 3/3] builtin/rebase: support running "git rebase <upstream>"

Now, there is a parallel "rebase-i-in-c" effort going on, and of course, setting various shell various and formulating a command line that essentially does

	. git-rebase--$backend
of course stops working.
Show 20 quoted lines
> +static int run_specific_rebase(struct rebase_options *opts)
> +{
> +	const char *argv[] = { NULL, NULL };
> + ...
> +	switch (opts->type) {
> +	case REBASE_AM:
> +		backend = "git-rebase--am";
> +		backend_func = "git_rebase__am";
> +		break;
> +	case REBASE_INTERACTIVE:
> +		backend = "git-rebase--interactive";
> +		backend_func = "git_rebase__interactive";
> +		break;
> + ...
> +	strbuf_addf(&script_snippet,
> +		    ". git-sh-setup && . git-rebase--common &&"
> +		    " . %s && %s", backend, backend_func);
> +	argv[0] = script_snippet.buf;
> +
> +	status = run_command_v_opt(argv, RUN_USING_SHELL);
Pratik Karki· Aug 6, 2018, 19:31 UTC · re: Pratik Karki · lore

[GSoC] [PATCH v6 2/3] rebase: refactor common shell functions into their own file

The functions present in `git-legacy-rebase.sh` are used by the rebase backends as they are implemented as shell script functions in the `git-rebase--<backend>` files.

To make the `builtin/rebase.c` work, we have to provide support via a Unix shell script snippet that uses these functions and so, we want to use the rebase backends *directly* from the builtin rebase without going through `git-legacy-rebase.sh`.

This commit extracts the functions to a separate file, `git-rebase--common`, that will be read by `git-legacy-rebase.sh` and by the shell script snippets which will be used extensively in the following commits.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
Unchanged since v5.
 .gitignore            |  1 +
 Makefile              |  1 +
 git-legacy-rebase.sh  | 69 ++-----------------------------------------
 git-rebase--common.sh | 68 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 67 deletions(-)
 create mode 100644 git-rebase--common.sh
Show changes to 4 files +72 −67

.gitignore, Makefile, git-legacy-rebase.sh, git-rebase--common.sh

diff --git a/.gitignore b/.gitignore
index ec23959014..824141cba1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -117,6 +117,7 @@
 /git-read-tree
 /git-rebase
 /git-rebase--am
+/git-rebase--common
 /git-rebase--helper
 /git-rebase--interactive
 /git-rebase--merge
diff --git a/Makefile b/Makefile
index c311532791..c210681a1d 100644
--- a/Makefile
+++ b/Makefile
@@ -619,6 +619,7 @@ SCRIPT_SH += git-web--browse.sh
 SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
+SCRIPT_LIB += git-rebase--common
 SCRIPT_LIB += git-rebase--interactive
 SCRIPT_LIB += git-rebase--preserve-merges
 SCRIPT_LIB += git-rebase--merge
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 7973447645..af2cdfef03 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -57,12 +57,7 @@ cd_to_toplevel
 LF='
 '
 ok_to_skip_pre_rebase=
-resolvemsg="
-$(gettext 'Resolve all conflicts manually, mark them as resolved with
-"git add/rm <conflicted_files>", then run "git rebase --continue".
-You can instead skip this commit: run "git rebase --skip".
-To abort and get back to the state before "git rebase", run "git rebase --abort".')
-"
+
 squash_onto=
 unset onto
 unset restrict_revision
@@ -102,6 +97,7 @@ case "$(git config --bool commit.gpgsign)" in
 true)	gpg_sign_opt=-S ;;
 *)	gpg_sign_opt= ;;
 esac
+. git-rebase--common
 
 read_basic_state () {
 	test -f "$state_dir/head-name" &&
@@ -132,67 +128,6 @@ read_basic_state () {
 	}
 }
 
-write_basic_state () {
-	echo "$head_name" > "$state_dir"/head-name &&
-	echo "$onto" > "$state_dir"/onto &&
-	echo "$orig_head" > "$state_dir"/orig-head &&
-	echo "$GIT_QUIET" > "$state_dir"/quiet &&
-	test t = "$verbose" && : > "$state_dir"/verbose
-	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
-	test -n "$strategy_opts" && echo "$strategy_opts" > \
-		"$state_dir"/strategy_opts
-	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
-		"$state_dir"/allow_rerere_autoupdate
-	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
-	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
-}
-
-output () {
-	case "$verbose" in
-	'')
-		output=$("$@" 2>&1 )
-		status=$?
-		test $status != 0 && printf "%s\n" "$output"
-		return $status
-		;;
-	*)
-		"$@"
-		;;
-	esac
-}
-
-move_to_original_branch () {
-	case "$head_name" in
-	refs/*)
-		message="rebase finished: $head_name onto $onto"
-		git update-ref -m "$message" \
-			$head_name $(git rev-parse HEAD) $orig_head &&
-		git symbolic-ref \
-			-m "rebase finished: returning to $head_name" \
-			HEAD $head_name ||
-		die "$(eval_gettext "Could not move back to \$head_name")"
-		;;
-	esac
-}
-
-apply_autostash () {
-	if test -f "$state_dir/autostash"
-	then
-		stash_sha1=$(cat "$state_dir/autostash")
-		if git stash apply $stash_sha1 >/dev/null 2>&1
-		then
-			echo "$(gettext 'Applied autostash.')" >&2
-		else
-			git stash store -m "autostash" -q $stash_sha1 ||
-			die "$(eval_gettext "Cannot store \$stash_sha1")"
-			gettext 'Applying autostash resulted in conflicts.
-Your changes are safe in the stash.
-You can run "git stash pop" or "git stash drop" at any time.
-' >&2
-		fi
-	fi
-}
-
 finish_rebase () {
 	rm -f "$(git rev-parse --git-path REBASE_HEAD)"
 	apply_autostash &&
diff --git a/git-rebase--common.sh b/git-rebase--common.sh
new file mode 100644
index 0000000000..7e39d22871
--- /dev/null
+++ b/git-rebase--common.sh
@@ -0,0 +1,68 @@
+
+resolvemsg="
+$(gettext 'Resolve all conflicts manually, mark them as resolved with
+"git add/rm <conflicted_files>", then run "git rebase --continue".
+You can instead skip this commit: run "git rebase --skip".
+To abort and get back to the state before "git rebase", run "git rebase --abort".')
+"
+
+write_basic_state () {
+	echo "$head_name" > "$state_dir"/head-name &&
+	echo "$onto" > "$state_dir"/onto &&
+	echo "$orig_head" > "$state_dir"/orig-head &&
+	echo "$GIT_QUIET" > "$state_dir"/quiet &&
+	test t = "$verbose" && : > "$state_dir"/verbose
+	test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
+	test -n "$strategy_opts" && echo "$strategy_opts" > \
+		"$state_dir"/strategy_opts
+	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
+		"$state_dir"/allow_rerere_autoupdate
+	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
+	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
+}
+
+apply_autostash () {
+	if test -f "$state_dir/autostash"
+	then
+		stash_sha1=$(cat "$state_dir/autostash")
+		if git stash apply $stash_sha1 >/dev/null 2>&1
+		then
+			echo "$(gettext 'Applied autostash.')" >&2
+		else
+			git stash store -m "autostash" -q $stash_sha1 ||
+			die "$(eval_gettext "Cannot store \$stash_sha1")"
+			gettext 'Applying autostash resulted in conflicts.
+Your changes are safe in the stash.
+You can run "git stash pop" or "git stash drop" at any time.
+' >&2
+		fi
+	fi
+}
+
+move_to_original_branch () {
+	case "$head_name" in
+	refs/*)
+		message="rebase finished: $head_name onto $onto"
+		git update-ref -m "$message" \
+			$head_name $(git rev-parse HEAD) $orig_head &&
+		git symbolic-ref \
+			-m "rebase finished: returning to $head_name" \
+			HEAD $head_name ||
+		die "$(eval_gettext "Could not move back to \$head_name")"
+		;;
+	esac
+}
+
+output () {
+	case "$verbose" in
+	'')
+		output=$("$@" 2>&1 )
+		status=$?
+		test $status != 0 && printf "%s\n" "$output"
+		return $status
+		;;
+	*)
+		"$@"
+		;;
+	esac
+}
-- 
2.18.0
Pratik Karki· Aug 6, 2018, 19:31 UTC · re: Pratik Karki · lore

[GSoC] [PATCH v6 1/3] rebase: start implementing it as a builtin

This commit imitates the strategy that was used to convert the difftool to a builtin. We start by renaming the shell script `git-rebase.sh` to `git-legacy-rebase.sh` and introduce a `builtin/rebase.c` that simply executes the shell script version, unless the config setting `rebase.useBuiltin` is set to `true`.

The motivation behind this is to rewrite all the functionality of the shell script version in the aforementioned `rebase.c`, one by one and be able to conveniently test new features by configuring `rebase.useBuiltin`.

In the original difftool conversion, if sane_execvp() that attempts to run the legacy scripted version returned with non-negative status, the command silently exited without doing anything with success, but sane_execvp() should not return with non-negative status in the first place, so we use die() to notice such an abnormal case.

We intentionally avoid reading the config directly to avoid messing up the GIT_* environment variables when we need to fall back to exec()ing the shell script. The test of builtin rebase can be done by `git -c rebase.useBuiltin=true rebase ...`

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
---
Unchanged since v5.
 .gitignore                            |  1 +
 Makefile                              |  3 +-
 builtin.h                             |  1 +
 builtin/rebase.c                      | 58 +++++++++++++++++++++++++++
 git-rebase.sh => git-legacy-rebase.sh |  0
 git.c                                 |  6 +++
 6 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 builtin/rebase.c
 rename git-rebase.sh => git-legacy-rebase.sh (100%)
Show changes to 6 files +68 −1

.gitignore, Makefile, builtin.h, builtin/rebase.c, git-rebase.sh, git.c

diff --git a/.gitignore b/.gitignore
index 3284a1e9b1..ec23959014 100644
--- a/.gitignore
+++ b/.gitignore
@@ -78,6 +78,7 @@
 /git-init-db
 /git-interpret-trailers
 /git-instaweb
+/git-legacy-rebase
 /git-log
 /git-ls-files
 /git-ls-remote
diff --git a/Makefile b/Makefile
index bc4fc8eeab..c311532791 100644
--- a/Makefile
+++ b/Makefile
@@ -609,7 +609,7 @@ SCRIPT_SH += git-merge-one-file.sh
 SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
 SCRIPT_SH += git-quiltimport.sh
-SCRIPT_SH += git-rebase.sh
+SCRIPT_SH += git-legacy-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
 SCRIPT_SH += git-request-pull.sh
 SCRIPT_SH += git-stash.sh
@@ -1063,6 +1063,7 @@ BUILTIN_OBJS += builtin/prune.o
 BUILTIN_OBJS += builtin/pull.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase.o
 BUILTIN_OBJS += builtin/rebase--helper.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
diff --git a/builtin.h b/builtin.h
index 0362f1ce25..44651a447f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -202,6 +202,7 @@ extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
 extern int cmd_pull(int argc, const char **argv, const char *prefix);
 extern int cmd_push(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase(int argc, const char **argv, const char *prefix);
 extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase.c b/builtin/rebase.c
new file mode 100644
index 0000000000..c44addb2a4
--- /dev/null
+++ b/builtin/rebase.c
@@ -0,0 +1,58 @@
+/*
+ * "git rebase" builtin command
+ *
+ * Copyright (c) 2018 Pratik Karki
+ */
+
+#include "builtin.h"
+#include "run-command.h"
+#include "exec-cmd.h"
+#include "argv-array.h"
+#include "dir.h"
+
+static int use_builtin_rebase(void)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf out = STRBUF_INIT;
+	int ret;
+
+	argv_array_pushl(&cp.args,
+			 "config", "--bool", "rebase.usebuiltin", NULL);
+	cp.git_cmd = 1;
+	if (capture_command(&cp, &out, 6)) {
+		strbuf_release(&out);
+		return 0;
+	}
+
+	strbuf_trim(&out);
+	ret = !strcmp("true", out.buf);
+	strbuf_release(&out);
+	return ret;
+}
+
+int cmd_rebase(int argc, const char **argv, const char *prefix)
+{
+	/*
+	 * NEEDSWORK: Once the builtin rebase has been tested enough
+	 * and git-legacy-rebase.sh is retired to contrib/, this preamble
+	 * can be removed.
+	 */
+
+	if (!use_builtin_rebase()) {
+		const char *path = mkpath("%s/git-legacy-rebase",
+					  git_exec_path());
+
+		if (sane_execvp(path, (char **)argv) < 0)
+			die_errno(_("could not exec %s"), path);
+		else
+			BUG("sane_execvp() returned???");
+	}
+
+	if (argc != 2)
+		die(_("Usage: %s <base>"), argv[0]);
+	prefix = setup_git_directory();
+	trace_repo_setup(prefix);
+	setup_work_tree();
+
+	die("TODO");
+}
diff --git a/git-rebase.sh b/git-legacy-rebase.sh
similarity index 100%
rename from git-rebase.sh
rename to git-legacy-rebase.sh
diff --git a/git.c b/git.c
index fc7d15d549..2c6b188c77 100644
--- a/git.c
+++ b/git.c
@@ -521,6 +521,12 @@ static struct cmd_struct commands[] = {
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
 	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
+	/*
+	 * NEEDSWORK: Until the rebase is independent and needs no redirection
+	 * to rebase shell script this is kept as is, then should be changed to
+	 * RUN_SETUP | NEED_WORK_TREE
+	 */
+	{ "rebase", cmd_rebase },
 	{ "rebase--helper", cmd_rebase__helper, RUN_SETUP | NEED_WORK_TREE },
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
-- 
2.18.0

← back to recent threads