Hi Fabian,
Thanks for looking into this.
On 05/27/2014 07:56 AM, Michael Haggerty wrote:
>> +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.
I agree. I was thinking that pick_one should get this new behavior
instead of do_pick, but some callers may not appreciate the new behavior
(i.e. squash/fixup), though I expect those callers have similar problems
as this commit is trying to fix.
I think adding a pick_one_with_reschedule function (to be called in both
places from do_pick) could solve the issue Michael mentioned without
breaking other pick_one callers.
I have not tested doing so, but I think it would look like this:
===================
diff --git i/git-rebase--interactive.sh w/git-rebase--interactive.sh
index fe813ba..ae5603a 100644
--- i/git-rebase--interactive.sh
+++ w/git-rebase--interactive.sh
@@ -235,6 +235,17 @@ git_sequence_editor () {
eval "$GIT_SEQUENCE_EDITOR" '"$@"'
}
+pick_one_with_reschedule () {
+ pick_one $1
+ ret=$?
+ case "$ret" in
+ 0) ;;
+ 1) ;;
+ *) reschedule_last_action ;;
+ esac
+ return $ret
+}
+
pick_one () {
ff=--ff
@@ -474,13 +485,13 @@ do_pick () {
# rebase --continue.
git commit --allow-empty --allow-empty-message --amend \
--no-post-rewrite -n -q -C $1 &&
- pick_one -n $1 &&
+ pick_one_with_reschedule -n $1 &&
git commit --allow-empty --allow-empty-message \
--amend --no-post-rewrite -n -q -C $1 \
${gpg_sign_opt:+"$gpg_sign_opt"} ||
die_with_patch $1 "Could not apply $1... $2"
else
- pick_one $1 ||
+ pick_one_with_reschedule $1 ||
die_with_patch $1 "Could not apply $1... $2"
fi
}
===================
I don't much like the name 'pick_one_with_reschedule', but I didn't like
my other choices, either.
I'll try to look at this and your patches more closely when I have a bit
more time.
Phil
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html