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

[Fabian Ruch <bafain@gmail.com>]

Hi Michael,

thanks for your reply.

On 06/19/2014 01:35 PM, Michael Haggerty wrote:
> On 06/18/2014 02:10 PM, Fabian Ruch wrote:
>> `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.

Now that you mention it, --keep-empty is crucial for this to be a bug
(except for the case where the branch consists solely of empty commits).
I intended to use --keep-empty merely as a pedagogic tool so nobody
would get confused about what is on the to-do list.

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

Ah, I really did not deduce that all empty commits would disappear with
--root and --keep-empty. Thanks.

>> 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 :-)

I did not plan to have this accepted but to amend the patch with a fix
later on. Also, I hoped the ready-to-apply tests would give someone else
a smoother start when taking over and compensate for a possibly
incomprehensible problem description.

>>     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

While I agree to enable pick to create orphan commits, I don't think a
user option --orphan is of much help. Firstly, does --orphan make sense
for any commit but the first one on the to-do list? Secondly, does
--orphan make sense when we are rebasing onto another branch? The second
point is related to the first in the sense that "pick --orphan" would be
used on a commit that is understood to have a parent.

> Michael

   Fabian

>>  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
>>