Re: "git symbolic-ref" doesn't do a very good job

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Jeff King <peff@peff.net>
Cc: Junio Hamano C <gitster@pobox.com>,
	Git List Mailing <git@vger.kernel.org>
Subject: Re: "git symbolic-ref" doesn't do a very good job
Date: Sat, 30 Jul 2022 17:44:25 -0700	[thread overview]
Message-ID: <CAHk-=wi5pfUcuaAUz=rifon9d51mshE7k6bkpMXddog0On9jow@mail.gmail.com> (raw)
In-Reply-To: <YuXLtIBXYG+JBKdV@coredump.intra.peff.net>

On Sat, Jul 30, 2022 at 5:24 PM Jeff King <peff@peff.net> wrote:
>>
> Hmph, maybe not. The sticking point was topgit, which points HEAD at
> refs/top-bases. There's a fork here:
>
>   https://github.com/mackyle/topgit
>
> which has been active in the last 12 months and which still uses that
> convention. So maybe people really are still using it.
>
> (Again, neither here nor there for your patch).

Well, it *is* relevant for my patch in the sense that I clearly didn't
think of all the crazy things people might have been doing.

That

     git symbolic-ref refs/heads/foo FETCH_HEAD

that you mentioned in the other mail would obviously be entirely
disallowed by my patch, and again, I didn't for a second imagine that
somebody would do something that strange. Junio mentioned a similarly
odd possible situation.

So while I think my patch is the right thing to do, I will also admit
that it's perhaps a "we should always have done this, but we didn't"
situation, and maybe those really odd cases need to be allowed.

Adding ALLOW_ONELEVEL would make those things presumably still work,
and would at least improve things *somewhat* - it would protect people
from syntactically invalid branches (ie bad characters in the branch
name etc).

That would imply still having to fix up that t4202-log.sh testcase,
and I didn't even know or realize about that REFFILES prerequisite,
since obviously in all my use it has been true. I still use and test
only on Linux..

You are also right that without the ALLOW_ONELEVEL, the special-case
check for HEAD should just be removed. That patch started out as the
minimal possible "let's just disallow invalid ref names" patch, so I
didn't touch that odd special case code.

Put another way: I think my patch is likely the right thing to do (and
I'd personally prefer the stricter check without the ALLOW_ONELEVEL
flag), but you and Junio are right about it being a bigger change than
I in my naivete thought it was.

So I won't really push for this, I suspect this needs very much to be
a judgement call by you guys.

Thanks,

                   Linus