Hi,
Overall, this approach seems reasonable.
Please see the inline comments below.
On 05/27/2014 12:19 AM, Fabian Ruch wrote:
When `rebase--interactive` processes a task, it removes the item from
the todo list and appends it to another list of executed tasks. If a
`pick` (this includes `squash` and `fixup`) fails before the index has
recorded the changes, take the corresponding item and put it on the todo
list again. Otherwise, the changes introduced by the scheduled commit
would be lost.
That kind of decision is possible since the `cherry-pick` command
signals why it failed to apply the changes of the given commit. Either
the changes are recorded in the index using a conflict (return value 1)
and `rebase` does not continue until they are resolved or the changes
are not recorded in the index (return value neither 0 nor 1) and
`rebase` has to try again with the same task.
Reported-by: Phil Hord ho...@cisco.com
Signed-off-by: Fabian Ruch baf...@gmail.com
---
git-rebase--interactive.sh | 27 ++-
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9e1dd1e..bba4f3a 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -132,6 +132,16 @@ mark_action_done () {
fi
}
+# Put the last action marked done at the beginning of the todo list
+# again. If there has not been an action marked done yet, the list of
+# items on the todo list is left unchanged.
The comment would read better if the second sentence were also in active
voice, like the first sentence:
If there has not been an action marked done yet, leave the list of
items on the todo list unchanged.
+reschedule_last_action () {
+ tail -n 1 $done | cat - $todo $todo.new
+ sed -e \$d $done $done.new
+ mv -f $todo.new $todo
+ mv -f $done.new $done
+}
+
append_todo_help () {
git stripspace --comment-lines $todo \EOF
@@ -470,11 +480,15 @@ do_pick () {
--no-post-rewrite -n -q -C $1
pick_one -n $1
git commit --allow-empty --allow-empty-message \
---amend --no-post-rewrite -n -q -C $1 ||
- die_with_patch $1 Could not apply $1... $2
+--amend --no-post-rewrite -n -q -C $1
git cherry-pick indicates its error status specifically as 1 or some
other value. But here it could be that pick_one succeeds but git
commit fails; in that case ret is set to the return code of git
commit. So, if git commit fails with a retcode different than 1,
then reschedule_last_action will be called a few lines later. This
seems incorrect to me.
else
- pick_one $1 ||
- die_with_patch $1 Could not apply $1... $2
+ pick_one $1
+ fi
+ ret=$?
+ if test $ret -ne 0
+ then
+ test $ret -ne 1 reschedule_last_action
+ die_with_patch $1 Could not apply $1... $2
fi
}
@@ -533,8 +547,11 @@ do_next () {
author_script_content=$(get_author_ident_from_commit HEAD)
echo $author_script_content $author_script
eval $author_script_content
- if ! pick_one -n $sha1
+ pick_one -n $sha1
+ ret=$?
+ if test $ret -ne 0
then
+ test $ret -ne 1 reschedule_last_action
git rev-parse --verify HEAD $amend
die_failed_squash $sha1 $rest
fi
I suggest that you add a comment for pick_one explaining that if it
fails, its failure code is like that of cherry-pick, namely ...etc...
This will warn future developers to preserve the error code semantics.
It is preferable to squash the next commit, containing the tests,
together with this commit.
Thanks,
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
