Re: [RFC 2/3] rebase -i: Reschedule tasks that failed before the index was touched
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- May 27, 2014, 11:56 UTC
- Message-ID
- <53847D7D.5050000@alum.mit.edu>
- In-Reply-To
- <5383BDEA.9070908@gmail.com>
Hi,
Overall, this approach seems reasonable.
Please see the inline comments below.
On 05/27/2014 12:19 AM, Fabian Ruch wrote:
Show 31 quoted lines
> 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 <hordp@cisco.com>
> Signed-off-by: Fabian Ruch <bafain@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.Show 17 quoted lines
> +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.
Show 27 quoted lines
> 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 mhagger@alum.mit.edu http://softwareswirl.blogspot.com/