Re: [RFC 2/3] rebase -i: Reschedule tasks that failed before the index was touched

[Michael Haggerty <mhagger@alum.mit.edu>]

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 <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.

> +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
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/