Re: [RFC] rebase --root: Empty root commit is replaced with sentinel

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Jun 19, 2014, 11:35 UTC
Message-ID: <53A2CB18.7020408@alum.mit.edu>
In-Reply-To: <53A18198.7070301@gmail.com>

On 06/18/2014 02:10 PM, Fabian Ruch wrote:

Show 14 quoted lines

> `rebase` supports the option `--root` both with and without `--onto`.
> The case where `--onto` is not specified is handled by creating a
> sentinel commit and squashing the root commit into it. The sentinel
> commit refers to the empty tree and does not have a log message
> associated with it. Its purpose is that `rebase` can rely on having a
> rebase base even without `--onto`.
> 
> The combination of `--root` and no `--onto` implies an interactive
> rebase. When `--preserve-merges` is not specified on the `rebase`
> command line, `rebase--interactive` uses `--cherry-pick` with
> git-rev-list to put the initial to-do list together. If the root commit
> is empty, it is treated as a cherry-pick of the sentinel commit and
> omitted from the todo-list. This is unexpected because the user does not
> know of the sentinel commit.

I see that your new tests below both use --keep-empty. Without --keep-empty, I would have expected empty commits to be discarded by design. If that is the case, then there is only a bug if --keep-empty is used, and I think you should mention that option earlier in this description.

Also, I think this bug strikes if *any* of the commits to be rebased is empty, not only the first commit.

Show 14 quoted lines

> Add a test case. Create an empty root commit, run `rebase --root` and
> check that it is still there. If the branch consists of the root commit
> only, the bug described above causes the resulting history to consist of
> the sentinel commit only. If the root commit has children, the resulting
> history contains neither the root nor the sentinel commit. This
> behaviour is the same with `--keep-empty`.
> 
> Signed-off-by: Fabian Ruch <bafain@gmail.com>
> ---
> 
> Notes:
>     Hi,
>     
>     This is not a fix yet.

It is actually OK to add failing tests to the test suite, but they must be added with 'test_expect_failure' instead of 'test_expect_success'. Though of course it is preferred if the new test is followed by a commit that fixes it :-)

Show 17 quoted lines

>     We are currently special casing in `do_pick` and whether the current
>     head is the sentinel commit is not a special case that would fit into
>     `do_pick`'s interface description. What if we added the feature of
>     creating root commits to `do_pick`, using `commit-tree` just like when
>     creating the sentinel commit? We would have to add another special case
>     (`test -z "$onto"`) to where the to-do list is put together in
>     `rebase--interactive`. An empty `$onto` would imply
>     
>         git rev-list $orig_head
>     
>     to form the to-do list. The rebase comment in the commit message editor
>     would have to become something similar to
>     
>         Rebase $shortrevisions as new history
>     
>     , which might be even less confusing than mentioning the hash of the
>     sentinel commit.

Since you are working on a hammer, I'm tempted to see this problem as a nail. Would it make it easier to encode the special behavior into the todo list itself?:

    pick --orphan 0cf23b1 New initial commit
    pick 144a852 Second commit
    pick 255f8de Third commit

Michael

Show 40 quoted lines

>  t/t3412-rebase-root.sh | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
> index 0b52105..a4fe3c7 100755
> --- a/t/t3412-rebase-root.sh
> +++ b/t/t3412-rebase-root.sh
> @@ -278,4 +278,31 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' '
>  	test_cmp expect-conflict-p out
>  '
>  
> +test_expect_success 'rebase --root recreates empty root commit' '
> +	echo Initial >expected.msg &&
> +	# commit the empty tree, no parents
> +	empty_tree=$(git hash-object -t tree /dev/null) &&
> +	empty_root_commit=$(git commit-tree $empty_tree -F expected.msg) &&
> +	git checkout -b empty-root-commit-only $empty_root_commit &&
> +	# implies interactive
> +	git rebase --keep-empty --root &&
> +	git show --pretty=format:%s HEAD >actual.msg &&
> +	test_cmp actual.msg expected.msg
> +'
> +
> +test_expect_success 'rebase --root recreates empty root commit (subsequent commits)' '
> +	echo Initial >expected.msg &&
> +	# commit the empty tree, no parents
> +	empty_tree=$(git hash-object -t tree /dev/null) &&
> +	empty_root_commit=$(git commit-tree $empty_tree -F expected.msg) &&
> +	git checkout -b empty-root-commit $empty_root_commit &&
> +	>file &&
> +	git add file &&
> +	git commit -m file &&
> +	# implies interactive
> +	git rebase --keep-empty --root &&
> +	git show --pretty=format:%s HEAD^ >actual.msg &&
> +	test_cmp actual.msg expected.msg
> +'
> +
>  test_done
>

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/