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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 17, 2017, 04:41 UTC
Message-ID: <20171017044111.ahe4eiepvokecnkr@aiede.mtv.corp.google.com>
In-Reply-To: <xmqqinffsibr.fsf@gitster.mtv.corp.google.com>

Hi,

Junio C Hamano wrote:

Show 13 quoted lines

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

Show 5 quoted lines

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

Show 6 quoted lines

> 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