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

[Michael Haggerty <mhagger@alum.mit.edu>]

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/