{"thread":{"id":"46326","subject":"bug with check-ref-format outside of repository","startedAt":"2017-07-08T01:18:02Z","lastAt":"2017-10-18T05:10:21Z","messageCount":36,"participants":["Marko Kungla","Jeff King","Jonathan Nieder","Junio C Hamano","Kevin Daudt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"323983","messageId":"CAKY_R-uk9hpR2hbkPsw2cqoMo6bQKoyp6cWTO20L3fOWfLW2-Q@mail.gmail.com","threadId":"46326","inReplyTo":null,"subject":"bug with check-ref-format outside of repository","fromName":"Marko Kungla","fromEmail":"marko.kungla@gmail.com","sentAt":"2017-07-08T01:17:32Z","receivedAt":"2017-07-08T01:18:02Z","isPatch":false,"sender":{"key":"marko.kungla@gmail.com","avatar":null},"body":"Hi,\n\nAs contrived e.g: if I have in my \"workspace\" sub directories mostly\ngit repositories, but some not and if I exec,\n\"for d in */ ; do (cd $d && git check-ref-format --branch @{-1});\ndone\" then I get 3 possible responses.\n\ngit version: 2.13.0\n1. Valid branch name\n2. fatal: '@{-1}' is not a valid branch name\n3. fatal: BUG: setup_git_env called without repository\n\ngit version 2.13.2.915.ga9c46e097\n1. Valid branch name\n2. fatal: '@{-1}' is not a valid branch name\n3. BUG: environment.c:178: git environment hasn't been setup\n\nbest marko\n"},{"id":"324543","messageId":"20170714180313.apsnbnw7no2nvtf5@sigill.intra.peff.net","threadId":"46326","inReplyTo":"CAKY_R-uk9hpR2hbkPsw2cqoMo6bQKoyp6cWTO20L3fOWfLW2-Q@mail.gmail.com","subject":"Re: bug with check-ref-format outside of repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-14T18:03:13Z","receivedAt":"2017-07-14T18:03:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 08, 2017 at 03:17:32AM +0200, Marko Kungla wrote:\n\n> As contrived e.g: if I have in my \"workspace\" sub directories mostly\n> git repositories, but some not and if I exec,\n> \"for d in */ ; do (cd $d && git check-ref-format --branch @{-1});\n> done\" then I get 3 possible responses.\n> \n> git version: 2.13.0\n> 1. Valid branch name\n> 2. fatal: '@{-1}' is not a valid branch name\n> 3. fatal: BUG: setup_git_env called without repository\n> \n> git version 2.13.2.915.ga9c46e097\n> 1. Valid branch name\n> 2. fatal: '@{-1}' is not a valid branch name\n> 3. BUG: environment.c:178: git environment hasn't been setup\n\nThanks for the report. It's right to return an error, but obviously we\nshould never hit the BUG() in the lazy-setup code.\n\nI think \"check-ref-format --branch\" is inherently a repository-only\noperation, since its purpose is to look in the reflog with @{-1} and\nsimilar branch-substitutions. Technically you can do:\n\n $ cd /not/a/git/repo\n $ git check-ref-format --branch 'refs/heads/foo'\n\nbut I'm not sure why you would want to. So I think the patch below is\nprobably the right direction. The other alternative is for\ninterpret_nth_prior_checkout() to detect the \"not in a repo\" case and\nquietly return \"nope, we couldn't find such a reflog entry\".\n\n+cc Jonathan, who added the original call in 49cc460d8 (Allow\n\"check-ref-format --branch\" from subdirectory, 2010-08-05). That commit\nmessage doesn't give any indication of why we used the gently form.\nLooking back at the original thread[1], I suspect it was mostly out of\nconservatism.\n\n[1] https://public-inbox.org/git/20100806033922.GS22369@burratino/\n\n---\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex eac499450..1e5f9835f 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -39,9 +39,8 @@ static char *collapse_slashes(const char *refname)\n static int check_ref_format_branch(const char *arg)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint nongit;\n \n-\tsetup_git_directory_gently(&nongit);\n+\tsetup_git_directory();\n \tif (strbuf_check_branch_ref(&sb, arg))\n \t\tdie(\"'%s' is not a valid branch name\", arg);\n \tprintf(\"%s\\n\", sb.buf + 11);\ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex 0790edf60..1674b061e 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -161,6 +161,10 @@ test_expect_success 'check-ref-format --branch from subdir' '\n \ttest \"$refname\" = \"$sha1\"\n '\n \n+test_expect_success 'check-ref-format --branch from non-repo' '\n+\ttest_must_fail nongit git check-ref-format --branch @{-1}\n+'\n+\n valid_ref_normalized() {\n \tprereq=\n \tcase $1 in\n"},{"id":"324544","messageId":"20170714181831.fvi2coppzhm747mk@sigill.intra.peff.net","threadId":"46326","inReplyTo":"20170714180313.apsnbnw7no2nvtf5@sigill.intra.peff.net","subject":"[PATCH] check-ref-format: require a repository for --branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-14T18:18:31Z","receivedAt":"2017-07-14T18:18:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 14, 2017 at 02:03:13PM -0400, Jeff King wrote:\n\n> So I think the patch below is probably the right direction.\n\nAnd here it is with a real commit message, if this is what we want to\ndo.\n\nI do feel like \"check-ref-format --branch\" is a misfeature. The\noperation seems to fit better with \"rev-parse\" (which yes, is a kitchen\nsink, but it does all sorts of similar resolution operations). I think\n\"git rev-parse --symbolic-full-name @{-1}\" is basically the same thing\n(modulo the refs/heads/ shortening).\n\n-- >8 --\nSubject: [PATCH] check-ref-format: require a repository for --branch\n\nWhen the user asks \"--branch\" to interpret a branch name\nlike \"@{-1}\", we have to dig the answer out of the HEAD\nreflog. We can obviously only do that if we have a\nrepository, and indeed, running it outside a repository\ncauses us to hit a BUG().\n\nWe basically have two options:\n\n 1. We can define \"@{-N}\" outside of a repository as \"no\n such branch\" and die with \"not a valid brach name\".\n\n 2. We can just declare that \"--branch\" must be run inside\n a repository, in which case we die with \"not a git\n repository\".\n\nThe effect is more or less the same for \"@{-N}\".\nTechnically one can use \"--branch\" outside of a repository\nas long as you don't use any names that actually need\ninterpreting. But since intrpreting is the option's\ndocumented purpose, there doesn't seem any point in trying\nto resolve vanilla names like \"foo\" (which we end up just\nprinting to stdout verbatim).\n\nSo let's go with option 2.\n\nSigned-off-by: Jeff King \n---\n builtin/check-ref-format.c | 3 +--\n t/t1402-check-ref-format.sh | 4 ++++\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex eac499450..1e5f9835f 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -39,9 +39,8 @@ static char *collapse_slashes(const char *refname)\n static int check_ref_format_branch(const char *arg)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint nongit;\n \n-\tsetup_git_directory_gently(&nongit);\n+\tsetup_git_directory();\n \tif (strbuf_check_branch_ref(&sb, arg))\n \t\tdie(\"'%s' is not a valid branch name\", arg);\n \tprintf(\"%s\\n\", sb.buf + 11);\ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex 0790edf60..1674b061e 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -161,6 +161,10 @@ test_expect_success 'check-ref-format --branch from subdir' '\n \ttest \"$refname\" = \"$sha1\"\n '\n \n+test_expect_success 'check-ref-format --branch from non-repo' '\n+\ttest_must_fail nongit git check-ref-format --branch @{-1}\n+'\n+\n valid_ref_normalized() {\n \tprereq=\n \tcase $1 in\n-- \n2.14.0.rc0.457.g08326e0ba\n\n"},{"id":"324650","messageId":"20170717172709.GL93855@aiede.mtv.corp.google.com","threadId":"46326","inReplyTo":"20170714181831.fvi2coppzhm747mk@sigill.intra.peff.net","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-07-17T17:27:09Z","receivedAt":"2017-07-17T17:27:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Fri, Jul 14, 2017 at 02:03:13PM -0400, Jeff King wrote:\n\n>> So I think the patch below is probably the right direction.\n>\n> And here it is with a real commit message, if this is what we want to\n> do.\n[...]\n> --- a/t/t1402-check-ref-format.sh\n> +++ b/t/t1402-check-ref-format.sh\n> @@ -161,6 +161,10 @@ test_expect_success 'check-ref-format --branch from subdir' '\n> \ttest \"$refname\" = \"$sha1\"\n> '\n> \n> +test_expect_success 'check-ref-format --branch from non-repo' '\n> +\ttest_must_fail nongit git check-ref-format --branch @{-1}\n> +'\n> +\n> valid_ref_normalized() {\n> \tprereq=\n> \tcase $1 in\n\nI don't think it's right. Today I can do\n\n\t$ cd /tmp\n\t$ git check-ref-format --branch master\n\tmaster\n\nYou might wonder why I'd ever do such a thing. But that's what \"git\ncheck-ref-format --branch\" is for --- it is for taking a \nargument and turning it into a branch name. For example, if you have\na script with an $opt_branch variable that defaults to \"master\", it\nmay do\n\n\tresolved_branch=$(git check-ref-format --branch \"$opt_branch\")\n\neven though it is in a mode that not going to have to use\n$resolved_branch and it is not running from a repository.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"324684","messageId":"CAKY_R-vsZo_OsA8F6DVx_4zi5=RpBfOn-w1Ou7pq-F7HTtptzg@mail.gmail.com","threadId":"46326","inReplyTo":"20170717172709.GL93855@aiede.mtv.corp.google.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Marko Kungla","fromEmail":"marko.kungla@gmail.com","sentAt":"2017-07-17T21:18:43Z","receivedAt":"2017-07-17T21:19:46Z","isPatch":true,"sender":{"key":"marko.kungla@gmail.com","avatar":null},"body":"I guess that should only be about that it should not hit a (BUG).\nIn my case in the example I gave I scan trough the directories to\ncheck repository status one of the tasks make use of check-ref-format.\nSince it may hit directory which is not a git repository it should not\nexpose error (BUG) right.\n\nOn Mon, Jul 17, 2017 at 7:27 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Jeff King wrote:\n>> On Fri, Jul 14, 2017 at 02:03:13PM -0400, Jeff King wrote:\n>\n>>> So I think the patch below is probably the right direction.\n>>\n>> And here it is with a real commit message, if this is what we want to\n>> do.\n> [...]\n>> --- a/t/t1402-check-ref-format.sh\n>> +++ b/t/t1402-check-ref-format.sh\n>> @@ -161,6 +161,10 @@ test_expect_success 'check-ref-format --branch from subdir' '\n>> test \"$refname\" = \"$sha1\"\n>> '\n>>\n>> +test_expect_success 'check-ref-format --branch from non-repo' '\n>> + test_must_fail nongit git check-ref-format --branch @{-1}\n>> +'\n>> +\n>> valid_ref_normalized() {\n>> prereq=\n>> case $1 in\n>\n> I don't think it's right. Today I can do\n>\n> $ cd /tmp\n> $ git check-ref-format --branch master\n> master\n>\n> You might wonder why I'd ever do such a thing. But that's what \"git\n> check-ref-format --branch\" is for --- it is for taking a \n> argument and turning it into a branch name. For example, if you have\n> a script with an $opt_branch variable that defaults to \"master\", it\n> may do\n>\n> resolved_branch=$(git check-ref-format --branch \"$opt_branch\")\n>\n> even though it is in a mode that not going to have to use\n> $resolved_branch and it is not running from a repository.\n>\n> Thanks and hope that helps,\n> Jonathan\n\n\n\n-- \nMail: marko.kungla@gmail.com\nPhone: +31 (0) 6 2546 0117\n"},{"id":"326594","messageId":"20170817102217.3yw7uxnkupdy3lh5@sigill.intra.peff.net","threadId":"46326","inReplyTo":"20170717172709.GL93855@aiede.mtv.corp.google.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-17T10:22:17Z","receivedAt":"2017-08-17T10:22:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 17, 2017 at 10:27:09AM -0700, Jonathan Nieder wrote:\n\n> > --- a/t/t1402-check-ref-format.sh\n> > +++ b/t/t1402-check-ref-format.sh\n> > @@ -161,6 +161,10 @@ test_expect_success 'check-ref-format --branch from subdir' '\n> > \ttest \"$refname\" = \"$sha1\"\n> > '\n> > \n> > +test_expect_success 'check-ref-format --branch from non-repo' '\n> > +\ttest_must_fail nongit git check-ref-format --branch @{-1}\n> > +'\n> > +\n> > valid_ref_normalized() {\n> > \tprereq=\n> > \tcase $1 in\n> \n> I don't think it's right. Today I can do\n> \n> \t$ cd /tmp\n> \t$ git check-ref-format --branch master\n> \tmaster\n> \n> You might wonder why I'd ever do such a thing. But that's what \"git\n> check-ref-format --branch\" is for --- it is for taking a \n> argument and turning it into a branch name. For example, if you have\n> a script with an $opt_branch variable that defaults to \"master\", it\n> may do\n> \n> \tresolved_branch=$(git check-ref-format --branch \"$opt_branch\")\n> \n> even though it is in a mode that not going to have to use\n> $resolved_branch and it is not running from a repository.\n\nI'm not sure I buy that. What does \"turning it into a branch name\" even\nmean when you are not in a repository? Clearly @{-1} must fail. And\neverything else is just going to output the exact input you provided.\nSo any script calling \"check-ref-format --branch\" outside of a repo\nwould be better off not calling it at all. At best it does nothing, and\nat worst it's going to give a confusing error when $opt_branch is\nsomething like \"@{-1}\".\n\nA more compelling argument along these lines is something like:\n\n Accepting --branch outside of a repo is pointless, but it's something\n we've historically accepted. To avoid breaking existing scripts (even\n if they are doing something pointless), we'll continue to allow it.\n\nI'm not sure I buy _that_ line of reasoning either, but it at least\nmakes sense to me (I just think it isn't worth the complexity of trying\nto audit the innards of interpret_branch_name()).\n\n-Peff\n"},{"id":"326595","messageId":"20170817102358.yiugmv5je6sfw4lo@sigill.intra.peff.net","threadId":"46326","inReplyTo":"CAKY_R-vsZo_OsA8F6DVx_4zi5=RpBfOn-w1Ou7pq-F7HTtptzg@mail.gmail.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-17T10:23:59Z","receivedAt":"2017-08-17T10:24:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 17, 2017 at 11:18:43PM +0200, Marko Kungla wrote:\n\n> I guess that should only be about that it should not hit a (BUG).\n> In my case in the example I gave I scan trough the directories to\n> check repository status one of the tasks make use of check-ref-format.\n> Since it may hit directory which is not a git repository it should not\n> expose error (BUG) right.\n\nRight. The BUG should definitely be corrected. Between what Jonathan is\nsuggesting and my patch, either would be fine for the case you described\noriginally (\"--branch @{-1}\" would always fail in a non-repo).\n\n-Peff\n"},{"id":"326645","messageId":"xmqqy3qhevaq.fsf@gitster.mtv.corp.google.com","threadId":"46326","inReplyTo":"20170817102217.3yw7uxnkupdy3lh5@sigill.intra.peff.net","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-17T21:30:53Z","receivedAt":"2017-08-17T21:31:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jul 17, 2017 at 10:27:09AM -0700, Jonathan Nieder wrote:\n>\n>> > --- a/t/t1402-check-ref-format.sh\n>> > +++ b/t/t1402-check-ref-format.sh\n>> > @@ -161,6 +161,10 @@ test_expect_success 'check-ref-format --branch from subdir' '\n>> > \ttest \"$refname\" = \"$sha1\"\n>> > '\n>> > \n>> > +test_expect_success 'check-ref-format --branch from non-repo' '\n>> > +\ttest_must_fail nongit git check-ref-format --branch @{-1}\n>> > +'\n>> > +\n>> > valid_ref_normalized() {\n>> > \tprereq=\n>> > \tcase $1 in\n>> \n>> I don't think it's right. Today I can do\n>> \n>> \t$ cd /tmp\n>> \t$ git check-ref-format --branch master\n>> \tmaster\n>> \n>> You might wonder why I'd ever do such a thing. But that's what \"git\n>> check-ref-format --branch\" is for --- it is for taking a \n>> argument and turning it into a branch name. For example, if you have\n>> a script with an $opt_branch variable that defaults to \"master\", it\n>> may do\n>> \n>> \tresolved_branch=$(git check-ref-format --branch \"$opt_branch\")\n>> \n>> even though it is in a mode that not going to have to use\n>> $resolved_branch and it is not running from a repository.\n>\n> I'm not sure I buy that. What does \"turning it into a branch name\" even\n> mean when you are not in a repository? Clearly @{-1} must fail. And\n> everything else is just going to output the exact input you provided.\n\nThis \"just going to output the exact input\" is not entirely correct;\nthere is just one use case for it.\n\n\"git check-ref-format --branch a..b\" would fail with a helpful error\nmessage, while the command run with \"a.b\" would tell you the name is\nsafe.\n\nUse of 'check-ref-format --branch \"@{-1}\"' *IS* a nonsense, whether\nit is inside or outside a repository; it is OK to fail it outside a\nrepository and I would say it is even OK to fail it inside a\nrepository. After all \"check-ref-format\" is about checking if the\nstring is a sensible name to use.\n\nI think calling interpret_branch_name() in the codepath is a mistake\nand we should instead report if \"refs/heads/@{-1}\" literally is a\nvalid refname we could create instead.\n"},{"id":"326673","messageId":"20170818062021.gqhsose5kepth55h@sigill.intra.peff.net","threadId":"46326","inReplyTo":"xmqqy3qhevaq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-18T06:20:22Z","receivedAt":"2017-08-18T06:20:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 17, 2017 at 02:30:53PM -0700, Junio C Hamano wrote:\n\n> > I'm not sure I buy that. What does \"turning it into a branch name\" even\n> > mean when you are not in a repository? Clearly @{-1} must fail. And\n> > everything else is just going to output the exact input you provided.\n> \n> This \"just going to output the exact input\" is not entirely correct;\n> there is just one use case for it.\n> \n> \"git check-ref-format --branch a..b\" would fail with a helpful error\n> message, while the command run with \"a.b\" would tell you the name is\n> safe.\n\nWell, yes. It's checking the syntax, as well. But you don't need\n--branch for that.\n\n> Use of 'check-ref-format --branch \"@{-1}\"' *IS* a nonsense, whether\n> it is inside or outside a repository; it is OK to fail it outside a\n> repository and I would say it is even OK to fail it inside a\n> repository. After all \"check-ref-format\" is about checking if the\n> string is a sensible name to use.\n> \n> I think calling interpret_branch_name() in the codepath is a mistake\n> and we should instead report if \"refs/heads/@{-1}\" literally is a\n> valid refname we could create instead.\n\nI don't think it's nonsense. It's the reason the feature was added in\nthe first place. See your own a31dca0393 (check-ref-format --branch:\ngive Porcelain a way to grok branch shorthand, 2009-03-21). Without\nthat interpretation, it does nothing that you could not equally well do\nas:\n\n git check-ref-format refs/heads/$name\n\nI _do_ think it's a misfeature to put it in check-ref-format. It should\nbe part of rev-parse. Which admittedly is a kitchen sink, but this kind\nof resolving is one of the main things it should be doing. And in fact\nyou can already do:\n\n git rev-parse --symbolic-full-name @{-1}\n\nBut I stopped short of suggesting we remove the feature entirely. It\nwould obviously require a deprecation period.\n\n-Peff\n"},{"id":"326681","messageId":"xmqqvallco9v.fsf@gitster.mtv.corp.google.com","threadId":"46326","inReplyTo":"20170818062021.gqhsose5kepth55h@sigill.intra.peff.net","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-18T07:45:32Z","receivedAt":"2017-08-18T07:45:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I _do_ think it's a misfeature to put it in check-ref-format. It should\n> be part of rev-parse. Which admittedly is a kitchen sink, but this kind\n> of resolving is one of the main things it should be doing. And in fact\n> you can already do:\n>\n> git rev-parse --symbolic-full-name @{-1}\n>\n> But I stopped short of suggesting we remove the feature entirely. It\n> would obviously require a deprecation period.\n\nYeah, I realize that \"nonsense\" was a bit too strong. I do agree\nthat it was a misfeature to place in \"check\" ref-format, though.\n\nThanks.\n"},{"id":"330445","messageId":"xmqqinffsibr.fsf@gitster.mtv.corp.google.com","threadId":"46326","inReplyTo":"20170817102217.3yw7uxnkupdy3lh5@sigill.intra.peff.net","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-16T06:44:08Z","receivedAt":"2017-10-16T06:44:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jul 17, 2017 at 10:27:09AM -0700, Jonathan Nieder wrote:\n>> ...\n>> I don't think it's right. Today I can do\n>> \n>> \t$ cd /tmp\n>> \t$ git check-ref-format --branch master\n>> \tmaster\n>> \n>> You might wonder why I'd ever do such a thing. But that's what \"git\n>> check-ref-format --branch\" is for --- it is for taking a \n>> argument and turning it into a branch name. For example, if you have\n>> a script with an $opt_branch variable that defaults to \"master\", it\n>> may do\n>> \n>> \tresolved_branch=$(git check-ref-format --branch \"$opt_branch\")\n>> \n>> even though it is in a mode that not going to have to use\n>> $resolved_branch and it is not running from a repository.\n>\n> I'm not sure I buy that. What does \"turning it into a branch name\" even\n> mean when you are not in a repository? Clearly @{-1} must fail. And\n> everything else is just going to output the exact input you provided.\n> So any script calling \"check-ref-format --branch\" outside of a repo\n> would be better off not calling it at all.\n\nI threw this topic in stalled category, hoping that one or the other\nopinion eventually turns out to be more prevalent, but it didn't\nseem to have happened X-<.\n\nThings like @{-1} would not make any sense when the command is run\noutside a repository, and the documentation is quite clear that it\nis the primary reason why we added \"--branch\" option to the command,\ni.e.\n\n With the `--branch` option, it expands the ``previous branch syntax''\n `@{-n}`. For example, `@{-1}` is a way to refer the last branch you\n were on. This option should be used by porcelains to accept this\n syntax anywhere a branch name is expected, so they can act as if you\n typed the branch name.\n\nSo I am tempted to take this patch to make sure that we won't gain\nmore people who abuse the command outside a repository.\n\nHaving said that, there may still be a use case where a Porcelain\nscript wants a way to see if a $name it has is appropriate as a\nbranch name before it has a repository (e.g. a wrapper to \"git\nclone\" that wants to verify the name it is going to give to the \"-b\"\noption), and a check desired in such a context is different from\n(and is stricter than) feeding refs/heads/$name to the same command\nwithout the \"--branch\" option.\n\nSo I think the right endgame in the longer term is:\n\n - Find (or add if it doesn't exist) a way to recommend to Porcelain\n scripts to use to expand an end-user generated string, and to map\n it to a branch name (it may be \"rev-parse --symbolic-full-name\n $name\"; I dunno).\n\n - Keep check-ref-format as (or revert it to be) a tool to \"check\".\n This would involve split strbuf_check_branch_ref() into two:\n\n - one that does not do the @{-1} thing and is used ONLY for\n format validity check (including rejecting a name that begins\n with a dash, which is OK for a random ref but not acceptable as\n a branch name);\n\n - the other that does @{-1} thing before doing the above.\n \n and then making the code call the former, not the latter.\n\nThe end result would be that check-ref-format becomes textual check\nonly, and can be usable (agian) outside repository, with or without\n\"--branch\". As the current code does not allow us do that yet, I\nthink it is safer to forbid use of --branch outside the repository\nfor now, purely as a bugfix.\n\n\n[Footnote]\n\n*1* In a sense, @{-1} is not something the scripts need to check its\n validity of---it is the branch you came from, so by definition\n it must be with a good name. What the scripts want is instead\n see what the branch actually is, which is not what\n \"check-ref-format\" is about.\n\n a31dca03 (\"check-ref-format --branch: give Porcelain a way to\n grok branch shorthand\", 2009-03-21) says:\n\n The command may not be the best place to add this new feature, but\n \n $ git check-ref-format --branch \"@{-1}\"\n \n allows Porcelains to figure out what branch you were on before the last\n branch switching.\n"},{"id":"330449","messageId":"xmqq1sm3s751.fsf@gitster.mtv.corp.google.com","threadId":"46326","inReplyTo":"xmqqinffsibr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-16T10:45:46Z","receivedAt":"2017-10-16T10:45:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Having said that, there may still be a use case where a Porcelain\n> script wants a way to see if a $name it has is appropriate as a\n> branch name before it has a repository (e.g. a wrapper to \"git\n> clone\" that wants to verify the name it is going to give to the \"-b\"\n> option), and a check desired in such a context is different from\n> (and is stricter than) feeding refs/heads/$name to the same command\n> without the \"--branch\" option.\n>\n> So I think the right endgame in the longer term is:\n> ...\n\nHere is to illustrate what I mean in a patch form. It resurrects\nthe gentle setup, and uses a purely textual format check when we are\noutside the repository, while bypassing the @{magic} interpolation\ncodepath that requires us to be in a repository. When we are in a\nrepository, we operate the same way as before.\n\nDesigned to be applied directly on top of 4d03f955\n(\"check-ref-format: require a repository for --branch\", 2017-07-14),\nso it is missing the \"'HEAD' is not a good branch name\" I showed a\nfew days ago.\n\n builtin/check-ref-format.c | 16 +++++++++++++---\n cache.h | 14 ++++++++++++++\n sha1_name.c | 22 +++++++++++++++++++---\n strbuf.h | 6 ++++++\n t/t1402-check-ref-format.sh | 10 +++++++++-\n 5 files changed, 61 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex 1e5f9835f0..4e62852089 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -38,12 +38,22 @@ static char *collapse_slashes(const char *refname)\n \n static int check_ref_format_branch(const char *arg)\n {\n+\tint nongit, malformed;\n \tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *name = arg;\n \n-\tsetup_git_directory();\n-\tif (strbuf_check_branch_ref(&sb, arg))\n+\tsetup_git_directory_gently(&nongit);\n+\n+\tif (!nongit)\n+\t\tmalformed = (strbuf_check_branch_ref(&sb, arg) ||\n+\t\t\t !skip_prefix(sb.buf, \"refs/heads/\", &name));\n+\telse\n+\t\tmalformed = check_branch_ref_format(arg);\n+\n+\tif (malformed)\n \t\tdie(\"'%s' is not a valid branch name\", arg);\n-\tprintf(\"%s\\n\", sb.buf + 11);\n+\tprintf(\"%s\\n\", name);\n+\tstrbuf_release(&sb);\n \treturn 0;\n }\n \ndiff --git a/cache.h b/cache.h\nindex 52b91f5b64..3815925122 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1444,6 +1444,20 @@ extern int parse_oid_hex(const char *hex, struct object_id *oid, const char **en\n #define INTERPRET_BRANCH_HEAD (1<<2)\n extern int interpret_branch_name(const char *str, int len, struct strbuf *,\n \t\t\t\t unsigned allowed);\n+\n+/*\n+ * NEEDSWORK: declare strbuf_branchname() and strbuf_check_branch_ref()\n+ * here, not in strbuf.h\n+ */\n+\n+/*\n+ * Check if a 'name' is suitable to be used as a branch name; this can\n+ * be and is stricter than what check_refname_format() returns for a\n+ * string that is a concatenation of \"name\" after \"refs/heads/\"; a\n+ * name that begins with \"-\" is not allowed, for example.\n+ */\n+extern int check_branch_ref_format(const char *name);\n+\n extern int get_oid_mb(const char *str, struct object_id *oid);\n \n extern int validate_headref(const char *ref);\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 5e2ec37b65..c95080258f 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1319,15 +1319,31 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)\n \tstrbuf_add(sb, name + used, len - used);\n }\n \n-int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n+static int strbuf_check_branch_ref_format(struct strbuf *sb)\n {\n-\tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n-\tif (name[0] == '-')\n+\tif (*sb->buf == '-')\n \t\treturn -1;\n \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\n \treturn check_refname_format(sb->buf, 0);\n }\n \n+int check_branch_ref_format(const char *name)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint result;\n+\n+\tstrbuf_addstr(&sb, name);\n+\tresult = strbuf_check_branch_ref_format(&sb);\n+\tstrbuf_release(&sb);\n+\treturn result;\n+}\n+\n+int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n+{\n+\tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n+\treturn strbuf_check_branch_ref_format(sb);\n+}\n+\n /*\n * This is like \"get_sha1_basic()\", except it allows \"sha1 expressions\",\n * notably \"xyz^\" for \"parent of xyz\"\ndiff --git a/strbuf.h b/strbuf.h\nindex d785258649..3da95685b2 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -568,6 +568,12 @@ static inline void strbuf_complete_line(struct strbuf *sb)\n \tstrbuf_complete(sb, '\\n');\n }\n \n+/*\n+ * NEEDSWORK: the following two functions should not be in this file;\n+ * these are about refnames, and should be declared next to\n+ * interpret_branch_name() in cache.h\n+ */\n+\n /*\n * Copy \"name\" to \"sb\", expanding any special @-marks as handled by\n * interpret_branch_name(). The result is a non-qualified branch name\ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex 1674b061e2..2ddefb4bd1 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -161,10 +161,18 @@ test_expect_success 'check-ref-format --branch from subdir' '\n \ttest \"$refname\" = \"$sha1\"\n '\n \n-test_expect_success 'check-ref-format --branch from non-repo' '\n+test_expect_success 'check-ref-format --branch @{-1} from non-repo' '\n \ttest_must_fail nongit git check-ref-format --branch @{-1}\n '\n \n+test_expect_success 'check-ref-format --branch master in non-repo' '\n+\tnongit git check-ref-format --branch master\n+'\n+\n+test_expect_success 'check-ref-format --branch -naster in repo' '\n+\ttest_must_fail git check-ref-format --branch -naster\n+'\n+\n valid_ref_normalized() {\n \tprereq=\n \tcase $1 in\n"},{"id":"330485","messageId":"20171016224239.yhvivrcuzeezcz3t@sigill.intra.peff.net","threadId":"46326","inReplyTo":"xmqqinffsibr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-16T22:42:39Z","receivedAt":"2017-10-16T22:42:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 16, 2017 at 03:44:08PM +0900, Junio C Hamano wrote:\n\n> I threw this topic in stalled category, hoping that one or the other\n> opinion eventually turns out to be more prevalent, but it didn't\n> seem to have happened X-<.\n\nI think it's sufficiently obscure that nobody really cares.\n\nI admit that _I_ don't actually care myself. We should fix the BUG(),\nobviously, but between the two I could live with it either way. Mostly I\ndidn't want to go to the work to write the patch for the direction that\nI didn't think was right, and was hoping Jonathan would if he felt\nstrongly about it.\n\n> So I think the right endgame in the longer term is:\n\nI won't quote the rest of your message because I agree with it\ncompletely, in terms of the endgame we'd like to see.\n\nI'll address a few specific comments on your followup patch.\n\n-Peff\n"},{"id":"330486","messageId":"20171016224512.6fhtce5anmff577b@sigill.intra.peff.net","threadId":"46326","inReplyTo":"xmqq1sm3s751.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-16T22:45:13Z","receivedAt":"2017-10-16T22:45:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 16, 2017 at 07:45:46PM +0900, Junio C Hamano wrote:\n\n> > So I think the right endgame in the longer term is:\n> > ...\n> \n> Here is to illustrate what I mean in a patch form. It resurrects\n> the gentle setup, and uses a purely textual format check when we are\n> outside the repository, while bypassing the @{magic} interpolation\n> codepath that requires us to be in a repository. When we are in a\n> repository, we operate the same way as before.\n\nI like the state this puts us in, but there's one catch: we're\ncompletely changing the meaning of \"check-ref-format --branch\", aren't\nwe?\n\nIt is going from \"this is how you resolve @{-1}\" to \"this is how you\ncheck the validity of a potential branch name\". Do we need to pick a\ndifferent name, and/or have a deprecation period?\n\n-Peff\n"},{"id":"330490","messageId":"20171016230004.uhsyhvtltorjwu5q@aiede.mtv.corp.google.com","threadId":"46326","inReplyTo":"20171016224512.6fhtce5anmff577b@sigill.intra.peff.net","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-16T23:00:04Z","receivedAt":"2017-10-16T23:00:37Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Oct 16, 2017 at 07:45:46PM +0900, Junio C Hamano wrote:\n\n>> Here is to illustrate what I mean in a patch form. It resurrects\n>> the gentle setup, and uses a purely textual format check when we are\n>> outside the repository, while bypassing the @{magic} interpolation\n>> codepath that requires us to be in a repository. When we are in a\n>> repository, we operate the same way as before.\n>\n> I like the state this puts us in, but there's one catch: we're\n> completely changing the meaning of \"check-ref-format --branch\", aren't\n> we?\n>\n> It is going from \"this is how you resolve @{-1}\" to \"this is how you\n> check the validity of a potential branch name\". Do we need to pick a\n> different name, and/or have a deprecation period?\n\nSorry to take so long on picking this up. I'll try to make an\nalternate patch today.\n\nFor what it's worth, I don't agree with this repurposing of\n\"check-ref-format --branch\" at all. The old command already existed.\nNo one asked for the new command. At most, we could get rid of the\nold command after a deprecation period. I don't understand at all why\nit's worth the confusion of changing its meaning.\n\nThanks,\nJonathan\n"},{"id":"330499","messageId":"xmqqbml6r2js.fsf@gitster.mtv.corp.google.com","threadId":"46326","inReplyTo":"20171016224512.6fhtce5anmff577b@sigill.intra.peff.net","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-17T01:22:31Z","receivedAt":"2017-10-17T01:22:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Oct 16, 2017 at 07:45:46PM +0900, Junio C Hamano wrote:\n>\n>> > So I think the right endgame in the longer term is:\n>> > ...\n>> \n>> Here is to illustrate what I mean in a patch form. It resurrects\n>> the gentle setup, and uses a purely textual format check when we are\n>> outside the repository, while bypassing the @{magic} interpolation\n>> codepath that requires us to be in a repository. When we are in a\n>> repository, we operate the same way as before.\n>\n> I like the state this puts us in, but there's one catch: we're\n> completely changing the meaning of \"check-ref-format --branch\", aren't\n> we?\n>\n> It is going from \"this is how you resolve @{-1}\" to \"this is how you\n> check the validity of a potential branch name\". Do we need to pick a\n> different name, and/or have a deprecation period?\n\nThat was not my intention. When used in a repository, it behaves\nexactly the same as before, including @{-1} resolution part. And by\nusing strbuf_check_branch_ref(), it has always been checking the\nvalidity of a potential branch name, even though it wasn't\nadvertised as such. The documentation needs to be updated, I would\nthink.\n\nWhen used outside a repository, @{-1} would not have worked anyway,\nand @{-1} continues not to work, but the part that checks the\nvalidity should continue to work.\n\nAt least that is what I wanted to happen in the patch.\n"},{"id":"330500","messageId":"20171017012718.GC19335@alpha.vpn.ikke.info","threadId":"46326","inReplyTo":"xmqq1sm3s751.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-10-17T01:27:18Z","receivedAt":"2017-10-17T01:27:25Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Mon, Oct 16, 2017 at 07:45:46PM +0900, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> [..]\n> \n> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\n> index 1e5f9835f0..4e62852089 100644\n> --- a/builtin/check-ref-format.c\n> +++ b/builtin/check-ref-format.c\n> @@ -38,12 +38,22 @@ static char *collapse_slashes(const char *refname)\n> \n> static int check_ref_format_branch(const char *arg)\n> {\n> +\tint nongit, malformed;\n> \tstruct strbuf sb = STRBUF_INIT;\n> +\tconst char *name = arg;\n> \n> -\tsetup_git_directory();\n> -\tif (strbuf_check_branch_ref(&sb, arg))\n> +\tsetup_git_directory_gently(&nongit);\n> +\n> +\tif (!nongit)\n> +\t\tmalformed = (strbuf_check_branch_ref(&sb, arg) ||\n> +\t\t\t !skip_prefix(sb.buf, \"refs/heads/\", &name));\n> +\telse\n> +\t\tmalformed = check_branch_ref_format(arg);\n> +\n\nWould it make sense to swap the logic and get rid of the double\nnegative (!nongit)?\n\n> +\tif (malformed)\n> \t\tdie(\"'%s' is not a valid branch name\", arg);\n> -\tprintf(\"%s\\n\", sb.buf + 11);\n> +\tprintf(\"%s\\n\", name);\n> +\tstrbuf_release(&sb);\n> \treturn 0;\n> }\n> \n"},{"id":"330504","messageId":"xmqq376iqyy6.fsf@gitster.mtv.corp.google.com","threadId":"46326","inReplyTo":"20171017012718.GC19335@alpha.vpn.ikke.info","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-17T02:40:17Z","receivedAt":"2017-10-17T02:40:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt writes:\n\n>> +\tsetup_git_directory_gently(&nongit);\n>> +\n>> +\tif (!nongit)\n>> +\t\tmalformed = (strbuf_check_branch_ref(&sb, arg) ||\n>> +\t\t\t !skip_prefix(sb.buf, \"refs/heads/\", &name));\n>> +\telse\n>> +\t\tmalformed = check_branch_ref_format(arg);\n>> +\n>\n> Would it make sense to swap the logic and get rid of the double\n> negative (!nongit)?\n\nI am trying to follow the pattern \"handle the normal case that have\nbeen supported forever first, and then handle new exception next\",\nso that it is easier to see that there is no behaviour change in the\nnormal case, so I do not think it makes it easier to see to swap the\nif/else cases.\n>\n>> +\tif (malformed)\n>> \t\tdie(\"'%s' is not a valid branch name\", arg);\n>> -\tprintf(\"%s\\n\", sb.buf + 11);\n>> +\tprintf(\"%s\\n\", name);\n>> +\tstrbuf_release(&sb);\n>> \treturn 0;\n>> }\n>> \n"},{"id":"330505","messageId":"20171017024203.6hqzy2paed6fyvym@sigill.intra.peff.net","threadId":"46326","inReplyTo":"xmqqbml6r2js.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-17T02:42:03Z","receivedAt":"2017-10-17T02:42:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 17, 2017 at 10:22:31AM +0900, Junio C Hamano wrote:\n\n> > I like the state this puts us in, but there's one catch: we're\n> > completely changing the meaning of \"check-ref-format --branch\", aren't\n> > we?\n> >\n> > It is going from \"this is how you resolve @{-1}\" to \"this is how you\n> > check the validity of a potential branch name\". Do we need to pick a\n> > different name, and/or have a deprecation period?\n> \n> That was not my intention. When used in a repository, it behaves\n> exactly the same as before, including @{-1} resolution part. And by\n> using strbuf_check_branch_ref(), it has always been checking the\n> validity of a potential branch name, even though it wasn't\n> advertised as such. The documentation needs to be updated, I would\n> think.\n> \n> When used outside a repository, @{-1} would not have worked anyway,\n> and @{-1} continues not to work, but the part that checks the\n> validity should continue to work.\n> \n> At least that is what I wanted to happen in the patch.\n\nAh, OK, I did not read carefully enough then. I think that would be OK,\nand probably close to what Jonathan was asking for.\n\nIt leaves unresolved the fact that the resolving feature does not belong\nin check-ref-format in the first place, but we can just accept that as a\nhistorical wart.\n\nI don't think there is any need to prepare it upon my 4d03f955, though.\nI'd think it could simply replace it.\n\n-Peff\n"},{"id":"330509","messageId":"xmqqo9p6phxg.fsf@gitster.mtv.corp.google.com","threadId":"46326","inReplyTo":"20171017024203.6hqzy2paed6fyvym@sigill.intra.peff.net","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-17T03:33:15Z","receivedAt":"2017-10-17T03:33:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Oct 17, 2017 at 10:22:31AM +0900, Junio C Hamano wrote:\n>\n>> > I like the state this puts us in, but there's one catch: we're\n>> > completely changing the meaning of \"check-ref-format --branch\", aren't\n>> > we?\n>> >\n>> > It is going from \"this is how you resolve @{-1}\" to \"this is how you\n>> > check the validity of a potential branch name\". Do we need to pick a\n>> > different name, and/or have a deprecation period?\n>> ...\n>> At least that is what I wanted to happen in the patch.\n>\n> Ah, OK, I did not read carefully enough then. I think that would be OK,\n> and probably close to what Jonathan was asking for.\n>\n> It leaves unresolved the fact that the resolving feature does not belong\n> in check-ref-format in the first place, but we can just accept that as a\n> historical wart.\n\nYup, I actually was in favor of removing that and making it a\n\"purely checking validity\" feature, but given that it has been\nadvertised in the documentation since 604e0cb5 (\"Documentation:\ndescribe check-ref-format --branch\", 2009-10-12), it is a bit too\nlate to tell users that rev-parse is the right/kosher thing to do.\n\n> I don't think there is any need to prepare it upon my 4d03f955,\n> though. I'd think it could simply replace it.\n\nYeah, it ended up that way, it seems. Still it needs a bit of doc\nupdates to balance the description. Right now we stress on @{-n}\nresolution too much.\n\nPerhaps something like this?\n\n Documentation/git-check-ref-format.txt | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt\nindex 92777cef25..cf0a0b7df2 100644\n--- a/Documentation/git-check-ref-format.txt\n+++ b/Documentation/git-check-ref-format.txt\n@@ -77,7 +77,14 @@ reference name expressions (see linkgit:gitrevisions[7]):\n \n . at-open-brace `@{` is used as a notation to access a reflog entry.\n \n-With the `--branch` option, it expands the ``previous branch syntax''\n+With the `--branch` option, the command takes a name and checks if\n+it can be used as a valid branch name (e.g. when creating a new\n+branch). The rule `git check-ref-format --branch $name` implements\n+may be stricter than what `git check-ref-format refs/heads/$name`\n+says (e.g. a dash may appear at the beginning of a ref component,\n+but it is explicitly forbidden at the beginning of a branch name).\n+When run with `--branch` option in a repository, the input is first\n+expanded for the ``previous branch syntax''\n `@{-n}`. For example, `@{-1}` is a way to refer the last branch you\n were on. This option should be used by porcelains to accept this\n syntax anywhere a branch name is expected, so they can act as if you\n"},{"id":"330517","messageId":"20171017043039.GD19335@alpha.vpn.ikke.info","threadId":"46326","inReplyTo":"xmqq376iqyy6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-10-17T04:30:39Z","receivedAt":"2017-10-17T04:30:45Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Tue, Oct 17, 2017 at 11:40:17AM +0900, Junio C Hamano wrote:\n> Kevin Daudt writes:\n> \n> >> +\tsetup_git_directory_gently(&nongit);\n> >> +\n> >> +\tif (!nongit)\n> >> +\t\tmalformed = (strbuf_check_branch_ref(&sb, arg) ||\n> >> +\t\t\t !skip_prefix(sb.buf, \"refs/heads/\", &name));\n> >> +\telse\n> >> +\t\tmalformed = check_branch_ref_format(arg);\n> >> +\n> >\n> > Would it make sense to swap the logic and get rid of the double\n> > negative (!nongit)?\n> \n> I am trying to follow the pattern \"handle the normal case that have\n> been supported forever first, and then handle new exception next\",\n> so that it is easier to see that there is no behaviour change in the\n> normal case, so I do not think it makes it easier to see to swap the\n> if/else cases.\n\nOk, thanks for your reasoning, makes sense.\n> >\n> >> +\tif (malformed)\n> >> \t\tdie(\"'%s' is not a valid branch name\", arg);\n> >> -\tprintf(\"%s\\n\", sb.buf + 11);\n> >> +\tprintf(\"%s\\n\", name);\n> >> +\tstrbuf_release(&sb);\n> >> \treturn 0;\n> >> }\n> >> \n"},{"id":"330518","messageId":"20171017044111.ahe4eiepvokecnkr@aiede.mtv.corp.google.com","threadId":"46326","inReplyTo":"xmqqinffsibr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-17T04:41:11Z","receivedAt":"2017-10-17T04:42:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n\n> Things like @{-1} would not make any sense when the command is run\n> outside a repository, and the documentation is quite clear that it\n> is the primary reason why we added \"--branch\" option to the command,\n> i.e.\n>\n> With the `--branch` option, it expands the ``previous branch syntax''\n> `@{-n}`. For example, `@{-1}` is a way to refer the last branch you\n> were on. This option should be used by porcelains to accept this\n> syntax anywhere a branch name is expected, so they can act as if you\n> typed the branch name.\n>\n> So I am tempted to take this patch to make sure that we won't gain\n> more people who abuse the command outside a repository.\n\nThat seems very sensible on its face. My only worry is that a script\nthat can be run both inside and outside a repository and does\n\n\tbranch=$(git check-ref-format --branch \"$user_supplied_branch_arg\")\n\ncurrently works with user_supplied_branch_arg='master' and would stop\nworking. If we have reason to believe that no such scripts exist,\nthen this would be a good way to go, but I don't believe we can count\non that.\n\nAnd in that spirit, I think the patch you replied with aims to go in\nthe right direction, by providing the core functionality when in a\nrepository while avoiding breaking such a script outside of one\n(though I do not understand it fully yet).\n\n> Having said that, there may still be a use case where a Porcelain\n> script wants a way to see if a $name it has is appropriate as a\n> branch name before it has a repository\n\nThis seems like a different goal than \"git check-ref-format --branch\"\nwas originally designed to fulfill (even though it fits well with the\ncheck-ref-format name and coincides with --branch behavior when in a\nrepository). I think it's fine for us not to fulfill it.\n\n> (e.g. a wrapper to \"git\n> clone\" that wants to verify the name it is going to give to the \"-b\"\n> option), and a check desired in such a context is different from\n> (and is stricter than) feeding refs/heads/$name to the same command\n> without the \"--branch\" option.\n\nCan you say more about this example? E.g. why is this hypothetical\nwrapper unable to rely on \"git clone -b\"'s own error handling?\n\n> So I think the right endgame in the longer term is:\n>\n> - Find (or add if it doesn't exist) a way to recommend to Porcelain\n> scripts to use to expand an end-user generated string, and to map\n> it to a branch name (it may be \"rev-parse --symbolic-full-name\n> $name\"; I dunno).\n\n--symbolic-full-name seems like a good fit. Do you remember why\ncheck-ref-format was introduced instead? Was it just a matter of\nimplementation simplicity, since --symbolic-full-name can handle a\nbroader class of revision specifications like --remotes? The commit\nmessage to v1.6.3-rc0~29^2~4 (give Porcelain a way to grok branch\nshorthand, 2009-03-21) is appropriately apologetic but doesn't give\nmore clues.\n\n> - Keep check-ref-format as (or revert it to be) a tool to \"check\".\n> This would involve split strbuf_check_branch_ref() into two:\n\nWithout an example of where this tool would be used, if we consider\n\"check-ref-format --branch\" to be a mistake then I'd rather deprecate\nit with a goal of removing it completely.\n\nOk, time to look in more detail.\n\nThanks for your thoughtfulness,\nJonathan\n"},{"id":"330519","messageId":"xmqq7evupemj.fsf@gitster.mtv.corp.google.com","threadId":"46326","inReplyTo":"xmqqo9p6phxg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-17T04:44:36Z","receivedAt":"2017-10-17T04:44:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> I don't think there is any need to prepare it upon my 4d03f955,\n>> though. I'd think it could simply replace it.\n>\n> Yeah, it ended up that way, it seems. Still it needs a bit of doc\n> updates to balance the description.\n\nSo here is what I have now on 'pu'. Clearly not a 2.15 material yet.\n\n-- >8 --\nSubject: [PATCH] check-ref-format: --branch cannot grok @{-1} outside a repository\n\n\"git check-ref-format --branch $name\" feature was originally\nintroduced (and was advertised) as a way for scripts to take any\nend-user supplied string (like \"master\", \"@{-1}\" etc.) and see if\nthat is usable when Git expects to see a branch name, and also\nobtain the concrete branch name that the at-mark magic expands to.\n\nWhen the user asks to interpret a branch name like \"@{-1}\", we have\nto dig the answer out of the HEAD reflog. We can obviously only do\nthat if we have a repository, and indeed, running it outside a\nrepository causes us to hit a BUG().\n\nLet's disable the \"expand @{-n}\" half of the feature when it is run\noutside a repository, but keep the feature to check the syntax of a\nproposed branch name, as \"git check-ref-format --branch $name\" can\nbe stricter than \"git check-ref-format refs/heads/$name\", and\nPorcelain scripts need to have a way to check a given name against\nthe stricter rule.\n\nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n Documentation/git-check-ref-format.txt | 9 ++++++++-\n builtin/check-ref-format.c | 15 ++++++++++++---\n cache.h | 14 ++++++++++++++\n sha1_name.c | 22 +++++++++++++++++++---\n strbuf.h | 6 ++++++\n t/t1402-check-ref-format.sh | 12 ++++++++++++\n 6 files changed, 71 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt\nindex 92777cef25..cf0a0b7df2 100644\n--- a/Documentation/git-check-ref-format.txt\n+++ b/Documentation/git-check-ref-format.txt\n@@ -77,7 +77,14 @@ reference name expressions (see linkgit:gitrevisions[7]):\n \n . at-open-brace `@{` is used as a notation to access a reflog entry.\n \n-With the `--branch` option, it expands the ``previous branch syntax''\n+With the `--branch` option, the command takes a name and checks if\n+it can be used as a valid branch name (e.g. when creating a new\n+branch). The rule `git check-ref-format --branch $name` implements\n+may be stricter than what `git check-ref-format refs/heads/$name`\n+says (e.g. a dash may appear at the beginning of a ref component,\n+but it is explicitly forbidden at the beginning of a branch name).\n+When run with `--branch` option in a repository, the input is first\n+expanded for the ``previous branch syntax''\n `@{-n}`. For example, `@{-1}` is a way to refer the last branch you\n were on. This option should be used by porcelains to accept this\n syntax anywhere a branch name is expected, so they can act as if you\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex eac499450f..4e62852089 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -38,13 +38,22 @@ static char *collapse_slashes(const char *refname)\n \n static int check_ref_format_branch(const char *arg)\n {\n+\tint nongit, malformed;\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint nongit;\n+\tconst char *name = arg;\n \n \tsetup_git_directory_gently(&nongit);\n-\tif (strbuf_check_branch_ref(&sb, arg))\n+\n+\tif (!nongit)\n+\t\tmalformed = (strbuf_check_branch_ref(&sb, arg) ||\n+\t\t\t !skip_prefix(sb.buf, \"refs/heads/\", &name));\n+\telse\n+\t\tmalformed = check_branch_ref_format(arg);\n+\n+\tif (malformed)\n \t\tdie(\"'%s' is not a valid branch name\", arg);\n-\tprintf(\"%s\\n\", sb.buf + 11);\n+\tprintf(\"%s\\n\", name);\n+\tstrbuf_release(&sb);\n \treturn 0;\n }\n \ndiff --git a/cache.h b/cache.h\nindex 52b91f5b64..3815925122 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1444,6 +1444,20 @@ extern int parse_oid_hex(const char *hex, struct object_id *oid, const char **en\n #define INTERPRET_BRANCH_HEAD (1<<2)\n extern int interpret_branch_name(const char *str, int len, struct strbuf *,\n \t\t\t\t unsigned allowed);\n+\n+/*\n+ * NEEDSWORK: declare strbuf_branchname() and strbuf_check_branch_ref()\n+ * here, not in strbuf.h\n+ */\n+\n+/*\n+ * Check if a 'name' is suitable to be used as a branch name; this can\n+ * be and is stricter than what check_refname_format() returns for a\n+ * string that is a concatenation of \"name\" after \"refs/heads/\"; a\n+ * name that begins with \"-\" is not allowed, for example.\n+ */\n+extern int check_branch_ref_format(const char *name);\n+\n extern int get_oid_mb(const char *str, struct object_id *oid);\n \n extern int validate_headref(const char *ref);\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 5e2ec37b65..c95080258f 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1319,15 +1319,31 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)\n \tstrbuf_add(sb, name + used, len - used);\n }\n \n-int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n+static int strbuf_check_branch_ref_format(struct strbuf *sb)\n {\n-\tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n-\tif (name[0] == '-')\n+\tif (*sb->buf == '-')\n \t\treturn -1;\n \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\n \treturn check_refname_format(sb->buf, 0);\n }\n \n+int check_branch_ref_format(const char *name)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint result;\n+\n+\tstrbuf_addstr(&sb, name);\n+\tresult = strbuf_check_branch_ref_format(&sb);\n+\tstrbuf_release(&sb);\n+\treturn result;\n+}\n+\n+int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n+{\n+\tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n+\treturn strbuf_check_branch_ref_format(sb);\n+}\n+\n /*\n * This is like \"get_sha1_basic()\", except it allows \"sha1 expressions\",\n * notably \"xyz^\" for \"parent of xyz\"\ndiff --git a/strbuf.h b/strbuf.h\nindex d785258649..3da95685b2 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -568,6 +568,12 @@ static inline void strbuf_complete_line(struct strbuf *sb)\n \tstrbuf_complete(sb, '\\n');\n }\n \n+/*\n+ * NEEDSWORK: the following two functions should not be in this file;\n+ * these are about refnames, and should be declared next to\n+ * interpret_branch_name() in cache.h\n+ */\n+\n /*\n * Copy \"name\" to \"sb\", expanding any special @-marks as handled by\n * interpret_branch_name(). The result is a non-qualified branch name\ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex 0790edf60d..2ddefb4bd1 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -161,6 +161,18 @@ test_expect_success 'check-ref-format --branch from subdir' '\n \ttest \"$refname\" = \"$sha1\"\n '\n \n+test_expect_success 'check-ref-format --branch @{-1} from non-repo' '\n+\ttest_must_fail nongit git check-ref-format --branch @{-1}\n+'\n+\n+test_expect_success 'check-ref-format --branch master in non-repo' '\n+\tnongit git check-ref-format --branch master\n+'\n+\n+test_expect_success 'check-ref-format --branch -naster in repo' '\n+\ttest_must_fail git check-ref-format --branch -naster\n+'\n+\n valid_ref_normalized() {\n \tprereq=\n \tcase $1 in\n-- \n2.15.0-rc1-178-geb01ff2fe8\n\n"},{"id":"330526","messageId":"xmqqinfentiw.fsf@gitster.mtv.corp.google.com","threadId":"46326","inReplyTo":"20171017044111.ahe4eiepvokecnkr@aiede.mtv.corp.google.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-17T07:05:43Z","receivedAt":"2017-10-17T07:05:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> And in that spirit, I think the patch you replied with aims to go in\n> the right direction, by providing the core functionality when in a\n> repository while avoiding breaking such a script outside of one\n> (though I do not understand it fully yet).\n\nGiven that, is it safe for me to ignore this earlier one\n\n> For what it's worth, I don't agree with this repurposing of\n> \"check-ref-format --branch\" at all.\n\nas reacting to the patch without reading what it does?\n\n>> (e.g. a wrapper to \"git\n>> clone\" that wants to verify the name it is going to give to the \"-b\"\n>> option), and a check desired in such a context is different from\n>> (and is stricter than) feeding refs/heads/$name to the same command\n>> without the \"--branch\" option.\n>\n> Can you say more about this example? E.g. why is this hypothetical\n> wrapper unable to rely on \"git clone -b\"'s own error handling?\n\nIf I have to guess what you meant, perhaps the Porcelain wanted to\ndiagnose bad parameter that would be rejected _before_ letting clone\nspend cycles and possibly network bandwidth?\n\nBut this was my way of rephrasing an earlier example you used while\nobjecting to Peff's change\n\n For example, if you have a script with an $opt_branch variable\n that defaults to \"master\", it may do\n\n resolved_branch=$(git check-ref-format --branch \"$opt_branch\")\n\n even though it is in a mode that not going to have to use\n $resolved_branch and it is not running from a repository.\n\nso my answer to the question in the message I am directly responding\nto would be \"You tell me.\" ;-)\n\n> --symbolic-full-name seems like a good fit. Do you remember why\n> check-ref-format was introduced instead?\n\nIt really serves two purposes, and at this point, I do not think it\nreally matters why it also does the @{-1} expansion as well as name\nvalidation. 604e0cb5 (\"Documentation: describe check-ref-format\n--branch\", 2009-10-12) happened 8 years ago, and since then it has\nbeen advertised long enough as if the 80% primary purpose of \"c-r-f\n--branch\" were to expand @{-1} to a branch name; even though the\ntext hints that it also does the usual checks, by definition\nvalidation of the result of expanding @{-1} ought to succeed; after\nall that was the branch we _were_ on ;-).\n\n"},{"id":"330527","messageId":"20171017070619.bdgvujmbw4uxvwle@aiede.mtv.corp.google.com","threadId":"46326","inReplyTo":"xmqq7evupemj.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 0/3] Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-17T07:06:19Z","receivedAt":"2017-10-17T07:06:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n\n> Subject: [PATCH] check-ref-format: --branch cannot grok @{-1} outside a repository\n\nHow about this? It is written to be more conservative than the patch\nI am replying to, but except for the commit message, it should be\npretty much equivalent.\n\n[...]\n> --- a/builtin/check-ref-format.c\n> +++ b/builtin/check-ref-format.c\n> @@ -38,13 +38,22 @@ static char *collapse_slashes(const char *refname)\n> \n> static int check_ref_format_branch(const char *arg)\n> {\n> +\tint nongit, malformed;\n> \tstruct strbuf sb = STRBUF_INIT;\n> -\tint nongit;\n> +\tconst char *name = arg;\n> \n> \tsetup_git_directory_gently(&nongit);\n> -\tif (strbuf_check_branch_ref(&sb, arg))\n> +\n> +\tif (!nongit)\n> +\t\tmalformed = (strbuf_check_branch_ref(&sb, arg) ||\n> +\t\t\t !skip_prefix(sb.buf, \"refs/heads/\", &name));\n> +\telse\n> +\t\tmalformed = check_branch_ref_format(arg);\n\nHandles the nongit case in strbuf_check_branch_ref instead of\nintroducing a new check_branch_ref_format helper.\n\n[...]\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1444,6 +1444,20 @@ extern int parse_oid_hex(const char *hex, struct object_id *oid, const char **en\n> #define INTERPRET_BRANCH_HEAD (1<<2)\n> extern int interpret_branch_name(const char *str, int len, struct strbuf *,\n> \t\t\t\t unsigned allowed);\n> +\n> +/*\n> + * NEEDSWORK: declare strbuf_branchname() and strbuf_check_branch_ref()\n> + * here, not in strbuf.h\n> + */\n\nAs a result, it doesn't touch headers. I agree that these functions\ndon't belong in strbuf.h (sorry for not updating the headers at the\nsame time I moved their implementations) but suspect e.g. branch.h,\nrevision.h, or some new header like revision-syntax.h would be a\nbetter place.\n\n[...]\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -568,6 +568,12 @@ static inline void strbuf_complete_line(struct strbuf *sb)\n> \tstrbuf_complete(sb, '\\n');\n> }\n> \n> +/*\n> + * NEEDSWORK: the following two functions should not be in this file;\n> + * these are about refnames, and should be declared next to\n> + * interpret_branch_name() in cache.h\n> + */\n\nDidn't touch headers.\n\n[...]\n> --- a/t/t1402-check-ref-format.sh\n> +++ b/t/t1402-check-ref-format.sh\n> @@ -161,6 +161,18 @@ test_expect_success 'check-ref-format --branch from subdir' '\n> \ttest \"$refname\" = \"$sha1\"\n> '\n> \n> +test_expect_success 'check-ref-format --branch @{-1} from non-repo' '\n> +\ttest_must_fail nongit git check-ref-format --branch @{-1}\n> +'\n\nSwapped test_must_fail and nongit to match existing tests.\n\nJunio C Hamano (3):\n check-ref-format --branch: do not expand @{...} outside repository\n check-ref-format --branch: strip refs/heads/ using skip_prefix\n check-ref-format doc: --branch validates and expands \n\n Documentation/git-check-ref-format.txt | 9 ++++++++-\n builtin/check-ref-format.c | 6 ++++--\n sha1_name.c | 5 ++++-\n t/t1402-check-ref-format.sh | 16 ++++++++++++++++\n 4 files changed, 32 insertions(+), 4 deletions(-)\n"},{"id":"330528","messageId":"20171017070808.plddffhzdobyekmo@aiede.mtv.corp.google.com","threadId":"46326","inReplyTo":"20171017070619.bdgvujmbw4uxvwle@aiede.mtv.corp.google.com","subject":"[PATCH 1/3] check-ref-format --branch: do not expand @{...} outside repository","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-17T07:08:08Z","receivedAt":"2017-10-17T07:08:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Junio C Hamano \n\nRunning \"git check-ref-format --branch @{-1}\" from outside any\nrepository produces\n\n\t$ git check-ref-format --branch @{-1}\n\tBUG: environment.c:182: git environment hasn't been setup\n\nThis is because the expansion of @{-1} must come from the HEAD reflog,\nwhich involves opening the repository. @{u} and @{push} (which are\nmore unusual because they typically would not expand to a local\nbranch) trigger the same assertion.\n\nThis has been broken since day one. Before v2.13.0-rc0~48^2\n(setup_git_env: avoid blind fall-back to \".git\", 2016-10-02), the\nbreakage was more subtle: Git would read reflogs from \".git\" within\nthe current directory even if it was not a valid repository. Usually\nthat is harmless because Git is not being run from the root directory\nof an invalid repository, but in edge cases such accesses can be\nconfusing or harmful. Since v2.13.0, the problem is easier to\ndiagnose because Git aborts with a BUG message.\n\nErroring out is the right behavior: when asked to interpret a branch\nname like \"@{-1}\", there is no reasonable answer in this context.\nBut we should print a message saying so instead of an assertion failure.\n\nWe do not forbid \"check-ref-format --branch\" from outside a repository\naltogether because it is ok for a script to pre-process branch\narguments without @{...} in such a context. For example, with\npre-2.13 Git, a script that does\n\n\tbranch='master'; # default value\n\tparse_options\n\tbranch=$(git check-ref-format --branch \"$branch\")\n\nto normalize an optional branch name provided by the user would work\nboth inside a repository (where the user could provide '@{-1}') and\noutside (where '@{-1}' should not be accepted).\n\nSo disable the \"expand @{...}\" half of the feature when run outside a\nrepository, but keep the check of the syntax of a proposed branch\nname. This way, when run from outside a repository, \"git\ncheck-ref-format --branch @{-1}\" will gracefully fail:\n\n\t$ git check-ref-format --branch @{-1}\n\tfatal: '@{-1}' is not a valid branch name\n\nand \"git check-ref-format --branch master\" will succeed as before:\n\n\t$ git check-ref-format --branch master\n\tmaster\n\nrestoring the usual pre-2.13 behavior.\n\n[jn: split out from a larger patch; moved conditional to\n strbuf_check_branch_ref instead of its caller; fleshed out commit\n message; some style tweaks in tests]\n\nReported-by: Marko Kungla \nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \nSigned-off-by: Jonathan Nieder \n---\n sha1_name.c | 5 ++++-\n t/t1402-check-ref-format.sh | 16 ++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex c7c5ab376c..603e667faa 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1331,7 +1331,10 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)\n \n int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n {\n-\tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n+\tif (startup_info->have_repository)\n+\t\tstrbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);\n+\telse\n+\t\tstrbuf_addstr(sb, name);\n \tif (name[0] == '-')\n \t\treturn -1;\n \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex 0790edf60d..98e4a8613b 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -144,6 +144,11 @@ test_expect_success \"check-ref-format --branch @{-1}\" '\n \trefname2=$(git check-ref-format --branch @{-2}) &&\n \ttest \"$refname2\" = master'\n \n+test_expect_success 'check-ref-format --branch -naster' '\n+\ttest_must_fail git check-ref-format --branch -naster >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'check-ref-format --branch from subdir' '\n \tmkdir subdir &&\n \n@@ -161,6 +166,17 @@ test_expect_success 'check-ref-format --branch from subdir' '\n \ttest \"$refname\" = \"$sha1\"\n '\n \n+test_expect_success 'check-ref-format --branch @{-1} from non-repo' '\n+\tnongit test_must_fail git check-ref-format --branch @{-1} >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'check-ref-format --branch master from non-repo' '\n+\techo master >expect &&\n+\tnongit git check-ref-format --branch master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n valid_ref_normalized() {\n \tprereq=\n \tcase $1 in\n-- \n2.15.0.rc1.287.g2b38de12cc\n\n"},{"id":"330529","messageId":"20171017071000.ms2jearljljhxtgm@aiede.mtv.corp.google.com","threadId":"46326","inReplyTo":"20171017070619.bdgvujmbw4uxvwle@aiede.mtv.corp.google.com","subject":"[PATCH 2/3] check-ref-format --branch: strip refs/heads/ using skip_prefix","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-17T07:10:00Z","receivedAt":"2017-10-17T07:10:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Junio C Hamano \n\nThe expansion returned from strbuf_check_branch_ref always starts with\n\"refs/heads/\" by construction, but there is nothing about its name or\nadvertised API making that obvious. This command is used to process\nhuman-supplied input from the command line and is usually not the\ninner loop, so we can spare some cycles to be more defensive. Instead\nof hard-coding the offset strlen(\"refs/heads/\") to skip, verify that\nthe expansion actually starts with refs/heads/.\n\n[jn: split out from a larger patch, added explanation]\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Jonathan Nieder \n---\n builtin/check-ref-format.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex 6c40ff110b..bc67d3f0a8 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -39,12 +39,14 @@ static char *collapse_slashes(const char *refname)\n static int check_ref_format_branch(const char *arg)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *name;\n \tint nongit;\n \n \tsetup_git_directory_gently(&nongit);\n-\tif (strbuf_check_branch_ref(&sb, arg))\n+\tif (strbuf_check_branch_ref(&sb, arg) ||\n+\t !skip_prefix(sb.buf, \"refs/heads/\", &name))\n \t\tdie(\"'%s' is not a valid branch name\", arg);\n-\tprintf(\"%s\\n\", sb.buf + 11);\n+\tprintf(\"%s\\n\", name);\n \tstrbuf_release(&sb);\n \treturn 0;\n }\n-- \n2.15.0.rc1.287.g2b38de12cc\n\n"},{"id":"330530","messageId":"xmqqefq2nt7w.fsf@gitster.mtv.corp.google.com","threadId":"46326","inReplyTo":"20171017070619.bdgvujmbw4uxvwle@aiede.mtv.corp.google.com","subject":"Re: [PATCH 0/3] Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-17T07:12:19Z","receivedAt":"2017-10-17T07:12:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Handles the nongit case in strbuf_check_branch_ref instead of\n> introducing a new check_branch_ref_format helper.\n\nI view that as a regression, actually. Don't we want a function\nthat does not require a strbuf when asking a simple question: \"I\nhave a string, and I want to see if that is a valid name\"?\n"},{"id":"330531","messageId":"20171017071234.m6j4rqn6izdvla6d@aiede.mtv.corp.google.com","threadId":"46326","inReplyTo":"20171017070619.bdgvujmbw4uxvwle@aiede.mtv.corp.google.com","subject":"[PATCH 3/3] check-ref-format doc: --branch validates and expands ","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-17T07:12:34Z","receivedAt":"2017-10-17T07:12:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Junio C Hamano \n\n\"git check-ref-format --branch $name\" feature was originally\nintroduced (and was advertised) as a way for scripts to take any\nend-user supplied string (like \"master\", \"@{-1}\" etc.) and see if it\nis usable when Git expects to see a branch name, and also obtain the\nconcrete branch name that the at-mark magic expands to.\n\nEmphasize that \"see if it is usable\" role in the description and\nclarify that the @{...} expansion only occurs when run from within a\nrepository.\n\n[jn: split out from a larger patch]\n\nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n Documentation/git-check-ref-format.txt | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt\nindex 92777cef25..cf0a0b7df2 100644\n--- a/Documentation/git-check-ref-format.txt\n+++ b/Documentation/git-check-ref-format.txt\n@@ -77,7 +77,14 @@ reference name expressions (see linkgit:gitrevisions[7]):\n \n . at-open-brace `@{` is used as a notation to access a reflog entry.\n \n-With the `--branch` option, it expands the ``previous branch syntax''\n+With the `--branch` option, the command takes a name and checks if\n+it can be used as a valid branch name (e.g. when creating a new\n+branch). The rule `git check-ref-format --branch $name` implements\n+may be stricter than what `git check-ref-format refs/heads/$name`\n+says (e.g. a dash may appear at the beginning of a ref component,\n+but it is explicitly forbidden at the beginning of a branch name).\n+When run with `--branch` option in a repository, the input is first\n+expanded for the ``previous branch syntax''\n `@{-n}`. For example, `@{-1}` is a way to refer the last branch you\n were on. This option should be used by porcelains to accept this\n syntax anywhere a branch name is expected, so they can act as if you\n-- \n2.15.0.rc1.287.g2b38de12cc\n\n"},{"id":"330533","messageId":"20171017071758.ltzustqxcbp3yet7@aiede.mtv.corp.google.com","threadId":"46326","inReplyTo":"xmqqefq2nt7w.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/3] Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-17T07:17:58Z","receivedAt":"2017-10-17T07:18:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> Handles the nongit case in strbuf_check_branch_ref instead of\n>> introducing a new check_branch_ref_format helper.\n>\n> I view that as a regression, actually. Don't we want a function\n> that does not require a strbuf when asking a simple question: \"I\n> have a string, and I want to see if that is a valid name\"?\n\n*shrug* I found the change easier to read, and it also sidesteps the\nwhich-header question. It also ensures that other\nstrbuf_check_branch_ref callers are safe without having to audit them.\n\nBut feel free to tweak that back if you like, or I can tomorrow.\n\nJonathan\n"},{"id":"330534","messageId":"20171017072552.e72yfbnggyh6jlo5@aiede.mtv.corp.google.com","threadId":"46326","inReplyTo":"xmqqinfentiw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-17T07:25:52Z","receivedAt":"2017-10-17T07:25:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> And in that spirit, I think the patch you replied with aims to go in\n>> the right direction, by providing the core functionality when in a\n>> repository while avoiding breaking such a script outside of one\n>> (though I do not understand it fully yet).\n>\n> Given that, is it safe for me to ignore this earlier one\n>\n>> For what it's worth, I don't agree with this repurposing of\n>> \"check-ref-format --branch\" at all.\n>\n> as reacting to the patch without reading what it does?\n\nAre you saying I should have ignored the commit message? Recording\nfuture plans via the commit message is part of what the patch does,\nafter all.\n\n>> Junio C Hamano wrote:\n\n>>> (e.g. a wrapper to \"git\n>>> clone\" that wants to verify the name it is going to give to the \"-b\"\n>>> option), and a check desired in such a context is different from\n>>> (and is stricter than) feeding refs/heads/$name to the same command\n>>> without the \"--branch\" option.\n>>\n>> Can you say more about this example? E.g. why is this hypothetical\n>> wrapper unable to rely on \"git clone -b\"'s own error handling?\n>\n> If I have to guess what you meant, perhaps the Porcelain wanted to\n> diagnose bad parameter that would be rejected _before_ letting clone\n> spend cycles and possibly network bandwidth?\n>\n> But this was my way of rephrasing an earlier example you used while\n> objecting to Peff's change\n[...]\n> so my answer to the question in the message I am directly responding\n> to would be \"You tell me.\" ;-)\n\nHm. Does the example in\nhttps://public-inbox.org/git/20171017070808.plddffhzdobyekmo@aiede.mtv.corp.google.com/\nmake it clearer?\n\nThe goal of such a script is *not* error handling --- that is just a\npleasant side-benefit. It is to be able to handle all branch\nspecifiers from the user (and even a default branch that is not from\nthe user) uniformly.\n\nThanks,\nJonathan\n"},{"id":"330536","messageId":"20171017073441.ovfhfnh5dsrbpxse@aiede.mtv.corp.google.com","threadId":"46326","inReplyTo":"xmqqinfentiw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-17T07:34:41Z","receivedAt":"2017-10-17T07:34:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> And in that spirit, I think the patch you replied with aims to go in\n>> the right direction, by providing the core functionality when in a\n>> repository while avoiding breaking such a script outside of one\n>> (though I do not understand it fully yet).\n>\n> Given that, is it safe for me to ignore this earlier one\n>\n>> For what it's worth, I don't agree with this repurposing of\n>> \"check-ref-format --branch\" at all.\n>\n> as reacting to the patch without reading what it does?\n\nOn second reading, yes, I was reacting to the discussion leading up to\nthe patch instead of the patch. The patch looks good.\n\nSorry for the noise,\nJonathan\n"},{"id":"330537","messageId":"xmqqa80qnq1f.fsf@gitster.mtv.corp.google.com","threadId":"46326","inReplyTo":"20171017071758.ltzustqxcbp3yet7@aiede.mtv.corp.google.com","subject":"Re: [PATCH 0/3] Re: [PATCH] check-ref-format: require a repository for --branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-17T08:21:00Z","receivedAt":"2017-10-17T08:21:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Junio C Hamano wrote:\n>> Jonathan Nieder writes:\n>\n>>> Handles the nongit case in strbuf_check_branch_ref instead of\n>>> introducing a new check_branch_ref_format helper.\n>>\n>> I view that as a regression, actually. Don't we want a function\n>> that does not require a strbuf when asking a simple question: \"I\n>> have a string, and I want to see if that is a valid name\"?\n>\n> *shrug* I found the change easier to read, and it also sidesteps the\n> which-header question. It also ensures that other\n> strbuf_check_branch_ref callers are safe without having to audit them.\n\nPlease ignore the above, which was merely an impression _without_\nand before having received any patch to comment on ;-)\n\nQuite frankly, this is a Meh topic that won't have to hit even\n'next' before the final. The color.ui=always thing has a lot more\nurgency, and this was merely what I did while waiting for others to\nreact to that topic.\n"},{"id":"330569","messageId":"xmqq60bdo5op.fsf@gitster.mtv.corp.google.com","threadId":"46326","inReplyTo":"20171017071234.m6j4rqn6izdvla6d@aiede.mtv.corp.google.com","subject":"Re: [PATCH 3/3] check-ref-format doc: --branch validates and expands ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-17T20:55:18Z","receivedAt":"2017-10-17T20:55:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> From: Junio C Hamano \n>\n> \"git check-ref-format --branch $name\" feature was originally\n> introduced (and was advertised) as a way for scripts to take any\n> end-user supplied string (like \"master\", \"@{-1}\" etc.) and see if it\n> is usable when Git expects to see a branch name, and also obtain the\n> concrete branch name that the at-mark magic expands to.\n>\n> Emphasize that \"see if it is usable\" role in the description and\n> clarify that the @{...} expansion only occurs when run from within a\n> repository.\n>\n> [jn: split out from a larger patch]\n>\n> Helped-by: Jeff King \n> Signed-off-by: Junio C Hamano \n> ---\n\nMissing sign-off, unlike the other two, intended?\n\nI'll take these three to replace what I tentatively queued, not\nnecessarily because I like the change in 1/3 better, but because\nthese are explained much better; besides I prefer a version that at\nleast two people deeply looked at.\n\nThanks.\n"},{"id":"330573","messageId":"20171017210607.rwvaxarkallzahq6@aiede.mtv.corp.google.com","threadId":"46326","inReplyTo":"xmqq60bdo5op.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] check-ref-format doc: --branch validates and expands ","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-17T21:06:07Z","receivedAt":"2017-10-17T21:06:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> From: Junio C Hamano \n>>\n>> \"git check-ref-format --branch $name\" feature was originally\n>> introduced (and was advertised) as a way for scripts to take any\n>> end-user supplied string (like \"master\", \"@{-1}\" etc.) and see if it\n>> is usable when Git expects to see a branch name, and also obtain the\n>> concrete branch name that the at-mark magic expands to.\n>>\n>> Emphasize that \"see if it is usable\" role in the description and\n>> clarify that the @{...} expansion only occurs when run from within a\n>> repository.\n>>\n>> [jn: split out from a larger patch]\n>>\n>> Helped-by: Jeff King \n>> Signed-off-by: Junio C Hamano \n>> ---\n>\n> Missing sign-off, unlike the other two, intended?\n\nOops.\nSigned-off-by: Jonathan Nieder \n\n> I'll take these three to replace what I tentatively queued, not\n> necessarily because I like the change in 1/3 better, but because\n> these are explained much better; besides I prefer a version that at\n> least two people deeply looked at.\n\nOkay, thanks. I'll add the rest of the change as a patch on top. :)\n\nThanks,\nJonathan\n"},{"id":"330593","messageId":"20171018051014.kfwq2ayeorr5jpc3@sigill.intra.peff.net","threadId":"46326","inReplyTo":"xmqq60bdo5op.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] check-ref-format doc: --branch validates and expands ","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-18T05:10:14Z","receivedAt":"2017-10-18T05:10:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 18, 2017 at 05:55:18AM +0900, Junio C Hamano wrote:\n\n> I'll take these three to replace what I tentatively queued, not\n> necessarily because I like the change in 1/3 better, but because\n> these are explained much better; besides I prefer a version that at\n> least two people deeply looked at.\n\nThese look good to me. Between your two versions, I think the code in\nJonathan's is slightly preferable. It's possible that some other caller\nof strbuf_check_branch_name() may run into the same thing and be fixed\n(I am trying to think of a hypothetical caller that would be\ninconvenienced by the new behavior, but I can't come up with one; and\ncertainly the existing code would BUG(), so this is an improvement).\n\n-Peff\n"}]}