{"thread":{"id":"36947","subject":"[RFC] rebase --root: Empty root commit is replaced with sentinel","startedAt":"2014-06-18T12:10:00Z","lastAt":"2014-07-20T20:52:06Z","messageCount":7,"participants":["Fabian Ruch","Michael Haggerty","Thomas Rast","Chris Webb"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"244511","messageId":"53A18198.7070301@gmail.com","threadId":"36947","inReplyTo":null,"subject":"[RFC] rebase --root: Empty root commit is replaced with sentinel","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-06-18T12:10:00Z","receivedAt":"2014-06-18T12:10:00Z","isPatch":false,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"`rebase` supports the option `--root` both with and without `--onto`.\nThe case where `--onto` is not specified is handled by creating a\nsentinel commit and squashing the root commit into it. The sentinel\ncommit refers to the empty tree and does not have a log message\nassociated with it. Its purpose is that `rebase` can rely on having a\nrebase base even without `--onto`.\n\nThe combination of `--root` and no `--onto` implies an interactive\nrebase. When `--preserve-merges` is not specified on the `rebase`\ncommand line, `rebase--interactive` uses `--cherry-pick` with\ngit-rev-list to put the initial to-do list together. If the root commit\nis empty, it is treated as a cherry-pick of the sentinel commit and\nomitted from the todo-list. This is unexpected because the user does not\nknow of the sentinel commit.\n\nAdd a test case. Create an empty root commit, run `rebase --root` and\ncheck that it is still there. If the branch consists of the root commit\nonly, the bug described above causes the resulting history to consist of\nthe sentinel commit only. If the root commit has children, the resulting\nhistory contains neither the root nor the sentinel commit. This\nbehaviour is the same with `--keep-empty`.\n\nSigned-off-by: Fabian Ruch <bafain@gmail.com>\n---\n\nNotes:\n    Hi,\n    \n    This is not a fix yet.\n    \n    We are currently special casing in `do_pick` and whether the current\n    head is the sentinel commit is not a special case that would fit into\n    `do_pick`'s interface description. What if we added the feature of\n    creating root commits to `do_pick`, using `commit-tree` just like when\n    creating the sentinel commit? We would have to add another special case\n    (`test -z \"$onto\"`) to where the to-do list is put together in\n    `rebase--interactive`. An empty `$onto` would imply\n    \n        git rev-list $orig_head\n    \n    to form the to-do list. The rebase comment in the commit message editor\n    would have to become something similar to\n    \n        Rebase $shortrevisions as new history\n    \n    , which might be even less confusing than mentioning the hash of the\n    sentinel commit.\n    \n       Fabian\n\n t/t3412-rebase-root.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh\nindex 0b52105..a4fe3c7 100755\n--- a/t/t3412-rebase-root.sh\n+++ b/t/t3412-rebase-root.sh\n@@ -278,4 +278,31 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' '\n \ttest_cmp expect-conflict-p out\n '\n \n+test_expect_success 'rebase --root recreates empty root commit' '\n+\techo Initial >expected.msg &&\n+\t# commit the empty tree, no parents\n+\tempty_tree=$(git hash-object -t tree /dev/null) &&\n+\tempty_root_commit=$(git commit-tree $empty_tree -F expected.msg) &&\n+\tgit checkout -b empty-root-commit-only $empty_root_commit &&\n+\t# implies interactive\n+\tgit rebase --keep-empty --root &&\n+\tgit show --pretty=format:%s HEAD >actual.msg &&\n+\ttest_cmp actual.msg expected.msg\n+'\n+\n+test_expect_success 'rebase --root recreates empty root commit (subsequent commits)' '\n+\techo Initial >expected.msg &&\n+\t# commit the empty tree, no parents\n+\tempty_tree=$(git hash-object -t tree /dev/null) &&\n+\tempty_root_commit=$(git commit-tree $empty_tree -F expected.msg) &&\n+\tgit checkout -b empty-root-commit $empty_root_commit &&\n+\t>file &&\n+\tgit add file &&\n+\tgit commit -m file &&\n+\t# implies interactive\n+\tgit rebase --keep-empty --root &&\n+\tgit show --pretty=format:%s HEAD^ >actual.msg &&\n+\ttest_cmp actual.msg expected.msg\n+'\n+\n test_done\n-- \n2.0.0\n"},{"id":"244638","messageId":"53A2CB18.7020408@alum.mit.edu","threadId":"36947","inReplyTo":"53A18198.7070301@gmail.com","subject":"Re: [RFC] rebase --root: Empty root commit is replaced with sentinel","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-06-19T11:35:52Z","receivedAt":"2014-06-19T11:35:52Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/18/2014 02:10 PM, Fabian Ruch wrote:\n> `rebase` supports the option `--root` both with and without `--onto`.\n> The case where `--onto` is not specified is handled by creating a\n> sentinel commit and squashing the root commit into it. The sentinel\n> commit refers to the empty tree and does not have a log message\n> associated with it. Its purpose is that `rebase` can rely on having a\n> rebase base even without `--onto`.\n> \n> The combination of `--root` and no `--onto` implies an interactive\n> rebase. When `--preserve-merges` is not specified on the `rebase`\n> command line, `rebase--interactive` uses `--cherry-pick` with\n> git-rev-list to put the initial to-do list together. If the root commit\n> is empty, it is treated as a cherry-pick of the sentinel commit and\n> omitted from the todo-list. This is unexpected because the user does not\n> know of the sentinel commit.\n\nI see that your new tests below both use --keep-empty.  Without\n--keep-empty, I would have expected empty commits to be discarded by\ndesign.  If that is the case, then there is only a bug if --keep-empty\nis used, and I think you should mention that option earlier in this\ndescription.\n\nAlso, I think this bug strikes if *any* of the commits to be rebased is\nempty, not only the first commit.\n\n> Add a test case. Create an empty root commit, run `rebase --root` and\n> check that it is still there. If the branch consists of the root commit\n> only, the bug described above causes the resulting history to consist of\n> the sentinel commit only. If the root commit has children, the resulting\n> history contains neither the root nor the sentinel commit. This\n> behaviour is the same with `--keep-empty`.\n> \n> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n> ---\n> \n> Notes:\n>     Hi,\n>     \n>     This is not a fix yet.\n\nIt is actually OK to add failing tests to the test suite, but they must\nbe added with 'test_expect_failure' instead of 'test_expect_success'.\nThough of course it is preferred if the new test is followed by a commit\nthat fixes it :-)\n\n>     We are currently special casing in `do_pick` and whether the current\n>     head is the sentinel commit is not a special case that would fit into\n>     `do_pick`'s interface description. What if we added the feature of\n>     creating root commits to `do_pick`, using `commit-tree` just like when\n>     creating the sentinel commit? We would have to add another special case\n>     (`test -z \"$onto\"`) to where the to-do list is put together in\n>     `rebase--interactive`. An empty `$onto` would imply\n>     \n>         git rev-list $orig_head\n>     \n>     to form the to-do list. The rebase comment in the commit message editor\n>     would have to become something similar to\n>     \n>         Rebase $shortrevisions as new history\n>     \n>     , which might be even less confusing than mentioning the hash of the\n>     sentinel commit.\n\nSince you are working on a hammer, I'm tempted to see this problem as a\nnail.  Would it make it easier to encode the special behavior into the\ntodo list itself?:\n\n    pick --orphan 0cf23b1 New initial commit\n    pick 144a852 Second commit\n    pick 255f8de Third commit\n\nMichael\n\n>  t/t3412-rebase-root.sh | 27 +++++++++++++++++++++++++++\n>  1 file changed, 27 insertions(+)\n> \n> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh\n> index 0b52105..a4fe3c7 100755\n> --- a/t/t3412-rebase-root.sh\n> +++ b/t/t3412-rebase-root.sh\n> @@ -278,4 +278,31 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' '\n>  \ttest_cmp expect-conflict-p out\n>  '\n>  \n> +test_expect_success 'rebase --root recreates empty root commit' '\n> +\techo Initial >expected.msg &&\n> +\t# commit the empty tree, no parents\n> +\tempty_tree=$(git hash-object -t tree /dev/null) &&\n> +\tempty_root_commit=$(git commit-tree $empty_tree -F expected.msg) &&\n> +\tgit checkout -b empty-root-commit-only $empty_root_commit &&\n> +\t# implies interactive\n> +\tgit rebase --keep-empty --root &&\n> +\tgit show --pretty=format:%s HEAD >actual.msg &&\n> +\ttest_cmp actual.msg expected.msg\n> +'\n> +\n> +test_expect_success 'rebase --root recreates empty root commit (subsequent commits)' '\n> +\techo Initial >expected.msg &&\n> +\t# commit the empty tree, no parents\n> +\tempty_tree=$(git hash-object -t tree /dev/null) &&\n> +\tempty_root_commit=$(git commit-tree $empty_tree -F expected.msg) &&\n> +\tgit checkout -b empty-root-commit $empty_root_commit &&\n> +\t>file &&\n> +\tgit add file &&\n> +\tgit commit -m file &&\n> +\t# implies interactive\n> +\tgit rebase --keep-empty --root &&\n> +\tgit show --pretty=format:%s HEAD^ >actual.msg &&\n> +\ttest_cmp actual.msg expected.msg\n> +'\n> +\n>  test_done\n> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"244640","messageId":"53A2DA17.4060905@gmail.com","threadId":"36947","inReplyTo":"53A2CB18.7020408@alum.mit.edu","subject":"Re: [RFC] rebase --root: Empty root commit is replaced with sentinel","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-06-19T12:39:51Z","receivedAt":"2014-06-19T12:39:51Z","isPatch":false,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"Hi Michael,\n\nthanks for your reply.\n\nOn 06/19/2014 01:35 PM, Michael Haggerty wrote:\n> On 06/18/2014 02:10 PM, Fabian Ruch wrote:\n>> `rebase` supports the option `--root` both with and without `--onto`.\n>> The case where `--onto` is not specified is handled by creating a\n>> sentinel commit and squashing the root commit into it. The sentinel\n>> commit refers to the empty tree and does not have a log message\n>> associated with it. Its purpose is that `rebase` can rely on having a\n>> rebase base even without `--onto`.\n>>\n>> The combination of `--root` and no `--onto` implies an interactive\n>> rebase. When `--preserve-merges` is not specified on the `rebase`\n>> command line, `rebase--interactive` uses `--cherry-pick` with\n>> git-rev-list to put the initial to-do list together. If the root commit\n>> is empty, it is treated as a cherry-pick of the sentinel commit and\n>> omitted from the todo-list. This is unexpected because the user does not\n>> know of the sentinel commit.\n> \n> I see that your new tests below both use --keep-empty.  Without\n> --keep-empty, I would have expected empty commits to be discarded by\n> design.  If that is the case, then there is only a bug if --keep-empty\n> is used, and I think you should mention that option earlier in this\n> description.\n\nNow that you mention it, --keep-empty is crucial for this to be a bug\n(except for the case where the branch consists solely of empty commits).\nI intended to use --keep-empty merely as a pedagogic tool so nobody\nwould get confused about what is on the to-do list.\n\n> Also, I think this bug strikes if *any* of the commits to be rebased is\n> empty, not only the first commit.\n\nAh, I really did not deduce that all empty commits would disappear with\n--root and --keep-empty. Thanks.\n\n>> Add a test case. Create an empty root commit, run `rebase --root` and\n>> check that it is still there. If the branch consists of the root commit\n>> only, the bug described above causes the resulting history to consist of\n>> the sentinel commit only. If the root commit has children, the resulting\n>> history contains neither the root nor the sentinel commit. This\n>> behaviour is the same with `--keep-empty`.\n>>\n>> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n>> ---\n>>\n>> Notes:\n>>     Hi,\n>>     \n>>     This is not a fix yet.\n> \n> It is actually OK to add failing tests to the test suite, but they must\n> be added with 'test_expect_failure' instead of 'test_expect_success'.\n> Though of course it is preferred if the new test is followed by a commit\n> that fixes it :-)\n\nI did not plan to have this accepted but to amend the patch with a fix\nlater on. Also, I hoped the ready-to-apply tests would give someone else\na smoother start when taking over and compensate for a possibly\nincomprehensible problem description.\n\n>>     We are currently special casing in `do_pick` and whether the current\n>>     head is the sentinel commit is not a special case that would fit into\n>>     `do_pick`'s interface description. What if we added the feature of\n>>     creating root commits to `do_pick`, using `commit-tree` just like when\n>>     creating the sentinel commit? We would have to add another special case\n>>     (`test -z \"$onto\"`) to where the to-do list is put together in\n>>     `rebase--interactive`. An empty `$onto` would imply\n>>     \n>>         git rev-list $orig_head\n>>     \n>>     to form the to-do list. The rebase comment in the commit message editor\n>>     would have to become something similar to\n>>     \n>>         Rebase $shortrevisions as new history\n>>     \n>>     , which might be even less confusing than mentioning the hash of the\n>>     sentinel commit.\n> \n> Since you are working on a hammer, I'm tempted to see this problem as a\n> nail.  Would it make it easier to encode the special behavior into the\n> todo list itself?:\n> \n>     pick --orphan 0cf23b1 New initial commit\n>     pick 144a852 Second commit\n>     pick 255f8de Third commit\n\nWhile I agree to enable pick to create orphan commits, I don't think a\nuser option --orphan is of much help. Firstly, does --orphan make sense\nfor any commit but the first one on the to-do list? Secondly, does\n--orphan make sense when we are rebasing onto another branch? The second\npoint is related to the first in the sense that \"pick --orphan\" would be\nused on a commit that is understood to have a parent.\n\n> Michael\n\n   Fabian\n\n>>  t/t3412-rebase-root.sh | 27 +++++++++++++++++++++++++++\n>>  1 file changed, 27 insertions(+)\n>>\n>> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh\n>> index 0b52105..a4fe3c7 100755\n>> --- a/t/t3412-rebase-root.sh\n>> +++ b/t/t3412-rebase-root.sh\n>> @@ -278,4 +278,31 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' '\n>>  \ttest_cmp expect-conflict-p out\n>>  '\n>>  \n>> +test_expect_success 'rebase --root recreates empty root commit' '\n>> +\techo Initial >expected.msg &&\n>> +\t# commit the empty tree, no parents\n>> +\tempty_tree=$(git hash-object -t tree /dev/null) &&\n>> +\tempty_root_commit=$(git commit-tree $empty_tree -F expected.msg) &&\n>> +\tgit checkout -b empty-root-commit-only $empty_root_commit &&\n>> +\t# implies interactive\n>> +\tgit rebase --keep-empty --root &&\n>> +\tgit show --pretty=format:%s HEAD >actual.msg &&\n>> +\ttest_cmp actual.msg expected.msg\n>> +'\n>> +\n>> +test_expect_success 'rebase --root recreates empty root commit (subsequent commits)' '\n>> +\techo Initial >expected.msg &&\n>> +\t# commit the empty tree, no parents\n>> +\tempty_tree=$(git hash-object -t tree /dev/null) &&\n>> +\tempty_root_commit=$(git commit-tree $empty_tree -F expected.msg) &&\n>> +\tgit checkout -b empty-root-commit $empty_root_commit &&\n>> +\t>file &&\n>> +\tgit add file &&\n>> +\tgit commit -m file &&\n>> +\t# implies interactive\n>> +\tgit rebase --keep-empty --root &&\n>> +\tgit show --pretty=format:%s HEAD^ >actual.msg &&\n>> +\ttest_cmp actual.msg expected.msg\n>> +'\n>> +\n>>  test_done\n>>\n"},{"id":"244641","messageId":"53A2E0C2.1000004@alum.mit.edu","threadId":"36947","inReplyTo":"53A2DA17.4060905@gmail.com","subject":"Re: [RFC] rebase --root: Empty root commit is replaced with sentinel","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-06-19T13:08:18Z","receivedAt":"2014-06-19T13:08:18Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/19/2014 02:39 PM, Fabian Ruch wrote:\n> Hi Michael,\n> \n> thanks for your reply.\n> \n> On 06/19/2014 01:35 PM, Michael Haggerty wrote:\n>> On 06/18/2014 02:10 PM, Fabian Ruch wrote:\n>>> `rebase` supports the option `--root` both with and without `--onto`.\n>>> The case where `--onto` is not specified is handled by creating a\n>>> sentinel commit and squashing the root commit into it. The sentinel\n>>> commit refers to the empty tree and does not have a log message\n>>> associated with it. Its purpose is that `rebase` can rely on having a\n>>> rebase base even without `--onto`.\n>>>\n>>> The combination of `--root` and no `--onto` implies an interactive\n>>> rebase. When `--preserve-merges` is not specified on the `rebase`\n>>> command line, `rebase--interactive` uses `--cherry-pick` with\n>>> git-rev-list to put the initial to-do list together. If the root commit\n>>> is empty, it is treated as a cherry-pick of the sentinel commit and\n>>> omitted from the todo-list. This is unexpected because the user does not\n>>> know of the sentinel commit.\n>>\n>> I see that your new tests below both use --keep-empty.  Without\n>> --keep-empty, I would have expected empty commits to be discarded by\n>> design.  If that is the case, then there is only a bug if --keep-empty\n>> is used, and I think you should mention that option earlier in this\n>> description.\n> \n> Now that you mention it, --keep-empty is crucial for this to be a bug\n> (except for the case where the branch consists solely of empty commits).\n> I intended to use --keep-empty merely as a pedagogic tool so nobody\n> would get confused about what is on the to-do list.\n> \n>> Also, I think this bug strikes if *any* of the commits to be rebased is\n>> empty, not only the first commit.\n> \n> Ah, I really did not deduce that all empty commits would disappear with\n> --root and --keep-empty. Thanks.\n> \n>>> Add a test case. Create an empty root commit, run `rebase --root` and\n>>> check that it is still there. If the branch consists of the root commit\n>>> only, the bug described above causes the resulting history to consist of\n>>> the sentinel commit only. If the root commit has children, the resulting\n>>> history contains neither the root nor the sentinel commit. This\n>>> behaviour is the same with `--keep-empty`.\n>>>\n>>> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n>>> ---\n>>>\n>>> Notes:\n>>>     Hi,\n>>>     \n>>>     This is not a fix yet.\n>>\n>> It is actually OK to add failing tests to the test suite, but they must\n>> be added with 'test_expect_failure' instead of 'test_expect_success'.\n>> Though of course it is preferred if the new test is followed by a commit\n>> that fixes it :-)\n> \n> I did not plan to have this accepted but to amend the patch with a fix\n> later on. Also, I hoped the ready-to-apply tests would give someone else\n> a smoother start when taking over and compensate for a possibly\n> incomprehensible problem description.\n> \n>>>     We are currently special casing in `do_pick` and whether the current\n>>>     head is the sentinel commit is not a special case that would fit into\n>>>     `do_pick`'s interface description. What if we added the feature of\n>>>     creating root commits to `do_pick`, using `commit-tree` just like when\n>>>     creating the sentinel commit? We would have to add another special case\n>>>     (`test -z \"$onto\"`) to where the to-do list is put together in\n>>>     `rebase--interactive`. An empty `$onto` would imply\n>>>     \n>>>         git rev-list $orig_head\n>>>     \n>>>     to form the to-do list. The rebase comment in the commit message editor\n>>>     would have to become something similar to\n>>>     \n>>>         Rebase $shortrevisions as new history\n>>>     \n>>>     , which might be even less confusing than mentioning the hash of the\n>>>     sentinel commit.\n>>\n>> Since you are working on a hammer, I'm tempted to see this problem as a\n>> nail.  Would it make it easier to encode the special behavior into the\n>> todo list itself?:\n>>\n>>     pick --orphan 0cf23b1 New initial commit\n>>     pick 144a852 Second commit\n>>     pick 255f8de Third commit\n> \n> While I agree to enable pick to create orphan commits, I don't think a\n> user option --orphan is of much help. Firstly, does --orphan make sense\n> for any commit but the first one on the to-do list? Secondly, does\n> --orphan make sense when we are rebasing onto another branch? The second\n> point is related to the first in the sense that \"pick --orphan\" would be\n> used on a commit that is understood to have a parent.\n\n--orphan as a user option would only really make sense if we get around\nto supporting interactive rebase of arbitrary DAGs.\n\nPerhaps a more practical problem with --orphan is that it makes it\nharder for the user to change the order of the first two commits.\n\nAnother possible construct would be a separate \"orphan\" command:\n\n    orphan\n    pick 0cf23b1 New initial commit\n    pick 144a852 Second commit\n    pick 255f8de Third commit\n\nBut these are just wild ideas.  I haven't thought enough about the\nproblem to advocate anything.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"246179","messageId":"8d5cf2e1ff45e2e60072bf6c6e05371e4b265709.1405539123.git.bafain@gmail.com","threadId":"36947","inReplyTo":"53A18198.7070301@gmail.com","subject":"[PATCH v1] rebase --root: sentinel commit cloaks empty commits","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-07-16T19:32:45Z","receivedAt":"2014-07-16T19:32:45Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"git-rebase supports the option `--root` both with and without\n`--onto`. In rebase root mode it replays all commits reachable from a\nbranch on top of another branch, including the very first commit. In\ncase `--onto` is not specified, that other branch is temporarily\nprovided by creating an empty commit. When the first commit on the\nto-do list is being replayed, the so-called sentinel commit is\namended using the log message and patch of the replayed commit. Since\nthe sentinel commit is empty, this results in a replacement of the\nsentinel commit with the new root commit of the rebased branch.\n\nThe combination of `--root` and no `--onto` implies an interactive\nrebase. When `--preserve-merges` is not specified on the command\nline, git-rebase--interactive uses `--cherry-pick` with git-rev-list\nto put the initial to-do list together. The left side is given by the\nfake base and the right side by the branch being rebased. What\nhappens now is that any empty commit on the original branch is\ntreated as a cherry-pick of the sentinel commit and subsequently\nomitted from the to-do list. This is a bug if `--keep-empty` is\nspecified also.\n\nEven without `--keep-empty`, using the sentinel commit as left side\nwith git-rev-list can result in a faulty rebased branch. Indeed, in\nthe unlikely case that the original branch consists solely of empty\ncommits, the bug crops up in the strangest fashion as all commits are\nskipped and the sentinel commit is not replaced. As a result,\ngit-rebase produces a branch with a single empty commit.\n\nTo trigger the replacement of the sentinel commit, git-rebase assigns\nthe variable `squash_onto`. Special case a second time regarding\n`squash_onto` and run git-rev-list without a left side if the\nvariable is assigned. The latter is the case if and only if `--root`\nis used without `--onto`, that is `upstream` points to the sentinel\ncommit and `$upstream...$orig_head` would subtract a commit that is\nnot actually there from the original branch.\n\nFix a typo in `is_empty_commit`. It always found root commits\nnon-empty so that empty root commits were scheduled even without\n`--keep-empty`. The POSIX specification states that command\nsubstitutions are to be executed in sub-shells, which makes exit(1)\nand variable assignments not affect the script execution state. That\nwas the reason why `ptree` was null for parentless commits and the\ntest `\"$tree\" = \"$ptree\"` always false for them.\n\nAdd tests.\n\nSigned-off-by: Fabian Ruch <bafain@gmail.com>\n---\nHi,\n\nThree test cases were added to the bug report to account for the\nadditional cases in which the bug strikes (raised by Michael on the\nother sub-thread). A bugfix is included now as well.\n\nConcerning the bugfix: Obviously, the patch misuses the `squash_onto`\nflag because it assumes that the new base is empty except for the\nsentinel commit. The variable name does not imply anything close to\nthat. An additional flag to disable the use of the git-rev-list\noption `--cherry-pick` would work and make sense again (for instance,\n`keep_redundant`). However, the following two bugs, not related to\nempty commits, seem to suggest that git-rebase--interactive cannot\nwork obliviously to non-existent bases.\n\n 1) git-rebase--interactive when used with `--root` and the to-do\n    list `noop` results in the original branch's history being\n    rewritten to contain only the sentinel commit.\n\n    git-rebase--interactive correctly checkouts `$onto` and replays\n    no commits on top of it but git-rebase has forgotten that `$onto`\n    was fake.\n\n 2) git-rebase--interactive when used with `--root` always creates a\n    fresh root commit, regardless of `--no-ff` being specified.\n\n    With the current meaning of `squash_onto`,\n    git-rebase--interactive cannot just reset the branch to the old\n    root commit. It is really the fault of git-rebase to start off\n    with a new commit.\n\nPlease take a closer look at the last two test cases that specify the\nexpected behaviour of rebasing a branch that tracks the empty tree.\nAt this point they expect the \"Nothing to do\" error (aborts with\nuntouched history). This is consistent with rebasing only empty\ncommits without `--root`, which also doesn't just delete them from\nthe history. Furthermore, I think the two alternatives adding a note\nthat all commits in the range were empty, and removing the empty\ncommits (thus making the branch empty) are better discussed in a\nseparate bug report.\n\nThanks for your time,\n   Fabian\n\n git-rebase--interactive.sh | 20 ++++++++++++++-----\n t/t3412-rebase-root.sh     | 49 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 64 insertions(+), 5 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex f267d8b..71ca0f0 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -201,10 +201,10 @@ has_action () {\n }\n \n is_empty_commit() {\n-\ttree=$(git rev-parse -q --verify \"$1\"^{tree} 2>/dev/null ||\n-\t\tdie \"$1: not a commit that can be picked\")\n-\tptree=$(git rev-parse -q --verify \"$1\"^^{tree} 2>/dev/null ||\n-\t\tptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904)\n+\ttree=$(git rev-parse -q --verify \"$1\"^{tree} 2>/dev/null) ||\n+\t\tdie \"$1: not a commit that can be picked\"\n+\tptree=$(git rev-parse -q --verify \"$1\"^^{tree} 2>/dev/null) ||\n+\t\tptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n \ttest \"$tree\" = \"$ptree\"\n }\n \n@@ -958,7 +958,17 @@ then\n \trevisions=$upstream...$orig_head\n \tshortrevisions=$shortupstream..$shorthead\n else\n-\trevisions=$onto...$orig_head\n+\tif test -n \"$squash_onto\"\n+\tthen\n+\t\t# $onto points to an empty commit (the sentinel\n+\t\t# commit) which was not created by the user.\n+\t\t# Exclude it from the rev list to avoid skipping\n+\t\t# empty user commits prematurely, i. e. before\n+\t\t# --keep-empty can take effect.\n+\t\trevisions=$orig_head\n+\telse\n+\t\trevisions=$onto...$orig_head\n+\tfi\n \tshortrevisions=$shorthead\n fi\n git rev-list $merges_option --pretty=oneline --abbrev-commit \\\ndiff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh\nindex 0b52105..7c09efc 100755\n--- a/t/t3412-rebase-root.sh\n+++ b/t/t3412-rebase-root.sh\n@@ -278,4 +278,53 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' '\n \ttest_cmp expect-conflict-p out\n '\n \n+test_expect_success 'recreate empty commits with --keep-empty (root commit only)' '\n+\t# commit the empty tree, no parents\n+\tempty_tree=$(git hash-object -t tree /dev/null) &&\n+\techo Empty\\ root >expected.msg &&\n+\tempty_root_commit=$(git commit-tree $empty_tree -F expected.msg) &&\n+\tgit checkout -b empty-root-commit $empty_root_commit &&\n+\tgit rebase --root --keep-empty &&\n+\tgit show -s --pretty=format:%s%n HEAD >actual.msg &&\n+\ttest_cmp expected.msg actual.msg\n+'\n+\n+test_expect_success 'recreate empty commits with --keep-empty (root commit with child)' '\n+\tgit checkout -b empty-root-commit-with-child empty-root-commit &&\n+\t>file &&\n+\tgit add file &&\n+\tgit commit -m file &&\n+\tgit rebase --root --keep-empty &&\n+\tgit show -s --pretty=format:%s%n HEAD^ >actual.msg &&\n+\ttest_cmp expected.msg actual.msg\n+'\n+\n+test_expect_success 'recreate empty commits with --keep-empty (child commit)' '\n+\tgit checkout -b empty-child-commit other &&\n+\techo Empty\\ child >expected.msg &&\n+\tgit commit --allow-empty -F expected.msg &&\n+\tgit rebase --root --keep-empty &&\n+\tgit show -s --pretty=format:%s%n HEAD >actual.msg &&\n+\ttest_cmp expected.msg actual.msg\n+'\n+\n+test_expect_success 'abort if branch has solely empty commits without --keep-empty (single)' '\n+\tgit checkout empty-root-commit &&\n+\tgit rev-parse HEAD >expected.rev &&\n+\ttest_must_fail git rebase --root &&\n+\ttest_path_is_missing .git/rebase-merge &&\n+\tgit rev-parse HEAD >actual.rev &&\n+\ttest_cmp expected.rev actual.rev\n+'\n+\n+test_expect_success 'abort if branch has solely empty commits without --keep-empty (many)' '\n+\tgit checkout -b empty-commits-only empty-root-commit &&\n+\tgit commit --allow-empty -m Child &&\n+\tgit rev-parse HEAD >expected.rev &&\n+\ttest_must_fail git rebase --root &&\n+\ttest_path_is_missing .git/rebase-merge &&\n+\tgit rev-parse HEAD >actual.rev &&\n+\ttest_cmp expected.rev actual.rev\n+'\n+\n test_done\n-- \n2.0.1\n"},{"id":"246306","messageId":"871tti50l8.fsf@thomasrast.ch","threadId":"36947","inReplyTo":"8d5cf2e1ff45e2e60072bf6c6e05371e4b265709.1405539123.git.bafain@gmail.com","subject":"Re: [PATCH v1] rebase --root: sentinel commit cloaks empty commits","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2014-07-18T12:10:43Z","receivedAt":"2014-07-18T12:10:43Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Hi Fabian\n\nImpressive analysis!\n\n> Concerning the bugfix: Obviously, the patch misuses the `squash_onto`\n> flag because it assumes that the new base is empty except for the\n> sentinel commit. The variable name does not imply anything close to\n> that. An additional flag to disable the use of the git-rev-list\n> option `--cherry-pick` would work and make sense again (for instance,\n> `keep_redundant`).\n\nSeeing as there are only two existing uses of the variable, you could\nalso rename it to make it more obvious what is going on.  I think either\nway is fine.\n\n[...]\n> Please take a closer look at the last two test cases that specify the\n> expected behaviour of rebasing a branch that tracks the empty tree.\n> At this point they expect the \"Nothing to do\" error (aborts with\n> untouched history). This is consistent with rebasing only empty\n> commits without `--root`, which also doesn't just delete them from\n> the history. Furthermore, I think the two alternatives adding a note\n> that all commits in the range were empty, and removing the empty\n> commits (thus making the branch empty) are better discussed in a\n> separate bug report.\n\nMakes sense to me, though I have never thought much about rebasing empty\ncommits.  Maybe Chris has a more informed opinion?\n\n>  is_empty_commit() {\n> -\ttree=$(git rev-parse -q --verify \"$1\"^{tree} 2>/dev/null ||\n> -\t\tdie \"$1: not a commit that can be picked\")\n> -\tptree=$(git rev-parse -q --verify \"$1\"^^{tree} 2>/dev/null ||\n> -\t\tptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904)\n> +\ttree=$(git rev-parse -q --verify \"$1\"^{tree} 2>/dev/null) ||\n> +\t\tdie \"$1: not a commit that can be picked\"\n> +\tptree=$(git rev-parse -q --verify \"$1\"^^{tree} 2>/dev/null) ||\n> +\t\tptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n>  \ttest \"$tree\" = \"$ptree\"\n>  }\n\nNice catch!\n\n> @@ -958,7 +958,17 @@ then\n>  \trevisions=$upstream...$orig_head\n>  \tshortrevisions=$shortupstream..$shorthead\n>  else\n> -\trevisions=$onto...$orig_head\n> +\tif test -n \"$squash_onto\"\n> +\tthen\n> +\t\t# $onto points to an empty commit (the sentinel\n> +\t\t# commit) which was not created by the user.\n> +\t\t# Exclude it from the rev list to avoid skipping\n> +\t\t# empty user commits prematurely, i. e. before\n> +\t\t# --keep-empty can take effect.\n> +\t\trevisions=$orig_head\n> +\telse\n> +\t\trevisions=$onto...$orig_head\n> +\tfi\n>  \tshortrevisions=$shorthead\n\nNit: I think this would be clearer if you phrased it using an 'elif',\ninstead of nesting (but keep the comment!).\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"246425","messageId":"291ABB60-4B66-4211-A561-048F10089B82@arachsys.com","threadId":"36947","inReplyTo":"871tti50l8.fsf@thomasrast.ch","subject":"Re: [PATCH v1] rebase --root: sentinel commit cloaks empty commits","fromName":"Chris Webb","fromEmail":"chris@arachsys.com","sentAt":"2014-07-20T20:52:06Z","receivedAt":"2014-07-20T20:52:06Z","isPatch":true,"sender":{"key":"chris@arachsys.com","avatar":"https://avatars.githubusercontent.com/u/299056?v=4"},"body":"Thomas Rast <tr@thomasrast.ch> wrote:\n\n>> Please take a closer look at the last two test cases that specify the\n>> expected behaviour of rebasing a branch that tracks the empty tree.\n>> At this point they expect the \"Nothing to do\" error (aborts with\n>> untouched history). This is consistent with rebasing only empty\n>> commits without `--root`, which also doesn't just delete them from\n>> the history. Furthermore, I think the two alternatives adding a note\n>> that all commits in the range were empty, and removing the empty\n>> commits (thus making the branch empty) are better discussed in a\n>> separate bug report.\n> \n> Makes sense to me, though I have never thought much about rebasing empty\n> commits.  Maybe Chris has a more informed opinion?\n\nI definitely agree with you both that --root should be (and isn't)\nconsistent with normal interactive rebasing. The difference isn't deliberate\non my part.\n\nOn a personal note, I've always disliked the way interactive rebase stops\nwhen you pick an existing empty commit or empty log message rather than\npreserving it. Jumping through a few hoops is perhaps sensible when you\ncreate that kind of strange commit, but just annoying when picking an\nexisting empty/logless commit as part of a series. But as you say, that's\na separate issue than --root behaving differently to non --root.\n\nCheers,\n\nChris.\n"}]}