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

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