login
Search
For the past two months, I have been working on refactoring the rebase code. See [1] for background information. I have been trying to polish the patch set for some time, but now I don't think I will get much further without your help. My goals with this series were: (1) Make it easier to add new features to git rebase by reducing the amount of duplicated code. FWIW, this series removes about 140 lines non-test code. (2) Make the behavior (towards the user) more consistent between interactive and non-interactive rebase. This mainly involves error messages and command line options. While not being a goal from the beginning, I have also tried to make the code more readable. This is of course very subjective, so we'll see - maybe in your eyes I just made things worse :-). At a high level, I tried to do what was suggested by Johannes Sixt in the second entry on [1], namely "... to write a command line processor, git-rebase.sh, that sets shell variables from options that it collects from various sources, then dispatches to one of git-rebase--interactive.sh, git-rebase--merge.sh, or git-rebase--am.sh (the latter two would be stripped-down copies of the current git-rebase.sh)." Patches 01-04 try to make git-rebase.sh more readable and extensible by factoring out the code that reads the saved state from .git/rebase-apply or .git/rebase-merge. Patches 05-09 set the stage for further refactoring by aligning git-rebase.sh and git-rebase--interactive.sh. Patches 10-16 factor out parts from git-rebase--interactive.sh and let it rely on the corresponding code in git-rebase.sh. Patch 17 just removes a duplicated variable. Patches 18 and 19 extract the am-specific and merge-specifc rebase code into two new source files (as suggested by Hannes). Patch 20 makes interactive rebase print the same message as non-interactive rebase in case of conflict. Patches 21 and 22 prepare for further refactoring by aligning interactive and non-interactive rebase a bit ...
The state stored in $GIT_DIR/rebase-merge/prev_head was introduced in
58634db (rebase: Allow merge strategies to be used when rebasing,
2006-06-21), but it was never used and should therefore be removed.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-rebase.sh | 8 --------
1 files changed, 0 insertions(+), 8 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index 9be831e..af9bd14 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -61,7 +61,6 @@ read_state () {
if test -d "$merge_dir"
then
state_dir="$merge_dir"
- prev_head=$(cat "$merge_dir"/prev_head) &&
onto_name=$(cat "$merge_dir"/onto_name) &&
end=$(cat "$merge_dir"/end) &&
msgnum=$(cat "$merge_dir"/msgnum)
@@ -75,7 +74,6 @@ read_state () {
}
continue_merge () {
- test -n "$prev_head" || die "prev_head must be defined"
test -d "$merge_dir" || die "$merge_dir directory does not exist"
unmerged=$(git ls-files -u)
@@ -109,10 +107,6 @@ continue_merge () {
test -z "$GIT_QUIET" &&
GIT_PAGER='' git log --format=%s -1 "$cmt"
- prev_head=`git rev-parse HEAD^0`
- # save the resulting commit so we can read-tree on it later
- echo "$prev_head" > "$merge_dir/prev_head"
-
# onto the next patch:
msgnum=$(($msgnum + 1))
echo "$msgnum" >"$merge_dir/msgnum"
@@ -567,8 +561,6 @@ fi
mkdir -p "$merge_dir"
echo "$onto_name" > "$merge_dir/onto_name"
-prev_head=$orig_head
-echo "$prev_head" > "$merge_dir/prev_head"
echo "$head_name" > "$merge_dir/head-name"
echo "$onto" > "$merge_dir/onto"
echo "$orig_head" > "$merge_dir/orig-head"
--
1.7.3.2.864.gbbb96
--
Make sure to interpret variables with the same name in the same way in
git-rebase.sh and git-rebase--interactive.sh. This will make it easier
to factor out code from git-rebase.sh to git-rebase--interactive and
export the variables.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-rebase--interactive.sh | 16 +++++++++++-----
git-rebase.sh | 4 +++-
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 310d80e..1af739a 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -339,7 +339,8 @@ pick_one_preserving_merges () {
# No point in merging the first parent, that's HEAD
new_parents=${new_parents# $first_parent}
if ! do_with_author output \
- git merge $strategy -m "$msg" $new_parents
+ git merge ${strategy:+-s $strategy} -m "$msg" \
+ $new_parents
then
printf "%s\n" "$msg" > "$GIT_DIR"/MERGE_MSG
die_with_patch $sha1 "Error redoing merge $sha1"
@@ -827,11 +828,11 @@ first and then run 'git rebase --continue' again."
-s)
case "$#,$1" in
*,*=*)
- strategy="-s "$(expr "z$1" : 'z-[^=]*=\(.*\)') ;;
+ strategy=$(expr "z$1" : 'z-[^=]*=\(.*\)') ;;
1,*)
usage ;;
*)
- strategy="-s $2"
+ strategy="$2"
shift ;;
esac
;;
@@ -860,9 +861,9 @@ first and then run 'git rebase --continue' again."
autosquash=
;;
--onto)
+ test 2 -le "$#" || usage
+ onto="$2"
shift
- onto=$(parse_onto "$1") ||
- die "Does not point to a valid commit: $1"
;;
--)
shift
@@ -872,6 +873,11 @@ first and then run 'git rebase --continue' again."
shift
done
+if test -n "$onto"
+then
+ onto=$(parse_onto "$onto") || die "Does not point to a valid commit: $1"
+fi
+
test -z "$rebase_root" -a $# -ge 1 -a $# -le 2 ||
test ! -z "$rebase_root" -a $# -le 1 || usage
test -d "$DOTEST" &&
diff --git a/git-rebase.sh b/git-rebase.sh
index dc133e3..21366ba ...Detect early on if a rebase is in progress and what type of rebase it
is (interactive, merge-based or am-based). This prepares for further
refactoring where am-based rebase will be dispatched to
git-rebase--am.sh and merge-based rebase will be dispatched to
git-rebase--merge.sh.
The idea is to use the same variables whether the type of rebase was
detected from rebase-apply/ or rebase-merge/ directories or from the
command line options. This will make the code more readable and will
later also make it easier to dispatch to the type-specific scripts.
Also show a consistent error message independent of the type of rebase
that was in progress and remove the obsolete wording about being in
the middle of a 'patch application', since that (an existing
"$GIT_DIR"/rebase-apply/applying) aborts 'git rebase' at an earlier
stage.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
Currently, the existence of rebase-merge/ is tested with 'test -d',
while the existence of rebase-apply/ is tested by creating the
directory and then deleting it again. Any good reason for this?
git-rebase.sh | 80 +++++++++++++++++++++++++++++---------------------------
1 files changed, 41 insertions(+), 39 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index af9bd14..718cb26 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -56,16 +56,19 @@ git_am_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=
read_state () {
- if test -d "$merge_dir"
+ if test "$type" = merge
then
- state_dir="$merge_dir"
- onto_name=$(cat "$merge_dir"/onto_name) &&
- end=$(cat "$merge_dir"/end) &&
- msgnum=$(cat "$merge_dir"/msgnum)
- else
- state_dir="$apply_dir"
+ onto_name=$(cat "$state_dir"/onto_name) &&
+ end=$(cat "$state_dir"/end) &&
+ msgnum=$(cat ...I don't recall how the merge side reached the current shape of the code, but I think the rebase-apply one was that we wanted to make sure not only we don't have a directory but also we actually _can_ create one. If somebody had a bad permission set, "test -d" wouldn't help us much. We would fail later and error diagnosis codepath should do the right thing anyway, so it is not a correctness issue, but is more about attempting to notice an error early rather than late. --
Yeah, I was wondering if that might have been the reason. Why would anyone set such permissions on .git (keep in mind that I am no Linux/Unix expert)? I saw that the code was introduced [1] when the directory was called .dotest and I believe this directory was created at the top level (i.e. a sibling to .git). Could it be that it was reasonable for the user to set such permissions back then (on the top level dir), but not any longer (on the .git dir)? Either way, if it is good to have the that check for rebase-apply/, wouldn't it be good to have for rebase-merge/ as well? /Martin [1] 7f4bd5d (rebase: one safety net, one bugfix and one optimization., 2005-11-28) --
To later be able to use the command line processing in git-rebase.sh for both interactive and non-interactive rebases, move anything that is specific to non-interactive rebase outside of the parsing loop. Keep only parsing and validation of command line options in the loop. Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> --- May want to view this patch with --ignore-all-space. git-rebase--interactive.sh | 300 ++++++++++++++++++++++---------------------- git-rebase.sh | 126 ++++++++++--------- 2 files changed, 217 insertions(+), 209 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index a5ffd9a..8cbdd3f 100755 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -866,152 +866,158 @@ first and then run 'git rebase --continue' again." ;; --) shift - test -z "$REBASE_ROOT" -a $# -ge 1 -a $# -le 2 || - test ! -z "$REBASE_ROOT" -a $# -le 1 || usage - test -d "$DOTEST" && - die "Interactive rebase already started" - - git var GIT_COMMITTER_IDENT >/dev/null || - die "You need to set your committer info first" - - if test -z "$REBASE_ROOT" - then - UPSTREAM_ARG="$1" - UPSTREAM=$(git rev-parse --verify "$1") || die "Invalid base" - test -z "$ONTO" && ONTO=$UPSTREAM - shift - else - UPSTREAM= - UPSTREAM_ARG=--root - test -z "$ONTO" && - die "You must specify --onto when using --root" - fi - run_pre_rebase_hook "$UPSTREAM_ARG" "$@" - - comment_for_reflog start - - require_clean_work_tree "rebase" "Please commit or stash them." - - if test ! -z "$1" - then - output git checkout "$1" || - die "Could not checkout $1" - fi + break + ;; + esac + shift +done - HEAD=$(git rev-parse --verify HEAD) || die "No HEAD?" - mkdir "$DOTEST" || die "Could not create temporary $DOTEST" +test -z "$REBASE_ROOT" -a $# -ge 1 -a $# -le 2 || +test ! -z "$REBASE_ROOT" -a $# -le 1 || usage +test -d "$DOTEST" && + die "Interactive rebase ...
Remove the call to the pre-rebase hook from
git-rebase--interactive.sh and rely on the call in
git-rebase.sh.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-rebase--interactive.sh | 14 --------------
git-rebase.sh | 15 ++++++++-------
2 files changed, 8 insertions(+), 21 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index edde1e5..0beeb8b 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -109,18 +109,6 @@ commit_message () {
git cat-file commit "$1" | sed "1,/^$/d"
}
-run_pre_rebase_hook () {
- if test -z "$OK_TO_SKIP_PRE_REBASE" &&
- test -x "$GIT_DIR/hooks/pre-rebase"
- then
- "$GIT_DIR/hooks/pre-rebase" ${1+"$@"} || {
- echo >&2 "The pre-rebase hook refused to rebase."
- exit 1
- }
- fi
-}
-
-
ORIG_REFLOG_ACTION="$GIT_REFLOG_ACTION"
comment_for_reflog () {
@@ -753,8 +741,6 @@ esac
git var GIT_COMMITTER_IDENT >/dev/null ||
die "You need to set your committer info first"
-run_pre_rebase_hook "$upstream_arg" "$@"
-
comment_for_reflog start
if test ! -z "$switch_to"
diff --git a/git-rebase.sh b/git-rebase.sh
index e1e5263..229e8d2 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -185,9 +185,8 @@ run_interactive_rebase () {
export GIT_EDITOR
fi
export onto autosquash strategy strategy_opts verbose rebase_root \
- force_rebase action preserve_merges OK_TO_SKIP_PRE_REBASE upstream \
- upstream_arg switch_to head_name
- exec git-rebase--interactive "$@"
+ force_rebase action preserve_merges upstream switch_to head_name
+ exec git-rebase--interactive
}
run_pre_rebase_hook () {
@@ -515,15 +514,15 @@ orig_head=$branch
require_clean_work_tree "rebase" "Please commit or stash them."
-test "$type" = interactive && run_interactive_rebase "$@"
-
# Now we are rebasing commits $upstream..$branch (or with --root,
# everything leading up to $branch) on top of $onto
# Check if we are already based ...Use the same names for variables that git-rebase--interactive.sh will
soon inherit from git-rebase.sh.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-rebase--interactive.sh | 138 ++++++++++++++++++++++----------------------
git-rebase.sh | 8 +-
2 files changed, 73 insertions(+), 73 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 8cbdd3f..310d80e 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -77,7 +77,7 @@ FIXUP_MSG="$DOTEST"/message-fixup
# $REWRITTEN is the name of a directory containing files for each
# commit that is reachable by at least one merge base of $HEAD and
-# $UPSTREAM. They are not necessarily rewritten, but their children
+# $upstream. They are not necessarily rewritten, but their children
# might be. This ensures that commits on merged, but otherwise
# unrelated side branches are left alone. (Think "X" in the man page's
# example.)
@@ -105,15 +105,15 @@ AMEND="$DOTEST"/amend
REWRITTEN_LIST="$DOTEST"/rewritten-list
REWRITTEN_PENDING="$DOTEST"/rewritten-pending
-PRESERVE_MERGES=
-STRATEGY=
-ONTO=
-VERBOSE=
+preserve_merges=
+strategy=
+onto=
+verbose=
OK_TO_SKIP_PRE_REBASE=
-REBASE_ROOT=
-AUTOSQUASH=
-test "$(git config --bool rebase.autosquash)" = "true" && AUTOSQUASH=t
-NEVER_FF=
+rebase_root=
+autosquash=
+test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
+force_rebase=
GIT_CHERRY_PICK_HELP="\
hint: after resolving the conflicts, mark the corrected paths
@@ -125,7 +125,7 @@ warn () {
}
output () {
- case "$VERBOSE" in
+ case "$verbose" in
'')
output=$("$@" 2>&1 )
status=$?
@@ -177,7 +177,7 @@ mark_action_done () {
then
last_count=$count
printf "Rebasing (%d/%d)\r" $count $total
- test -z "$VERBOSE" || echo
+ test -z "$verbose" || echo
fi
}
@@ -228,11 +228,11 @@ do_with_author () {
pick_one () {
ff=--ff
case "$1" in -n) sha1=$2; ff= ;; ...AFAICS this is partly about spelling the variables in lowercase
instead of all-caps.
Wouldn't it be nicer to simply downcase *all* variables, so that the
[...]
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
Factor out the command line processing in git-rebase--interactive.sh to git-rebase.sh. Store the options in variables in git-rebase.sh and export them before calling git-rebase--interactive.sh. Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> --- Since this removes the command line processing from git-rebase--interactive.sh, it completely changes its command line interface. Since it is not listed as even a plumbing command, I hope this is fine. git-rebase--interactive.sh | 224 ++++++++++++-------------------------------- git-rebase.sh | 60 ++++++++---- 2 files changed, 102 insertions(+), 182 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 1af739a..4d3dc63 100755 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -10,31 +10,7 @@ # The original idea comes from Eric W. Biederman, in # http://article.gmane.org/gmane.comp.version-control.git/22407 -OPTIONS_KEEPDASHDASH= -OPTIONS_SPEC="\ -git-rebase [-i] [options] [--] <upstream> [<branch>] -git-rebase [-i] (--continue | --abort | --skip) --- - Available options are -v,verbose display a diffstat of what changed upstream -onto= rebase onto given branch instead of upstream -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 always used (no-op) -i,interactive always used (no-op) - Actions: -continue continue rebasing process -abort abort rebasing process and restore original branch -skip skip current patch and continue rebasing process -no-verify override pre-rebase hook from stopping the operation -verify allow pre-rebase hook to run -root rebase all reachable commmits up to the root(s) -autosquash move commits that begin with squash!/fixup! under -i -" - . ...
Move up the code that displays the diffstat if '--stat' is passed, so that it will be executed before calling git-rebase--interactive.sh. A side effect is that the diffstat is now displayed before "First, rewinding head to replay your work on top of it...". Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> --- git-rebase.sh | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-) diff --git a/git-rebase.sh b/git-rebase.sh index 229e8d2..0fc580a 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -540,13 +540,6 @@ fi # If a hook exists, give it a chance to interrupt run_pre_rebase_hook "$upstream_arg" "$@" -test "$type" = interactive && run_interactive_rebase - -# Detach HEAD and reset the tree -say "First, rewinding head to replay your work on top of it..." -git checkout -q "$onto^0" || die "could not detach HEAD" -git update-ref ORIG_HEAD $branch - if test -n "$diffstat" then if test -n "$verbose" @@ -557,6 +550,13 @@ then GIT_PAGER='' git diff --stat --summary "$mb" "$onto" fi +test "$type" = interactive && run_interactive_rebase + +# Detach HEAD and reset the tree +say "First, rewinding head to replay your work on top of it..." +git checkout -q "$onto^0" || die "could not detach HEAD" +git update-ref ORIG_HEAD $branch + # If the $onto is a proper descendant of the tip of the branch, then # we just fast-forwarded. if test "$mb" = "$branch" -- 1.7.3.2.864.gbbb96 --
Hi, Hmpf... After a rebasing merge to junio/next: -- snip -- [...] Applying: rebase -i: support --stat fatal: sha1 information is lacking or useless (git-rebase.sh). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. [...] -- snap -- Is this supposed to apply on top of junio/master, junio/next, junio/maint? Ciao, Dscho --
It is supposed to apply on top of junio/master. I tried exporting the recevied emails using Alpine. This is the first time I do this, but I managed to export them to one big file. I then applied it using 'git am' onto a new branch created from junio/master and it was successful. I'm very surprised that patch 16 failed for you if the first 15 patches applied correctly. I'm not sure what to use the hashes in the beginning of your mail for, but I have verified that they match the hashes of git-rebase.sh before and after patch 16 (in both my original branch and in the temporary branch where I ran 'git am' on my own emails). Regards, Martin --
The series applied cleanly near the tip of my 'master'. I've been preparing to push out 1.7.4-rc0 so the commit I applied the series was probably a few commits ahead of the public 'master', but I don't think there is anything that may conflict with or may be required for this series to make any difference. Please check tonight's 'pu' where the series is parked. Thanks. --
Hi, Sorry, I should have been more specific. I updated to current 'next' Thanks, but unfortunately I have to tend to other things now. Ciao, Johannes --
Extract the code for merge-based rebase to git-rebase--merge.sh.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
What copyright year? Most of the code is just extracted from
git-rebase.sh, which has copyright year 2005. Does that matter?
Would read_initial_state be a better name than read_basic_state?
.gitignore | 1 +
Makefile | 1 +
git-rebase--merge.sh | 154 ++++++++++++++++++++++++++++++++++++++++++++++
git-rebase.sh | 167 +++++---------------------------------------------
4 files changed, 171 insertions(+), 152 deletions(-)
create mode 100644 git-rebase--merge.sh
diff --git a/.gitignore b/.gitignore
index 87b833c..40506ff 100644
--- a/.gitignore
+++ b/.gitignore
@@ -103,6 +103,7 @@
/git-read-tree
/git-rebase
/git-rebase--interactive
+/git-rebase--merge
/git-receive-pack
/git-reflog
/git-relink
diff --git a/Makefile b/Makefile
index ff35154..ffc3a5d 100644
--- a/Makefile
+++ b/Makefile
@@ -370,6 +370,7 @@ SCRIPT_SH += git-mergetool.sh
SCRIPT_SH += git-pull.sh
SCRIPT_SH += git-quiltimport.sh
SCRIPT_SH += git-rebase--interactive.sh
+SCRIPT_SH += git-rebase--merge.sh
SCRIPT_SH += git-rebase.sh
SCRIPT_SH += git-repack.sh
SCRIPT_SH += git-request-pull.sh
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
new file mode 100644
index 0000000..8cfdcf1
--- /dev/null
+++ b/git-rebase--merge.sh
@@ -0,0 +1,154 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Junio C Hamano.
+#
+
+. git-sh-setup
+
+prec=4
+
+read_state () {
+ onto_name=$(cat "$state_dir"/onto_name) &&
+ end=$(cat "$state_dir"/end) &&
+ msgnum=$(cat "$state_dir"/msgnum)
+}
+
+continue_merge () {
+ test -d "$state_dir" || die "$state_dir directory does not exist"
+
+ unmerged=$(git ls-files -u)
+ if test -n "$unmerged"
+ then
+ echo "You still have unmerged paths in your index"
+ echo "did you forget to use git add?"
+ die "$RESOLVEMSG"
+ fi
+
+ cmt=`cat "$state_dir/current"`
+ if ! git diff-index ...I'd prefer to dispatch to the final rebase type using . git-rebase--$type This way, you can avoid to export the huge list of helper variables and the function. (And it might be faster by a millisecond - or a few dozens on Windows.) -- Hannes --
Makes a lot of sense. Will change. Why didn't I do that from the beginning? Thanks, Martin --
On my system I have POSIX docs for various commands, coming from the
'man-pages-posix' package. Maybe your distribution has those too?
Then you can simply run 'man 1p export' for the documentation.
Anything that is documented there should be safe (except on Windows
maybe).
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
Thanks. I'm running Debian and I just installed the package from non-free. Thanks for the hint on the '1p' syntax as well. /Martin --
The variables $branch and $orig_head were used as synonyms. To avoid
confusion, remove $branch. The name 'orig_head' seems more suitable,
since that is the name used when the variable is persisted.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-rebase.sh | 19 +++++++++----------
1 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index 0fc580a..eae2f7a 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -487,10 +487,10 @@ case "$#" in
switch_to="$1"
if git show-ref --verify --quiet -- "refs/heads/$1" &&
- branch=$(git rev-parse -q --verify "refs/heads/$1")
+ orig_head=$(git rev-parse -q --verify "refs/heads/$1")
then
head_name="refs/heads/$1"
- elif branch=$(git rev-parse -q --verify "$1")
+ elif orig_head=$(git rev-parse -q --verify "$1")
then
head_name="detached HEAD"
else
@@ -507,24 +507,23 @@ case "$#" in
head_name="detached HEAD"
branch_name=HEAD ;# detached
fi
- branch=$(git rev-parse --verify "${branch_name}^0") || exit
+ orig_head=$(git rev-parse --verify "${branch_name}^0") || exit
;;
esac
-orig_head=$branch
require_clean_work_tree "rebase" "Please commit or stash them."
-# Now we are rebasing commits $upstream..$branch (or with --root,
-# everything leading up to $branch) on top of $onto
+# 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" "$branch")
+mb=$(git merge-base "$onto" "$orig_head")
if test "$type" != interactive && test "$upstream" = "$onto" &&
test "$mb" = "$onto" &&
# linear history?
- ! (git rev-list --parents "$onto".."$branch" | sane_grep " .* ") > /dev/null
+ ! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > ...To make it possible to later remove the handling of --abort from
git-rebase--interactive.sh, align the implementation in git-rebase.sh
with the former by making it a bit more verbose.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
What do we really want to print when -v is passed? Interactive rebase
is currently quite a bit more verbose than non-interactive rebase.
git-rebase--interactive.sh | 14 --------------
git-rebase.sh | 20 +++++++++++++++++---
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index eace4e5..acd0258 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -86,20 +86,6 @@ warn () {
printf '%s\n' "$*" >&2
}
-output () {
- case "$verbose" in
- '')
- output=$("$@" 2>&1 )
- status=$?
- test $status != 0 && printf "%s\n" "$output"
- return $status
- ;;
- *)
- "$@"
- ;;
- esac
-}
-
# Output the commit message for the specified commit.
commit_message () {
git cat-file commit "$1" | sed "1,/^$/d"
diff --git a/git-rebase.sh b/git-rebase.sh
index 615d9dd..0322f27 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -74,6 +74,20 @@ read_basic_state () {
GIT_QUIET=$(cat "$state_dir"/quiet)
}
+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/*)
@@ -95,7 +109,7 @@ run_specific_rebase () {
force_rebase action preserve_merges upstream switch_to head_name \
state_dir orig_head onto_name GIT_QUIET revisions RESOLVEMSG \
allow_rerere_autoupdate git_am_opt
- export -f move_to_original_branch
+ export -f move_to_original_branch output
exec git-rebase--$type
}
@@ -268,7 +282,7 @@ continue)
run_specific_rebase
;;
skip)
- git reset --hard HEAD || exit $?
+ output git reset ...Reorder validation steps in preparation for the validation to be factored out from git-rebase--interactive.sh into git-rebase.sh. The main functional difference is that the pre-rebase hook will no longer be run if the work tree is dirty. Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> --- git-rebase--interactive.sh | 4 ++-- git-rebase.sh | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 21a9774..5a8f582 100755 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -792,12 +792,12 @@ else test -z "$onto" && die "You must specify --onto when using --root" fi +require_clean_work_tree "rebase" "Please commit or stash them." + run_pre_rebase_hook "$upstream_arg" "$@" comment_for_reflog start -require_clean_work_tree "rebase" "Please commit or stash them." - if test ! -z "$1" then output git checkout "$1" || diff --git a/git-rebase.sh b/git-rebase.sh index e646b8f..26e4218 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -435,8 +435,6 @@ fi test "$type" = interactive && run_interactive_rebase "$@" -require_clean_work_tree "rebase" "Please commit or stash them." - if test -z "$rebase_root" then # The upstream head must be given. Make sure it is valid. @@ -478,9 +476,6 @@ case "$onto_name" in ;; esac -# If a hook exists, give it a chance to interrupt -run_pre_rebase_hook "$upstream_arg" "$@" - # If the branch to rebase is given, that is the branch we will rebase # $branch_name -- branch being rebased, or HEAD (already detached) # $orig_head -- commit object name of tip of the branch before rebasing @@ -518,6 +513,8 @@ case "$#" in esac orig_head=$branch +require_clean_work_tree "rebase" "Please commit or stash them." + # Now we are rebasing commits $upstream..$branch (or with --root, # everything leading up to $branch) on top of $onto @@ -539,6 +536,9 @@ then ...
Remove the check for clean work tree from git-rebase--interactive.sh and rely on the check in git-rebase.sh. Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> --- git-rebase--interactive.sh | 2 -- git-rebase.sh | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index c055fc4..edde1e5 100755 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -753,8 +753,6 @@ esac git var GIT_COMMITTER_IDENT >/dev/null || die "You need to set your committer info first" -require_clean_work_tree "rebase" "Please commit or stash them." - run_pre_rebase_hook "$upstream_arg" "$@" comment_for_reflog start diff --git a/git-rebase.sh b/git-rebase.sh index d8c7c8d..e1e5263 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -513,10 +513,10 @@ case "$#" in esac orig_head=$branch -test "$type" = interactive && run_interactive_rebase "$@" - require_clean_work_tree "rebase" "Please commit or stash them." +test "$type" = interactive && run_interactive_rebase "$@" + # Now we are rebasing commits $upstream..$branch (or with --root, # everything leading up to $branch) on top of $onto -- 1.7.3.2.864.gbbb96 --
Rename variables HEAD and OLDHEAD to orig_head and HEADNAME to head_name, which are the names used in git-rebase.sh. This prepares for factoring out of the code that persists these variables during the entire rebase process. Using the same variable names to mean the same thing in both files also makes the code easier to read. While at it, also remove the DOTEST variable and use the state_dir variable that was inherited from git-rebase.sh instead. Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> --- Squash this commit with earlier commit that also renamed variables in git-rebase--interactive.sh? git-rebase--interactive.sh | 120 +++++++++++++++++++++---------------------- 1 files changed, 59 insertions(+), 61 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index e848ea2..eace4e5 100755 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -12,22 +12,20 @@ . git-sh-setup -DOTEST="$GIT_DIR/rebase-merge" - # The file containing rebase commands, comments, and empty lines. # This file is created by "git rebase -i" then edited by the user. As # the lines are processed, they are removed from the front of this # file and written to the tail of $DONE. -TODO="$DOTEST"/git-rebase-todo +TODO="$state_dir"/git-rebase-todo # The rebase command lines that have already been processed. A line # is moved here when it is first handled, before any associated user # actions. -DONE="$DOTEST"/done +DONE="$state_dir"/done # The commit message that is planned to be used for any changes that # need to be committed following a user interaction. -MSG="$DOTEST"/message +MSG="$state_dir"/message # The file into which is accumulated the suggested commit message for # squash/fixup commands. When the first of a series of squash/fixups @@ -42,14 +40,14 @@ MSG="$DOTEST"/message # written to the file so far (including the initial "pick" commit). # Each time that a commit message is processed, ...
Factor out the common parts of the handling of the sub commands
'--continue', '--skip' and '--abort'. The '--abort' handling can
handled completely in git-rebase.sh.
After this refactoring, the calls to git-rebase--am.sh,
git-rebase--merge.sh and git-rebase--interactive.sh will be better
aligned. There will only be one call to interactive rebase that will
shortcut the very last part of git-rebase.sh.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-rebase--interactive.sh | 34 +++-------------------------------
git-rebase.sh | 17 +++++++++++++++--
2 files changed, 18 insertions(+), 33 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index acd0258..1079994 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -509,9 +509,7 @@ do_next () {
test -s "$TODO" && return
comment_for_reflog finish &&
- head_name=$(cat "$state_dir"/head-name) &&
- orig_head=$(cat "$state_dir"/head) &&
- SHORTONTO=$(git rev-parse --short $(cat "$state_dir"/onto)) &&
+ SHORTONTO=$(git rev-parse --short $onto) &&
NEWHEAD=$(git rev-parse HEAD) &&
case $head_name in
refs/*)
@@ -521,7 +519,7 @@ do_next () {
;;
esac && {
test ! -f "$state_dir"/verbose ||
- git diff-tree --stat $(cat "$state_dir"/head)..HEAD
+ git diff-tree --stat $orig_head..HEAD
} &&
{
test -s "$REWRITTEN_LIST" &&
@@ -655,14 +653,6 @@ rearrange_squash () {
case "$action" in
continue)
get_saved_options
- comment_for_reflog continue
-
- # Sanity check
- git rev-parse --verify HEAD >/dev/null ||
- die "Cannot read HEAD"
- git update-index --ignore-submodules --refresh &&
- git diff-files --quiet --ignore-submodules ||
- die "Working tree is dirty"
# do we have anything to commit?
if git diff-index --cached --quiet --ignore-submodules HEAD --
@@ -693,30 +683,12 @@ first and then run 'git rebase --continue' again."
require_clean_work_tree "rebase"
do_rest
...Since 8e4a91b (rebase -i: remember the settings of -v, -s and -p when
interrupted, 2007-07-08), the variable preserve_merges (then called
PRESERVE_MERGES) was detected from the state saved in
$GIT_DIR/rebase-merge in order to be used when the rebase resumed, but
its value was never actually used. The variable's value was only used
when the rebase was initated.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-rebase--interactive.sh | 5 -----
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0763ef5..d20a9b2 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -583,7 +583,6 @@ skip_unnecessary_picks () {
}
get_saved_options () {
- test -d "$REWRITTEN" && preserve_merges=t
test -f "$state_dir"/rebase-root && rebase_root=t
}
@@ -650,8 +649,6 @@ rearrange_squash () {
case "$action" in
continue)
- get_saved_options
-
# do we have anything to commit?
if git diff-index --cached --quiet --ignore-submodules HEAD --
then
@@ -682,8 +679,6 @@ first and then run 'git rebase --continue' again."
do_rest
;;
skip)
- get_saved_options
-
git rerere clear
do_rest
--
1.7.3.2.864.gbbb96
--
When rebase stops due to conflict, interactive rebase currently
displays a different hint to the user than non-interactive rebase
does. Use the same message for both types of rebase.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
Should we include the "mark the corrected paths with 'git add
<paths>'" part?
git-rebase--interactive.sh | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0beeb8b..e848ea2 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -81,9 +81,7 @@ AMEND="$DOTEST"/amend
REWRITTEN_LIST="$DOTEST"/rewritten-list
REWRITTEN_PENDING="$DOTEST"/rewritten-pending
-GIT_CHERRY_PICK_HELP="\
-hint: after resolving the conflicts, mark the corrected paths
-hint: with 'git add <paths>' and run 'git rebase --continue'"
+GIT_CHERRY_PICK_HELP="$RESOLVEMSG"
export GIT_CHERRY_PICK_HELP
warn () {
--
1.7.3.2.864.gbbb96
--
Before calling 'git cherry-pick', interactive rebase currently checks
if we are rebasing from root (if --root was passed). If we are, the
'--ff' flag to 'git cherry-pick' is omitted. However, according to the
documentation for 'git cherry-pick --ff', "If the current HEAD is the
same as the parent of the cherry-picked commit, then a fast forward to
this commit will be performed.". This should never be the case when
rebasing from root, so it should not matter whether --ff is passed, so
simplify the code by removing the condition.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
While factoring out the state writing code a few patches back, I went
through each of the pieces of state that was written. I was a bit
hesitant to include this patch since I'm not quite sure why the code
was introduced, but I thought I would include it anyway to hear what
you have to say.
There used to be bug when using --ff when rebasing a root commit. This
was fixed in 6355e50 (builtin/revert.c: don't dereference a NULL
pointer, 2010-09-27). Could this have been the reason for the check?
Thomas, do you remember?
git-rebase--interactive.sh | 15 ---------------
1 files changed, 0 insertions(+), 15 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d20a9b2..a9f44d8 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -168,11 +168,6 @@ pick_one () {
output git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
test -d "$REWRITTEN" &&
pick_one_preserving_merges "$@" && return
- if test -n "$rebase_root"
- then
- output git cherry-pick "$@"
- return
- fi
output git cherry-pick $ff "$@"
}
@@ -582,10 +577,6 @@ skip_unnecessary_picks () {
die "Could not skip unnecessary pick commands"
}
-get_saved_options () {
- test -f "$state_dir"/rebase-root && rebase_root=t
-}
-
# Rearrange the todo list that has both "pick sha1 msg" and
# "pick sha1 fixup!/squash! msg" appears in ...I think this just ended up being such a strange test because of the
following hunk in 8e75abf (rebase -i: use new --ff cherry-pick option,
2010-03-06):
@@ -232,16 +232,7 @@ pick_one () {
output git cherry-pick "$@"
return
fi
- parent_sha1=$(git rev-parse --verify $sha1^) ||
- die "Could not get the parent of $sha1"
- current_sha1=$(git rev-parse --verify HEAD)
- if test -z "$no_ff" && test "$current_sha1" = "$parent_sha1"
- then
- output git reset --hard $sha1
- output warn Fast-forward to $(git rev-parse --short $sha1)
- else
- output git cherry-pick "$@"
- fi
+ output git cherry-pick $ff "$@"
}
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
Yes, I saw that one as well, so it would probably have made more sense to ask Christian instead (the author of 8e75abf). So do you remember, Christian? Anyway, thanks for your input, Thomas. That makes me feel a little less worried about this patch. /Martin --
Hi, On Wed, Dec 29, 2010 at 11:31 PM, Martin von Zweigbergk Yeah, I must say that I did not try to understand why there was a special case for $rebase_root above the code I was changing. Yeah I think it's a good one. Thanks, Christian. --
Extract the code for am-based rebase to git-rebase--am.sh. Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> --- .gitignore | 1 + Makefile | 1 + git-rebase--am.sh | 34 ++++++++++++++++++++++++++++++++++ git-rebase.sh | 33 +++------------------------------ 4 files changed, 39 insertions(+), 30 deletions(-) create mode 100644 git-rebase--am.sh diff --git a/.gitignore b/.gitignore index 40506ff..ef04058 100644 --- a/.gitignore +++ b/.gitignore @@ -102,6 +102,7 @@ /git-quiltimport /git-read-tree /git-rebase +/git-rebase--am /git-rebase--interactive /git-rebase--merge /git-receive-pack diff --git a/Makefile b/Makefile index ffc3a5d..459df3a 100644 --- a/Makefile +++ b/Makefile @@ -369,6 +369,7 @@ SCRIPT_SH += git-merge-resolve.sh SCRIPT_SH += git-mergetool.sh SCRIPT_SH += git-pull.sh SCRIPT_SH += git-quiltimport.sh +SCRIPT_SH += git-rebase--am.sh SCRIPT_SH += git-rebase--interactive.sh SCRIPT_SH += git-rebase--merge.sh SCRIPT_SH += git-rebase.sh diff --git a/git-rebase--am.sh b/git-rebase--am.sh new file mode 100644 index 0000000..9316761 --- /dev/null +++ b/git-rebase--am.sh @@ -0,0 +1,34 @@ +#!/bin/sh +# +# Copyright (c) 2010 Junio C Hamano. +# + +. git-sh-setup + +case "$action" in +continue) + git am --resolved --3way --resolvemsg="$RESOLVEMSG" && + move_to_original_branch + exit + ;; +skip) + git am --skip -3 --resolvemsg="$RESOLVEMSG" && + move_to_original_branch + exit + ;; +esac + +test -n "$rebase_root" && root_flag=--root + +git format-patch -k --stdout --full-index --ignore-if-in-upstream \ + --src-prefix=a/ --dst-prefix=b/ \ + --no-renames $root_flag "$revisions" | +git am $git_am_opt --rebasing --resolvemsg="$RESOLVEMSG" && +move_to_original_branch +ret=$? +test 0 != $ret -a -d "$state_dir" && + echo $head_name > "$state_dir/head-name" && + echo $onto > "$state_dir/onto" && + echo $orig_head > "$state_dir/orig-head" && + echo "$GIT_QUIET" > ...
When a rebase is resumed, interactive rebase remembers any merge
strategy passed when the rebase was initated. Make non-interactive
rebase remember any merge strategy as well. Also make non-interactive
rebase remember any merge strategy options.
To be able to resume a rebase that was initiated with an older version
of git (older than this commit), make sure not to expect the saved
option files to exist.
Test case idea taken from Junio's 71fc224 (t3402: test "rebase
-s<strategy> -X<opt>", 2010-11-11).
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
How to add support for strategy options to interactive rebase?
git-rebase--interactive.sh | 2 --
git-rebase.sh | 6 ++++++
t/t3418-rebase-continue.sh | 29 +++++++++++++++++++++++++++++
3 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 7809932..0763ef5 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -584,7 +584,6 @@ skip_unnecessary_picks () {
get_saved_options () {
test -d "$REWRITTEN" && preserve_merges=t
- test -f "$state_dir"/strategy && strategy="$(cat "$state_dir"/strategy)"
test -f "$state_dir"/rebase-root && rebase_root=t
}
@@ -713,7 +712,6 @@ case "$rebase_root" in
*)
: >"$state_dir"/rebase-root ;;
esac
-test -z "$strategy" || echo "$strategy" > "$state_dir"/strategy
if test t = "$preserve_merges"
then
if test -z "$rebase_root"
diff --git a/git-rebase.sh b/git-rebase.sh
index e5be7e5..d192038 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -78,6 +78,9 @@ read_basic_state () {
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)"
}
write_basic_state () {
@@ -91,6 +94,9 @@ write_basic_state () {
fi &&
echo ...AFAICS rebase -i currently only uses the strategy when dealing with
multiple parents, i.e., in --preserve-merges mode. I think
git-cherry-pick needs to learn about the -s and -X options, and then
it'll be a simple matter of passing them along.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
The code reading the state saved in $merge_dir or $rebase_dir is
currently spread out in many places, making it harder to read and to
introduce additional state. Extract this code into one method that reads
the state.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-rebase.sh | 53 +++++++++++++++++++++++------------------------------
1 files changed, 23 insertions(+), 30 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index 79f8008..bf144dc 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -57,6 +57,22 @@ rebase_root=
force_rebase=
allow_rerere_autoupdate=
+read_state () {
+ if test -d "$merge_dir"
+ then
+ state_dir="$merge_dir"
+ prev_head=$(cat "$merge_dir"/prev_head) &&
+ end=$(cat "$merge_dir"/end) &&
+ msgnum=$(cat "$merge_dir"/msgnum)
+ else
+ state_dir="$apply_dir"
+ fi &&
+ head_name=$(cat "$state_dir"/head-name) &&
+ onto=$(cat "$state_dir"/onto) &&
+ orig_head=$(cat "$state_dir"/orig-head) &&
+ GIT_QUIET=$(cat "$state_dir"/quiet)
+}
+
continue_merge () {
test -n "$prev_head" || die "prev_head must be defined"
test -d "$merge_dir" || die "$merge_dir directory does not exist"
@@ -138,10 +154,6 @@ call_merge () {
}
move_to_original_branch () {
- test -z "$head_name" &&
- head_name="$(cat "$merge_dir"/head-name)" &&
- onto="$(cat "$merge_dir"/onto)" &&
- orig_head="$(cat "$merge_dir"/orig-head)"
case "$head_name" in
refs/*)
message="rebase finished: $head_name onto $onto"
@@ -220,13 +232,9 @@ do
echo "mark them as resolved using git add"
exit 1
}
+ read_state
if test -d "$merge_dir"
then
- prev_head=$(cat "$merge_dir/prev_head")
- end=$(cat "$merge_dir/end")
- msgnum=$(cat "$merge_dir/msgnum")
- onto=$(cat "$merge_dir/onto")
- GIT_QUIET=$(cat "$merge_dir/quiet")
continue_merge
while test "$msgnum" -le "$end"
do
@@ -236,10 +244,6 @@ do
finish_rb_merge
exit
fi
- head_name=$(cat "$apply_dir"/head-name) ...It used to be safe to call this without head_name and friends set, because the function took care of reading these itself. Nobody calls this without head_name set anymore? I am not complaining nor suggesting to add an unnecessary "read_state" here only to slow things down---I am making sure that you removed this Even though this patch may make the code shorter, it starts to read head_name and orig_head that we previously did not open and change the values of variables with what are read from them. Does this change affect Earlier move-to-original-branch was Ok to be called without head_name, and the old code here read from the file anyway, so it didn't matter, but now it seems that the first check and assignment you removed from the function may matter because this caller does not even read from head_name. Are you My heartbeat skipped when I first saw this. Thanks to the previous commit, it was exposed that somebody reused $dotest that was only to be used when using merge machinery because the things left to be done in this codepath are common between the merge and apply, which is kind of sloppy, but that sloppiness is now gone ;-). Are there places that read from individual files for states left after this patch, or read_state is the only interface to get to the states? If the latter that would be a great news, and also would suggest that we may want to have a corresponding write_state function (and we may even want to make the state into a single file to reduce I/O---but that is a separate issue that can be done at the very end of the series if it turns out to be beneficial). Of course it is fine if introduction of read_state is an attempt to catch most common cases, but it would reduce chances of mistake if the coverage were 100% (as opposed to 99.9%) hence this question. --
Correct. It used to be called without head_name set from finish_rb_merge, which would in turn be called from the --continue and --skip arms. onto would already have been set in these cases, but True. Previously, head_name and orig_head were lazily read only when the rebase had been finished (in the finish_rb_merge, as I mentioned above). Now these values are read unnecessarily when the rebase is resumed, but a later patch fails. The am-based rebase is not affacted by this patch as it already read head_name and orig_head eagerly. Good point about the correctness. I looked into the correctness issue arising if a file could not be found and I think I concluded that all of the files would always be written (do we need to care about the case where a user deletes e.g. .git/rebase-apply/onto?). However, I did not think about the possibility of overwriting variables. It seems fine, though, as neither continue_merge nor call_merge uses either of these variables. Regarding performance, I would say there is definitely a cost associated with this patch. How big this cost is, though, I don't dare to speculate. I will leave that up to the rest of you. It should be noted that read_state is only called when a rebase is resumed with --continue or --skip, or when it is aborted. There are no changes to the code in the call_merge-continue_merge loop. The performance hit is probably biggest in the --abort case, which previously only read head_name and orig_head. It now ends up reading If I understand your question correctly, then yes, it is ok, because of the previous hunk that calls read_state. That call is made before/outside the if block for merge-based rebase, so the variables There are still a few other places where state is read, mainly in call_merge. It reads things that vary from iteration to iteration, such as a counter. I forgot to say in the commit message, but I tried to extract only the code that reads the initial state. The write_state function actually is there, but ...
Remove directory checks from git-rebase--interactive.sh that are done in git-rebase.sh. Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> --- Squash with previous? git-rebase--interactive.sh | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 4d3dc63..21a9774 100755 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -706,8 +706,6 @@ continue) get_saved_options comment_for_reflog continue - test -d "$DOTEST" || die "No interactive rebase running" - # Sanity check git rev-parse --verify HEAD >/dev/null || die "Cannot read HEAD" @@ -749,7 +747,6 @@ abort) comment_for_reflog abort git rerere clear - test -d "$DOTEST" || die "No interactive rebase running" HEADNAME=$(cat "$DOTEST"/head-name) HEAD=$(cat "$DOTEST"/head) @@ -767,7 +764,6 @@ skip) comment_for_reflog skip git rerere clear - test -d "$DOTEST" || die "No interactive rebase running" output git reset --hard && do_rest ;; @@ -780,8 +776,6 @@ fi test -z "$rebase_root" -a $# -ge 1 -a $# -le 2 || test ! -z "$rebase_root" -a $# -le 1 || usage -test -d "$DOTEST" && - die "Interactive rebase already started" git var GIT_COMMITTER_IDENT >/dev/null || die "You need to set your committer info first" -- 1.7.3.2.864.gbbb96 --
When the merge strategy fails, a message suggesting the user to try
another strategy is displayed. Remove the "$rv" (which is always equal
to "2" in this case) from that message.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-rebase--merge.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index 705d2f5..4ea3ca7 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -78,7 +78,7 @@ call_merge () {
die "$RESOLVEMSG"
;;
2)
- echo "Strategy: $rv $strategy failed, try another" 1>&2
+ echo "Strategy: $strategy failed, try another" 1>&2
die "$RESOLVEMSG"
;;
*)
--
1.7.3.2.864.gbbb96
--
If '--[no-]allow_rerere_autoupdate' is passed when 'git rebase -m' is
called and a merge conflict occurs, the flag will be forgotten for the
rest of the rebase process. Make rebase remember it by saving the
value.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
allow_rerere_autoupdate is only used by git-rebase--merge. Still ok to
write and read it here?
Should allow_rerere_autoupdate also be added to git_am_opt?
git-rebase.sh | 4 ++++
t/t3418-rebase-continue.sh | 21 +++++++++++++++++++++
2 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index d192038..05b4fe1 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -81,6 +81,8 @@ read_basic_state () {
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)"
}
write_basic_state () {
@@ -97,6 +99,8 @@ write_basic_state () {
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
}
output () {
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 5469546..1e855cd 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -74,4 +74,25 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
test -f funny.was.run
'
+test_expect_success 'rebase --continue remembers --rerere-autoupdate' '
+ rm -fr .git/rebase-* &&
+ git reset --hard commit-new-file-F3-on-topic-branch &&
+ git checkout master
+ test_commit "commit-new-file-F3" F3 3 &&
+ git config rerere.enabled true &&
+ test_must_fail git rebase -m master topic ...The sub commands '--continue', '--skip' or '--abort' may only be used
standalone according to the documentation. Other options following the
sub command are currently not accepted, but options preceeding them
are. For example, 'git rebase --continue -v' is not accepted, while
'git rebase -v --continue' is. Tighten up the check and allow no other
options when one of these sub commands are used.
Only check that it is standalone for non-interactive rebase for
now. Once the command line processing for interactive rebase has been
replaced by the command line processing in git-rebase.sh, this check
will also apply to interactive rebase.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
Is this too simplistic? Do we forsee that we want to support passing
options when resuming a rebase? Is it better to check for each other
option that it is not passed (i.e. no '-v', no '-s' etc.)?
Might some users be depending on the current behavior, even though it
is undocumented?
git-rebase.sh | 4 ++--
t/t3403-rebase-skip.sh | 5 +++++
t/t3407-rebase-abort.sh | 10 ++++++++++
t/t3418-rebase-continue.sh | 5 +++++
4 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index 29f1214..1cb0564 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -229,6 +229,7 @@ then
fi
test -n "$type" && in_progress=t
+total_argc=$#
while test $# != 0
do
case "$1" in
@@ -239,9 +240,8 @@ do
OK_TO_SKIP_PRE_REBASE=
;;
--continue|--skip|--abort)
+ test $total_argc -eq 1 || usage
action=${1##--}
- shift
- break
;;
--onto)
test 2 -le "$#" || usage
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index 64446e3..826500b 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -35,6 +35,11 @@ test_expect_success 'rebase with git am -3 (default)' '
test_must_fail git rebase master
'
+test_expect_success 'rebase --skip can not be used with other ...Currently, only interactive rebase remembers the value of the '-v'
flag from the initial invocation. Make non-interactive rebase also
remember it.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-rebase--interactive.sh | 2 --
git-rebase.sh | 6 ++++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a33b246..7809932 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -585,7 +585,6 @@ skip_unnecessary_picks () {
get_saved_options () {
test -d "$REWRITTEN" && preserve_merges=t
test -f "$state_dir"/strategy && strategy="$(cat "$state_dir"/strategy)"
- test -f "$state_dir"/verbose && verbose=t
test -f "$state_dir"/rebase-root && rebase_root=t
}
@@ -715,7 +714,6 @@ case "$rebase_root" in
: >"$state_dir"/rebase-root ;;
esac
test -z "$strategy" || echo "$strategy" > "$state_dir"/strategy
-test t = "$verbose" && : > "$state_dir"/verbose
if test t = "$preserve_merges"
then
if test -z "$rebase_root"
diff --git a/git-rebase.sh b/git-rebase.sh
index 95c0d05..e5be7e5 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -76,7 +76,8 @@ read_basic_state () {
else
orig_head=$(cat "$state_dir"/orig-head)
fi &&
- GIT_QUIET=$(cat "$state_dir"/quiet)
+ GIT_QUIET=$(cat "$state_dir"/quiet) &&
+ test -f "$state_dir"/verbose && verbose=t
}
write_basic_state () {
@@ -88,7 +89,8 @@ write_basic_state () {
else
echo "$orig_head" > "$state_dir"/orig-head
fi &&
- echo "$GIT_QUIET" > "$state_dir"/quiet
+ echo "$GIT_QUIET" > "$state_dir"/quiet &&
+ test t = "$verbose" && : > "$state_dir"/verbose
}
output () {
--
1.7.3.2.864.gbbb96
--
The 'onto_name' state used in 'git rebase --merge' is currently read
once for each commit that need to be applied. It doesn't change
between each iteration, however, so it should be moved out of the
loop. This also makes the code more readable. Also remove the unused
variable 'end'.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-rebase.sh | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index bf144dc..9be831e 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -62,6 +62,7 @@ read_state () {
then
state_dir="$merge_dir"
prev_head=$(cat "$merge_dir"/prev_head) &&
+ onto_name=$(cat "$merge_dir"/onto_name) &&
end=$(cat "$merge_dir"/end) &&
msgnum=$(cat "$merge_dir"/msgnum)
else
@@ -123,9 +124,8 @@ call_merge () {
hd=$(git rev-parse --verify HEAD)
cmt_name=$(git symbolic-ref HEAD 2> /dev/null || echo HEAD)
msgnum=$(cat "$merge_dir/msgnum")
- end=$(cat "$merge_dir/end")
eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end - $msgnum))"'
- eval GITHEAD_$hd='$(cat "$merge_dir/onto_name")'
+ eval GITHEAD_$hd='$onto_name'
export GITHEAD_$cmt GITHEAD_$hd
if test -n "$GIT_QUIET"
then
--
1.7.3.2.864.gbbb96
--
Extrace the code for writing the state to rebase-apply/ or rebase-merge/ when a rebase is initiated. This will make it easier to later make both interactive and non-interactive rebase remember the options used. Note that non-interactive rebase stores the sha1 of the original head in a file called orig-head, while interactive rebase stores it in a file called head. Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> --- git-rebase--am.sh | 6 +----- git-rebase--interactive.sh | 5 +---- git-rebase--merge.sh | 5 +---- git-rebase.sh | 16 ++++++++++++++-- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/git-rebase--am.sh b/git-rebase--am.sh index 9316761..5acfa00 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -26,9 +26,5 @@ git format-patch -k --stdout --full-index --ignore-if-in-upstream \ git am $git_am_opt --rebasing --resolvemsg="$RESOLVEMSG" && move_to_original_branch ret=$? -test 0 != $ret -a -d "$state_dir" && - 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 0 != $ret -a -d "$state_dir" && write_basic_state exit $ret diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 1079994..a33b246 100755 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -707,16 +707,13 @@ orig_head=$(git rev-parse --verify HEAD) || die "No HEAD?" mkdir "$state_dir" || die "Could not create temporary $state_dir" : > "$state_dir"/interactive || die "Could not mark as interactive" -echo "$head_name" > "$state_dir"/head-name - -echo $orig_head > "$state_dir"/head +write_basic_state case "$rebase_root" in '') rm -f "$state_dir"/rebase-root ;; *) : >"$state_dir"/rebase-root ;; esac -echo $onto > "$state_dir"/onto test -z "$strategy" || echo "$strategy" > "$state_dir"/strategy test t = "$verbose" && : > ...
Do we have to cater to the use-case where the user starts a rebase,
downgrades at a conflict, and then continues?
If not, you could read 'orig-head' first and fall back to 'head', only
writing 'orig-head' in the state saving independent of the mode. That
would give us the chance of removing the redundancy at some point.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
Remove the parsing and validation of references (onto, upstream, branch)
from git-rebase--interactive.sh and rely on the information exported from
git-rebase.sh.
By using the parsing of the --onto parameter in git-rebase.sh, this
improves the error message when the parameter is invalid.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
Should the check for set GIT_COMMITTER_IDENT be done for
non-interactive rebase as well or does it only make sense for
interactive rebase?
git-rebase--interactive.sh | 48 +++----------------------------------------
git-rebase.sh | 12 ++++++----
2 files changed, 11 insertions(+), 49 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 5a8f582..c055fc4 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -682,25 +682,6 @@ rearrange_squash () {
rm -f "$1.sq" "$1.rearranged"
}
-LF='
-'
-parse_onto () {
- case "$1" in
- *...*)
- if left=${1%...*} right=${1#*...} &&
- onto=$(git merge-base --all ${left:-HEAD} ${right:-HEAD})
- then
- case "$onto" in
- ?*"$LF"?* | '')
- exit 1 ;;
- esac
- echo "$onto"
- exit 0
- fi
- esac
- git rev-parse --verify "$1^0"
-}
-
case "$action" in
continue)
get_saved_options
@@ -769,47 +750,26 @@ skip)
;;
esac
-if test -n "$onto"
-then
- onto=$(parse_onto "$onto") || die "Does not point to a valid commit: $1"
-fi
-
-test -z "$rebase_root" -a $# -ge 1 -a $# -le 2 ||
-test ! -z "$rebase_root" -a $# -le 1 || usage
-
git var GIT_COMMITTER_IDENT >/dev/null ||
die "You need to set your committer info first"
-if test -z "$rebase_root"
-then
- upstream_arg="$1"
- upstream=$(git rev-parse --verify "$1") || die "Invalid base"
- test -z "$onto" && onto=$upstream
- shift
-else
- upstream=
- upstream_arg=--root
- test -z "$onto" &&
- die "You must specify --onto when using --root"
-fi
require_clean_work_tree "rebase" "Please commit or stash them."
...Since 22db240 (git-am: propagate --3way options as well, 2008-12-04), the --3way has been propageted across failure, so it is since pointless to pass it to git-am when resuming. Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> --- git-rebase--am.sh | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/git-rebase--am.sh b/git-rebase--am.sh index 5acfa00..95608c6 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -7,12 +7,12 @@ case "$action" in continue) - git am --resolved --3way --resolvemsg="$RESOLVEMSG" && + git am --resolved --resolvemsg="$RESOLVEMSG" && move_to_original_branch exit ;; skip) - git am --skip -3 --resolvemsg="$RESOLVEMSG" && + git am --skip --resolvemsg="$RESOLVEMSG" && move_to_original_branch exit ;; -- 1.7.3.2.864.gbbb96 --
Instead of using the old variable name 'dotest' for
"$GIT_DIR"/rebase-merge and no variable for "$GIT_DIR"/rebase-apply,
introduce two variables 'merge_dir' and 'apply_dir' for these paths.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-rebase.sh | 141 +++++++++++++++++++++++++++++----------------------------
1 files changed, 71 insertions(+), 70 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index d8e1903..79f8008 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -46,7 +46,8 @@ unset newbase
strategy=recursive
strategy_opts=
do_merge=
-dotest="$GIT_DIR"/rebase-merge
+merge_dir="$GIT_DIR"/rebase-merge
+apply_dir="$GIT_DIR"/rebase-apply
prec=4
verbose=
diffstat=
@@ -58,7 +59,7 @@ allow_rerere_autoupdate=
continue_merge () {
test -n "$prev_head" || die "prev_head must be defined"
- test -d "$dotest" || die "$dotest directory does not exist"
+ test -d "$merge_dir" || die "$merge_dir directory does not exist"
unmerged=$(git ls-files -u)
if test -n "$unmerged"
@@ -68,7 +69,7 @@ continue_merge () {
die "$RESOLVEMSG"
fi
- cmt=`cat "$dotest/current"`
+ cmt=`cat "$merge_dir/current"`
if ! git diff-index --quiet --ignore-submodules HEAD --
then
if ! git commit --no-verify -C "$cmt"
@@ -81,7 +82,7 @@ continue_merge () {
then
printf "Committed: %0${prec}d " $msgnum
fi
- echo "$cmt $(git rev-parse HEAD^0)" >> "$dotest/rewritten"
+ echo "$cmt $(git rev-parse HEAD^0)" >> "$merge_dir/rewritten"
else
if test -z "$GIT_QUIET"
then
@@ -93,22 +94,22 @@ continue_merge () {
prev_head=`git rev-parse HEAD^0`
# save the resulting commit so we can read-tree on it later
- echo "$prev_head" > "$dotest/prev_head"
+ echo "$prev_head" > "$merge_dir/prev_head"
# onto the next patch:
msgnum=$(($msgnum + 1))
- echo "$msgnum" >"$dotest/msgnum"
+ echo "$msgnum" >"$merge_dir/msgnum"
}
call_merge () {
- cmt="$(cat "$dotest/cmt.$1")"
- echo "$cmt" > ...If this were just "s/dotest/merge_dir/g" and the same for rebase-apply, I would have to say it is long overdue ;-) I read the patch and didn't spot any glaring mistake, but I wasn't being as careful as I usually am. Thanks. --
Yes, that should be all! Well, to be precise, in the "@@ -560,35 +561,35 @@ then" hunk, I also moved the quotes to contain the file name as was done for rebase-merge. I redid the search/replace just to double check. --
Thanks a lot for undertaking this effort! I just finished looking
through the entire series, and it all seems sane to me. Apart from
j6t's reply I think we're mostly nit-picking or agreeing with you, and
this is the first iteration!
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
