Re: [PATCH v2] cvsserver: avoid precedence problem between ! and %s
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 21, 2025, 17:03 UTC
- Message-ID
- <xmqq1pshc2vs.fsf@gitster.g>
- In-Reply-To
- <xmqqh61ear4s.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> "Ondřej Pohořelský via GitGitGadget" <gitgitgadget@gmail.com> > writes: > >> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <opohorel@redhat.com> >> >> With perl-5.41.4 and newer, git-cvsserver fails to build because of >> possible precedence problem[0] > > What is the exact symptom? As Perl is not a language to compile and > run separately, "fails to build" does not look like what exactly is > going on. "gives a warning and then refuses to run"? "gives a warning > before running"? Something else?
Stepping back a bit, the original that the new warning complains about is this:
- if(! $refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/)
And the complaint is that due to operator binding precedence, this does
(!$refname) =~ /pattern/
Unless the behaviour has changed as well as warning, which is highly unlikely, doesn't it mean that the code was wrong, with or without the warning, all along? The intent of the code was to see if the refname conformed to dotted decimal, and if it does not, the refname gets munged in the block guarded by that if (condition). But the condition was a total nonsense. !$refname would most likely to be an empty string (unless $refname contains '0' or an empty string), which would not match the /^[1-9][0-9]*(\.[1-9][0-9]*)*$/ pattern, so we probably have always munged the $refName in escapeRefName sub.
Which I do not see used anywhere in the program, though. There is a call to unescapeRefname method, but it seems to me that escapeRefName is never called.
What made you send a patch for this program? Do you or anybody you know use git-cvsserver? Unless I am reading the program incorrectly, despite the claim in front of that escapeRefName sub that we avoid sending a tag whose name is not something CVS would be happy with, we did not sanitize the refs and relied solely on the users' repository to use only safe characters in the refs to keep CVS clients happy, and the fact that this expression used as if() condition is totally broken does not really make any difference, since it is in an unused sub. I have to wonder if (1) it is a better fix to just remove the unused sub, and/or (2) perhaps nobody uses cvsserver to allow cvs clients to talk to a Git repository?
Show 6 quoted lines
>> Added parentheses avoid this issue. > > We phrase such "this is how the patch addresses the issue" statement > in imperative, as if we are telling the codebase to become-like-so, > e.g., "Enclose the pattern matching =~ in parentheses to force the > right order of binding", or something like that.