threads / discuss / 36339

git-rebase-todo gets popped prematurely

Subject: git-rebase-todo gets popped prematurely

## tl;dr

13 messages between Apr 2, 2014 and Jun 10, 2014.

replies: 12people: 4as markdown or json

Phil Hord· Apr 2, 2014, 22:37 UTC · lore

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
Fabian Ruch· May 26, 2014, 22:19 UTC · re: Phil Hord · lore

[RFC 1/3] 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. -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
Junio C Hamano· May 27, 2014, 18:42 UTC · re: Fabian Ruch · lore

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

Fabian Ruch <bafain@gmail.com> writes:
Show 18 quoted lines
> `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?
Show 19 quoted lines
>
> 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));
Fabian Ruch· Jun 9, 2014, 15:04 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On 05/27/2014 08:42 PM, Junio C Hamano wrote:
Show 10 quoted lines
> 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
Junio C Hamano· Jun 10, 2014, 17:56 UTC · re: Fabian Ruch · lore

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

Fabian Ruch <bafain@gmail.com> writes:
Show 21 quoted lines
> 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.
Show 42 quoted lines
> -- >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));
Phil Hord· Jun 10, 2014, 18:51 UTC · re: Junio C Hamano · lore

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

On 06/10/2014 01:56 PM, Junio C Hamano wrote:
Show 29 quoted lines
> 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.

Junio C Hamano· Jun 10, 2014, 19:17 UTC · re: Phil Hord · lore

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

Phil Hord <hordp@cisco.com> writes:
Show 6 quoted 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?

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

Show 5 quoted lines
> 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.

Fabian Ruch· May 26, 2014, 22:19 UTC · re: Phil Hord · lore

[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 <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
Michael Haggerty· May 27, 2014, 11:56 UTC · re: Fabian Ruch · lore

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:
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/
Phil Hord· May 27, 2014, 18:26 UTC · re: Michael Haggerty · lore

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:
Show 23 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.

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
Fabian Ruch· May 26, 2014, 22:19 UTC · re: Phil Hord · lore

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

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
Michael Haggerty· May 27, 2014, 13:15 UTC · re: Fabian Ruch · lore

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

On 05/27/2014 12:19 AM, Fabian Ruch wrote:
Show 9 quoted lines
> 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.
Show 31 quoted lines
> 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.

Show 29 quoted lines
> +	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/
Junio C Hamano· May 27, 2014, 18:47 UTC · re: Fabian Ruch · lore

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

Fabian Ruch <bafain@gmail.com> writes:
Show 31 quoted lines
> 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.

← back to recent threads