Junio C Hamano <gitster@xxxxxxxxx> writes:
> "Ondřej Pohořelský via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> writes:
>
>> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <opohorel@xxxxxxxxxx>
>>
>> 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?
>> 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.
