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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 17, 2017, 07:05 UTC
Message-ID: <xmqqinfentiw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171017044111.ahe4eiepvokecnkr@aiede.mtv.corp.google.com>

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?

Show 8 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?

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