git-rebase-todo gets popped prematurely

git-rebase-todo gets popped prematurely

[Phil Hord]

During a 'git rebase --continue', I got an error about having left a
file in place which the next commit intended to add as new.  Stupid me.

So I rm'ed the file and tried again.  This time, git rebase --continue
succeeded.  But it accidentally left out the next commit in my rebase-todo.

I looked in the code and it seems that when the "pick" returns an error,
rebase--interactive stops and lets the user clean up.  But it assumes
the index  already tracks a conflicted merge, and so it removes the
commit from the todo list.  In this case, however, the conflicted merge
was avoided by detecting it in advance.  The result is that the "would
be overwritten" conflict evicts the entire commit from the rebase action.

I think the code needs to detect the difference between "merge failed;
conflicted index" and "merge failed; no change".  I think I can do this
with 'git-status -s -uno', maybe.  But I haven't tried it yet and it
feels like I'm missing a case or two also.

I tried to bisect this to some specific change, but it fails the same
way as far back as 1.6.5. 

Test script follows in case anyone has a better idea how to approach
this and wants to understand it better.

    #!/bin/sh

    set -x
    git --version
    rm -rf baz
    git init baz && cd baz
    echo initial>initial && git add initial && git commit -minitial
    echo foo>foo && git add foo && git commit -mfoo
    echo bar>bar && git add bar && git commit -mbar
    git log --oneline

    GIT_EDITOR='sed -i -e s/^pick/edit/ -e /^#/d' git rebase -i HEAD^^
    touch bar
    git rebase --continue
    rm bar
    git rebase --continue
    git log --oneline


And the tail of the output (note the missing "bar" commit even though
"Successfully rebased"):

    + git log --oneline
    fcc9b6e bar
    8121f15 foo
    a521fa1 initial
    + GIT_EDITOR='sed -i -e s/^pick/edit/ -e /^#/d'
    + git rebase -i 'HEAD^^'
    Stopped at 8121f1593ea5c66dc7e9af7719100c1fcf4ab721... foo
    You can amend the commit now, with

        git commit --amend

    Once you are satisfied with your changes, run

        git rebase --continue

    + touch bar
    + git rebase --continue
    error: The following untracked working tree files would be
    overwritten by merge:
        bar
    Please move or remove them before you can merge.
    Aborting
    Could not apply fcc9b6ef2941e870f88362edbe0f1078cebb20e5... bar
    + rm bar
    + git rebase --continue
    Successfully rebased and updated refs/heads/master.
    + git log --oneline
    8121f15 foo
    a521fa1 initial

[RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge

[Fabian Ruch]

`do_pick_commit` handles three situations if it is not fast-forwarding.
In order for `do_pick_commit` to identify the situation, it examines the
return value of the selected merge command.

1. return value 0 stands for a clean merge
2. 1 is passed in case of a failed merge due to conflict
3. any other return value means that the merge did not even start

So far, the sequencer returns 1 in case of a failed fast-forward, which
would mean "failed merge due to conflict". However, a fast-forward
either starts and succeeds or does not start at all. In particular, it
cannot fail in between like a merge with a dirty index due to conflicts.

In order to signal the three possible situations (not only success and
failure to complete) after a pick through porcelain commands such as
`cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was
chosen in line with the other situations in which the sequencer
encounters an error.

Signed-off-by: Fabian Ruch <bafain@gmail.com>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 90cac7b..97cecca 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -278,7 +278,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 
 	read_cache();
 	if (checkout_fast_forward(from, to, 1))
-		exit(1); /* the callee should have complained already */
+		exit(-1); /* the callee should have complained already */
 	ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
 					   0, NULL);
 	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
-- 
1.9.3

Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge

[Junio C Hamano]

Fabian Ruch <bafain@gmail.com> writes:

> `do_pick_commit` handles three situations if it is not fast-forwarding.
> In order for `do_pick_commit` to identify the situation, it examines the
> return value of the selected merge command.
>
> 1. return value 0 stands for a clean merge
> 2. 1 is passed in case of a failed merge due to conflict
> 3. any other return value means that the merge did not even start
>
> So far, the sequencer returns 1 in case of a failed fast-forward, which
> would mean "failed merge due to conflict". However, a fast-forward
> either starts and succeeds or does not start at all. In particular, it
> cannot fail in between like a merge with a dirty index due to conflicts.
>
> In order to signal the three possible situations (not only success and
> failure to complete) after a pick through porcelain commands such as
> `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was
> chosen in line with the other situations in which the sequencer
> encounters an error.

Hmph... do we still pass negative to exit() anywhere in our codebase?

>
> Signed-off-by: Fabian Ruch <bafain@gmail.com>
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 90cac7b..97cecca 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -278,7 +278,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
>  
>  	read_cache();
>  	if (checkout_fast_forward(from, to, 1))
> -		exit(1); /* the callee should have complained already */
> +		exit(-1); /* the callee should have complained already */
>  	ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
>  					   0, NULL);
>  	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));

Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge

[Fabian Ruch]

Hi Junio,

On 05/27/2014 08:42 PM, Junio C Hamano wrote:
> Fabian Ruch <bafain@gmail.com> writes:
>> [..]
>>
>> In order to signal the three possible situations (not only success and
>> failure to complete) after a pick through porcelain commands such as
>> `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was
>> chosen in line with the other situations in which the sequencer
>> encounters an error.
> 
> Hmph... do we still pass negative to exit() anywhere in our codebase?

No, but I thought `cmd_cherry_pick` would just forward the `return -1` from the
sequencer to the shell. I had another look and found that `cmd_cherry_pick`
calls `die` instead. Now, the commit inserts 128 as exit status in
`fast_forward_to`.

Would it be appropriate to mention the choice of exit status in the coding
guidelines? I didn't know that the int-argument to exit(3) gets truncated and
that this is why it is a general rule to only use values in the range from 0 to
255 with exit(3).

Kind regards,
   Fabian

-- >8 --
Subject: sequencer: Signal failed ff as an aborted, not a conflicted merge

`do_pick_commit` handles three situations if it is not fast-forwarding.
In order for `do_pick_commit` to identify the situation, it examines the
return value of the selected merge command.

1. return value 0 stands for a clean merge
2. 1 is passed in case of a failed merge due to conflict
3. any other return value means that the merge did not even start

So far, the sequencer returns 1 in case of a failed fast-forward, which
would mean "failed merge due to conflict". However, a fast-forward
either starts and succeeds or does not start at all. In particular, it
cannot fail in between like a merge with a dirty index due to conflicts.

In order to signal the three possible situations (not only success and
failure to complete) after a pick through porcelain commands such as
`cherry-pick`, exit with a return value that is neither 0 nor 1. 128 was
chosen in line with the other situations in which the sequencer
encounters an error. In such situations, the sequencer returns a
negative value and `cherry-pick` translates this into a call to `die`.
`die` then terminates the process with exit status 128.

Signed-off-by: Fabian Ruch <bafain@gmail.com>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 90cac7b..225afcb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -278,7 +278,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 
 	read_cache();
 	if (checkout_fast_forward(from, to, 1))
-		exit(1); /* the callee should have complained already */
+		exit(128); /* the callee should have complained already */
 	ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
 					   0, NULL);
 	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
-- 
2.0.0

Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge

[Junio C Hamano]

Fabian Ruch <bafain@gmail.com> writes:

> On 05/27/2014 08:42 PM, Junio C Hamano wrote:
>> Fabian Ruch <bafain@gmail.com> writes:
>>> [..]
>>>
>>> In order to signal the three possible situations (not only success and
>>> failure to complete) after a pick through porcelain commands such as
>>> `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was
>>> chosen in line with the other situations in which the sequencer
>>> encounters an error.
>> 
>> Hmph... do we still pass negative to exit() anywhere in our codebase?
>
> No, but I thought `cmd_cherry_pick` would just forward the `return -1` from the
> sequencer to the shell. I had another look and found that `cmd_cherry_pick`
> calls `die` instead. Now, the commit inserts 128 as exit status in
> `fast_forward_to`.
>
> Would it be appropriate to mention the choice of exit status in the coding
> guidelines? I didn't know that the int-argument to exit(3) gets truncated and
> that this is why it is a general rule to only use values in the range from 0 to
> 255 with exit(3).

I personally do not think of a reason why it is necessary to mention
how the argument to exit(3) is expected to be used by the system, but
if it is common not to know it, I guess it would not hurt to have a
single paragraph with at most two lines.

In any case, I agree that exiting with 1 that signals "failed with
conflict" can be confusing to the caller.  Can we have a test to
demonstrate when this fix matters?

Thanks.

> -- >8 --
> Subject: sequencer: Signal failed ff as an aborted, not a conflicted merge
>
> `do_pick_commit` handles three situations if it is not fast-forwarding.
> In order for `do_pick_commit` to identify the situation, it examines the
> return value of the selected merge command.
>
> 1. return value 0 stands for a clean merge
> 2. 1 is passed in case of a failed merge due to conflict
> 3. any other return value means that the merge did not even start
>
> So far, the sequencer returns 1 in case of a failed fast-forward, which
> would mean "failed merge due to conflict". However, a fast-forward
> either starts and succeeds or does not start at all. In particular, it
> cannot fail in between like a merge with a dirty index due to conflicts.
>
> In order to signal the three possible situations (not only success and
> failure to complete) after a pick through porcelain commands such as
> `cherry-pick`, exit with a return value that is neither 0 nor 1. 128 was
> chosen in line with the other situations in which the sequencer
> encounters an error. In such situations, the sequencer returns a
> negative value and `cherry-pick` translates this into a call to `die`.
> `die` then terminates the process with exit status 128.
>
> Signed-off-by: Fabian Ruch <bafain@gmail.com>
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 90cac7b..225afcb 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -278,7 +278,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
>  
>  	read_cache();
>  	if (checkout_fast_forward(from, to, 1))
> -		exit(1); /* the callee should have complained already */
> +		exit(128); /* the callee should have complained already */
>  	ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
>  					   0, NULL);
>  	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));

Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge

[Phil Hord]

On 06/10/2014 01:56 PM, Junio C Hamano wrote:
> Fabian Ruch <bafain@gmail.com> writes:
>
>> On 05/27/2014 08:42 PM, Junio C Hamano wrote:
>>> Fabian Ruch <bafain@gmail.com> writes:
>>>> [..]
>>>>
>>>> In order to signal the three possible situations (not only success and
>>>> failure to complete) after a pick through porcelain commands such as
>>>> `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was
>>>> chosen in line with the other situations in which the sequencer
>>>> encounters an error.
>>> Hmph... do we still pass negative to exit() anywhere in our codebase?
>> No, but I thought `cmd_cherry_pick` would just forward the `return -1` from the
>> sequencer to the shell. I had another look and found that `cmd_cherry_pick`
>> calls `die` instead. Now, the commit inserts 128 as exit status in
>> `fast_forward_to`.
>>
>> Would it be appropriate to mention the choice of exit status in the coding
>> guidelines? I didn't know that the int-argument to exit(3) gets truncated and
>> that this is why it is a general rule to only use values in the range from 0 to
>> 255 with exit(3).
> I personally do not think of a reason why it is necessary to mention
> how the argument to exit(3) is expected to be used by the system, but
> if it is common not to know it, I guess it would not hurt to have a
> single paragraph with at most two lines.
>
> In any case, I agree that exiting with 1 that signals "failed with
> conflict" can be confusing to the caller.  Can we have a test to
> demonstrate when this fix matters?

I think you are asking for a test and not for clarification.  But a test
was provided in 3/3 in this series.  Was it not related directly enough?

For clarification, this tri-state return value matters when the caller
is planning to do some cleanup and needs to handle the fallout
correctly.  Maybe changing this return value is not the correct way
forward, though.  It might be better if the caller could examine the
result after-the-fact instead.  This would require some reliable state
functions which I recall were somewhat scattered last time I looked. 
Also I cannot think of a reliable test for "the previous cherry-pick
failed during pre-condition checks" and I'm not sure anything should
exist to track this in .git outside of the return value for this function. 

Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge

[Junio C Hamano]

Phil Hord <hordp@cisco.com> writes:

>> In any case, I agree that exiting with 1 that signals "failed with
>> conflict" can be confusing to the caller.  Can we have a test to
>> demonstrate when this fix matters?
>
> I think you are asking for a test and not for clarification.  But a test
> was provided in 3/3 in this series.  Was it not related directly enough?

X-<  Somehow I missed the "3" in "1/3" above and did not look beyond
this first patch.

> For clarification, this tri-state return value matters when the caller
> is planning to do some cleanup and needs to handle the fallout
> correctly.  Maybe changing this return value is not the correct way
> forward, though.  It might be better if the caller could examine the
> result after-the-fact instead.

I am not sure about that.  For merge strategies "exit with 1 iff you
left the conflict in the index" is the contract between "git merge"
frontend and the strategies backend; if a similar contract is needed
between the sequencer and its users, it is good to follow the same
pattern for consistency.  The resulting index and/or the working
tree may or may not match the contents recorded in the HEAD commit
but without the backend telling the caller, the caller cannot tell
if the difference was from before the operation or created by the
operation.

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

[Fabian Ruch]

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

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

[Michael Haggerty]

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/

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

[Phil Hord]

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

[RFC 3/3] tests: Add 'rebase -i commits that overwrite untracked files'

[Fabian Ruch]

If a todo list will cherry-pick a commit that adds some file and the
working tree already contains a file with the same name, the rebase
sequence for that todo list will be interrupted and the cherry-picked
commit will be lost after the rebasing process is resumed.

This is fixed.

Add as a test case for regression testing to the "rebase-interactive"
test suite.

Reported-by: Phil Hord <hordp@cisco.com>
Signed-off-by: Fabian Ruch <bafain@gmail.com>
---
 t/t3404-rebase-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 50e22b1..7f5ac18 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1074,4 +1074,48 @@ test_expect_success 'short SHA-1 collide' '
 	)
 '
 
+test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
+	git checkout branch2 &&
+	set_fake_editor &&
+	FAKE_LINES="edit 1 2" git rebase -i A &&
+	test_cmp_rev HEAD F &&
+	test_path_is_missing file6 &&
+	touch file6 &&
+	test_must_fail git rebase --continue &&
+	test_cmp_rev HEAD F &&
+	rm file6 &&
+	git rebase --continue &&
+	test_cmp_rev HEAD I
+'
+
+test_expect_success 'rebase -i commits that overwrite untracked files (squash)' '
+	git checkout branch2 &&
+	git tag original-branch2 &&
+	set_fake_editor &&
+	FAKE_LINES="edit 1 squash 2" git rebase -i A &&
+	test_cmp_rev HEAD F &&
+	test_path_is_missing file6 &&
+	touch file6 &&
+	test_must_fail git rebase --continue &&
+	test_cmp_rev HEAD F &&
+	rm file6 &&
+	git rebase --continue &&
+	test $(git cat-file commit HEAD | sed -ne \$p) = I &&
+	git reset --hard original-branch2
+'
+
+test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
+	git checkout branch2 &&
+	set_fake_editor &&
+	FAKE_LINES="edit 1 2" git rebase -i --no-ff A &&
+	test $(git cat-file commit HEAD | sed -ne \$p) = F &&
+	test_path_is_missing file6 &&
+	touch file6 &&
+	test_must_fail git rebase --continue &&
+	test $(git cat-file commit HEAD | sed -ne \$p) = F &&
+	rm file6 &&
+	git rebase --continue &&
+	test $(git cat-file commit HEAD | sed -ne \$p) = I
+'
+
 test_done
-- 
1.9.3

Re: [RFC 3/3] tests: Add 'rebase -i commits that overwrite untracked files'

[Michael Haggerty]

On 05/27/2014 12:19 AM, Fabian Ruch wrote:
> If a todo list will cherry-pick a commit that adds some file and the
> working tree already contains a file with the same name, the rebase
> sequence for that todo list will be interrupted and the cherry-picked
> commit will be lost after the rebasing process is resumed.
> 
> This is fixed.
> 
> Add as a test case for regression testing to the "rebase-interactive"
> test suite.

The tests look fine.  (I assume you tested the tests against the
pre-fixed version of the software to make sure that they caught the
problem that you fixed.)

As I mentioned in patch 2/3, I think it would be better to add the tests
in the same commit where you fix the problem.

The commit message is, I think, confusing because the first paragraph is
in future tense even though it is describing a problem that was just
fixed.  It will probably change completely when you squash this with the
previous commit, but for future reference, I would have suggested
something more like

    t3404: "rebase -i" retries pick when blocked by untracked file

    If a commit that is being "pick"ed adds a file that already exists
    as an untracked file in the working tree, cherry-pick fails before
    even trying to apply the change.  "rebase --interactive" didn't
    distinguish this error from a conflict, and when "rebase --continue"
    was run it thought that the user had just resolved and committed
    the conflict.  The result was that the commit was omitted entirely
    from the rebased series.

    This problem was fixed in the previous commit.  Add tests.

> Reported-by: Phil Hord <hordp@cisco.com>
> Signed-off-by: Fabian Ruch <bafain@gmail.com>
> ---
>  t/t3404-rebase-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 50e22b1..7f5ac18 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1074,4 +1074,48 @@ test_expect_success 'short SHA-1 collide' '
>  	)
>  '
>  
> +test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
> +	git checkout branch2 &&
> +	set_fake_editor &&
> +	FAKE_LINES="edit 1 2" git rebase -i A &&
> +	test_cmp_rev HEAD F &&
> +	test_path_is_missing file6 &&
> +	touch file6 &&
> +	test_must_fail git rebase --continue &&
> +	test_cmp_rev HEAD F &&
> +	rm file6 &&
> +	git rebase --continue &&
> +	test_cmp_rev HEAD I
> +'
> +
> +test_expect_success 'rebase -i commits that overwrite untracked files (squash)' '
> +	git checkout branch2 &&
> +	git tag original-branch2 &&

It might be easier (and better decoupled from other tests) to

        git checkout -b squash-overwrite branch2 &&

rather than moving branch2 then resetting it.  That way you can also
leave the branch in the repo when you are done rather than having to
clean it up.

> +	set_fake_editor &&
> +	FAKE_LINES="edit 1 squash 2" git rebase -i A &&
> +	test_cmp_rev HEAD F &&
> +	test_path_is_missing file6 &&
> +	touch file6 &&
> +	test_must_fail git rebase --continue &&
> +	test_cmp_rev HEAD F &&
> +	rm file6 &&
> +	git rebase --continue &&
> +	test $(git cat-file commit HEAD | sed -ne \$p) = I &&
> +	git reset --hard original-branch2
> +'
> +
> +test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
> +	git checkout branch2 &&
> +	set_fake_editor &&
> +	FAKE_LINES="edit 1 2" git rebase -i --no-ff A &&
> +	test $(git cat-file commit HEAD | sed -ne \$p) = F &&
> +	test_path_is_missing file6 &&
> +	touch file6 &&
> +	test_must_fail git rebase --continue &&
> +	test $(git cat-file commit HEAD | sed -ne \$p) = F &&
> +	rm file6 &&
> +	git rebase --continue &&
> +	test $(git cat-file commit HEAD | sed -ne \$p) = I
> +'
> +
>  test_done
> 

Thanks!
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [RFC 3/3] tests: Add 'rebase -i commits that overwrite untracked files'

[Junio C Hamano]

Fabian Ruch <bafain@gmail.com> writes:

> If a todo list will cherry-pick a commit that adds some file and the
> working tree already contains a file with the same name, the rebase
> sequence for that todo list will be interrupted and the cherry-picked
> commit will be lost after the rebasing process is resumed.
>
> This is fixed.
>
> Add as a test case for regression testing to the "rebase-interactive"
> test suite.
>
> Reported-by: Phil Hord <hordp@cisco.com>
> Signed-off-by: Fabian Ruch <bafain@gmail.com>
> ---
>  t/t3404-rebase-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 50e22b1..7f5ac18 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1074,4 +1074,48 @@ test_expect_success 'short SHA-1 collide' '
>  	)
>  '
>  
> +test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
> +	git checkout branch2 &&
> +	set_fake_editor &&
> +	FAKE_LINES="edit 1 2" git rebase -i A &&
> +	test_cmp_rev HEAD F &&
> +	test_path_is_missing file6 &&
> +	touch file6 &&

Unless you care deeply about updating the timestamp file6 has, use
of "touch" is misleading.  Use something like this instead:

	>file6 &&

when it is the existence of "file6" that you care about.

Thanks.