Re: [RFC 2/3] rebase -i: Reschedule tasks that failed before the index was touched
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/3] rebase -i: Reschedule tasks that failed before the index was touched
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 > Signed-off-by: Fabian Ruch > --- > 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
[RFC 2/3] rebase -i: Reschedule tasks that failed before the index was touched
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 Signed-off-by: Fabian Ruch --- 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. +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 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 -- 1.9.3 -- 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