{"thread":{"id":"49558","subject":"[Question] builtin/branch.c","startedAt":"2018-10-13T08:11:11Z","lastAt":"2018-10-17T12:58:35Z","messageCount":8,"participants":["Tao Qingyun","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"360385","messageId":"20181013081101.6602-1-taoqy@ls-a.me","threadId":"49558","inReplyTo":null,"subject":"[Question] builtin/branch.c","fromName":"Tao Qingyun","fromEmail":"taoqy@ls-a.me","sentAt":"2018-10-13T08:11:01Z","receivedAt":"2018-10-13T08:11:11Z","isPatch":false,"sender":{"key":"taoqy@ls-a.me","avatar":"https://avatars.githubusercontent.com/u/38895844?v=4"},"body":"Hi, I am learning `builtin/branch.c`. I find that it will call `branch_get`\nbefore create and [un]set upstream, and die with \"no such branch\" if failed.\nbut `branch_get` seems never fail, it is a get_or_create. Also, it was\nconfused that getting a branch before it has created.\n\nbuiltin/branch.c #811\n\n } else if (argc > 0 && argc <= 2) {\n struct branch *branch = branch_get(argv[0]);\n\n if (!branch)\n die(_(\"no such branch '%s'\"), argv[0]);\n\n if (filter.kind != FILTER_REFS_BRANCHES)\n die(_(\"-a and -r options to 'git branch' do not make sense with a branch name\"));\n\n if (track == BRANCH_TRACK_OVERRIDE)\n die(_(\"the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead.\"));\n\n create_branch(argv[0], (argc == 2) ? argv[1] : head,\n force, 0, reflog, quiet, track);\n\n\n"},{"id":"360403","messageId":"CACBZZX7i-Uob9EJ8GeDKYVTyKtdRiy=qPbxSOe=FGh2cbHG9Zw@mail.gmail.com","threadId":"49558","inReplyTo":"20181013081101.6602-1-taoqy@ls-a.me","subject":"Re: [Question] builtin/branch.c","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-14T10:19:35Z","receivedAt":"2018-10-14T10:19:49Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Oct 13, 2018 at 10:12 AM Tao Qingyun wrote:\n> Hi, I am learning `builtin/branch.c`. I find that it will call `branch_get`\n> before create and [un]set upstream, and die with \"no such branch\" if failed.\n> but `branch_get` seems never fail, it is a get_or_create. Also, it was\n> confused that getting a branch before it has created.\n>\n> builtin/branch.c #811\n>\n> } else if (argc > 0 && argc <= 2) {\n> struct branch *branch = branch_get(argv[0]);\n>\n> if (!branch)\n> die(_(\"no such branch '%s'\"), argv[0]);\n\nFrom my reading of the source you're correct. That !branch case is\npointless. The only way that function can fail is in the x*() family\nof functions, which'll make the function die instead of returning\nNULL.\n"},{"id":"360523","messageId":"20181015140839.4925-1-taoqy@ls-a.me","threadId":"49558","inReplyTo":"CACBZZX7i-Uob9EJ8GeDKYVTyKtdRiy=qPbxSOe=FGh2cbHG9Zw@mail.gmail.com","subject":"[PATCH] builtin/branch.c: remove useless branch_get","fromName":"Tao Qingyun","fromEmail":"taoqy@ls-a.me","sentAt":"2018-10-15T14:08:39Z","receivedAt":"2018-10-15T14:16:33Z","isPatch":true,"sender":{"key":"taoqy@ls-a.me","avatar":"https://avatars.githubusercontent.com/u/38895844?v=4"},"body":"Signed-off-by: Tao Qingyun \n---\n builtin/branch.c | 5 -----\n 1 file changed, 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex c396c41533..2367703034 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -809,11 +809,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tgit_config_set_multivar(buf.buf, NULL, NULL, 1);\n \t\tstrbuf_release(&buf);\n \t} else if (argc > 0 && argc <= 2) {\n-\t\tstruct branch *branch = branch_get(argv[0]);\n-\n-\t\tif (!branch)\n-\t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n-\n \t\tif (filter.kind != FILTER_REFS_BRANCHES)\n \t\t\tdie(_(\"-a and -r options to 'git branch' do not make sense with a branch name\"));\n \n-- \n2.19.0\n\n"},{"id":"360544","messageId":"20181015171417.GA1301@sigill.intra.peff.net","threadId":"49558","inReplyTo":"CACBZZX7i-Uob9EJ8GeDKYVTyKtdRiy=qPbxSOe=FGh2cbHG9Zw@mail.gmail.com","subject":"Re: [Question] builtin/branch.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-15T17:14:17Z","receivedAt":"2018-10-15T17:14:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 14, 2018 at 12:19:35PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On Sat, Oct 13, 2018 at 10:12 AM Tao Qingyun wrote:\n> > Hi, I am learning `builtin/branch.c`. I find that it will call `branch_get`\n> > before create and [un]set upstream, and die with \"no such branch\" if failed.\n> > but `branch_get` seems never fail, it is a get_or_create. Also, it was\n> > confused that getting a branch before it has created.\n> >\n> > builtin/branch.c #811\n> >\n> > } else if (argc > 0 && argc <= 2) {\n> > struct branch *branch = branch_get(argv[0]);\n> >\n> > if (!branch)\n> > die(_(\"no such branch '%s'\"), argv[0]);\n> \n> From my reading of the source you're correct. That !branch case is\n> pointless. The only way that function can fail is in the x*() family\n> of functions, which'll make the function die instead of returning\n> NULL.\n\nIt sometimes returns current_branch, which can be NULL (e.g., if you're\non a detached HEAD). Try:\n\n $ git branch HEAD\n fatal: no such branch 'HEAD'\n\n $ git branch ''\n fatal: no such branch ''\n\nHowever, it seems weird that we'd check those cases here (and provide\nsuch lousy messages). And indeed, dropping that and letting us\neventually hit create_branch() gives a much better message:\n\n $ git branch HEAD\n fatal: 'HEAD' is not a valid branch name.\n\n $ git branch ''\n fatal: '' is not a valid branch name.\n\nI think we'd want to see that reasoning in the commit message.\n\n-Peff\n"},{"id":"360545","messageId":"20181015171707.GB1301@sigill.intra.peff.net","threadId":"49558","inReplyTo":"20181015140839.4925-1-taoqy@ls-a.me","subject":"Re: [PATCH] builtin/branch.c: remove useless branch_get","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-15T17:17:07Z","receivedAt":"2018-10-15T17:17:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 15, 2018 at 10:08:39PM +0800, Tao Qingyun wrote:\n\n> Signed-off-by: Tao Qingyun \n\nThe commit message should describe the \"why\" here. I gave some reasoning\nnearby in:\n\n https://public-inbox.org/git/20181015171417.GA1301@sigill.intra.peff.net/\n\nFrom your initial message, it sounds like this might also be fixing a\nbug (\"confusing that getting a branch before it has created\"). Can you\ndescribe that (and ideally show the fix with a test)?\n\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index c396c41533..2367703034 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -809,11 +809,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t\tgit_config_set_multivar(buf.buf, NULL, NULL, 1);\n> \t\tstrbuf_release(&buf);\n> \t} else if (argc > 0 && argc <= 2) {\n> -\t\tstruct branch *branch = branch_get(argv[0]);\n> -\n> -\t\tif (!branch)\n> -\t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n> -\n\nFrom what I can tell, the patch itself _is_ an improvement. I just think\nwe need to explain why for the record.\n\n-Peff\n"},{"id":"360629","messageId":"20181016145428.2901-1-taoqy@ls-a.me","threadId":"49558","inReplyTo":"20181015171707.GB1301@sigill.intra.peff.net","subject":"[PATCH v2] builtin/branch.c: remove useless branch_get","fromName":"Tao Qingyun","fromEmail":"taoqy@ls-a.me","sentAt":"2018-10-16T14:54:28Z","receivedAt":"2018-10-16T15:02:14Z","isPatch":true,"sender":{"key":"taoqy@ls-a.me","avatar":"https://avatars.githubusercontent.com/u/38895844?v=4"},"body":"branch_get sometimes returns current_branch, which can be NULL (e.g., if\nyou're on a detached HEAD). Try:\n\n $ git branch HEAD\n fatal: no such branch 'HEAD'\n\n $ git branch ''\n fatal: no such branch ''\n\nHowever, it seems weird that we'd check those cases here (and provide\nsuch lousy messages). And indeed, dropping that and letting us\neventually hit create_branch() gives a much better message:\n\n $ git branch HEAD\n fatal: 'HEAD' is not a valid branch name.\n\n $ git branch ''\n fatal: '' is not a valid branch name.\n\nSigned-off-by: Tao Qingyun \n---\n builtin/branch.c | 5 -----\n 1 file changed, 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex c396c41533..2367703034 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -809,11 +809,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tgit_config_set_multivar(buf.buf, NULL, NULL, 1);\n \t\tstrbuf_release(&buf);\n \t} else if (argc > 0 && argc <= 2) {\n-\t\tstruct branch *branch = branch_get(argv[0]);\n-\n-\t\tif (!branch)\n-\t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n-\n \t\tif (filter.kind != FILTER_REFS_BRANCHES)\n \t\t\tdie(_(\"-a and -r options to 'git branch' do not make sense with a branch name\"));\n \n-- \n2.19.0\n\n"},{"id":"360732","messageId":"20181017083906.GC31932@sigill.intra.peff.net","threadId":"49558","inReplyTo":"20181016145428.2901-1-taoqy@ls-a.me","subject":"Re: [PATCH v2] builtin/branch.c: remove useless branch_get","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-17T08:39:07Z","receivedAt":"2018-10-17T08:39:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 16, 2018 at 10:54:28PM +0800, Tao Qingyun wrote:\n\n> branch_get sometimes returns current_branch, which can be NULL (e.g., if\n> you're on a detached HEAD). Try:\n> \n> $ git branch HEAD\n> fatal: no such branch 'HEAD'\n> \n> $ git branch ''\n> fatal: no such branch ''\n> \n> However, it seems weird that we'd check those cases here (and provide\n> such lousy messages). And indeed, dropping that and letting us\n> eventually hit create_branch() gives a much better message:\n> \n> $ git branch HEAD\n> fatal: 'HEAD' is not a valid branch name.\n> \n> $ git branch ''\n> fatal: '' is not a valid branch name.\n\nThis explanation is perfect, of course. ;)\n\nI still wondered if you had another motivation hinted at in your\noriginal mail, though (some weirdness with running branch_get early).\nIt's OK if there isn't one, but I just want to make sure we capture all\nof the details.\n\nOther than that question, the patch looks good to me.\n\n-Peff\n"},{"id":"360743","messageId":"20181017125818.1872-1-taoqy@ls-a.me","threadId":"49558","inReplyTo":"20181017083906.GC31932@sigill.intra.peff.net","subject":"Re: [PATCH v2] builtin/branch.c: remove useless branch_get","fromName":"Tao Qingyun","fromEmail":"taoqy@ls-a.me","sentAt":"2018-10-17T12:58:18Z","receivedAt":"2018-10-17T12:58:35Z","isPatch":true,"sender":{"key":"taoqy@ls-a.me","avatar":"https://avatars.githubusercontent.com/u/38895844?v=4"},"body":"> > branch_get sometimes returns current_branch, which can be NULL (e.g., if\n> > you're on a detached HEAD). Try:\n> > \n> > $ git branch HEAD\n> > fatal: no such branch 'HEAD'\n> > \n> > $ git branch ''\n> > fatal: no such branch ''\n> > \n> > However, it seems weird that we'd check those cases here (and provide\n> > such lousy messages). And indeed, dropping that and letting us\n> > eventually hit create_branch() gives a much better message:\n> > \n> > $ git branch HEAD\n> > fatal: 'HEAD' is not a valid branch name.\n> > \n> > $ git branch ''\n> > fatal: '' is not a valid branch name.\n> \n> This explanation is perfect, of course. ;)\n> \n> I still wondered if you had another motivation hinted at in your\n> original mail, though (some weirdness with running branch_get early).\n> It's OK if there isn't one, but I just want to make sure we capture all\n> of the details.\n> \n\nYes, this explanation is perfect. ;)\n\n> Other than that question, the patch looks good to me.\n> \n> -Peff\n\n"}]}