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

[Junio C Hamano]

Jonathan Nieder  writes:

> Junio C Hamano wrote:
>> Jonathan Nieder  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] check-ref-format: require a repository for --branch

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder  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] check-ref-format: require a repository for --branch

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder  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.plddffhzdobye...@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 0/3] Re: [PATCH] check-ref-format: require a repository for --branch

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder  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 0/3] Re: [PATCH] check-ref-format: require a repository for --branch

[Junio C Hamano]

Jonathan Nieder  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 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();
> - if (strbuf_check_branch_ref(, arg))
> +
> + if (!nongit)
> + malformed = (strbuf_check_branch_ref(, arg) ||
> +  !skip_prefix(sb.buf, "refs/heads/", ));
> + 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 

 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(-)

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

[Junio C Hamano]

Jonathan Nieder  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 ;-).

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

[Junio C Hamano]

Junio C Hamano  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 
Signed-off-by: Junio C Hamano 
---
 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();
-   if (strbuf_check_branch_ref(, arg))
+
+   if (!nongit)
+   malformed = (strbuf_check_branch_ref(, arg) ||
+!skip_prefix(sb.buf, "refs/heads/", ));
+   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();
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 

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

[Kevin Daudt]

On Tue, Oct 17, 2017 at 11:40:17AM +0900, Junio C Hamano wrote:
> Kevin Daudt  writes:
> 
> >> +  setup_git_directory_gently();
> >> +
> >> +  if (!nongit)
> >> +  malformed = (strbuf_check_branch_ref(, arg) ||
> >> +   !skip_prefix(sb.buf, "refs/heads/", ));
> >> +  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();
> >>return 0;
> >>  }
> >>  

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

[Junio C Hamano]

Jeff King  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

[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]

Kevin Daudt  writes:

>> +setup_git_directory_gently();
>> +
>> +if (!nongit)
>> +malformed = (strbuf_check_branch_ref(, arg) ||
>> + !skip_prefix(sb.buf, "refs/heads/", ));
>> +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();
>>  return 0;
>>  }
>>  

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  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(, arg))
> + setup_git_directory_gently();
> +
> + if (!nongit)
> + malformed = (strbuf_check_branch_ref(, arg) ||
> +  !skip_prefix(sb.buf, "refs/heads/", ));
> + 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();
>   return 0;
>  }
>  

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

[Junio C Hamano]

Jeff King  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

[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

[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

[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

[Junio C Hamano]

Junio C Hamano  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(, arg))
+   setup_git_directory_gently();
+
+   if (!nongit)
+   malformed = (strbuf_check_branch_ref(, arg) ||
+!skip_prefix(sb.buf, "refs/heads/", ));
+   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();
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(, name);
+   result = strbuf_check_branch_ref_format();
+   strbuf_release();
+   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 

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

[Junio C Hamano]

Jeff King  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 
>> 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]

Jeff King  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

[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  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 
>> 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 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

[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 
> 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

[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  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 
> 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.kun...@gmail.com
Phone: +31 (0) 6 2546 0117

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 
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