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

Jeff King [off-list ref] writes:

On Mon, Jul 17, 2017 at 10:27:09AM -0700, Jonathan Nieder wrote:

quoted

quoted

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

On Thu, Aug 17, 2017 at 02:30:53PM -0700, Junio C Hamano wrote:

quoted

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