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

[Jonathan Nieder <jrnieder@gmail.com>]

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