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

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

On 06/19/2014 02:39 PM, Fabian Ruch wrote:
> 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.

--orphan as a user option would only really make sense if we get around
to supporting interactive rebase of arbitrary DAGs.

Perhaps a more practical problem with --orphan is that it makes it
harder for the user to change the order of the first two commits.

Another possible construct would be a separate "orphan" command:

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

But these are just wild ideas.  I haven't thought enough about the
problem to advocate anything.

Michael

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