{"thread":{"id":"50994","subject":"[PATCH 0/3] checkout: allow -b/-B to work on a merge base","startedAt":"2019-04-25T21:10:38Z","lastAt":"2019-04-27T12:02:30Z","messageCount":21,"participants":["Denton Liu","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"374526","messageId":"cover.1556226502.git.liu.denton@gmail.com","threadId":"50994","inReplyTo":null,"subject":"[PATCH 0/3] checkout: allow -b/-B to work on a merge base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-25T21:10:34Z","receivedAt":"2019-04-25T21:10:38Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"I noticed earlier that running\n\n\t$ git checkout -b test master...\n\nwould result in the following failure message:\n\n\tfatal: Not a valid object name: 'master...'.\n\nI believe that this is a bug, so this patchset will allow this to succeed.\n\nDenton Liu (3):\n  t2018: cleanup in current test\n  t2018: demonstrate checkout -b merge base bug\n  checkout: allow -b/-B to work on a merge base\n\n builtin/checkout.c         |  4 ++-\n t/t2018-checkout-branch.sh | 52 +++++++++++++++++++-------------------\n 2 files changed, 29 insertions(+), 27 deletions(-)\n\n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374527","messageId":"c0c7171e3d523e5d4a0ac01810378447a38854da.1556226502.git.liu.denton@gmail.com","threadId":"50994","inReplyTo":"cover.1556226502.git.liu.denton@gmail.com","subject":"[PATCH 1/3] t2018: cleanup in current test","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-25T21:10:36Z","receivedAt":"2019-04-25T21:10:39Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, in t2018, if do_checkout failed to create `branch2`, the next\ntest-case would run `git branch -D branch2` but then fail because it was\nexpecting `branch2` to exist, even though it doesn't. As a result, an\nearly failure could cause a cascading failure of tests.\n\nMake test-case responsible for cleaning up their own branches so that\nfuture tests can start with a sane environment.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n t/t2018-checkout-branch.sh | 39 +++++++++++++-------------------------\n 1 file changed, 13 insertions(+), 26 deletions(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex c5014ad9a6..fdb7fd282d 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -60,38 +60,36 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'checkout -b to a new branch, set to HEAD' '\n+\ttest_when_finished test_might_fail git branch -D branch2 &&\n+\ttest_when_finished git checkout branch1 &&\n \tdo_checkout branch2\n '\n \n test_expect_success 'checkout -b to a new branch, set to an explicit ref' '\n-\tgit checkout branch1 &&\n-\tgit branch -D branch2 &&\n-\n+\ttest_when_finished test_might_fail git branch -D branch2 &&\n+\ttest_when_finished git checkout branch1 &&\n \tdo_checkout branch2 $HEAD1\n '\n \n test_expect_success 'checkout -b to a new branch with unmergeable changes fails' '\n-\tgit checkout branch1 &&\n-\n-\t# clean up from previous test\n-\tgit branch -D branch2 &&\n-\n \tsetup_dirty_unmergeable &&\n \ttest_must_fail do_checkout branch2 $HEAD1 &&\n \ttest_dirty_unmergeable\n '\n \n test_expect_success 'checkout -f -b to a new branch with unmergeable changes discards changes' '\n+\ttest_when_finished test_might_fail git branch -D branch2 &&\n+\ttest_when_finished git checkout branch1 &&\n+\n \t# still dirty and on branch1\n \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n \ttest_must_fail test_dirty_unmergeable\n '\n \n test_expect_success 'checkout -b to a new branch preserves mergeable changes' '\n-\tgit checkout branch1 &&\n-\n-\t# clean up from previous test\n-\tgit branch -D branch2 &&\n+\ttest_when_finished test_might_fail git branch -D branch2 &&\n+\ttest_when_finished git checkout branch1 &&\n+\ttest_when_finished git reset --hard &&\n \n \tsetup_dirty_mergeable &&\n \tdo_checkout branch2 $HEAD1 &&\n@@ -99,27 +97,18 @@ test_expect_success 'checkout -b to a new branch preserves mergeable changes' '\n '\n \n test_expect_success 'checkout -f -b to a new branch with mergeable changes discards changes' '\n-\t# clean up from previous test\n-\tgit reset --hard &&\n-\n-\tgit checkout branch1 &&\n-\n-\t# clean up from previous test\n-\tgit branch -D branch2 &&\n-\n+\ttest_when_finished git reset --hard HEAD &&\n \tsetup_dirty_mergeable &&\n \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n \ttest_must_fail test_dirty_mergeable\n '\n \n test_expect_success 'checkout -b to an existing branch fails' '\n-\tgit reset --hard HEAD &&\n-\n+\ttest_when_finished git reset --hard HEAD &&\n \ttest_must_fail do_checkout branch2 $HEAD2\n '\n \n test_expect_success 'checkout -b to @{-1} fails with the right branch name' '\n-\tgit reset --hard HEAD &&\n \tgit checkout branch1 &&\n \tgit checkout branch2 &&\n \techo  >expect \"fatal: A branch named '\\''branch1'\\'' already exists.\" &&\n@@ -160,6 +149,7 @@ test_expect_success 'checkout -f -B to an existing branch with unmergeable chang\n '\n \n test_expect_success 'checkout -B to an existing branch preserves mergeable changes' '\n+\ttest_when_finished git reset --hard &&\n \tgit checkout branch1 &&\n \n \tsetup_dirty_mergeable &&\n@@ -168,9 +158,6 @@ test_expect_success 'checkout -B to an existing branch preserves mergeable chang\n '\n \n test_expect_success 'checkout -f -B to an existing branch with mergeable changes discards changes' '\n-\t# clean up from previous test\n-\tgit reset --hard &&\n-\n \tgit checkout branch1 &&\n \n \tsetup_dirty_mergeable &&\n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374528","messageId":"ff38bdb5649c3bb028a5227bb0f9569073b8a500.1556226502.git.liu.denton@gmail.com","threadId":"50994","inReplyTo":"cover.1556226502.git.liu.denton@gmail.com","subject":"[PATCH 2/3] t2018: demonstrate checkout -b merge base bug","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-25T21:10:37Z","receivedAt":"2019-04-25T21:10:41Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-checkout.txt, it states\n\n\tAs a special case, you may use `\"A...B\"` as a shortcut for the\n\tmerge base of `A` and `B` if there is exactly one merge base. You can\n\tleave out at most one of `A` and `B`, in which case it defaults to `HEAD`.\n\nHowever, there exists a bug where performing\n\n\t$ git checkout -b test master...\n\nfails with the message\n\n\tfatal: Not a valid object name: 'master...'.\n\nDemonstrate this failure so that it can be corrected later.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n t/t2018-checkout-branch.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex fdb7fd282d..a3fa520d2e 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -65,6 +65,12 @@ test_expect_success 'checkout -b to a new branch, set to HEAD' '\n \tdo_checkout branch2\n '\n \n+test_expect_failure 'checkout -b to a merge base' '\n+\ttest_when_finished test_might_fail git branch -D branch2 &&\n+\ttest_when_finished git checkout branch1 &&\n+\tgit checkout -b branch2 branch1...\n+'\n+\n test_expect_success 'checkout -b to a new branch, set to an explicit ref' '\n \ttest_when_finished test_might_fail git branch -D branch2 &&\n \ttest_when_finished git checkout branch1 &&\n@@ -122,6 +128,13 @@ test_expect_success 'checkout -B to an existing branch resets branch to HEAD' '\n \tdo_checkout branch2 \"\" -B\n '\n \n+test_expect_failure 'checkout -B to a merge base' '\n+\tgit checkout branch1 &&\n+\tgit branch -D branch2 &&\n+\n+\tgit checkout -B branch2 branch1...\n+'\n+\n test_expect_success 'checkout -B to an existing branch from detached HEAD resets branch to HEAD' '\n \tgit checkout $(git rev-parse --verify HEAD) &&\n \n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374529","messageId":"031780431d790c16b3862d6f155693e197bb74ed.1556226502.git.liu.denton@gmail.com","threadId":"50994","inReplyTo":"cover.1556226502.git.liu.denton@gmail.com","subject":"[PATCH 3/3] checkout: allow -b/-B to work on a merge base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-25T21:10:38Z","receivedAt":"2019-04-25T21:10:43Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When we ran something like\n\n    $ git checkout -b test master...\n\nit would fail with the message\n\n    fatal: Not a valid object name: 'master...'.\n\nThis was caused by the call to `create_branch` where `start_name` is\nexpected to be a valid rev. However, git-checkout allows the branch to\nnot be a valid rev in the case where we have \"...\" to specify getting\nthe merge base.\n\nIn the case where a branch with \"...\" is specified, use the oid instead\nso that `start_name` is a valid rev.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n builtin/checkout.c         | 4 +++-\n t/t2018-checkout-branch.sh | 4 ++--\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex ffa776c6e1..d99b3f3925 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1229,7 +1229,9 @@ static int parse_branchname_arg(int argc, const char **argv,\n \targv++;\n \targc--;\n \n-\tnew_branch_info->name = arg;\n+\tnew_branch_info->name = strstr(arg, \"...\") ?\n+\t\txstrdup(oid_to_hex(rev)) :\n+\t\targ;\n \tsetup_branch_path(new_branch_info);\n \n \tif (!check_refname_format(new_branch_info->path, 0) &&\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex a3fa520d2e..d6ea556d84 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -65,7 +65,7 @@ test_expect_success 'checkout -b to a new branch, set to HEAD' '\n \tdo_checkout branch2\n '\n \n-test_expect_failure 'checkout -b to a merge base' '\n+test_expect_success 'checkout -b to a merge base' '\n \ttest_when_finished test_might_fail git branch -D branch2 &&\n \ttest_when_finished git checkout branch1 &&\n \tgit checkout -b branch2 branch1...\n@@ -128,7 +128,7 @@ test_expect_success 'checkout -B to an existing branch resets branch to HEAD' '\n \tdo_checkout branch2 \"\" -B\n '\n \n-test_expect_failure 'checkout -B to a merge base' '\n+test_expect_success 'checkout -B to a merge base' '\n \tgit checkout branch1 &&\n \tgit branch -D branch2 &&\n \n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374533","messageId":"CAPig+cTdJv9-M+zmp+Jo2bjOrkO3NgsguJ2xM+aXhf38OjjEEw@mail.gmail.com","threadId":"50994","inReplyTo":"c0c7171e3d523e5d4a0ac01810378447a38854da.1556226502.git.liu.denton@gmail.com","subject":"Re: [PATCH 1/3] t2018: cleanup in current test","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-04-25T22:34:07Z","receivedAt":"2019-04-25T22:34:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Apr 25, 2019 at 5:10 PM Denton Liu <liu.denton@gmail.com> wrote:\n> Before, in t2018, if do_checkout failed to create `branch2`, the next\n> test-case would run `git branch -D branch2` but then fail because it was\n> expecting `branch2` to exist, even though it doesn't. As a result, an\n> early failure could cause a cascading failure of tests.\n>\n> Make test-case responsible for cleaning up their own branches so that\n> future tests can start with a sane environment.\n>\n> Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> ---\n> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\n> @@ -60,38 +60,36 @@ test_expect_success 'setup' '\n>  test_expect_success 'checkout -b to a new branch, set to HEAD' '\n> +       test_when_finished test_might_fail git branch -D branch2 &&\n> +       test_when_finished git checkout branch1 &&\n\nI'm aware that when-finished actions fire in reverse order but the\ninherent subtlety of ordering of these two invocations still caught me\noff-guard for a moment since they are reverse the order in which one\nlogically thinks about the actions which need to be performed. I\nwonder if it would be easier to digest if written like this:\n\n    test_when_finished '\n        git checkout branch1 &&\n        test_might_fail git branch -D branch2\n    ' &&\n\n(Probably not worth a re-roll.)\n"},{"id":"374534","messageId":"20190426004032.GA19743@dev-l","threadId":"50994","inReplyTo":"CAPig+cTdJv9-M+zmp+Jo2bjOrkO3NgsguJ2xM+aXhf38OjjEEw@mail.gmail.com","subject":"Re: [PATCH 1/3] t2018: cleanup in current test","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-26T00:40:32Z","receivedAt":"2019-04-26T00:40:37Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Thu, Apr 25, 2019 at 06:34:07PM -0400, Eric Sunshine wrote:\n> On Thu, Apr 25, 2019 at 5:10 PM Denton Liu <liu.denton@gmail.com> wrote:\n> > Before, in t2018, if do_checkout failed to create `branch2`, the next\n> > test-case would run `git branch -D branch2` but then fail because it was\n> > expecting `branch2` to exist, even though it doesn't. As a result, an\n> > early failure could cause a cascading failure of tests.\n> >\n> > Make test-case responsible for cleaning up their own branches so that\n> > future tests can start with a sane environment.\n> >\n> > Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> > ---\n> > diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\n> > @@ -60,38 +60,36 @@ test_expect_success 'setup' '\n> >  test_expect_success 'checkout -b to a new branch, set to HEAD' '\n> > +       test_when_finished test_might_fail git branch -D branch2 &&\n> > +       test_when_finished git checkout branch1 &&\n> \n> I'm aware that when-finished actions fire in reverse order but the\n> inherent subtlety of ordering of these two invocations still caught me\n> off-guard for a moment since they are reverse the order in which one\n> logically thinks about the actions which need to be performed. I\n> wonder if it would be easier to digest if written like this:\n> \n>     test_when_finished '\n>         git checkout branch1 &&\n>         test_might_fail git branch -D branch2\n>     ' &&\n> \n> (Probably not worth a re-roll.)\n\nThis sounds like a good idea. If Junio doesn't get around to it, I'll\nsend a v1.5 of this patch later. Thanks for the suggestion!\n"},{"id":"374540","messageId":"xmqqtvelv5t0.fsf@gitster-ct.c.googlers.com","threadId":"50994","inReplyTo":"CAPig+cTdJv9-M+zmp+Jo2bjOrkO3NgsguJ2xM+aXhf38OjjEEw@mail.gmail.com","subject":"Re: [PATCH 1/3] t2018: cleanup in current test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-26T02:50:03Z","receivedAt":"2019-04-26T02:50:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>>  test_expect_success 'checkout -b to a new branch, set to HEAD' '\n>> +       test_when_finished test_might_fail git branch -D branch2 &&\n>> +       test_when_finished git checkout branch1 &&\n>\n> I'm aware that when-finished actions fire in reverse order but the\n> inherent subtlety of ordering of these two invocations still caught me\n> off-guard for a moment since they are reverse the order in which one\n> logically thinks about the actions which need to be performed. I\n> wonder if it would be easier to digest if written like this:\n>\n>     test_when_finished '\n>         git checkout branch1 &&\n>         test_might_fail git branch -D branch2\n>     ' &&\n\nPerhaps.  Be careful in choosing the quotes between sq and dq,\nthough.\n"},{"id":"374541","messageId":"xmqqmukdv57q.fsf@gitster-ct.c.googlers.com","threadId":"50994","inReplyTo":"031780431d790c16b3862d6f155693e197bb74ed.1556226502.git.liu.denton@gmail.com","subject":"Re: [PATCH 3/3] checkout: allow -b/-B to work on a merge base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-26T03:02:49Z","receivedAt":"2019-04-26T03:02:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> -\tnew_branch_info->name = arg;\n> +\tnew_branch_info->name = strstr(arg, \"...\") ?\n> +\t\txstrdup(oid_to_hex(rev)) :\n> +\t\targ;\n\nCan we do better?\n\nI am not sure why we want to hardcode the knowledge of \"...\" syntax\nlike this here.  \"git checkout A...B\" introduced in 2009 needed only\na single-liner change from get_sha1() to get_sha1_mb() without making\nthe ugly implementation detail seep into this layer.\n"},{"id":"374543","messageId":"xmqqh8aluzsu.fsf@gitster-ct.c.googlers.com","threadId":"50994","inReplyTo":"xmqqmukdv57q.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] checkout: allow -b/-B to work on a merge base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-26T04:59:45Z","receivedAt":"2019-04-26T04:59:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Denton Liu <liu.denton@gmail.com> writes:\n>\n>> -\tnew_branch_info->name = arg;\n>> +\tnew_branch_info->name = strstr(arg, \"...\") ?\n>> +\t\txstrdup(oid_to_hex(rev)) :\n>> +\t\targ;\n>\n> Can we do better?\n>\n> I am not sure why we want to hardcode the knowledge of \"...\" syntax\n> like this here.  \"git checkout A...B\" introduced in 2009 needed only\n> a single-liner change from get_sha1() to get_sha1_mb() without making\n> the ugly implementation detail seep into this layer.\n\nI _think_ what you are trying to work around is that a syntax that\nis not understood as a reference to a single revision by get_oid()\nbut is understood as such by get_oid_mb() can be in the .name field,\nand that eventually gets passed to branch.c::create_branch() as\n\"start_name\" in the codepath.  The function ought to know that\nstart_name wants to name a single revision and never a revision\nrange, but fails to use get_oid_mb() but just a plain get_oid(),\nand fails.\n\nIf that is the case, wouldn't a better fix be more like the\nattached?  This hasn't even been compile tested, but I suspect that\nwithout taking this approach, you would introduce a new \"bug\",\nnamely, that\n\n\t$ git checkout -b newbranch master...\n\nought to behave exactly like\n\n\t$ git branch newbranch master...\n\t$ git checkout newbranch\n\nBut the first step would hit the same branch.c::create_branch()\nand would not work, no?\n\nThe reason I care about hiding syntax details like \"...\" from users\nof get_*_mb() is because I anticipate that we'll want to extend it\nto things other than just \"merge base between two\" with syntax\ndifferent from \"...\" (while probably renaming _mb suffix to\nsomething else).\n\nThe codebase is not ready to replace get_oid() with get_oid_mb()\nblindly and wholesale, I think, as the former is used as an\nimplementation detail of parsing range syntax like A..B but places\nthat are end user facing *and* expect to take only a single revision\n(e.g. \"rebase --onto <commit>\", \"checkout <commit>\", etc.) and never\na range that currently use get_oid() should be able to get replaced\nwith get_oid_mb() to learn \"A...B means their merge base, not a\nsymmetric range\" semantics for free.\n\n branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/branch.c b/branch.c\nindex 28b81a7e02..a84c8aaca2 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -268,7 +268,7 @@ void create_branch(struct repository *r,\n \t}\n \n \treal_ref = NULL;\n-\tif (get_oid(start_name, &oid)) {\n+\tif (get_oid_mb(start_name, &oid)) {\n \t\tif (explicit_tracking) {\n \t\t\tif (advice_set_upstream_failure) {\n \t\t\t\terror(_(upstream_missing), start_name);\n\n"},{"id":"374548","messageId":"CAPig+cQNF1LSwBO_hArCfPGdy-Y=mhMYbEO3eOY0Z6xNW9AJbw@mail.gmail.com","threadId":"50994","inReplyTo":"xmqqtvelv5t0.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] t2018: cleanup in current test","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-04-26T06:58:25Z","receivedAt":"2019-04-26T06:58:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Apr 25, 2019 at 10:50 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> >     test_when_finished '\n> >         git checkout branch1 &&\n> >         test_might_fail git branch -D branch2\n> >     ' &&\n>\n> Perhaps.  Be careful in choosing the quotes between sq and dq,\n> though.\n\nYep, thanks for catching my mistake. Definitely want double-quote, not\nsingle-quote.\n"},{"id":"374569","messageId":"cover.1556305561.git.liu.denton@gmail.com","threadId":"50994","inReplyTo":"cover.1556226502.git.liu.denton@gmail.com","subject":"[PATCH v2 0/3] allow checkout and branch to create branches on a merge base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-26T19:21:06Z","receivedAt":"2019-04-26T19:21:11Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Thanks for your comments, Eric and Junio.\n\nEric, I've combined the `test_when_finished` calls together so that the\nstatements within appear in a more \"logical\" order.\n\nJunio, I've taken your suggestion and moved the change into\n`create_branch`. Initially, I didn't want to do this because I didn't\nwant to change the semantics of git-branch but introducing the merge\nbase syntax seems to be a positive change so let's do it.\n\nThanks,\n\nDenton\n\n---\n\nChanges since v1:\n\n* Moved multiple `test_when_finished` calls that appeared in \"reverse\n  order\" into one call that appears in the logical order\n* Made create_branch handle merge base revs instead of putting a hack\n  into checkout\n\nDenton Liu (3):\n  t2018: cleanup in current test\n  t2018: demonstrate checkout -b merge base bug\n  branch: make create_branch accept a merge base rev\n\n Documentation/git-branch.txt |  6 +++-\n branch.c                     |  2 +-\n t/t2018-checkout-branch.sh   | 56 +++++++++++++++++++-----------------\n t/t3200-branch.sh            | 14 ++++++---\n 4 files changed, 46 insertions(+), 32 deletions(-)\n\nRange-diff against v1:\n1:  c0c7171e3d ! 1:  9d04faf29d t2018: cleanup in current test\n    @@ -19,8 +19,9 @@\n      '\n      \n      test_expect_success 'checkout -b to a new branch, set to HEAD' '\n    -+\ttest_when_finished test_might_fail git branch -D branch2 &&\n    -+\ttest_when_finished git checkout branch1 &&\n    ++\ttest_when_finished \"\n    ++\t\tgit checkout branch1 &&\n    ++\t\ttest_might_fail git branch -D branch2\" &&\n      \tdo_checkout branch2\n      '\n      \n    @@ -28,8 +29,9 @@\n     -\tgit checkout branch1 &&\n     -\tgit branch -D branch2 &&\n     -\n    -+\ttest_when_finished test_might_fail git branch -D branch2 &&\n    -+\ttest_when_finished git checkout branch1 &&\n    ++\ttest_when_finished \"\n    ++\t\tgit checkout branch1 &&\n    ++\t\ttest_might_fail git branch -D branch2\" &&\n      \tdo_checkout branch2 $HEAD1\n      '\n      \n    @@ -45,8 +47,9 @@\n      '\n      \n      test_expect_success 'checkout -f -b to a new branch with unmergeable changes discards changes' '\n    -+\ttest_when_finished test_might_fail git branch -D branch2 &&\n    -+\ttest_when_finished git checkout branch1 &&\n    ++\ttest_when_finished \"\n    ++\t\tgit checkout branch1 &&\n    ++\t\ttest_might_fail git branch -D branch2\" &&\n     +\n      \t# still dirty and on branch1\n      \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n    @@ -58,9 +61,10 @@\n     -\n     -\t# clean up from previous test\n     -\tgit branch -D branch2 &&\n    -+\ttest_when_finished test_might_fail git branch -D branch2 &&\n    -+\ttest_when_finished git checkout branch1 &&\n    -+\ttest_when_finished git reset --hard &&\n    ++\ttest_when_finished \"\n    ++\t\tgit reset --hard &&\n    ++\t\tgit checkout branch1 &&\n    ++\t\ttest_might_fail git branch -D branch2\" &&\n      \n      \tsetup_dirty_mergeable &&\n      \tdo_checkout branch2 $HEAD1 &&\n2:  ff38bdb564 ! 2:  5e8320cd80 t2018: demonstrate checkout -b merge base bug\n    @@ -28,21 +28,21 @@\n      '\n      \n     +test_expect_failure 'checkout -b to a merge base' '\n    -+\ttest_when_finished test_might_fail git branch -D branch2 &&\n    -+\ttest_when_finished git checkout branch1 &&\n    ++\ttest_when_finished \"\n    ++\t\tgit checkout branch1 &&\n    ++\t\ttest_might_fail git branch -D branch2\" &&\n     +\tgit checkout -b branch2 branch1...\n     +'\n     +\n      test_expect_success 'checkout -b to a new branch, set to an explicit ref' '\n    - \ttest_when_finished test_might_fail git branch -D branch2 &&\n    - \ttest_when_finished git checkout branch1 &&\n    + \ttest_when_finished \"\n    + \t\tgit checkout branch1 &&\n     @@\n      \tdo_checkout branch2 \"\" -B\n      '\n      \n     +test_expect_failure 'checkout -B to a merge base' '\n     +\tgit checkout branch1 &&\n    -+\tgit branch -D branch2 &&\n     +\n     +\tgit checkout -B branch2 branch1...\n     +'\n3:  031780431d < -:  ---------- checkout: allow -b/-B to work on a merge base\n-:  ---------- > 3:  c91c7535a7 branch: make create_branch accept a merge base rev\n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374570","messageId":"9d04faf29dd61eb04ea292786d72eda0a3832e51.1556305561.git.liu.denton@gmail.com","threadId":"50994","inReplyTo":"cover.1556305561.git.liu.denton@gmail.com","subject":"[PATCH v2 1/3] t2018: cleanup in current test","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-26T19:21:08Z","receivedAt":"2019-04-26T19:21:12Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, in t2018, if do_checkout failed to create `branch2`, the next\ntest-case would run `git branch -D branch2` but then fail because it was\nexpecting `branch2` to exist, even though it doesn't. As a result, an\nearly failure could cause a cascading failure of tests.\n\nMake test-case responsible for cleaning up their own branches so that\nfuture tests can start with a sane environment.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n t/t2018-checkout-branch.sh | 43 +++++++++++++++-----------------------\n 1 file changed, 17 insertions(+), 26 deletions(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex c5014ad9a6..f1c7023e1a 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -60,38 +60,40 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'checkout -b to a new branch, set to HEAD' '\n+\ttest_when_finished \"\n+\t\tgit checkout branch1 &&\n+\t\ttest_might_fail git branch -D branch2\" &&\n \tdo_checkout branch2\n '\n \n test_expect_success 'checkout -b to a new branch, set to an explicit ref' '\n-\tgit checkout branch1 &&\n-\tgit branch -D branch2 &&\n-\n+\ttest_when_finished \"\n+\t\tgit checkout branch1 &&\n+\t\ttest_might_fail git branch -D branch2\" &&\n \tdo_checkout branch2 $HEAD1\n '\n \n test_expect_success 'checkout -b to a new branch with unmergeable changes fails' '\n-\tgit checkout branch1 &&\n-\n-\t# clean up from previous test\n-\tgit branch -D branch2 &&\n-\n \tsetup_dirty_unmergeable &&\n \ttest_must_fail do_checkout branch2 $HEAD1 &&\n \ttest_dirty_unmergeable\n '\n \n test_expect_success 'checkout -f -b to a new branch with unmergeable changes discards changes' '\n+\ttest_when_finished \"\n+\t\tgit checkout branch1 &&\n+\t\ttest_might_fail git branch -D branch2\" &&\n+\n \t# still dirty and on branch1\n \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n \ttest_must_fail test_dirty_unmergeable\n '\n \n test_expect_success 'checkout -b to a new branch preserves mergeable changes' '\n-\tgit checkout branch1 &&\n-\n-\t# clean up from previous test\n-\tgit branch -D branch2 &&\n+\ttest_when_finished \"\n+\t\tgit reset --hard &&\n+\t\tgit checkout branch1 &&\n+\t\ttest_might_fail git branch -D branch2\" &&\n \n \tsetup_dirty_mergeable &&\n \tdo_checkout branch2 $HEAD1 &&\n@@ -99,27 +101,18 @@ test_expect_success 'checkout -b to a new branch preserves mergeable changes' '\n '\n \n test_expect_success 'checkout -f -b to a new branch with mergeable changes discards changes' '\n-\t# clean up from previous test\n-\tgit reset --hard &&\n-\n-\tgit checkout branch1 &&\n-\n-\t# clean up from previous test\n-\tgit branch -D branch2 &&\n-\n+\ttest_when_finished git reset --hard HEAD &&\n \tsetup_dirty_mergeable &&\n \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n \ttest_must_fail test_dirty_mergeable\n '\n \n test_expect_success 'checkout -b to an existing branch fails' '\n-\tgit reset --hard HEAD &&\n-\n+\ttest_when_finished git reset --hard HEAD &&\n \ttest_must_fail do_checkout branch2 $HEAD2\n '\n \n test_expect_success 'checkout -b to @{-1} fails with the right branch name' '\n-\tgit reset --hard HEAD &&\n \tgit checkout branch1 &&\n \tgit checkout branch2 &&\n \techo  >expect \"fatal: A branch named '\\''branch1'\\'' already exists.\" &&\n@@ -160,6 +153,7 @@ test_expect_success 'checkout -f -B to an existing branch with unmergeable chang\n '\n \n test_expect_success 'checkout -B to an existing branch preserves mergeable changes' '\n+\ttest_when_finished git reset --hard &&\n \tgit checkout branch1 &&\n \n \tsetup_dirty_mergeable &&\n@@ -168,9 +162,6 @@ test_expect_success 'checkout -B to an existing branch preserves mergeable chang\n '\n \n test_expect_success 'checkout -f -B to an existing branch with mergeable changes discards changes' '\n-\t# clean up from previous test\n-\tgit reset --hard &&\n-\n \tgit checkout branch1 &&\n \n \tsetup_dirty_mergeable &&\n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374571","messageId":"5e8320cd80415f50a79815b9fcd55c051e45e38b.1556305561.git.liu.denton@gmail.com","threadId":"50994","inReplyTo":"cover.1556305561.git.liu.denton@gmail.com","subject":"[PATCH v2 2/3] t2018: demonstrate checkout -b merge base bug","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-26T19:21:09Z","receivedAt":"2019-04-26T19:21:14Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-checkout.txt, it states\n\n\tAs a special case, you may use `\"A...B\"` as a shortcut for the\n\tmerge base of `A` and `B` if there is exactly one merge base. You can\n\tleave out at most one of `A` and `B`, in which case it defaults to `HEAD`.\n\nHowever, there exists a bug where performing\n\n\t$ git checkout -b test master...\n\nfails with the message\n\n\tfatal: Not a valid object name: 'master...'.\n\nDemonstrate this failure so that it can be corrected later.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n t/t2018-checkout-branch.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex f1c7023e1a..3cd142657a 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -66,6 +66,13 @@ test_expect_success 'checkout -b to a new branch, set to HEAD' '\n \tdo_checkout branch2\n '\n \n+test_expect_failure 'checkout -b to a merge base' '\n+\ttest_when_finished \"\n+\t\tgit checkout branch1 &&\n+\t\ttest_might_fail git branch -D branch2\" &&\n+\tgit checkout -b branch2 branch1...\n+'\n+\n test_expect_success 'checkout -b to a new branch, set to an explicit ref' '\n \ttest_when_finished \"\n \t\tgit checkout branch1 &&\n@@ -126,6 +133,12 @@ test_expect_success 'checkout -B to an existing branch resets branch to HEAD' '\n \tdo_checkout branch2 \"\" -B\n '\n \n+test_expect_failure 'checkout -B to a merge base' '\n+\tgit checkout branch1 &&\n+\n+\tgit checkout -B branch2 branch1...\n+'\n+\n test_expect_success 'checkout -B to an existing branch from detached HEAD resets branch to HEAD' '\n \tgit checkout $(git rev-parse --verify HEAD) &&\n \n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374572","messageId":"c91c7535a7908febb70499e570e1db3209dd465f.1556305561.git.liu.denton@gmail.com","threadId":"50994","inReplyTo":"cover.1556305561.git.liu.denton@gmail.com","subject":"[PATCH v2 3/3] branch: make create_branch accept a merge base rev","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-26T19:21:11Z","receivedAt":"2019-04-26T19:21:15Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When we ran something like\n\n    $ git checkout -b test master...\n\nit would fail with the message\n\n    fatal: Not a valid object name: 'master...'.\n\nThis was caused by the call to `create_branch` where `start_name` is\nexpected to be a valid rev. However, git-checkout allows the branch to\nbe a valid _merge base_ rev (i.e. with a \"...\") so it was possible for\nan invalid rev to be passed in.\n\nMake `create_branch` accept a merge base rev so that this case does not\nerror out.\n\nAs a side-effect, teach git-branch how to handle merge base revs as\nwell.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n Documentation/git-branch.txt |  6 +++++-\n branch.c                     |  2 +-\n t/t2018-checkout-branch.sh   |  4 ++--\n t/t3200-branch.sh            | 14 ++++++++++----\n 4 files changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 0cd87ddeff..6ebd512b4f 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -45,7 +45,11 @@ argument is missing it defaults to `HEAD` (i.e. the tip of the current\n branch).\n \n The command's second form creates a new branch head named <branchname>\n-which points to the current `HEAD`, or <start-point> if given.\n+which points to the current `HEAD`, or <start-point> if given. As a\n+special case, for <start-point>, you may use `\"A...B\"` as a shortcut for\n+the merge base of `A` and `B` if there is exactly one merge base. You\n+can leave out at most one of `A` and `B`, in which case it defaults to\n+`HEAD`.\n \n Note that this will create the new branch, but it will not switch the\n working tree to it; use \"git checkout <newbranch>\" to switch to the\ndiff --git a/branch.c b/branch.c\nindex 28b81a7e02..a84c8aaca2 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -268,7 +268,7 @@ void create_branch(struct repository *r,\n \t}\n \n \treal_ref = NULL;\n-\tif (get_oid(start_name, &oid)) {\n+\tif (get_oid_mb(start_name, &oid)) {\n \t\tif (explicit_tracking) {\n \t\t\tif (advice_set_upstream_failure) {\n \t\t\t\terror(_(upstream_missing), start_name);\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 3cd142657a..822381dd9d 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -66,7 +66,7 @@ test_expect_success 'checkout -b to a new branch, set to HEAD' '\n \tdo_checkout branch2\n '\n \n-test_expect_failure 'checkout -b to a merge base' '\n+test_expect_success 'checkout -b to a merge base' '\n \ttest_when_finished \"\n \t\tgit checkout branch1 &&\n \t\ttest_might_fail git branch -D branch2\" &&\n@@ -133,7 +133,7 @@ test_expect_success 'checkout -B to an existing branch resets branch to HEAD' '\n \tdo_checkout branch2 \"\" -B\n '\n \n-test_expect_failure 'checkout -B to a merge base' '\n+test_expect_success 'checkout -B to a merge base' '\n \tgit checkout branch1 &&\n \n \tgit checkout -B branch2 branch1...\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 478b82cf9b..acb16b62dd 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -42,6 +42,10 @@ test_expect_success 'git branch a/b/c should create a branch' '\n \tgit branch a/b/c && test_path_is_file .git/refs/heads/a/b/c\n '\n \n+test_expect_success 'git branch mb master... should create a branch' '\n+\tgit branch mb master... && test_path_is_file .git/refs/heads/mb\n+'\n+\n test_expect_success 'git branch HEAD should fail' '\n \ttest_must_fail git branch HEAD\n '\n@@ -292,8 +296,8 @@ test_expect_success 'git branch --list -v with --abbrev' '\n test_expect_success 'git branch --column' '\n \tCOLUMNS=81 git branch --column=column >actual &&\n \tcat >expected <<\\EOF &&\n-  a/b/c     bam       foo       l       * master    n         o/p       r\n-  abc       bar       j/k       m/m       master2   o/o       q\n+  a/b/c     bam       foo       l       * master    mb        o/o       q\n+  abc       bar       j/k       m/m       master2   n         o/p       r\n EOF\n \ttest_cmp expected actual\n '\n@@ -315,6 +319,7 @@ test_expect_success 'git branch --column with an extremely long branch name' '\n   m/m\n * master\n   master2\n+  mb\n   n\n   o/o\n   o/p\n@@ -332,8 +337,8 @@ test_expect_success 'git branch with column.*' '\n \tgit config --unset column.branch &&\n \tgit config --unset column.ui &&\n \tcat >expected <<\\EOF &&\n-  a/b/c   bam   foo   l   * master    n     o/p   r\n-  abc     bar   j/k   m/m   master2   o/o   q\n+  a/b/c   bam   foo   l   * master    mb   o/o   q\n+  abc     bar   j/k   m/m   master2   n    o/p   r\n EOF\n \ttest_cmp expected actual\n '\n@@ -357,6 +362,7 @@ test_expect_success 'git branch -v with column.ui ignored' '\n   m/m\n * master\n   master2\n+  mb\n   n\n   o/o\n   o/p\n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374581","messageId":"xmqqlfzwtlft.fsf@gitster-ct.c.googlers.com","threadId":"50994","inReplyTo":"cover.1556305561.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 0/3] allow checkout and branch to create branches on a merge base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-26T23:07:34Z","receivedAt":"2019-04-26T23:07:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> Thanks for your comments, Eric and Junio.\n>\n> Eric, I've combined the `test_when_finished` calls together so that the\n> statements within appear in a more \"logical\" order.\n>\n> Junio, I've taken your suggestion and moved the change into\n> `create_branch`. Initially, I didn't want to do this because I didn't\n> want to change the semantics of git-branch but introducing the merge\n> base syntax seems to be a positive change so let's do it.\n> ...\n> Denton Liu (3):\n>   t2018: cleanup in current test\n>   t2018: demonstrate checkout -b merge base bug\n>   branch: make create_branch accept a merge base rev\n\nBecause \"checkout -b new\" is supposed to be merely a short-hand for\na \"branch new\" followed by \"checkout new\", the lack of \"branch new\nA...B\" is the same \"bug\" as the lack of \"checkout -b new A...B\".\n\nThe second patch that does not talk about the former but singles out\nonly the latter is being inconsistent.\n\nOne person's lack of feature is a bug to another person, and indeed,\nwhen we did \"checkout A...B\" in 2009, we weren't interested in doing\nthe same for \"checkout -b new\", and nobody thought about adding that\nuntil now, and/or considered the lack of feature as a bug.\n\nWe do not \"demonstrate\" the lack of a new feature in a patch with\nexpect-failure, followed by another patch that adds the feature that\nflips expect-failure to expect-success.  A patch that teaches\n\"checkout -b\" about A...B, that is adding a missing feature, should\nnot have to do so.  As it is shades of gray between a change being a\nbugfix and adding a new feature, switching the style of testing\nbased on the distinction between them does not make much sense.  Be\nconsistent and stick to just one style.  And having the test and the\ncode change (be it adding a missing feature or fixing a bug) in a\nsingle patch makes patch management a lot simpler by making it\nharder to lose only one half.\n\nHaving a preliminary clean-up as a separate step is a good idea, but\nfor this topic, I think the latter two should be combined into a\nsingle patch that changes the code and adds tests at the same time.\n\nThanks.\n"},{"id":"374583","messageId":"20190426234024.GA28055@dev-l","threadId":"50994","inReplyTo":"xmqqlfzwtlft.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/3] allow checkout and branch to create branches on a merge base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-26T23:40:24Z","receivedAt":"2019-04-26T23:40:29Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Sat, Apr 27, 2019 at 08:07:34AM +0900, Junio C Hamano wrote:\n> Denton Liu <liu.denton@gmail.com> writes:\n> \n> > Thanks for your comments, Eric and Junio.\n> >\n> > Eric, I've combined the `test_when_finished` calls together so that the\n> > statements within appear in a more \"logical\" order.\n> >\n> > Junio, I've taken your suggestion and moved the change into\n> > `create_branch`. Initially, I didn't want to do this because I didn't\n> > want to change the semantics of git-branch but introducing the merge\n> > base syntax seems to be a positive change so let's do it.\n> > ...\n> > Denton Liu (3):\n> >   t2018: cleanup in current test\n> >   t2018: demonstrate checkout -b merge base bug\n> >   branch: make create_branch accept a merge base rev\n> \n> Because \"checkout -b new\" is supposed to be merely a short-hand for\n> a \"branch new\" followed by \"checkout new\", the lack of \"branch new\n> A...B\" is the same \"bug\" as the lack of \"checkout -b new A...B\".\n\nI didn't consider it to be the same \"bug\" because git-checkout.txt\nmentions the \"...\" syntax, whereas it isn't mentioned in git-branch.txt.\nThat being said, now that I look at git-checkout.txt again, when doing a\n\"regular\" checkout, the parameter is called <branch> whereas when we're\ndoing a checkout -b, it's called <start_point>, which doesn't mention\n\"...\".\n\n> \n> The second patch that does not talk about the former but singles out\n> only the latter is being inconsistent.\n> \n> One person's lack of feature is a bug to another person, and indeed,\n> when we did \"checkout A...B\" in 2009, we weren't interested in doing\n> the same for \"checkout -b new\", and nobody thought about adding that\n> until now, and/or considered the lack of feature as a bug.\n\nI agree, based on the above, I now see that it's a lack of feature and\nnot a bug.\n\n> \n> We do not \"demonstrate\" the lack of a new feature in a patch with\n> expect-failure, followed by another patch that adds the feature that\n> flips expect-failure to expect-success.  A patch that teaches\n> \"checkout -b\" about A...B, that is adding a missing feature, should\n> not have to do so.  As it is shades of gray between a change being a\n> bugfix and adding a new feature, switching the style of testing\n> based on the distinction between them does not make much sense.  Be\n> consistent and stick to just one style.  And having the test and the\n> code change (be it adding a missing feature or fixing a bug) in a\n> single patch makes patch management a lot simpler by making it\n> harder to lose only one half.\n> \n> Having a preliminary clean-up as a separate step is a good idea, but\n> for this topic, I think the latter two should be combined into a\n> single patch that changes the code and adds tests at the same time.\n\nI'm going send a reroll to update the documentation to mention \"...\" in\n<start_point> and, while I'm at it, I'll do the squash.\n\nThanks for your comments,\n\nDenton\n\n> \n> Thanks.\n"},{"id":"374587","messageId":"20190426235220.GB28055@dev-l","threadId":"50994","inReplyTo":"20190426234024.GA28055@dev-l","subject":"Re: [PATCH v2 0/3] allow checkout and branch to create branches on a merge base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-26T23:52:20Z","receivedAt":"2019-04-26T23:52:25Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Fri, Apr 26, 2019 at 04:40:24PM -0700, Denton Liu wrote:\n\n[snip]\n\n> I'm going send a reroll to update the documentation to mention \"...\" in\n> <start_point> and, while I'm at it, I'll do the squash.\n\nActually, I have a quick question for you:\n\nOut of respect for your time as the maintainer (since you have a lot of\ntopics to deal with), how do you prefer small fixups be submitted?\n\n* A complete reroll of the whole series\n* A replacement for one patch in the series\n* A follow-up email with an \"oops, please change x to y\"\n\nThanks,\n\nDenton\n\n> \n> Thanks for your comments,\n> \n> Denton\n> \n> > \n> > Thanks.\n"},{"id":"374588","messageId":"xmqq8svwtixi.fsf@gitster-ct.c.googlers.com","threadId":"50994","inReplyTo":"20190426235220.GB28055@dev-l","subject":"Re: [PATCH v2 0/3] allow checkout and branch to create branches on a merge base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-27T00:01:45Z","receivedAt":"2019-04-27T00:01:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> Out of respect for your time as the maintainer (since you have a lot of\n> topics to deal with), how do you prefer small fixups be submitted?\n>\n> * A complete reroll of the whole series\n> * A replacement for one patch in the series\n> * A follow-up email with an \"oops, please change x to y\"\n\nDepends on how small your \"small\" is, but in general, the above is a\ngood approximation of the preference order.\n\nWholesale replacement without having to think is the easiest.  When\nthe latest round is not from a distant past (i.e. less than two\nweeks old), replacement patches for only selected steps is also\nfine, as I can pretend it is a wholesale replacement by completing\nthe missing patches by picking them from the mailbox while using the\nupdated ones.\n\n\"Please change x to y\" is the least welcome, with a big exception\nthat it is the easiest when it is a change to a few words in a\nsingle place, either in the log message or a file contents, of a\nsingle patch topic---in other words, something I can deal with\n\"commit --amend\" and needs less than 2 minutes to handle.  Anything\nlarger than that size and requires me to fire up \"rebase -i\" starts\nto make me wonder why _I_ am doing that, not the topic owner.\n\nThanks.\n"},{"id":"374592","messageId":"cover.1556366347.git.liu.denton@gmail.com","threadId":"50994","inReplyTo":"cover.1556305561.git.liu.denton@gmail.com","subject":"[PATCH v3 0/2] allow checkout and branch to create branches on a merge base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-27T12:02:16Z","receivedAt":"2019-04-27T12:02:22Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Thanks again for the review, Junio.\n\nI've squashed 2/3 and 3/3 together and made that documentation change I\nwas talking about earlier.\n\n---\n\nChanges since v2:\n\n* Squashed 2/3 with 3/3\n* Document merge base syntax for <start_point> in git-checkout.txt\n\nChanges since v1:\n\n* Moved multiple `test_when_finished` calls that appeared in \"reverse\n  order\" into one call that appears in the logical order\n* Made create_branch handle merge base revs instead of putting a hack\n  into checkout\n\nDenton Liu (2):\n  t2018: cleanup in current test\n  branch: make create_branch accept a merge base rev\n\n Documentation/git-branch.txt   |  6 +++-\n Documentation/git-checkout.txt |  4 +++\n branch.c                       |  2 +-\n t/t2018-checkout-branch.sh     | 56 ++++++++++++++++++----------------\n t/t3200-branch.sh              | 14 ++++++---\n 5 files changed, 50 insertions(+), 32 deletions(-)\n\nRange-diff against v2:\n1:  9d04faf29d = 1:  9d04faf29d t2018: cleanup in current test\n2:  5e8320cd80 < -:  ---------- t2018: demonstrate checkout -b merge base bug\n3:  c91c7535a7 ! 2:  bb25852740 branch: make create_branch accept a merge base rev\n    @@ -41,6 +41,21 @@\n      Note that this will create the new branch, but it will not switch the\n      working tree to it; use \"git checkout <newbranch>\" to switch to the\n     \n    + diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt\n    + --- a/Documentation/git-checkout.txt\n    + +++ b/Documentation/git-checkout.txt\n    +@@\n    + <start_point>::\n    + \tThe name of a commit at which to start the new branch; see\n    + \tlinkgit:git-branch[1] for details. Defaults to HEAD.\n    +++\n    ++As a special case, you may use `\"A...B\"` as a shortcut for the\n    ++merge base of `A` and `B` if there is exactly one merge base. You can\n    ++leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.\n    + \n    + <tree-ish>::\n    + \tTree to checkout from (when paths are given). If not specified,\n    +\n      diff --git a/branch.c b/branch.c\n      --- a/branch.c\n      +++ b/branch.c\n    @@ -61,20 +76,29 @@\n      \tdo_checkout branch2\n      '\n      \n    --test_expect_failure 'checkout -b to a merge base' '\n     +test_expect_success 'checkout -b to a merge base' '\n    ++\ttest_when_finished \"\n    ++\t\tgit checkout branch1 &&\n    ++\t\ttest_might_fail git branch -D branch2\" &&\n    ++\tgit checkout -b branch2 branch1...\n    ++'\n    ++\n    + test_expect_success 'checkout -b to a new branch, set to an explicit ref' '\n      \ttest_when_finished \"\n      \t\tgit checkout branch1 &&\n    - \t\ttest_might_fail git branch -D branch2\" &&\n     @@\n      \tdo_checkout branch2 \"\" -B\n      '\n      \n    --test_expect_failure 'checkout -B to a merge base' '\n     +test_expect_success 'checkout -B to a merge base' '\n    - \tgit checkout branch1 &&\n    ++\tgit checkout branch1 &&\n    ++\n    ++\tgit checkout -B branch2 branch1...\n    ++'\n    ++\n    + test_expect_success 'checkout -B to an existing branch from detached HEAD resets branch to HEAD' '\n    + \tgit checkout $(git rev-parse --verify HEAD) &&\n      \n    - \tgit checkout -B branch2 branch1...\n     \n      diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n      --- a/t/t3200-branch.sh\n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374593","messageId":"9d04faf29dd61eb04ea292786d72eda0a3832e51.1556366347.git.liu.denton@gmail.com","threadId":"50994","inReplyTo":"cover.1556366347.git.liu.denton@gmail.com","subject":"[PATCH v3 1/2] t2018: cleanup in current test","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-27T12:02:19Z","receivedAt":"2019-04-27T12:02:29Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, in t2018, if do_checkout failed to create `branch2`, the next\ntest-case would run `git branch -D branch2` but then fail because it was\nexpecting `branch2` to exist, even though it doesn't. As a result, an\nearly failure could cause a cascading failure of tests.\n\nMake test-case responsible for cleaning up their own branches so that\nfuture tests can start with a sane environment.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n t/t2018-checkout-branch.sh | 43 +++++++++++++++-----------------------\n 1 file changed, 17 insertions(+), 26 deletions(-)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex c5014ad9a6..f1c7023e1a 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -60,38 +60,40 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'checkout -b to a new branch, set to HEAD' '\n+\ttest_when_finished \"\n+\t\tgit checkout branch1 &&\n+\t\ttest_might_fail git branch -D branch2\" &&\n \tdo_checkout branch2\n '\n \n test_expect_success 'checkout -b to a new branch, set to an explicit ref' '\n-\tgit checkout branch1 &&\n-\tgit branch -D branch2 &&\n-\n+\ttest_when_finished \"\n+\t\tgit checkout branch1 &&\n+\t\ttest_might_fail git branch -D branch2\" &&\n \tdo_checkout branch2 $HEAD1\n '\n \n test_expect_success 'checkout -b to a new branch with unmergeable changes fails' '\n-\tgit checkout branch1 &&\n-\n-\t# clean up from previous test\n-\tgit branch -D branch2 &&\n-\n \tsetup_dirty_unmergeable &&\n \ttest_must_fail do_checkout branch2 $HEAD1 &&\n \ttest_dirty_unmergeable\n '\n \n test_expect_success 'checkout -f -b to a new branch with unmergeable changes discards changes' '\n+\ttest_when_finished \"\n+\t\tgit checkout branch1 &&\n+\t\ttest_might_fail git branch -D branch2\" &&\n+\n \t# still dirty and on branch1\n \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n \ttest_must_fail test_dirty_unmergeable\n '\n \n test_expect_success 'checkout -b to a new branch preserves mergeable changes' '\n-\tgit checkout branch1 &&\n-\n-\t# clean up from previous test\n-\tgit branch -D branch2 &&\n+\ttest_when_finished \"\n+\t\tgit reset --hard &&\n+\t\tgit checkout branch1 &&\n+\t\ttest_might_fail git branch -D branch2\" &&\n \n \tsetup_dirty_mergeable &&\n \tdo_checkout branch2 $HEAD1 &&\n@@ -99,27 +101,18 @@ test_expect_success 'checkout -b to a new branch preserves mergeable changes' '\n '\n \n test_expect_success 'checkout -f -b to a new branch with mergeable changes discards changes' '\n-\t# clean up from previous test\n-\tgit reset --hard &&\n-\n-\tgit checkout branch1 &&\n-\n-\t# clean up from previous test\n-\tgit branch -D branch2 &&\n-\n+\ttest_when_finished git reset --hard HEAD &&\n \tsetup_dirty_mergeable &&\n \tdo_checkout branch2 $HEAD1 \"-f -b\" &&\n \ttest_must_fail test_dirty_mergeable\n '\n \n test_expect_success 'checkout -b to an existing branch fails' '\n-\tgit reset --hard HEAD &&\n-\n+\ttest_when_finished git reset --hard HEAD &&\n \ttest_must_fail do_checkout branch2 $HEAD2\n '\n \n test_expect_success 'checkout -b to @{-1} fails with the right branch name' '\n-\tgit reset --hard HEAD &&\n \tgit checkout branch1 &&\n \tgit checkout branch2 &&\n \techo  >expect \"fatal: A branch named '\\''branch1'\\'' already exists.\" &&\n@@ -160,6 +153,7 @@ test_expect_success 'checkout -f -B to an existing branch with unmergeable chang\n '\n \n test_expect_success 'checkout -B to an existing branch preserves mergeable changes' '\n+\ttest_when_finished git reset --hard &&\n \tgit checkout branch1 &&\n \n \tsetup_dirty_mergeable &&\n@@ -168,9 +162,6 @@ test_expect_success 'checkout -B to an existing branch preserves mergeable chang\n '\n \n test_expect_success 'checkout -f -B to an existing branch with mergeable changes discards changes' '\n-\t# clean up from previous test\n-\tgit reset --hard &&\n-\n \tgit checkout branch1 &&\n \n \tsetup_dirty_mergeable &&\n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374594","messageId":"bb258527407126fa6e3d56de64dc76fd52c5d504.1556366347.git.liu.denton@gmail.com","threadId":"50994","inReplyTo":"cover.1556366347.git.liu.denton@gmail.com","subject":"[PATCH v3 2/2] branch: make create_branch accept a merge base rev","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-27T12:02:22Z","receivedAt":"2019-04-27T12:02:30Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When we ran something like\n\n    $ git checkout -b test master...\n\nit would fail with the message\n\n    fatal: Not a valid object name: 'master...'.\n\nThis was caused by the call to `create_branch` where `start_name` is\nexpected to be a valid rev. However, git-checkout allows the branch to\nbe a valid _merge base_ rev (i.e. with a \"...\") so it was possible for\nan invalid rev to be passed in.\n\nMake `create_branch` accept a merge base rev so that this case does not\nerror out.\n\nAs a side-effect, teach git-branch how to handle merge base revs as\nwell.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n Documentation/git-branch.txt   |  6 +++++-\n Documentation/git-checkout.txt |  4 ++++\n branch.c                       |  2 +-\n t/t2018-checkout-branch.sh     | 13 +++++++++++++\n t/t3200-branch.sh              | 14 ++++++++++----\n 5 files changed, 33 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 0cd87ddeff..6ebd512b4f 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -45,7 +45,11 @@ argument is missing it defaults to `HEAD` (i.e. the tip of the current\n branch).\n \n The command's second form creates a new branch head named <branchname>\n-which points to the current `HEAD`, or <start-point> if given.\n+which points to the current `HEAD`, or <start-point> if given. As a\n+special case, for <start-point>, you may use `\"A...B\"` as a shortcut for\n+the merge base of `A` and `B` if there is exactly one merge base. You\n+can leave out at most one of `A` and `B`, in which case it defaults to\n+`HEAD`.\n \n Note that this will create the new branch, but it will not switch the\n working tree to it; use \"git checkout <newbranch>\" to switch to the\ndiff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt\nindex 877e5f503a..964f912d29 100644\n--- a/Documentation/git-checkout.txt\n+++ b/Documentation/git-checkout.txt\n@@ -313,6 +313,10 @@ leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.\n <start_point>::\n \tThe name of a commit at which to start the new branch; see\n \tlinkgit:git-branch[1] for details. Defaults to HEAD.\n++\n+As a special case, you may use `\"A...B\"` as a shortcut for the\n+merge base of `A` and `B` if there is exactly one merge base. You can\n+leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.\n \n <tree-ish>::\n \tTree to checkout from (when paths are given). If not specified,\ndiff --git a/branch.c b/branch.c\nindex 28b81a7e02..a84c8aaca2 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -268,7 +268,7 @@ void create_branch(struct repository *r,\n \t}\n \n \treal_ref = NULL;\n-\tif (get_oid(start_name, &oid)) {\n+\tif (get_oid_mb(start_name, &oid)) {\n \t\tif (explicit_tracking) {\n \t\t\tif (advice_set_upstream_failure) {\n \t\t\t\terror(_(upstream_missing), start_name);\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex f1c7023e1a..822381dd9d 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -66,6 +66,13 @@ test_expect_success 'checkout -b to a new branch, set to HEAD' '\n \tdo_checkout branch2\n '\n \n+test_expect_success 'checkout -b to a merge base' '\n+\ttest_when_finished \"\n+\t\tgit checkout branch1 &&\n+\t\ttest_might_fail git branch -D branch2\" &&\n+\tgit checkout -b branch2 branch1...\n+'\n+\n test_expect_success 'checkout -b to a new branch, set to an explicit ref' '\n \ttest_when_finished \"\n \t\tgit checkout branch1 &&\n@@ -126,6 +133,12 @@ test_expect_success 'checkout -B to an existing branch resets branch to HEAD' '\n \tdo_checkout branch2 \"\" -B\n '\n \n+test_expect_success 'checkout -B to a merge base' '\n+\tgit checkout branch1 &&\n+\n+\tgit checkout -B branch2 branch1...\n+'\n+\n test_expect_success 'checkout -B to an existing branch from detached HEAD resets branch to HEAD' '\n \tgit checkout $(git rev-parse --verify HEAD) &&\n \ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 478b82cf9b..acb16b62dd 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -42,6 +42,10 @@ test_expect_success 'git branch a/b/c should create a branch' '\n \tgit branch a/b/c && test_path_is_file .git/refs/heads/a/b/c\n '\n \n+test_expect_success 'git branch mb master... should create a branch' '\n+\tgit branch mb master... && test_path_is_file .git/refs/heads/mb\n+'\n+\n test_expect_success 'git branch HEAD should fail' '\n \ttest_must_fail git branch HEAD\n '\n@@ -292,8 +296,8 @@ test_expect_success 'git branch --list -v with --abbrev' '\n test_expect_success 'git branch --column' '\n \tCOLUMNS=81 git branch --column=column >actual &&\n \tcat >expected <<\\EOF &&\n-  a/b/c     bam       foo       l       * master    n         o/p       r\n-  abc       bar       j/k       m/m       master2   o/o       q\n+  a/b/c     bam       foo       l       * master    mb        o/o       q\n+  abc       bar       j/k       m/m       master2   n         o/p       r\n EOF\n \ttest_cmp expected actual\n '\n@@ -315,6 +319,7 @@ test_expect_success 'git branch --column with an extremely long branch name' '\n   m/m\n * master\n   master2\n+  mb\n   n\n   o/o\n   o/p\n@@ -332,8 +337,8 @@ test_expect_success 'git branch with column.*' '\n \tgit config --unset column.branch &&\n \tgit config --unset column.ui &&\n \tcat >expected <<\\EOF &&\n-  a/b/c   bam   foo   l   * master    n     o/p   r\n-  abc     bar   j/k   m/m   master2   o/o   q\n+  a/b/c   bam   foo   l   * master    mb   o/o   q\n+  abc     bar   j/k   m/m   master2   n    o/p   r\n EOF\n \ttest_cmp expected actual\n '\n@@ -357,6 +362,7 @@ test_expect_success 'git branch -v with column.ui ignored' '\n   m/m\n * master\n   master2\n+  mb\n   n\n   o/o\n   o/p\n-- \n2.21.0.1000.g11cd861522\n\n"}]}