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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Jul 31, 2022, 00:44 UTC
Message-ID: <CAHk-=wi5pfUcuaAUz=rifon9d51mshE7k6bkpMXddog0On9jow@mail.gmail.com>
In-Reply-To: <YuXLtIBXYG+JBKdV@coredump.intra.peff.net>

On Sat, Jul 30, 2022 at 5:24 PM Jeff King <peff@peff.net> wrote:

Show 10 quoted lines

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