git-rebase-todo gets popped prematurely
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 --onelineAnd 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
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.
--- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
sequencer.c +1 -1
@@ -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));
Signature · 1 line
1.9.3
Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge
Quoted 13 lines · Fabian Ruch
do_pick_commithandles three situations if it is not fast-forwarding. In order fordo_pick_committo 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 startSo 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.
Quoted 5 lines
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?
Quoted 19 lines
Signed-off-by Fabian Ruch <[email protected]>--- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)sequencer.c +1
-1@@ -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
Hi Junio,
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Fabian Ruch
[..]
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.
--- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
sequencer.c +1 -1
@@ -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));
Signature · 1 line
2.0.0
Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge
Quoted 17 lines · Fabian Ruch
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Fabian Ruch
[..]
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_pickwould just forward thereturn -1from the sequencer to the shell. I had another look and found thatcmd_cherry_pickcallsdieinstead. Now, the commit inserts 128 as exit status infast_forward_to.
Quoted 4 lines
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.
Quoted 42 lines
-- >8 --
Subject: sequencer: Signal failed ff as an aborted, not a conflicted merge
do_pick_commithandles three situations if it is not fast-forwarding. In order fordo_pick_committo 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 startSo 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 andcherry-picktranslates this into a call todie.diethen terminates the process with exit status 128.Signed-off-by Fabian Ruch <[email protected]>--- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)sequencer.c +1
-1@@ -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
Quoted 21 lines · Junio C Hamano
Quoted 15 lines · Fabian Ruch
Quoted 9 lines · Junio C Hamano
Quoted 7 lines · Fabian Ruch
[..]
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_pickwould just forward thereturn -1from the sequencer to the shell. I had another look and found thatcmd_cherry_pickcallsdieinstead. Now, the commit inserts 128 as exit status infast_forward_to.Quoted 4 lines
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).
Quoted 8 lines
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
Quoted 6 lines · Phil Hord
Quoted 3 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.
Quoted 5 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.
RFC 2/3 rebase -i: Reschedule tasks that failed before the index was touched
In reply to Phil HordWhen 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.
--- git-rebase--interactive.sh | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
git-rebase--interactive.sh +22 -5
@@ -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
Signature · 1 line
1.9.3
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.
Quoted 25 lines · Fabian Ruch
When
rebase--interactiveprocesses a task, it removes the item from the todo list and appends it to another list of executed tasks. If apick(this includessquashandfixup) 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-pickcommand 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) andrebasedoes not continue until they are resolved or the changes are not recorded in the index (return value neither 0 nor 1) andrebasehas to try again with the same task.Reported-by Phil Hord <[email protected]>Signed-off-by Fabian Ruch <[email protected]>--- git-rebase--interactive.sh | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)git-rebase--interactive.sh +0
-0
git-rebase--interactive.sh mark_action_done () { · line 137
Quoted 6 lines
git-rebase--interactive.sh +3
-0@@ -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.
Quoted 11 lines
git-rebase--interactive.sh +7
-0+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
git-rebase--interactive.sh do_pick () { · line 483
Quoted 6 lines
git-rebase--interactive.sh +1
-2@@ -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.
Quoted 17 lines
git-rebase--interactive.sh +7
-2else - 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"
git-rebase--interactive.sh do_next () { · line 557
Quoted 10 lines
git-rebase--interactive.sh +4
-1eval "$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
Signature · 3 lines
Michael Haggerty [email protected] http://softwareswirl.blogspot.com/
Re: [RFC 2/3] rebase -i: Reschedule tasks that failed before the index was touched
Hi Fabian,
Thanks for looking into this.
Quoted 10 lines · Michael Haggerty
Quoted 10 lines
(quoted diff) +7
-0+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
Quoted 13 lines
Review do_pick () { · line 483
Quoted 7 lines
(quoted diff) +1
-2@@ -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_onesucceeds 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, thenreschedule_last_actionwill 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:
===================
git-rebase--interactive.sh +13 -2
@@ -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'
In reply to Phil HordIf 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.
--- t/t3404-rebase-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
t/t3404-rebase-interactive.sh +44 -0
@@ -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
Signature · 1 line
1.9.3
Re: [RFC 3/3] tests: Add 'rebase -i commits that overwrite untracked files'
Quoted 9 lines · 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.
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.
Quoted 21 lines
Reported-by Phil Hord <[email protected]>Signed-off-by Fabian Ruch <[email protected]>--- t/t3404-rebase-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)t/t3404-rebase-interactive.sh +7
-0@@ -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 &&
t/t3404-rebase-interactive.sh test_expect_success 'short SHA-1 collide' ' · line 1093
Quoted 10 lines
t/t3404-rebase-interactive.sh +10
-0+ 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.
Quoted 19 lines
t/t3404-rebase-interactive.sh +18
-0+ 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 &&
t/t3404-rebase-interactive.sh test_expect_success 'short SHA-1 collide' ' · line 1121
Quoted 10 lines
t/t3404-rebase-interactive.sh +9
-0+ 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
Signature · 3 lines
Michael Haggerty [email protected] http://softwareswirl.blogspot.com/
Re: [RFC 3/3] tests: Add 'rebase -i commits that overwrite untracked files'
In reply to Fabian RuchQuoted 21 lines · 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 <[email protected]>Signed-off-by Fabian Ruch <[email protected]>--- t/t3404-rebase-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)t/t3404-rebase-interactive.sh +0
-0
t/t3404-rebase-interactive.sh test_expect_success 'short SHA-1 collide' ' · line 1083
Quoted 10 lines
t/t3404-rebase-interactive.sh +7
-0@@ -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.