* [Question] builtin/branch.c
@ 2018-10-13 8:11 Tao Qingyun
2018-10-14 10:19 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 8+ messages in thread
From: Tao Qingyun @ 2018-10-13 8:11 UTC (permalink / raw)
To: git
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);
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Question] builtin/branch.c
2018-10-13 8:11 [Question] builtin/branch.c Tao Qingyun
@ 2018-10-14 10:19 ` Ævar Arnfjörð Bjarmason
2018-10-15 14:08 ` [PATCH] builtin/branch.c: remove useless branch_get Tao Qingyun
2018-10-15 17:14 ` [Question] builtin/branch.c Jeff King
0 siblings, 2 replies; 8+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-10-14 10:19 UTC (permalink / raw)
To: taoqy; +Cc: Git Mailing List
On Sat, Oct 13, 2018 at 10:12 AM Tao Qingyun <taoqy@ls-a.me> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH] builtin/branch.c: remove useless branch_get
2018-10-14 10:19 ` Ævar Arnfjörð Bjarmason
@ 2018-10-15 14:08 ` Tao Qingyun
2018-10-15 17:17 ` Jeff King
2018-10-15 17:14 ` [Question] builtin/branch.c Jeff King
1 sibling, 1 reply; 8+ messages in thread
From: Tao Qingyun @ 2018-10-15 14:08 UTC (permalink / raw)
To: gitster; +Cc: git, avarab, Tao Qingyun
Signed-off-by: Tao Qingyun <taoqy@ls-a.me>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] builtin/branch.c: remove useless branch_get
2018-10-15 14:08 ` [PATCH] builtin/branch.c: remove useless branch_get Tao Qingyun
@ 2018-10-15 17:17 ` Jeff King
2018-10-16 14:54 ` [PATCH v2] " Tao Qingyun
0 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2018-10-15 17:17 UTC (permalink / raw)
To: Tao Qingyun; +Cc: gitster, git, avarab
On Mon, Oct 15, 2018 at 10:08:39PM +0800, Tao Qingyun wrote:
> Signed-off-by: Tao Qingyun <taoqy@ls-a.me>
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2] builtin/branch.c: remove useless branch_get
2018-10-15 17:17 ` Jeff King
@ 2018-10-16 14:54 ` Tao Qingyun
2018-10-17 8:39 ` Jeff King
0 siblings, 1 reply; 8+ messages in thread
From: Tao Qingyun @ 2018-10-16 14:54 UTC (permalink / raw)
To: peff; +Cc: avarab, git, gitster, Tao Qingyun
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 <taoqy@ls-a.me>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2] builtin/branch.c: remove useless branch_get
2018-10-16 14:54 ` [PATCH v2] " Tao Qingyun
@ 2018-10-17 8:39 ` Jeff King
2018-10-17 12:58 ` Tao Qingyun
0 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2018-10-17 8:39 UTC (permalink / raw)
To: Tao Qingyun; +Cc: avarab, git, gitster
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] builtin/branch.c: remove useless branch_get
2018-10-17 8:39 ` Jeff King
@ 2018-10-17 12:58 ` Tao Qingyun
0 siblings, 0 replies; 8+ messages in thread
From: Tao Qingyun @ 2018-10-17 12:58 UTC (permalink / raw)
To: peff; +Cc: avarab, git, gitster
> > 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Question] builtin/branch.c
2018-10-14 10:19 ` Ævar Arnfjörð Bjarmason
2018-10-15 14:08 ` [PATCH] builtin/branch.c: remove useless branch_get Tao Qingyun
@ 2018-10-15 17:14 ` Jeff King
1 sibling, 0 replies; 8+ messages in thread
From: Jeff King @ 2018-10-15 17:14 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: taoqy, Git Mailing List
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 <taoqy@ls-a.me> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-10-17 12:58 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-13 8:11 [Question] builtin/branch.c Tao Qingyun
2018-10-14 10:19 ` Ævar Arnfjörð Bjarmason
2018-10-15 14:08 ` [PATCH] builtin/branch.c: remove useless branch_get Tao Qingyun
2018-10-15 17:17 ` Jeff King
2018-10-16 14:54 ` [PATCH v2] " Tao Qingyun
2018-10-17 8:39 ` Jeff King
2018-10-17 12:58 ` Tao Qingyun
2018-10-15 17:14 ` [Question] builtin/branch.c Jeff King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).