rebase --root: Empty root commit is replaced with sentinel
RFC 1/1 rebase --root: Empty root commit is replaced with sentinel
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.
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.
---
Notes:
Hi,
This is not a fix yet.
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.
Fabian
t/t3412-rebase-root.sh | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
t/t3412-rebase-root.sh +27 -0
@@ -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
Signature · 1 line
2.0.0
Quoted 7 lines · Fabian Ruch
rebasesupports the option--rootboth with and without--onto. The case where--ontois 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 thatrebasecan rely on having a rebase base even without--onto.
Quoted 7 lines
The combination of
--rootand no--ontoimplies an interactive rebase. When--preserve-mergesis not specified on therebasecommand line,rebase--interactiveuses--cherry-pickwith 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.
Quoted 7 lines
Add a test case. Create an empty root commit, run
rebase --rootand 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.
Quoted 7 lines
Signed-off-by Fabian Ruch <[email protected]>---
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 :-)
Quoted 10 lines
We are currently special casing in
do_pickand whether the current head is the sentinel commit is not a special case that would fit intodo_pick's interface description. What if we added the feature of creating root commits todo_pick, usingcommit-treejust 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 inrebase--interactive. An empty$ontowould implygit rev-list $orig_head
Quoted 7 lines
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
Quoted 40 lines
t/t3412-rebase-root.sh | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)t/t3412-rebase-root.sh +27
-0@@ -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
Signature · 3 lines
Michael Haggerty [email protected] http://softwareswirl.blogspot.com/
Hi Michael,
thanks for your reply.
Quoted 16 lines · Michael Haggerty
Quoted 7 lines · Fabian Ruch
rebasesupports the option--rootboth with and without--onto. The case where--ontois 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 thatrebasecan rely on having a rebase base even without--onto.Quoted 7 lines
The combination of
--rootand no--ontoimplies an interactive rebase. When--preserve-mergesis not specified on therebasecommand line,rebase--interactiveuses--cherry-pickwith 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.
Quoted 5 lines
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.
Quoted 2 lines
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.
Quoted 13 lines
Quoted 13 lines
Add a test case. Create an empty root commit, run
rebase --rootand 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 <[email protected]>---
Notes:
Hi,
Quoted 6 lines
Quoted 1 line
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.
Quoted 18 lines
Quoted 10 lines
We are currently special casing in
do_pickand whether the current head is the sentinel commit is not a special case that would fit intodo_pick's interface description. What if we added the feature of creating root commits todo_pick, usingcommit-treejust 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 inrebase--interactive. An empty$ontowould implygit rev-list $orig_head
Quoted 7 lines
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.
Quoted 7 lines
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.
Quoted 1 line
Michael
Fabian
Quoted 40 lines
Quoted 40 lines
t/t3412-rebase-root.sh | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)t/t3412-rebase-root.sh +27
-0@@ -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
Quoted 89 lines · Fabian Ruch
Hi Michael,
thanks for your reply.
Quoted 16 lines · Michael Haggerty
Quoted 7 lines · Fabian Ruch
rebasesupports the option--rootboth with and without--onto. The case where--ontois 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 thatrebasecan rely on having a rebase base even without--onto.Quoted 7 lines
The combination of
--rootand no--ontoimplies an interactive rebase. When--preserve-mergesis not specified on therebasecommand line,rebase--interactiveuses--cherry-pickwith 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.Quoted 5 lines
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.
Quoted 2 lines
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.
Quoted 13 lines
Quoted 13 lines
Add a test case. Create an empty root commit, run
rebase --rootand 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 <[email protected]>---
Notes:
Hi,Quoted 6 lines
Quoted 1 line
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.
Quoted 18 lines
Quoted 10 lines
We are currently special casing in
do_pickand whether the current head is the sentinel commit is not a special case that would fit intodo_pick's interface description. What if we added the feature of creating root commits todo_pick, usingcommit-treejust 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 inrebase--interactive. An empty$ontowould implygit rev-list $orig_head
Quoted 7 lines
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.
Quoted 7 lines
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
Quoted 6 lines
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
Signature · 3 lines
Michael Haggerty [email protected] http://softwareswirl.blogspot.com/
PATCH 1/1 rebase --root: sentinel commit cloaks empty commits
In reply to Fabian Ruchgit-rebase supports the option --root both with and without --onto. In rebase root mode it replays all commits reachable from a branch on top of another branch, including the very first commit. In case --onto is not specified, that other branch is temporarily provided by creating an empty commit. When the first commit on the to-do list is being replayed, the so-called sentinel commit is amended using the log message and patch of the replayed commit. Since the sentinel commit is empty, this results in a replacement of the sentinel commit with the new root commit of the rebased branch.
The combination of --root and no --onto implies an interactive rebase. When --preserve-merges is not specified on the command line, git-rebase--interactive uses --cherry-pick with git-rev-list to put the initial to-do list together. The left side is given by the fake base and the right side by the branch being rebased. What happens now is that any empty commit on the original branch is treated as a cherry-pick of the sentinel commit and subsequently omitted from the to-do list. This is a bug if --keep-empty is specified also.
Even without --keep-empty, using the sentinel commit as left side with git-rev-list can result in a faulty rebased branch. Indeed, in the unlikely case that the original branch consists solely of empty commits, the bug crops up in the strangest fashion as all commits are skipped and the sentinel commit is not replaced. As a result, git-rebase produces a branch with a single empty commit.
To trigger the replacement of the sentinel commit, git-rebase assigns the variable squash_onto. Special case a second time regarding squash_onto and run git-rev-list without a left side if the variable is assigned. The latter is the case if and only if --root is used without --onto, that is upstream points to the sentinel commit and $upstream...$orig_head would subtract a commit that is not actually there from the original branch.
Fix a typo in is_empty_commit. It always found root commits non-empty so that empty root commits were scheduled even without --keep-empty. The POSIX specification states that command substitutions are to be executed in sub-shells, which makes exit(1) and variable assignments not affect the script execution state. That was the reason why ptree was null for parentless commits and the test "$tree" = "$ptree" always false for them.
Add tests.
---
Hi,
Three test cases were added to the bug report to account for the additional cases in which the bug strikes (raised by Michael on the other sub-thread). A bugfix is included now as well.
Concerning the bugfix: Obviously, the patch misuses the squash_onto flag because it assumes that the new base is empty except for the sentinel commit. The variable name does not imply anything close to that. An additional flag to disable the use of the git-rev-list option --cherry-pick would work and make sense again (for instance, keep_redundant). However, the following two bugs, not related to empty commits, seem to suggest that git-rebase--interactive cannot work obliviously to non-existent bases.
1) git-rebase--interactive when used with --root and the to-do list noop results in the original branch's history being rewritten to contain only the sentinel commit.
git-rebase--interactive correctly checkouts $onto and replays no commits on top of it but git-rebase has forgotten that $onto was fake.
2) git-rebase--interactive when used with --root always creates a fresh root commit, regardless of --no-ff being specified.
With the current meaning of squash_onto, git-rebase--interactive cannot just reset the branch to the old root commit. It is really the fault of git-rebase to start off with a new commit.
Please take a closer look at the last two test cases that specify the expected behaviour of rebasing a branch that tracks the empty tree. At this point they expect the "Nothing to do" error (aborts with untouched history). This is consistent with rebasing only empty commits without --root, which also doesn't just delete them from the history. Furthermore, I think the two alternatives adding a note that all commits in the range were empty, and removing the empty commits (thus making the branch empty) are better discussed in a separate bug report.
Thanks for your time,
Fabian
git-rebase--interactive.sh | 20 ++++++++++++++----- t/t3412-rebase-root.sh | 49 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 5 deletions(-)
git-rebase--interactive.sh +15 -5
@@ -201,10 +201,10 @@ has_action () { } is_empty_commit() { - tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null || - die "$1: not a commit that can be picked") - ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null || - ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904) + tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null) || + die "$1: not a commit that can be picked" + ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null) || + ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904 test "$tree" = "$ptree" }
@@ -958,7 +958,17 @@ then revisions=$upstream...$orig_head shortrevisions=$shortupstream..$shorthead else - revisions=$onto...$orig_head + if test -n "$squash_onto" + then + # $onto points to an empty commit (the sentinel + # commit) which was not created by the user. + # Exclude it from the rev list to avoid skipping + # empty user commits prematurely, i. e. before + # --keep-empty can take effect. + revisions=$orig_head + else + revisions=$onto...$orig_head + fi shortrevisions=$shorthead fi git rev-list $merges_option --pretty=oneline --abbrev-commit \
t/t3412-rebase-root.sh +49 -0
53 lines not on this page · raw message
Signature · 1 line
2.0.1
Re: [PATCH v1] rebase --root: sentinel commit cloaks empty commits
Hi Fabian
Impressive analysis!
Quoted 6 lines
Concerning the bugfix: Obviously, the patch misuses the
squash_ontoflag because it assumes that the new base is empty except for the sentinel commit. The variable name does not imply anything close to that. An additional flag to disable the use of the git-rev-list option--cherry-pickwould work and make sense again (for instance,keep_redundant).
Seeing as there are only two existing uses of the variable, you could also rename it to make it more obvious what is going on. I think either way is fine.
[...]
Quoted 9 lines
Please take a closer look at the last two test cases that specify the expected behaviour of rebasing a branch that tracks the empty tree. At this point they expect the "Nothing to do" error (aborts with untouched history). This is consistent with rebasing only empty commits without
--root, which also doesn't just delete them from the history. Furthermore, I think the two alternatives adding a note that all commits in the range were empty, and removing the empty commits (thus making the branch empty) are better discussed in a separate bug report.
Makes sense to me, though I have never thought much about rebasing empty commits. Maybe Chris has a more informed opinion?
git-rebase--interactive.sh has_action () { · line 209
Quoted 11 lines
is_empty_commit(){git-rebase--interactive.sh +4
-4- tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null || - die "$1: not a commit that can be picked") - ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null || - ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904) + tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null) || + die "$1: not a commit that can be picked" + ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null) || + ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904 test "$tree" = "$ptree" }
Nice catch!
Quoted 7 lines
git-rebase--interactive.sh +2
-1@@ -958,7 +958,17 @@ then revisions=$upstream...$orig_head shortrevisions=$shortupstream..$shorthead else - revisions=$onto...$orig_head + if test -n "$squash_onto" + then
git-rebase--interactive.sh then · line 972
Quoted 10 lines
git-rebase--interactive.sh +9
-0+ # $onto points to an empty commit (the sentinel + # commit) which was not created by the user. + # Exclude it from the rev list to avoid skipping + # empty user commits prematurely, i. e. before + # --keep-empty can take effect. + revisions=$orig_head + else + revisions=$onto...$orig_head + fi shortrevisions=$shorthead
Nit: I think this would be clearer if you phrased it using an 'elif', instead of nesting (but keep the comment!).
Signature · 2 lines
Thomas Rast [email protected]
Re: [PATCH v1] rebase --root: sentinel commit cloaks empty commits
Quoted 10 lines · Thomas Rast
Quoted 9 lines
Please take a closer look at the last two test cases that specify the expected behaviour of rebasing a branch that tracks the empty tree. At this point they expect the "Nothing to do" error (aborts with untouched history). This is consistent with rebasing only empty commits without
--root, which also doesn't just delete them from the history. Furthermore, I think the two alternatives adding a note that all commits in the range were empty, and removing the empty commits (thus making the branch empty) are better discussed in a separate bug report.
Quoted 2 lines
Makes sense to me, though I have never thought much about rebasing empty commits. Maybe Chris has a more informed opinion?
I definitely agree with you both that --root should be (and isn't) consistent with normal interactive rebasing. The difference isn't deliberate on my part.
On a personal note, I've always disliked the way interactive rebase stops when you pick an existing empty commit or empty log message rather than preserving it. Jumping through a few hoops is perhaps sensible when you create that kind of strange commit, but just annoying when picking an existing empty/logless commit as part of a series. But as you say, that's a separate issue than --root behaving differently to non --root.
Cheers,
Chris.