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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: May 27, 2014, 13:15 UTC
Message-ID: <53849002.7060502@alum.mit.edu>
In-Reply-To: <5383BDF0.2040904@gmail.com>

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/