bug with check-ref-format outside of repository

bug with check-ref-format outside of repository

[Marko Kungla]

Hi,

As contrived e.g: if I have in my "workspace" sub directories mostly
git repositories, but some not and if I exec,
"for d in */ ; do (cd $d && git check-ref-format --branch @{-1});
done" then I get 3 possible responses.

git version: 2.13.0
1. Valid branch name
2. fatal: '@{-1}' is not a valid branch name
3. fatal: BUG: setup_git_env called without repository

git version 2.13.2.915.ga9c46e097
1. Valid branch name
2. fatal: '@{-1}' is not a valid branch name
3. BUG: environment.c:178: git environment hasn't been setup

best marko

Re: bug with check-ref-format outside of repository

[Jeff King]

On Sat, Jul 08, 2017 at 03:17:32AM +0200, Marko Kungla wrote:

> As contrived e.g: if I have in my "workspace" sub directories mostly
> git repositories, but some not and if I exec,
> "for d in */ ; do (cd $d && git check-ref-format --branch @{-1});
> done" then I get 3 possible responses.
> 
> git version: 2.13.0
> 1. Valid branch name
> 2. fatal: '@{-1}' is not a valid branch name
> 3. fatal: BUG: setup_git_env called without repository
> 
> git version 2.13.2.915.ga9c46e097
> 1. Valid branch name
> 2. fatal: '@{-1}' is not a valid branch name
> 3. BUG: environment.c:178: git environment hasn't been setup

Thanks for the report. It's right to return an error, but obviously we
should never hit the BUG() in the lazy-setup code.

I think "check-ref-format --branch" is inherently a repository-only
operation, since its purpose is to look in the reflog with @{-1} and
similar branch-substitutions. Technically you can do:

  $ cd /not/a/git/repo
  $ git check-ref-format --branch 'refs/heads/foo'

but I'm not sure why you would want to. So I think the patch below is
probably the right direction. The other alternative is for
interpret_nth_prior_checkout() to detect the "not in a repo" case and
quietly return "nope, we couldn't find such a reflog entry".

+cc Jonathan, who added the original call in 49cc460d8 (Allow
"check-ref-format --branch" from subdirectory, 2010-08-05). That commit
message doesn't give any indication of why we used the gently form.
Looking back at the original thread[1], I suspect it was mostly out of
conservatism.

[1] https://public-inbox.org/git/20100806033922.GS22369@burratino/

---
diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index eac499450..1e5f9835f 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -39,9 +39,8 @@ static char *collapse_slashes(const char *refname)
 static int check_ref_format_branch(const char *arg)
 {
 	struct strbuf sb = STRBUF_INIT;
-	int nongit;
 
-	setup_git_directory_gently(&nongit);
+	setup_git_directory();
 	if (strbuf_check_branch_ref(&sb, arg))
 		die("'%s' is not a valid branch name", arg);
 	printf("%s\n", sb.buf + 11);
diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index 0790edf60..1674b061e 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -161,6 +161,10 @@ test_expect_success 'check-ref-format --branch from subdir' '
 	test "$refname" = "$sha1"
 '
 
+test_expect_success 'check-ref-format --branch from non-repo' '
+	test_must_fail nongit git check-ref-format --branch @{-1}
+'
+
 valid_ref_normalized() {
 	prereq=
 	case $1 in

[PATCH] check-ref-format: require a repository for --branch

[Jeff King]

On Fri, Jul 14, 2017 at 02:03:13PM -0400, Jeff King wrote:

> So I think the patch below is probably the right direction.

And here it is with a real commit message, if this is what we want to
do.

I do feel like "check-ref-format --branch" is a misfeature. The
operation seems to fit better with "rev-parse" (which yes, is a kitchen
sink, but it does all sorts of similar resolution operations). I think
"git rev-parse --symbolic-full-name @{-1}" is basically the same thing
(modulo the refs/heads/ shortening).

-- >8 --
Subject: [PATCH] check-ref-format: require a repository for --branch

When the user asks "--branch" to interpret a branch name
like "@{-1}", we have to dig the answer out of the HEAD
reflog. We can obviously only do that if we have a
repository, and indeed, running it outside a repository
causes us to hit a BUG().

We basically have two options:

  1. We can define "@{-N}" outside of a repository as "no
     such branch" and die with "not a valid brach name".

  2. We can just declare that "--branch" must be run inside
     a repository, in which case we die with "not a git
     repository".

The effect is more or less the same for "@{-N}".
Technically one can use "--branch" outside of a repository
as long as you don't use any names that actually need
interpreting. But since intrpreting is the option's
documented purpose, there doesn't seem any point in trying
to resolve vanilla names like "foo" (which we end up just
printing to stdout verbatim).

So let's go with option 2.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/check-ref-format.c  | 3 +--
 t/t1402-check-ref-format.sh | 4 ++++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index eac499450..1e5f9835f 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -39,9 +39,8 @@ static char *collapse_slashes(const char *refname)
 static int check_ref_format_branch(const char *arg)
 {
 	struct strbuf sb = STRBUF_INIT;
-	int nongit;
 
-	setup_git_directory_gently(&nongit);
+	setup_git_directory();
 	if (strbuf_check_branch_ref(&sb, arg))
 		die("'%s' is not a valid branch name", arg);
 	printf("%s\n", sb.buf + 11);
diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index 0790edf60..1674b061e 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -161,6 +161,10 @@ test_expect_success 'check-ref-format --branch from subdir' '
 	test "$refname" = "$sha1"
 '
 
+test_expect_success 'check-ref-format --branch from non-repo' '
+	test_must_fail nongit git check-ref-format --branch @{-1}
+'
+
 valid_ref_normalized() {
 	prereq=
 	case $1 in
-- 
2.14.0.rc0.457.g08326e0ba

Re: [PATCH] check-ref-format: require a repository for --branch

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Fri, Jul 14, 2017 at 02:03:13PM -0400, Jeff King wrote:

>> So I think the patch below is probably the right direction.
>
> And here it is with a real commit message, if this is what we want to
> do.
[...]
> --- a/t/t1402-check-ref-format.sh
> +++ b/t/t1402-check-ref-format.sh
> @@ -161,6 +161,10 @@ test_expect_success 'check-ref-format --branch from subdir' '
>  	test "$refname" = "$sha1"
>  '
>  
> +test_expect_success 'check-ref-format --branch from non-repo' '
> +	test_must_fail nongit git check-ref-format --branch @{-1}
> +'
> +
>  valid_ref_normalized() {
>  	prereq=
>  	case $1 in

I don't think it's right.  Today I can do

	$ cd /tmp
	$ git check-ref-format --branch master
	master

You might wonder why I'd ever do such a thing.  But that's what "git
check-ref-format --branch" is for --- it is for taking a <branch>
argument and turning it into a branch name.  For example, if you have
a script with an $opt_branch variable that defaults to "master", it
may do

	resolved_branch=$(git check-ref-format --branch "$opt_branch")

even though it is in a mode that not going to have to use
$resolved_branch and it is not running from a repository.

Thanks and hope that helps,
Jonathan

Re: [PATCH] check-ref-format: require a repository for --branch

[Marko Kungla]

I guess that should only be about that it should not hit a (BUG).
In my case in the example I gave I scan trough the directories to
check repository status one of the tasks make use of check-ref-format.
Since it may hit directory which is not a git repository it should not
expose error (BUG) right.

On Mon, Jul 17, 2017 at 7:27 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Jeff King wrote:
>> On Fri, Jul 14, 2017 at 02:03:13PM -0400, Jeff King wrote:
>
>>> So I think the patch below is probably the right direction.
>>
>> And here it is with a real commit message, if this is what we want to
>> do.
> [...]
>> --- a/t/t1402-check-ref-format.sh
>> +++ b/t/t1402-check-ref-format.sh
>> @@ -161,6 +161,10 @@ test_expect_success 'check-ref-format --branch from subdir' '
>>       test "$refname" = "$sha1"
>>  '
>>
>> +test_expect_success 'check-ref-format --branch from non-repo' '
>> +     test_must_fail nongit git check-ref-format --branch @{-1}
>> +'
>> +
>>  valid_ref_normalized() {
>>       prereq=
>>       case $1 in
>
> I don't think it's right.  Today I can do
>
>         $ cd /tmp
>         $ git check-ref-format --branch master
>         master
>
> You might wonder why I'd ever do such a thing.  But that's what "git
> check-ref-format --branch" is for --- it is for taking a <branch>
> argument and turning it into a branch name.  For example, if you have
> a script with an $opt_branch variable that defaults to "master", it
> may do
>
>         resolved_branch=$(git check-ref-format --branch "$opt_branch")
>
> even though it is in a mode that not going to have to use
> $resolved_branch and it is not running from a repository.
>
> Thanks and hope that helps,
> Jonathan



-- 
Mail:    marko.kungla@gmail.com
Phone: +31 (0) 6 2546 0117

Re: [PATCH] check-ref-format: require a repository for --branch

[Jeff King]

On Mon, Jul 17, 2017 at 10:27:09AM -0700, Jonathan Nieder wrote:

> > --- a/t/t1402-check-ref-format.sh
> > +++ b/t/t1402-check-ref-format.sh
> > @@ -161,6 +161,10 @@ test_expect_success 'check-ref-format --branch from subdir' '
> >  	test "$refname" = "$sha1"
> >  '
> >  
> > +test_expect_success 'check-ref-format --branch from non-repo' '
> > +	test_must_fail nongit git check-ref-format --branch @{-1}
> > +'
> > +
> >  valid_ref_normalized() {
> >  	prereq=
> >  	case $1 in
> 
> I don't think it's right.  Today I can do
> 
> 	$ cd /tmp
> 	$ git check-ref-format --branch master
> 	master
> 
> You might wonder why I'd ever do such a thing.  But that's what "git
> check-ref-format --branch" is for --- it is for taking a <branch>
> argument and turning it into a branch name.  For example, if you have
> a script with an $opt_branch variable that defaults to "master", it
> may do
> 
> 	resolved_branch=$(git check-ref-format --branch "$opt_branch")
> 
> even though it is in a mode that not going to have to use
> $resolved_branch and it is not running from a repository.

I'm not sure I buy that. What does "turning it into a branch name" even
mean when you are not in a repository? Clearly @{-1} must fail. And
everything else is just going to output the exact input you provided.
So any script calling "check-ref-format --branch" outside of a repo
would be better off not calling it at all. At best it does nothing, and
at worst it's going to give a confusing error when $opt_branch is
something like "@{-1}".

A more compelling argument along these lines is something like:

  Accepting --branch outside of a repo is pointless, but it's something
  we've historically accepted. To avoid breaking existing scripts (even
  if they are doing something pointless), we'll continue to allow it.

I'm not sure I buy _that_ line of reasoning either, but it at least
makes sense to me (I just think it isn't worth the complexity of trying
to audit the innards of interpret_branch_name()).

-Peff

Re: [PATCH] check-ref-format: require a repository for --branch

[Jeff King]

On Mon, Jul 17, 2017 at 11:18:43PM +0200, Marko Kungla wrote:

> I guess that should only be about that it should not hit a (BUG).
> In my case in the example I gave I scan trough the directories to
> check repository status one of the tasks make use of check-ref-format.
> Since it may hit directory which is not a git repository it should not
> expose error (BUG) right.

Right. The BUG should definitely be corrected. Between what Jonathan is
suggesting and my patch, either would be fine for the case you described
originally ("--branch @{-1}" would always fail in a non-repo).

-Peff

Re: [PATCH] check-ref-format: require a repository for --branch

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jul 17, 2017 at 10:27:09AM -0700, Jonathan Nieder wrote:
>
>> > --- a/t/t1402-check-ref-format.sh
>> > +++ b/t/t1402-check-ref-format.sh
>> > @@ -161,6 +161,10 @@ test_expect_success 'check-ref-format --branch from subdir' '
>> >  	test "$refname" = "$sha1"
>> >  '
>> >  
>> > +test_expect_success 'check-ref-format --branch from non-repo' '
>> > +	test_must_fail nongit git check-ref-format --branch @{-1}
>> > +'
>> > +
>> >  valid_ref_normalized() {
>> >  	prereq=
>> >  	case $1 in
>> 
>> I don't think it's right.  Today I can do
>> 
>> 	$ cd /tmp
>> 	$ git check-ref-format --branch master
>> 	master
>> 
>> You might wonder why I'd ever do such a thing.  But that's what "git
>> check-ref-format --branch" is for --- it is for taking a <branch>
>> argument and turning it into a branch name.  For example, if you have
>> a script with an $opt_branch variable that defaults to "master", it
>> may do
>> 
>> 	resolved_branch=$(git check-ref-format --branch "$opt_branch")
>> 
>> even though it is in a mode that not going to have to use
>> $resolved_branch and it is not running from a repository.
>
> I'm not sure I buy that. What does "turning it into a branch name" even
> mean when you are not in a repository? Clearly @{-1} must fail. And
> everything else is just going to output the exact input you provided.

This "just going to output the exact input" is not entirely correct;
there is just one use case for it.

"git check-ref-format --branch a..b" would fail with a helpful error
message, while the command run with "a.b" would tell you the name is
safe.

Use of 'check-ref-format --branch "@{-1}"' *IS* a nonsense, whether
it is inside or outside a repository; it is OK to fail it outside a
repository and I would say it is even OK to fail it inside a
repository.  After all "check-ref-format" is about checking if the
string is a sensible name to use.

I think calling interpret_branch_name() in the codepath is a mistake
and we should instead report if "refs/heads/@{-1}" literally is a
valid refname we could create instead.

Re: [PATCH] check-ref-format: require a repository for --branch

[Jeff King]

On Thu, Aug 17, 2017 at 02:30:53PM -0700, Junio C Hamano wrote:

> > I'm not sure I buy that. What does "turning it into a branch name" even
> > mean when you are not in a repository? Clearly @{-1} must fail. And
> > everything else is just going to output the exact input you provided.
> 
> This "just going to output the exact input" is not entirely correct;
> there is just one use case for it.
> 
> "git check-ref-format --branch a..b" would fail with a helpful error
> message, while the command run with "a.b" would tell you the name is
> safe.

Well, yes. It's checking the syntax, as well. But you don't need
--branch for that.

> Use of 'check-ref-format --branch "@{-1}"' *IS* a nonsense, whether
> it is inside or outside a repository; it is OK to fail it outside a
> repository and I would say it is even OK to fail it inside a
> repository.  After all "check-ref-format" is about checking if the
> string is a sensible name to use.
> 
> I think calling interpret_branch_name() in the codepath is a mistake
> and we should instead report if "refs/heads/@{-1}" literally is a
> valid refname we could create instead.

I don't think it's nonsense. It's the reason the feature was added in
the first place. See your own a31dca0393 (check-ref-format --branch:
give Porcelain a way to grok branch shorthand, 2009-03-21). Without
that interpretation, it does nothing that you could not equally well do
as:

  git check-ref-format refs/heads/$name

I _do_ think it's a misfeature to put it in check-ref-format. It should
be part of rev-parse. Which admittedly is a kitchen sink, but this kind
of resolving is one of the main things it should be doing. And in fact
you can already do:

  git rev-parse --symbolic-full-name @{-1}

But I stopped short of suggesting we remove the feature entirely. It
would obviously require a deprecation period.

-Peff

Re: [PATCH] check-ref-format: require a repository for --branch

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I _do_ think it's a misfeature to put it in check-ref-format. It should
> be part of rev-parse. Which admittedly is a kitchen sink, but this kind
> of resolving is one of the main things it should be doing. And in fact
> you can already do:
>
>   git rev-parse --symbolic-full-name @{-1}
>
> But I stopped short of suggesting we remove the feature entirely. It
> would obviously require a deprecation period.

Yeah, I realize that "nonsense" was a bit too strong.  I do agree
that it was a misfeature to place in "check" ref-format, though.

Thanks.

Re: [PATCH] check-ref-format: require a repository for --branch

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jul 17, 2017 at 10:27:09AM -0700, Jonathan Nieder wrote:
>>  ...
>> I don't think it's right.  Today I can do
>> 
>> 	$ cd /tmp
>> 	$ git check-ref-format --branch master
>> 	master
>> 
>> You might wonder why I'd ever do such a thing.  But that's what "git
>> check-ref-format --branch" is for --- it is for taking a <branch>
>> argument and turning it into a branch name.  For example, if you have
>> a script with an $opt_branch variable that defaults to "master", it
>> may do
>> 
>> 	resolved_branch=$(git check-ref-format --branch "$opt_branch")
>> 
>> even though it is in a mode that not going to have to use
>> $resolved_branch and it is not running from a repository.
>
> I'm not sure I buy that. What does "turning it into a branch name" even
> mean when you are not in a repository? Clearly @{-1} must fail. And
> everything else is just going to output the exact input you provided.
> So any script calling "check-ref-format --branch" outside of a repo
> would be better off not calling it at all.

I threw this topic in stalled category, hoping that one or the other
opinion eventually turns out to be more prevalent, but it didn't
seem to have happened X-<.

Things like @{-1} would not make any sense when the command is run
outside a repository, and the documentation is quite clear that it
is the primary reason why we added "--branch" option to the command,
i.e.

    With the `--branch` option, it expands the ``previous branch syntax''
    `@{-n}`.  For example, `@{-1}` is a way to refer the last branch you
    were on.  This option should be used by porcelains to accept this
    syntax anywhere a branch name is expected, so they can act as if you
    typed the branch name.

So I am tempted to take this patch to make sure that we won't gain
more people who abuse the command outside a repository.

Having said that, there may still be a use case where a Porcelain
script wants a way to see if a $name it has is appropriate as a
branch name before it has a repository (e.g. a wrapper to "git
clone" that wants to verify the name it is going to give to the "-b"
option), and a check desired in such a context is different from
(and is stricter than) feeding refs/heads/$name to the same command
without the "--branch" option.

So I think the right endgame in the longer term is:

 - Find (or add if it doesn't exist) a way to recommend to Porcelain
   scripts to use to expand an end-user generated string, and to map
   it to a branch name (it may be "rev-parse --symbolic-full-name
   $name"; I dunno).

 - Keep check-ref-format as (or revert it to be) a tool to "check".
   This would involve split strbuf_check_branch_ref() into two:

   - one that does not do the @{-1} thing and is used ONLY for
     format validity check (including rejecting a name that begins
     with a dash, which is OK for a random ref but not acceptable as
     a branch name);

   - the other that does @{-1} thing before doing the above.
   
   and then making the code call the former, not the latter.

The end result would be that check-ref-format becomes textual check
only, and can be usable (agian) outside repository, with or without
"--branch".  As the current code does not allow us do that yet, I
think it is safer to forbid use of --branch outside the repository
for now, purely as a bugfix.


[Footnote]

*1* In a sense, @{-1} is not something the scripts need to check its
    validity of---it is the branch you came from, so by definition
    it must be with a good name.  What the scripts want is instead
    see what the branch actually is, which is not what
    "check-ref-format" is about.

    a31dca03 ("check-ref-format --branch: give Porcelain a way to
    grok branch shorthand", 2009-03-21) says:

    The command may not be the best place to add this new feature, but
    
        $ git check-ref-format --branch "@{-1}"
    
    allows Porcelains to figure out what branch you were on before the last
    branch switching.

Re: [PATCH] check-ref-format: require a repository for --branch

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Having said that, there may still be a use case where a Porcelain
> script wants a way to see if a $name it has is appropriate as a
> branch name before it has a repository (e.g. a wrapper to "git
> clone" that wants to verify the name it is going to give to the "-b"
> option), and a check desired in such a context is different from
> (and is stricter than) feeding refs/heads/$name to the same command
> without the "--branch" option.
>
> So I think the right endgame in the longer term is:
> ...

Here is to illustrate what I mean in a patch form.  It resurrects
the gentle setup, and uses a purely textual format check when we are
outside the repository, while bypassing the @{magic} interpolation
codepath that requires us to be in a repository.  When we are in a
repository, we operate the same way as before.

Designed to be applied directly on top of 4d03f955
("check-ref-format: require a repository for --branch", 2017-07-14),
so it is missing the "'HEAD' is not a good branch name" I showed a
few days ago.

 builtin/check-ref-format.c  | 16 +++++++++++++---
 cache.h                     | 14 ++++++++++++++
 sha1_name.c                 | 22 +++++++++++++++++++---
 strbuf.h                    |  6 ++++++
 t/t1402-check-ref-format.sh | 10 +++++++++-
 5 files changed, 61 insertions(+), 7 deletions(-)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index 1e5f9835f0..4e62852089 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -38,12 +38,22 @@ static char *collapse_slashes(const char *refname)
 
 static int check_ref_format_branch(const char *arg)
 {
+	int nongit, malformed;
 	struct strbuf sb = STRBUF_INIT;
+	const char *name = arg;
 
-	setup_git_directory();
-	if (strbuf_check_branch_ref(&sb, arg))
+	setup_git_directory_gently(&nongit);
+
+	if (!nongit)
+		malformed = (strbuf_check_branch_ref(&sb, arg) ||
+			     !skip_prefix(sb.buf, "refs/heads/", &name));
+	else
+		malformed = check_branch_ref_format(arg);
+
+	if (malformed)
 		die("'%s' is not a valid branch name", arg);
-	printf("%s\n", sb.buf + 11);
+	printf("%s\n", name);
+	strbuf_release(&sb);
 	return 0;
 }
 
diff --git a/cache.h b/cache.h
index 52b91f5b64..3815925122 100644
--- a/cache.h
+++ b/cache.h
@@ -1444,6 +1444,20 @@ extern int parse_oid_hex(const char *hex, struct object_id *oid, const char **en
 #define INTERPRET_BRANCH_HEAD (1<<2)
 extern int interpret_branch_name(const char *str, int len, struct strbuf *,
 				 unsigned allowed);
+
+/*
+ * NEEDSWORK: declare strbuf_branchname() and strbuf_check_branch_ref()
+ * here, not in strbuf.h
+ */
+
+/*
+ * Check if a 'name' is suitable to be used as a branch name; this can
+ * be and is stricter than what check_refname_format() returns for a
+ * string that is a concatenation of "name" after "refs/heads/"; a
+ * name that begins with "-" is not allowed, for example.
+ */
+extern int check_branch_ref_format(const char *name);
+
 extern int get_oid_mb(const char *str, struct object_id *oid);
 
 extern int validate_headref(const char *ref);
diff --git a/sha1_name.c b/sha1_name.c
index 5e2ec37b65..c95080258f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1319,15 +1319,31 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
 	strbuf_add(sb, name + used, len - used);
 }
 
-int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
+static int strbuf_check_branch_ref_format(struct strbuf *sb)
 {
-	strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
-	if (name[0] == '-')
+	if (*sb->buf == '-')
 		return -1;
 	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
 	return check_refname_format(sb->buf, 0);
 }
 
+int check_branch_ref_format(const char *name)
+{
+	struct strbuf sb = STRBUF_INIT;
+	int result;
+
+	strbuf_addstr(&sb, name);
+	result = strbuf_check_branch_ref_format(&sb);
+	strbuf_release(&sb);
+	return result;
+}
+
+int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
+{
+	strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
+	return strbuf_check_branch_ref_format(sb);
+}
+
 /*
  * This is like "get_sha1_basic()", except it allows "sha1 expressions",
  * notably "xyz^" for "parent of xyz"
diff --git a/strbuf.h b/strbuf.h
index d785258649..3da95685b2 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -568,6 +568,12 @@ static inline void strbuf_complete_line(struct strbuf *sb)
 	strbuf_complete(sb, '\n');
 }
 
+/*
+ * NEEDSWORK: the following two functions should not be in this file;
+ * these are about refnames, and should be declared next to
+ * interpret_branch_name() in cache.h
+ */
+
 /*
  * Copy "name" to "sb", expanding any special @-marks as handled by
  * interpret_branch_name(). The result is a non-qualified branch name
diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index 1674b061e2..2ddefb4bd1 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -161,10 +161,18 @@ test_expect_success 'check-ref-format --branch from subdir' '
 	test "$refname" = "$sha1"
 '
 
-test_expect_success 'check-ref-format --branch from non-repo' '
+test_expect_success 'check-ref-format --branch @{-1} from non-repo' '
 	test_must_fail nongit git check-ref-format --branch @{-1}
 '
 
+test_expect_success 'check-ref-format --branch master in non-repo' '
+	nongit git check-ref-format --branch master
+'
+
+test_expect_success 'check-ref-format --branch -naster in repo' '
+	test_must_fail git check-ref-format --branch -naster
+'
+
 valid_ref_normalized() {
 	prereq=
 	case $1 in

Re: [PATCH] check-ref-format: require a repository for --branch

[Jeff King]

On Mon, Oct 16, 2017 at 03:44:08PM +0900, Junio C Hamano wrote:

> I threw this topic in stalled category, hoping that one or the other
> opinion eventually turns out to be more prevalent, but it didn't
> seem to have happened X-<.

I think it's sufficiently obscure that nobody really cares.

I admit that _I_ don't actually care myself. We should fix the BUG(),
obviously, but between the two I could live with it either way. Mostly I
didn't want to go to the work to write the patch for the direction that
I didn't think was right, and was hoping Jonathan would if he felt
strongly about it.

> So I think the right endgame in the longer term is:

I won't quote the rest of your message because I agree with it
completely, in terms of the endgame we'd like to see.

I'll address a few specific comments on your followup patch.

-Peff

Re: [PATCH] check-ref-format: require a repository for --branch

[Jeff King]

On Mon, Oct 16, 2017 at 07:45:46PM +0900, Junio C Hamano wrote:

> > So I think the right endgame in the longer term is:
> > ...
> 
> Here is to illustrate what I mean in a patch form.  It resurrects
> the gentle setup, and uses a purely textual format check when we are
> outside the repository, while bypassing the @{magic} interpolation
> codepath that requires us to be in a repository.  When we are in a
> repository, we operate the same way as before.

I like the state this puts us in, but there's one catch: we're
completely changing the meaning of "check-ref-format --branch", aren't
we?

It is going from "this is how you resolve @{-1}" to "this is how you
check the validity of a potential branch name". Do we need to pick a
different name, and/or have a deprecation period?

-Peff

Re: [PATCH] check-ref-format: require a repository for --branch

[Jonathan Nieder]

Jeff King wrote:
> On Mon, Oct 16, 2017 at 07:45:46PM +0900, Junio C Hamano wrote:

>> Here is to illustrate what I mean in a patch form.  It resurrects
>> the gentle setup, and uses a purely textual format check when we are
>> outside the repository, while bypassing the @{magic} interpolation
>> codepath that requires us to be in a repository.  When we are in a
>> repository, we operate the same way as before.
>
> I like the state this puts us in, but there's one catch: we're
> completely changing the meaning of "check-ref-format --branch", aren't
> we?
>
> It is going from "this is how you resolve @{-1}" to "this is how you
> check the validity of a potential branch name". Do we need to pick a
> different name, and/or have a deprecation period?

Sorry to take so long on picking this up.  I'll try to make an
alternate patch today.

For what it's worth, I don't agree with this repurposing of
"check-ref-format --branch" at all.  The old command already existed.
No one asked for the new command.  At most, we could get rid of the
old command after a deprecation period.  I don't understand at all why
it's worth the confusion of changing its meaning.

Thanks,
Jonathan

Re: [PATCH] check-ref-format: require a repository for --branch

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Oct 16, 2017 at 07:45:46PM +0900, Junio C Hamano wrote:
>
>> > So I think the right endgame in the longer term is:
>> > ...
>> 
>> Here is to illustrate what I mean in a patch form.  It resurrects
>> the gentle setup, and uses a purely textual format check when we are
>> outside the repository, while bypassing the @{magic} interpolation
>> codepath that requires us to be in a repository.  When we are in a
>> repository, we operate the same way as before.
>
> I like the state this puts us in, but there's one catch: we're
> completely changing the meaning of "check-ref-format --branch", aren't
> we?
>
> It is going from "this is how you resolve @{-1}" to "this is how you
> check the validity of a potential branch name". Do we need to pick a
> different name, and/or have a deprecation period?

That was not my intention.  When used in a repository, it behaves
exactly the same as before, including @{-1} resolution part.  And by
using strbuf_check_branch_ref(), it has always been checking the
validity of a potential branch name, even though it wasn't
advertised as such.  The documentation needs to be updated, I would
think.

When used outside a repository, @{-1} would not have worked anyway,
and @{-1} continues not to work, but the part that checks the
validity should continue to work.

At least that is what I wanted to happen in the patch.

Re: [PATCH] check-ref-format: require a repository for --branch

[Kevin Daudt]

On Mon, Oct 16, 2017 at 07:45:46PM +0900, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> [..]
> 
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index 1e5f9835f0..4e62852089 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -38,12 +38,22 @@ static char *collapse_slashes(const char *refname)
>  
>  static int check_ref_format_branch(const char *arg)
>  {
> +	int nongit, malformed;
>  	struct strbuf sb = STRBUF_INIT;
> +	const char *name = arg;
>  
> -	setup_git_directory();
> -	if (strbuf_check_branch_ref(&sb, arg))
> +	setup_git_directory_gently(&nongit);
> +
> +	if (!nongit)
> +		malformed = (strbuf_check_branch_ref(&sb, arg) ||
> +			     !skip_prefix(sb.buf, "refs/heads/", &name));
> +	else
> +		malformed = check_branch_ref_format(arg);
> +

Would it make sense to swap the logic and get rid of the double
negative (!nongit)?

> +	if (malformed)
>  		die("'%s' is not a valid branch name", arg);
> -	printf("%s\n", sb.buf + 11);
> +	printf("%s\n", name);
> +	strbuf_release(&sb);
>  	return 0;
>  }
>  

Re: [PATCH] check-ref-format: require a repository for --branch

[Junio C Hamano]

Kevin Daudt <me@ikke.info> writes:

>> +	setup_git_directory_gently(&nongit);
>> +
>> +	if (!nongit)
>> +		malformed = (strbuf_check_branch_ref(&sb, arg) ||
>> +			     !skip_prefix(sb.buf, "refs/heads/", &name));
>> +	else
>> +		malformed = check_branch_ref_format(arg);
>> +
>
> Would it make sense to swap the logic and get rid of the double
> negative (!nongit)?

I am trying to follow the pattern "handle the normal case that have
been supported forever first, and then handle new exception next",
so that it is easier to see that there is no behaviour change in the
normal case, so I do not think it makes it easier to see to swap the
if/else cases.
>
>> +	if (malformed)
>>  		die("'%s' is not a valid branch name", arg);
>> -	printf("%s\n", sb.buf + 11);
>> +	printf("%s\n", name);
>> +	strbuf_release(&sb);
>>  	return 0;
>>  }
>>  

Re: [PATCH] check-ref-format: require a repository for --branch

[Jeff King]

On Tue, Oct 17, 2017 at 10:22:31AM +0900, Junio C Hamano wrote:

> > I like the state this puts us in, but there's one catch: we're
> > completely changing the meaning of "check-ref-format --branch", aren't
> > we?
> >
> > It is going from "this is how you resolve @{-1}" to "this is how you
> > check the validity of a potential branch name". Do we need to pick a
> > different name, and/or have a deprecation period?
> 
> That was not my intention.  When used in a repository, it behaves
> exactly the same as before, including @{-1} resolution part.  And by
> using strbuf_check_branch_ref(), it has always been checking the
> validity of a potential branch name, even though it wasn't
> advertised as such.  The documentation needs to be updated, I would
> think.
> 
> When used outside a repository, @{-1} would not have worked anyway,
> and @{-1} continues not to work, but the part that checks the
> validity should continue to work.
> 
> At least that is what I wanted to happen in the patch.

Ah, OK, I did not read carefully enough then. I think that would be OK,
and probably close to what Jonathan was asking for.

It leaves unresolved the fact that the resolving feature does not belong
in check-ref-format in the first place, but we can just accept that as a
historical wart.

I don't think there is any need to prepare it upon my 4d03f955, though.
I'd think it could simply replace it.

-Peff

Re: [PATCH] check-ref-format: require a repository for --branch

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Oct 17, 2017 at 10:22:31AM +0900, Junio C Hamano wrote:
>
>> > I like the state this puts us in, but there's one catch: we're
>> > completely changing the meaning of "check-ref-format --branch", aren't
>> > we?
>> >
>> > It is going from "this is how you resolve @{-1}" to "this is how you
>> > check the validity of a potential branch name". Do we need to pick a
>> > different name, and/or have a deprecation period?
>>  ...
>> At least that is what I wanted to happen in the patch.
>
> Ah, OK, I did not read carefully enough then. I think that would be OK,
> and probably close to what Jonathan was asking for.
>
> It leaves unresolved the fact that the resolving feature does not belong
> in check-ref-format in the first place, but we can just accept that as a
> historical wart.

Yup, I actually was in favor of removing that and making it a
"purely checking validity" feature, but given that it has been
advertised in the documentation since 604e0cb5 ("Documentation:
describe check-ref-format --branch", 2009-10-12), it is a bit too
late to tell users that rev-parse is the right/kosher thing to do.

> I don't think there is any need to prepare it upon my 4d03f955,
> though.  I'd think it could simply replace it.

Yeah, it ended up that way, it seems.  Still it needs a bit of doc
updates to balance the description.  Right now we stress on @{-n}
resolution too much.

Perhaps something like this?

 Documentation/git-check-ref-format.txt | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
index 92777cef25..cf0a0b7df2 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -77,7 +77,14 @@ reference name expressions (see linkgit:gitrevisions[7]):
 
 . at-open-brace `@{` is used as a notation to access a reflog entry.
 
-With the `--branch` option, it expands the ``previous branch syntax''
+With the `--branch` option, the command takes a name and checks if
+it can be used as a valid branch name (e.g. when creating a new
+branch).  The rule `git check-ref-format --branch $name` implements
+may be stricter than what `git check-ref-format refs/heads/$name`
+says (e.g. a dash may appear at the beginning of a ref component,
+but it is explicitly forbidden at the beginning of a branch name).
+When run with `--branch` option in a repository, the input is first
+expanded for the ``previous branch syntax''
 `@{-n}`.  For example, `@{-1}` is a way to refer the last branch you
 were on.  This option should be used by porcelains to accept this
 syntax anywhere a branch name is expected, so they can act as if you

Re: [PATCH] check-ref-format: require a repository for --branch

[Kevin Daudt]

On Tue, Oct 17, 2017 at 11:40:17AM +0900, Junio C Hamano wrote:
> Kevin Daudt <me@ikke.info> writes:
> 
> >> +	setup_git_directory_gently(&nongit);
> >> +
> >> +	if (!nongit)
> >> +		malformed = (strbuf_check_branch_ref(&sb, arg) ||
> >> +			     !skip_prefix(sb.buf, "refs/heads/", &name));
> >> +	else
> >> +		malformed = check_branch_ref_format(arg);
> >> +
> >
> > Would it make sense to swap the logic and get rid of the double
> > negative (!nongit)?
> 
> I am trying to follow the pattern "handle the normal case that have
> been supported forever first, and then handle new exception next",
> so that it is easier to see that there is no behaviour change in the
> normal case, so I do not think it makes it easier to see to swap the
> if/else cases.

Ok, thanks for your reasoning, makes sense.
> >
> >> +	if (malformed)
> >>  		die("'%s' is not a valid branch name", arg);
> >> -	printf("%s\n", sb.buf + 11);
> >> +	printf("%s\n", name);
> >> +	strbuf_release(&sb);
> >>  	return 0;
> >>  }
> >>  

Re: [PATCH] check-ref-format: require a repository for --branch

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

> Things like @{-1} would not make any sense when the command is run
> outside a repository, and the documentation is quite clear that it
> is the primary reason why we added "--branch" option to the command,
> i.e.
>
>     With the `--branch` option, it expands the ``previous branch syntax''
>     `@{-n}`.  For example, `@{-1}` is a way to refer the last branch you
>     were on.  This option should be used by porcelains to accept this
>     syntax anywhere a branch name is expected, so they can act as if you
>     typed the branch name.
>
> So I am tempted to take this patch to make sure that we won't gain
> more people who abuse the command outside a repository.

That seems very sensible on its face.  My only worry is that a script
that can be run both inside and outside a repository and does

	branch=$(git check-ref-format --branch "$user_supplied_branch_arg")

currently works with user_supplied_branch_arg='master' and would stop
working.  If we have reason to believe that no such scripts exist,
then this would be a good way to go, but I don't believe we can count
on that.

And in that spirit, I think the patch you replied with aims to go in
the right direction, by providing the core functionality when in a
repository while avoiding breaking such a script outside of one
(though I do not understand it fully yet).

> Having said that, there may still be a use case where a Porcelain
> script wants a way to see if a $name it has is appropriate as a
> branch name before it has a repository

This seems like a different goal than "git check-ref-format --branch"
was originally designed to fulfill (even though it fits well with the
check-ref-format name and coincides with --branch behavior when in a
repository).  I think it's fine for us not to fulfill it.

>                                        (e.g. a wrapper to "git
> clone" that wants to verify the name it is going to give to the "-b"
> option), and a check desired in such a context is different from
> (and is stricter than) feeding refs/heads/$name to the same command
> without the "--branch" option.

Can you say more about this example?  E.g. why is this hypothetical
wrapper unable to rely on "git clone -b"'s own error handling?

> So I think the right endgame in the longer term is:
>
>  - Find (or add if it doesn't exist) a way to recommend to Porcelain
>    scripts to use to expand an end-user generated string, and to map
>    it to a branch name (it may be "rev-parse --symbolic-full-name
>    $name"; I dunno).

--symbolic-full-name seems like a good fit.  Do you remember why
check-ref-format was introduced instead?  Was it just a matter of
implementation simplicity, since --symbolic-full-name can handle a
broader class of revision specifications like --remotes?  The commit
message to v1.6.3-rc0~29^2~4 (give Porcelain a way to grok branch
shorthand, 2009-03-21) is appropriately apologetic but doesn't give
more clues.

>  - Keep check-ref-format as (or revert it to be) a tool to "check".
>    This would involve split strbuf_check_branch_ref() into two:

Without an example of where this tool would be used, if we consider
"check-ref-format --branch" to be a mistake then I'd rather deprecate
it with a goal of removing it completely.

Ok, time to look in more detail.

Thanks for your thoughtfulness,
Jonathan

Re: [PATCH] check-ref-format: require a repository for --branch

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> I don't think there is any need to prepare it upon my 4d03f955,
>> though.  I'd think it could simply replace it.
>
> Yeah, it ended up that way, it seems.  Still it needs a bit of doc
> updates to balance the description.

So here is what I have now on 'pu'.  Clearly not a 2.15 material yet.

-- >8 --
Subject: [PATCH] check-ref-format: --branch cannot grok @{-1} outside a repository

"git check-ref-format --branch $name" feature was originally
introduced (and was advertised) as a way for scripts to take any
end-user supplied string (like "master", "@{-1}" etc.) and see if
that is usable when Git expects to see a branch name, and also
obtain the concrete branch name that the at-mark magic expands to.

When the user asks to interpret a branch name like "@{-1}", we have
to dig the answer out of the HEAD reflog.  We can obviously only do
that if we have a repository, and indeed, running it outside a
repository causes us to hit a BUG().

Let's disable the "expand @{-n}" half of the feature when it is run
outside a repository, but keep the feature to check the syntax of a
proposed branch name, as "git check-ref-format --branch $name" can
be stricter than "git check-ref-format refs/heads/$name", and
Porcelain scripts need to have a way to check a given name against
the stricter rule.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-check-ref-format.txt |  9 ++++++++-
 builtin/check-ref-format.c             | 15 ++++++++++++---
 cache.h                                | 14 ++++++++++++++
 sha1_name.c                            | 22 +++++++++++++++++++---
 strbuf.h                               |  6 ++++++
 t/t1402-check-ref-format.sh            | 12 ++++++++++++
 6 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
index 92777cef25..cf0a0b7df2 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -77,7 +77,14 @@ reference name expressions (see linkgit:gitrevisions[7]):
 
 . at-open-brace `@{` is used as a notation to access a reflog entry.
 
-With the `--branch` option, it expands the ``previous branch syntax''
+With the `--branch` option, the command takes a name and checks if
+it can be used as a valid branch name (e.g. when creating a new
+branch).  The rule `git check-ref-format --branch $name` implements
+may be stricter than what `git check-ref-format refs/heads/$name`
+says (e.g. a dash may appear at the beginning of a ref component,
+but it is explicitly forbidden at the beginning of a branch name).
+When run with `--branch` option in a repository, the input is first
+expanded for the ``previous branch syntax''
 `@{-n}`.  For example, `@{-1}` is a way to refer the last branch you
 were on.  This option should be used by porcelains to accept this
 syntax anywhere a branch name is expected, so they can act as if you
diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index eac499450f..4e62852089 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -38,13 +38,22 @@ static char *collapse_slashes(const char *refname)
 
 static int check_ref_format_branch(const char *arg)
 {
+	int nongit, malformed;
 	struct strbuf sb = STRBUF_INIT;
-	int nongit;
+	const char *name = arg;
 
 	setup_git_directory_gently(&nongit);
-	if (strbuf_check_branch_ref(&sb, arg))
+
+	if (!nongit)
+		malformed = (strbuf_check_branch_ref(&sb, arg) ||
+			     !skip_prefix(sb.buf, "refs/heads/", &name));
+	else
+		malformed = check_branch_ref_format(arg);
+
+	if (malformed)
 		die("'%s' is not a valid branch name", arg);
-	printf("%s\n", sb.buf + 11);
+	printf("%s\n", name);
+	strbuf_release(&sb);
 	return 0;
 }
 
diff --git a/cache.h b/cache.h
index 52b91f5b64..3815925122 100644
--- a/cache.h
+++ b/cache.h
@@ -1444,6 +1444,20 @@ extern int parse_oid_hex(const char *hex, struct object_id *oid, const char **en
 #define INTERPRET_BRANCH_HEAD (1<<2)
 extern int interpret_branch_name(const char *str, int len, struct strbuf *,
 				 unsigned allowed);
+
+/*
+ * NEEDSWORK: declare strbuf_branchname() and strbuf_check_branch_ref()
+ * here, not in strbuf.h
+ */
+
+/*
+ * Check if a 'name' is suitable to be used as a branch name; this can
+ * be and is stricter than what check_refname_format() returns for a
+ * string that is a concatenation of "name" after "refs/heads/"; a
+ * name that begins with "-" is not allowed, for example.
+ */
+extern int check_branch_ref_format(const char *name);
+
 extern int get_oid_mb(const char *str, struct object_id *oid);
 
 extern int validate_headref(const char *ref);
diff --git a/sha1_name.c b/sha1_name.c
index 5e2ec37b65..c95080258f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1319,15 +1319,31 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
 	strbuf_add(sb, name + used, len - used);
 }
 
-int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
+static int strbuf_check_branch_ref_format(struct strbuf *sb)
 {
-	strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
-	if (name[0] == '-')
+	if (*sb->buf == '-')
 		return -1;
 	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
 	return check_refname_format(sb->buf, 0);
 }
 
+int check_branch_ref_format(const char *name)
+{
+	struct strbuf sb = STRBUF_INIT;
+	int result;
+
+	strbuf_addstr(&sb, name);
+	result = strbuf_check_branch_ref_format(&sb);
+	strbuf_release(&sb);
+	return result;
+}
+
+int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
+{
+	strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
+	return strbuf_check_branch_ref_format(sb);
+}
+
 /*
  * This is like "get_sha1_basic()", except it allows "sha1 expressions",
  * notably "xyz^" for "parent of xyz"
diff --git a/strbuf.h b/strbuf.h
index d785258649..3da95685b2 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -568,6 +568,12 @@ static inline void strbuf_complete_line(struct strbuf *sb)
 	strbuf_complete(sb, '\n');
 }
 
+/*
+ * NEEDSWORK: the following two functions should not be in this file;
+ * these are about refnames, and should be declared next to
+ * interpret_branch_name() in cache.h
+ */
+
 /*
  * Copy "name" to "sb", expanding any special @-marks as handled by
  * interpret_branch_name(). The result is a non-qualified branch name
diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index 0790edf60d..2ddefb4bd1 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -161,6 +161,18 @@ test_expect_success 'check-ref-format --branch from subdir' '
 	test "$refname" = "$sha1"
 '
 
+test_expect_success 'check-ref-format --branch @{-1} from non-repo' '
+	test_must_fail nongit git check-ref-format --branch @{-1}
+'
+
+test_expect_success 'check-ref-format --branch master in non-repo' '
+	nongit git check-ref-format --branch master
+'
+
+test_expect_success 'check-ref-format --branch -naster in repo' '
+	test_must_fail git check-ref-format --branch -naster
+'
+
 valid_ref_normalized() {
 	prereq=
 	case $1 in
-- 
2.15.0-rc1-178-geb01ff2fe8

Re: [PATCH] check-ref-format: require a repository for --branch

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> And in that spirit, I think the patch you replied with aims to go in
> the right direction, by providing the core functionality when in a
> repository while avoiding breaking such a script outside of one
> (though I do not understand it fully yet).

Given that, is it safe for me to ignore this earlier one

> For what it's worth, I don't agree with this repurposing of
> "check-ref-format --branch" at all.

as reacting to the patch without reading what it does?

>>                                        (e.g. a wrapper to "git
>> clone" that wants to verify the name it is going to give to the "-b"
>> option), and a check desired in such a context is different from
>> (and is stricter than) feeding refs/heads/$name to the same command
>> without the "--branch" option.
>
> Can you say more about this example?  E.g. why is this hypothetical
> wrapper unable to rely on "git clone -b"'s own error handling?

If I have to guess what you meant, perhaps the Porcelain wanted to
diagnose bad parameter that would be rejected _before_ letting clone
spend cycles and possibly network bandwidth?

But this was my way of rephrasing an earlier example you used while
objecting to Peff's change

    For example, if you have a script with an $opt_branch variable
    that defaults to "master", it may do

            resolved_branch=$(git check-ref-format --branch "$opt_branch")

    even though it is in a mode that not going to have to use
    $resolved_branch and it is not running from a repository.

so my answer to the question in the message I am directly responding
to would be "You tell me." ;-)

> --symbolic-full-name seems like a good fit.  Do you remember why
> check-ref-format was introduced instead?

It really serves two purposes, and at this point, I do not think it
really matters why it also does the @{-1} expansion as well as name
validation.  604e0cb5 ("Documentation: describe check-ref-format
--branch", 2009-10-12) happened 8 years ago, and since then it has
been advertised long enough as if the 80% primary purpose of "c-r-f
--branch" were to expand @{-1} to a branch name; even though the
text hints that it also does the usual checks, by definition
validation of the result of expanding @{-1} ought to succeed; after
all that was the branch we _were_ on ;-).

[PATCH 0/3] Re: [PATCH] check-ref-format: require a repository for --branch

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

> Subject: [PATCH] check-ref-format: --branch cannot grok @{-1} outside a repository

How about this?  It is written to be more conservative than the patch
I am replying to, but except for the commit message, it should be
pretty much equivalent.

[...]
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -38,13 +38,22 @@ static char *collapse_slashes(const char *refname)
>  
>  static int check_ref_format_branch(const char *arg)
>  {
> +	int nongit, malformed;
>  	struct strbuf sb = STRBUF_INIT;
> -	int nongit;
> +	const char *name = arg;
>  
>  	setup_git_directory_gently(&nongit);
> -	if (strbuf_check_branch_ref(&sb, arg))
> +
> +	if (!nongit)
> +		malformed = (strbuf_check_branch_ref(&sb, arg) ||
> +			     !skip_prefix(sb.buf, "refs/heads/", &name));
> +	else
> +		malformed = check_branch_ref_format(arg);

Handles the nongit case in strbuf_check_branch_ref instead of
introducing a new check_branch_ref_format helper.

[...]
> --- a/cache.h
> +++ b/cache.h
> @@ -1444,6 +1444,20 @@ extern int parse_oid_hex(const char *hex, struct object_id *oid, const char **en
>  #define INTERPRET_BRANCH_HEAD (1<<2)
>  extern int interpret_branch_name(const char *str, int len, struct strbuf *,
>  				 unsigned allowed);
> +
> +/*
> + * NEEDSWORK: declare strbuf_branchname() and strbuf_check_branch_ref()
> + * here, not in strbuf.h
> + */

As a result, it doesn't touch headers.  I agree that these functions
don't belong in strbuf.h (sorry for not updating the headers at the
same time I moved their implementations) but suspect e.g. branch.h,
revision.h, or some new header like revision-syntax.h would be a
better place.

[...]
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -568,6 +568,12 @@ static inline void strbuf_complete_line(struct strbuf *sb)
>  	strbuf_complete(sb, '\n');
>  }
>  
> +/*
> + * NEEDSWORK: the following two functions should not be in this file;
> + * these are about refnames, and should be declared next to
> + * interpret_branch_name() in cache.h
> + */

Didn't touch headers.

[...]
> --- a/t/t1402-check-ref-format.sh
> +++ b/t/t1402-check-ref-format.sh
> @@ -161,6 +161,18 @@ test_expect_success 'check-ref-format --branch from subdir' '
>  	test "$refname" = "$sha1"
>  '
>  
> +test_expect_success 'check-ref-format --branch @{-1} from non-repo' '
> +	test_must_fail nongit git check-ref-format --branch @{-1}
> +'

Swapped test_must_fail and nongit to match existing tests.

Junio C Hamano (3):
  check-ref-format --branch: do not expand @{...} outside repository
  check-ref-format --branch: strip refs/heads/ using skip_prefix
  check-ref-format doc: --branch validates and expands <branch>

 Documentation/git-check-ref-format.txt |  9 ++++++++-
 builtin/check-ref-format.c             |  6 ++++--
 sha1_name.c                            |  5 ++++-
 t/t1402-check-ref-format.sh            | 16 ++++++++++++++++
 4 files changed, 32 insertions(+), 4 deletions(-)

[PATCH 1/3] check-ref-format --branch: do not expand @{...} outside repository

[Jonathan Nieder]

From: Junio C Hamano <gitster@pobox.com>

Running "git check-ref-format --branch @{-1}" from outside any
repository produces

	$ git check-ref-format --branch @{-1}
	BUG: environment.c:182: git environment hasn't been setup

This is because the expansion of @{-1} must come from the HEAD reflog,
which involves opening the repository.  @{u} and @{push} (which are
more unusual because they typically would not expand to a local
branch) trigger the same assertion.

This has been broken since day one.  Before v2.13.0-rc0~48^2
(setup_git_env: avoid blind fall-back to ".git", 2016-10-02), the
breakage was more subtle: Git would read reflogs from ".git" within
the current directory even if it was not a valid repository.  Usually
that is harmless because Git is not being run from the root directory
of an invalid repository, but in edge cases such accesses can be
confusing or harmful.  Since v2.13.0, the problem is easier to
diagnose because Git aborts with a BUG message.

Erroring out is the right behavior: when asked to interpret a branch
name like "@{-1}", there is no reasonable answer in this context.
But we should print a message saying so instead of an assertion failure.

We do not forbid "check-ref-format --branch" from outside a repository
altogether because it is ok for a script to pre-process branch
arguments without @{...} in such a context.  For example, with
pre-2.13 Git, a script that does

	branch='master'; # default value
	parse_options
	branch=$(git check-ref-format --branch "$branch")

to normalize an optional branch name provided by the user would work
both inside a repository (where the user could provide '@{-1}') and
outside (where '@{-1}' should not be accepted).

So disable the "expand @{...}" half of the feature when run outside a
repository, but keep the check of the syntax of a proposed branch
name. This way, when run from outside a repository, "git
check-ref-format --branch @{-1}" will gracefully fail:

	$ git check-ref-format --branch @{-1}
	fatal: '@{-1}' is not a valid branch name

and "git check-ref-format --branch master" will succeed as before:

	$ git check-ref-format --branch master
	master

restoring the usual pre-2.13 behavior.

[jn: split out from a larger patch; moved conditional to
 strbuf_check_branch_ref instead of its caller; fleshed out commit
 message; some style tweaks in tests]

Reported-by: Marko Kungla <marko.kungla@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 sha1_name.c                 |  5 ++++-
 t/t1402-check-ref-format.sh | 16 ++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index c7c5ab376c..603e667faa 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1331,7 +1331,10 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
 
 int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 {
-	strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
+	if (startup_info->have_repository)
+		strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
+	else
+		strbuf_addstr(sb, name);
 	if (name[0] == '-')
 		return -1;
 	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index 0790edf60d..98e4a8613b 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -144,6 +144,11 @@ test_expect_success "check-ref-format --branch @{-1}" '
 	refname2=$(git check-ref-format --branch @{-2}) &&
 	test "$refname2" = master'
 
+test_expect_success 'check-ref-format --branch -naster' '
+	test_must_fail git check-ref-format --branch -naster >actual &&
+	test_must_be_empty actual
+'
+
 test_expect_success 'check-ref-format --branch from subdir' '
 	mkdir subdir &&
 
@@ -161,6 +166,17 @@ test_expect_success 'check-ref-format --branch from subdir' '
 	test "$refname" = "$sha1"
 '
 
+test_expect_success 'check-ref-format --branch @{-1} from non-repo' '
+	nongit test_must_fail git check-ref-format --branch @{-1} >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'check-ref-format --branch master from non-repo' '
+	echo master >expect &&
+	nongit git check-ref-format --branch master >actual &&
+	test_cmp expect actual
+'
+
 valid_ref_normalized() {
 	prereq=
 	case $1 in
-- 
2.15.0.rc1.287.g2b38de12cc

[PATCH 2/3] check-ref-format --branch: strip refs/heads/ using skip_prefix

[Jonathan Nieder]

From: Junio C Hamano <gitster@pobox.com>

The expansion returned from strbuf_check_branch_ref always starts with
"refs/heads/" by construction, but there is nothing about its name or
advertised API making that obvious.  This command is used to process
human-supplied input from the command line and is usually not the
inner loop, so we can spare some cycles to be more defensive.  Instead
of hard-coding the offset strlen("refs/heads/") to skip, verify that
the expansion actually starts with refs/heads/.

[jn: split out from a larger patch, added explanation]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/check-ref-format.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index 6c40ff110b..bc67d3f0a8 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -39,12 +39,14 @@ static char *collapse_slashes(const char *refname)
 static int check_ref_format_branch(const char *arg)
 {
 	struct strbuf sb = STRBUF_INIT;
+	const char *name;
 	int nongit;
 
 	setup_git_directory_gently(&nongit);
-	if (strbuf_check_branch_ref(&sb, arg))
+	if (strbuf_check_branch_ref(&sb, arg) ||
+	    !skip_prefix(sb.buf, "refs/heads/", &name))
 		die("'%s' is not a valid branch name", arg);
-	printf("%s\n", sb.buf + 11);
+	printf("%s\n", name);
 	strbuf_release(&sb);
 	return 0;
 }
-- 
2.15.0.rc1.287.g2b38de12cc

Re: [PATCH 0/3] Re: [PATCH] check-ref-format: require a repository for --branch

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Handles the nongit case in strbuf_check_branch_ref instead of
> introducing a new check_branch_ref_format helper.

I view that as a regression, actually.  Don't we want a function
that does not require a strbuf when asking a simple question: "I
have a string, and I want to see if that is a valid name"?

[PATCH 3/3] check-ref-format doc: --branch validates and expands <branch>

[Jonathan Nieder]

From: Junio C Hamano <gitster@pobox.com>

"git check-ref-format --branch $name" feature was originally
introduced (and was advertised) as a way for scripts to take any
end-user supplied string (like "master", "@{-1}" etc.) and see if it
is usable when Git expects to see a branch name, and also obtain the
concrete branch name that the at-mark magic expands to.

Emphasize that "see if it is usable" role in the description and
clarify that the @{...} expansion only occurs when run from within a
repository.

[jn: split out from a larger patch]

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-check-ref-format.txt | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
index 92777cef25..cf0a0b7df2 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -77,7 +77,14 @@ reference name expressions (see linkgit:gitrevisions[7]):
 
 . at-open-brace `@{` is used as a notation to access a reflog entry.
 
-With the `--branch` option, it expands the ``previous branch syntax''
+With the `--branch` option, the command takes a name and checks if
+it can be used as a valid branch name (e.g. when creating a new
+branch).  The rule `git check-ref-format --branch $name` implements
+may be stricter than what `git check-ref-format refs/heads/$name`
+says (e.g. a dash may appear at the beginning of a ref component,
+but it is explicitly forbidden at the beginning of a branch name).
+When run with `--branch` option in a repository, the input is first
+expanded for the ``previous branch syntax''
 `@{-n}`.  For example, `@{-1}` is a way to refer the last branch you
 were on.  This option should be used by porcelains to accept this
 syntax anywhere a branch name is expected, so they can act as if you
-- 
2.15.0.rc1.287.g2b38de12cc

Re: [PATCH 0/3] Re: [PATCH] check-ref-format: require a repository for --branch

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> Handles the nongit case in strbuf_check_branch_ref instead of
>> introducing a new check_branch_ref_format helper.
>
> I view that as a regression, actually.  Don't we want a function
> that does not require a strbuf when asking a simple question: "I
> have a string, and I want to see if that is a valid name"?

*shrug* I found the change easier to read, and it also sidesteps the
which-header question.  It also ensures that other
strbuf_check_branch_ref callers are safe without having to audit them.

But feel free to tweak that back if you like, or I can tomorrow.

Jonathan

Re: [PATCH] check-ref-format: require a repository for --branch

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> And in that spirit, I think the patch you replied with aims to go in
>> the right direction, by providing the core functionality when in a
>> repository while avoiding breaking such a script outside of one
>> (though I do not understand it fully yet).
>
> Given that, is it safe for me to ignore this earlier one
>
>> For what it's worth, I don't agree with this repurposing of
>> "check-ref-format --branch" at all.
>
> as reacting to the patch without reading what it does?

Are you saying I should have ignored the commit message?  Recording
future plans via the commit message is part of what the patch does,
after all.

>> Junio C Hamano wrote:

>>>                                        (e.g. a wrapper to "git
>>> clone" that wants to verify the name it is going to give to the "-b"
>>> option), and a check desired in such a context is different from
>>> (and is stricter than) feeding refs/heads/$name to the same command
>>> without the "--branch" option.
>>
>> Can you say more about this example?  E.g. why is this hypothetical
>> wrapper unable to rely on "git clone -b"'s own error handling?
>
> If I have to guess what you meant, perhaps the Porcelain wanted to
> diagnose bad parameter that would be rejected _before_ letting clone
> spend cycles and possibly network bandwidth?
>
> But this was my way of rephrasing an earlier example you used while
> objecting to Peff's change
[...]
> so my answer to the question in the message I am directly responding
> to would be "You tell me." ;-)

Hm.  Does the example in
https://public-inbox.org/git/20171017070808.plddffhzdobyekmo@aiede.mtv.corp.google.com/
make it clearer?

The goal of such a script is *not* error handling --- that is just a
pleasant side-benefit.  It is to be able to handle all branch
specifiers from the user (and even a default branch that is not from
the user) uniformly.

Thanks,
Jonathan

Re: [PATCH] check-ref-format: require a repository for --branch

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> And in that spirit, I think the patch you replied with aims to go in
>> the right direction, by providing the core functionality when in a
>> repository while avoiding breaking such a script outside of one
>> (though I do not understand it fully yet).
>
> Given that, is it safe for me to ignore this earlier one
>
>> For what it's worth, I don't agree with this repurposing of
>> "check-ref-format --branch" at all.
>
> as reacting to the patch without reading what it does?

On second reading, yes, I was reacting to the discussion leading up to
the patch instead of the patch.  The patch looks good.

Sorry for the noise,
Jonathan

Re: [PATCH 0/3] Re: [PATCH] check-ref-format: require a repository for --branch

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> Handles the nongit case in strbuf_check_branch_ref instead of
>>> introducing a new check_branch_ref_format helper.
>>
>> I view that as a regression, actually.  Don't we want a function
>> that does not require a strbuf when asking a simple question: "I
>> have a string, and I want to see if that is a valid name"?
>
> *shrug* I found the change easier to read, and it also sidesteps the
> which-header question.  It also ensures that other
> strbuf_check_branch_ref callers are safe without having to audit them.

Please ignore the above, which was merely an impression _without_
and before having received any patch to comment on ;-)

Quite frankly, this is a Meh topic that won't have to hit even
'next' before the final.  The color.ui=always thing has a lot more
urgency, and this was merely what I did while waiting for others to
react to that topic.

Re: [PATCH 3/3] check-ref-format doc: --branch validates and expands <branch>

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> From: Junio C Hamano <gitster@pobox.com>
>
> "git check-ref-format --branch $name" feature was originally
> introduced (and was advertised) as a way for scripts to take any
> end-user supplied string (like "master", "@{-1}" etc.) and see if it
> is usable when Git expects to see a branch name, and also obtain the
> concrete branch name that the at-mark magic expands to.
>
> Emphasize that "see if it is usable" role in the description and
> clarify that the @{...} expansion only occurs when run from within a
> repository.
>
> [jn: split out from a larger patch]
>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

Missing sign-off, unlike the other two, intended?

I'll take these three to replace what I tentatively queued, not
necessarily because I like the change in 1/3 better, but because
these are explained much better; besides I prefer a version that at
least two people deeply looked at.

Thanks.

Re: [PATCH 3/3] check-ref-format doc: --branch validates and expands <branch>

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> From: Junio C Hamano <gitster@pobox.com>
>>
>> "git check-ref-format --branch $name" feature was originally
>> introduced (and was advertised) as a way for scripts to take any
>> end-user supplied string (like "master", "@{-1}" etc.) and see if it
>> is usable when Git expects to see a branch name, and also obtain the
>> concrete branch name that the at-mark magic expands to.
>>
>> Emphasize that "see if it is usable" role in the description and
>> clarify that the @{...} expansion only occurs when run from within a
>> repository.
>>
>> [jn: split out from a larger patch]
>>
>> Helped-by: Jeff King <peff@peff.net>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>
> Missing sign-off, unlike the other two, intended?

Oops.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

> I'll take these three to replace what I tentatively queued, not
> necessarily because I like the change in 1/3 better, but because
> these are explained much better; besides I prefer a version that at
> least two people deeply looked at.

Okay, thanks.  I'll add the rest of the change as a patch on top. :)

Thanks,
Jonathan

Re: [PATCH 3/3] check-ref-format doc: --branch validates and expands <branch>

[Jeff King]

On Wed, Oct 18, 2017 at 05:55:18AM +0900, Junio C Hamano wrote:

> I'll take these three to replace what I tentatively queued, not
> necessarily because I like the change in 1/3 better, but because
> these are explained much better; besides I prefer a version that at
> least two people deeply looked at.

These look good to me. Between your two versions, I think the code in
Jonathan's is slightly preferable. It's possible that some other caller
of strbuf_check_branch_name() may run into the same thing and be fixed
(I am trying to think of a hypothetical caller that would be
inconvenienced by the new behavior, but I can't come up with one; and
certainly the existing code would BUG(), so this is an improvement).

-Peff