Re: [RFC] rebase --root: Empty root commit is replaced with sentinel
- From
Fabian Ruch <bafain@gmail.com>
- Date
- Jun 19, 2014, 12:39 UTC
- Message-ID
- <53A2DA17.4060905@gmail.com>
- In-Reply-To
- <53A2CB18.7020408@alum.mit.edu>
Hi Michael,
thanks for your reply.
On 06/19/2014 01:35 PM, Michael Haggerty wrote:
Show 21 quoted lines
> 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.
Show 19 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 :-)
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.
Show 25 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
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
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 >>