# [Question] builtin/branch.c 8 messages from 2018-10-13 to 2018-10-17. Participants: Tao Qingyun, Ævar Arnfjörð Bjarmason, Jeff King. Thread: https://gitlist.dev/t/49558 ## Tao Qingyun, 2018-10-13 08:11 Subject: [Question] builtin/branch.c Message-ID: <20181013081101.6602-1-taoqy@ls-a.me> URL: https://gitlist.dev/e/20181013081101.6602-1-taoqy%40ls-a.me ``` Hi, I am learning `builtin/branch.c`. I find that it will call `branch_get` before create and [un]set upstream, and die with "no such branch" if failed. but `branch_get` seems never fail, it is a get_or_create. Also, it was confused that getting a branch before it has created. builtin/branch.c #811 } else if (argc > 0 && argc <= 2) { struct branch *branch = branch_get(argv[0]); if (!branch) die(_("no such branch '%s'"), argv[0]); if (filter.kind != FILTER_REFS_BRANCHES) die(_("-a and -r options to 'git branch' do not make sense with a branch name")); if (track == BRANCH_TRACK_OVERRIDE) die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead.")); create_branch(argv[0], (argc == 2) ? argv[1] : head, force, 0, reflog, quiet, track); ``` ## Ævar Arnfjörð Bjarmason, 2018-10-14 10:19 Subject: Re: [Question] builtin/branch.c Message-ID: URL: https://gitlist.dev/e/CACBZZX7i-Uob9EJ8GeDKYVTyKtdRiy%3DqPbxSOe%3DFGh2cbHG9Zw%40mail.gmail.com In-Reply-To: <20181013081101.6602-1-taoqy@ls-a.me> ``` On Sat, Oct 13, 2018 at 10:12 AM Tao Qingyun wrote: > Hi, I am learning `builtin/branch.c`. I find that it will call `branch_get` > before create and [un]set upstream, and die with "no such branch" if failed. > but `branch_get` seems never fail, it is a get_or_create. Also, it was > confused that getting a branch before it has created. > > builtin/branch.c #811 > > } else if (argc > 0 && argc <= 2) { > struct branch *branch = branch_get(argv[0]); > > if (!branch) > die(_("no such branch '%s'"), argv[0]); From my reading of the source you're correct. That !branch case is pointless. The only way that function can fail is in the x*() family of functions, which'll make the function die instead of returning NULL. ``` ## Tao Qingyun, 2018-10-15 14:08 Subject: [PATCH] builtin/branch.c: remove useless branch_get Message-ID: <20181015140839.4925-1-taoqy@ls-a.me> URL: https://gitlist.dev/e/20181015140839.4925-1-taoqy%40ls-a.me In-Reply-To: ``` Signed-off-by: Tao Qingyun --- builtin/branch.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index c396c41533..2367703034 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -809,11 +809,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix) git_config_set_multivar(buf.buf, NULL, NULL, 1); strbuf_release(&buf); } else if (argc > 0 && argc <= 2) { - struct branch *branch = branch_get(argv[0]); - - if (!branch) - die(_("no such branch '%s'"), argv[0]); - if (filter.kind != FILTER_REFS_BRANCHES) die(_("-a and -r options to 'git branch' do not make sense with a branch name")); -- 2.19.0 ``` ## Jeff King, 2018-10-15 17:14 Subject: Re: [Question] builtin/branch.c Message-ID: <20181015171417.GA1301@sigill.intra.peff.net> URL: https://gitlist.dev/e/20181015171417.GA1301%40sigill.intra.peff.net In-Reply-To: ``` On Sun, Oct 14, 2018 at 12:19:35PM +0200, Ævar Arnfjörð Bjarmason wrote: > On Sat, Oct 13, 2018 at 10:12 AM Tao Qingyun wrote: > > Hi, I am learning `builtin/branch.c`. I find that it will call `branch_get` > > before create and [un]set upstream, and die with "no such branch" if failed. > > but `branch_get` seems never fail, it is a get_or_create. Also, it was > > confused that getting a branch before it has created. > > > > builtin/branch.c #811 > > > > } else if (argc > 0 && argc <= 2) { > > struct branch *branch = branch_get(argv[0]); > > > > if (!branch) > > die(_("no such branch '%s'"), argv[0]); > > From my reading of the source you're correct. That !branch case is > pointless. The only way that function can fail is in the x*() family > of functions, which'll make the function die instead of returning > NULL. It sometimes returns current_branch, which can be NULL (e.g., if you're on a detached HEAD). Try: $ git branch HEAD fatal: no such branch 'HEAD' $ git branch '' fatal: no such branch '' However, it seems weird that we'd check those cases here (and provide such lousy messages). And indeed, dropping that and letting us eventually hit create_branch() gives a much better message: $ git branch HEAD fatal: 'HEAD' is not a valid branch name. $ git branch '' fatal: '' is not a valid branch name. I think we'd want to see that reasoning in the commit message. -Peff ``` ## Jeff King, 2018-10-15 17:17 Subject: Re: [PATCH] builtin/branch.c: remove useless branch_get Message-ID: <20181015171707.GB1301@sigill.intra.peff.net> URL: https://gitlist.dev/e/20181015171707.GB1301%40sigill.intra.peff.net In-Reply-To: <20181015140839.4925-1-taoqy@ls-a.me> ``` On Mon, Oct 15, 2018 at 10:08:39PM +0800, Tao Qingyun wrote: > Signed-off-by: Tao Qingyun The commit message should describe the "why" here. I gave some reasoning nearby in: https://public-inbox.org/git/20181015171417.GA1301@sigill.intra.peff.net/ From your initial message, it sounds like this might also be fixing a bug ("confusing that getting a branch before it has created"). Can you describe that (and ideally show the fix with a test)? > diff --git a/builtin/branch.c b/builtin/branch.c > index c396c41533..2367703034 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -809,11 +809,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > git_config_set_multivar(buf.buf, NULL, NULL, 1); > strbuf_release(&buf); > } else if (argc > 0 && argc <= 2) { > - struct branch *branch = branch_get(argv[0]); > - > - if (!branch) > - die(_("no such branch '%s'"), argv[0]); > - From what I can tell, the patch itself _is_ an improvement. I just think we need to explain why for the record. -Peff ``` ## Tao Qingyun, 2018-10-16 14:54 Subject: [PATCH v2] builtin/branch.c: remove useless branch_get Message-ID: <20181016145428.2901-1-taoqy@ls-a.me> URL: https://gitlist.dev/e/20181016145428.2901-1-taoqy%40ls-a.me In-Reply-To: <20181015171707.GB1301@sigill.intra.peff.net> ``` branch_get sometimes returns current_branch, which can be NULL (e.g., if you're on a detached HEAD). Try: $ git branch HEAD fatal: no such branch 'HEAD' $ git branch '' fatal: no such branch '' However, it seems weird that we'd check those cases here (and provide such lousy messages). And indeed, dropping that and letting us eventually hit create_branch() gives a much better message: $ git branch HEAD fatal: 'HEAD' is not a valid branch name. $ git branch '' fatal: '' is not a valid branch name. Signed-off-by: Tao Qingyun --- builtin/branch.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index c396c41533..2367703034 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -809,11 +809,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix) git_config_set_multivar(buf.buf, NULL, NULL, 1); strbuf_release(&buf); } else if (argc > 0 && argc <= 2) { - struct branch *branch = branch_get(argv[0]); - - if (!branch) - die(_("no such branch '%s'"), argv[0]); - if (filter.kind != FILTER_REFS_BRANCHES) die(_("-a and -r options to 'git branch' do not make sense with a branch name")); -- 2.19.0 ``` ## Jeff King, 2018-10-17 08:39 Subject: Re: [PATCH v2] builtin/branch.c: remove useless branch_get Message-ID: <20181017083906.GC31932@sigill.intra.peff.net> URL: https://gitlist.dev/e/20181017083906.GC31932%40sigill.intra.peff.net In-Reply-To: <20181016145428.2901-1-taoqy@ls-a.me> ``` On Tue, Oct 16, 2018 at 10:54:28PM +0800, Tao Qingyun wrote: > branch_get sometimes returns current_branch, which can be NULL (e.g., if > you're on a detached HEAD). Try: > > $ git branch HEAD > fatal: no such branch 'HEAD' > > $ git branch '' > fatal: no such branch '' > > However, it seems weird that we'd check those cases here (and provide > such lousy messages). And indeed, dropping that and letting us > eventually hit create_branch() gives a much better message: > > $ git branch HEAD > fatal: 'HEAD' is not a valid branch name. > > $ git branch '' > fatal: '' is not a valid branch name. This explanation is perfect, of course. ;) I still wondered if you had another motivation hinted at in your original mail, though (some weirdness with running branch_get early). It's OK if there isn't one, but I just want to make sure we capture all of the details. Other than that question, the patch looks good to me. -Peff ``` ## Tao Qingyun, 2018-10-17 12:58 Subject: Re: [PATCH v2] builtin/branch.c: remove useless branch_get Message-ID: <20181017125818.1872-1-taoqy@ls-a.me> URL: https://gitlist.dev/e/20181017125818.1872-1-taoqy%40ls-a.me In-Reply-To: <20181017083906.GC31932@sigill.intra.peff.net> ``` > > branch_get sometimes returns current_branch, which can be NULL (e.g., if > > you're on a detached HEAD). Try: > > > > $ git branch HEAD > > fatal: no such branch 'HEAD' > > > > $ git branch '' > > fatal: no such branch '' > > > > However, it seems weird that we'd check those cases here (and provide > > such lousy messages). And indeed, dropping that and letting us > > eventually hit create_branch() gives a much better message: > > > > $ git branch HEAD > > fatal: 'HEAD' is not a valid branch name. > > > > $ git branch '' > > fatal: '' is not a valid branch name. > > This explanation is perfect, of course. ;) > > I still wondered if you had another motivation hinted at in your > original mail, though (some weirdness with running branch_get early). > It's OK if there isn't one, but I just want to make sure we capture all > of the details. > Yes, this explanation is perfect. ;) > Other than that question, the patch looks good to me. > > -Peff ```