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

2014-05-27 Thread 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 picked 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 ho...@cisco.com
 Signed-off-by: Fabian Ruch baf...@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
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2014-05-27 Thread Junio C Hamano
Fabian Ruch baf...@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 ho...@cisco.com
 Signed-off-by: Fabian Ruch baf...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2014-05-26 Thread 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 ho...@cisco.com
Signed-off-by: Fabian Ruch baf...@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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html