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@xxxxxxxxx>
> Signed-off-by: Fabian Ruch <bafain@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
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