Re: [PATCH v2] cvsserver: avoid precedence problem between ! and %s

From: Junio C Hamano <gitster@pobox.com>
Date: May 22, 2025, 15:55 UTC
Message-ID: <xmqq7c287i7n.fsf@gitster.g>
In-Reply-To: <CA+B51BGLK-3R9ev4a8EwkGHQEBi2QhgxvAd0CHMbphrxPM74eg@mail.gmail.com>

Ondrej Pohorelsky <opohorel@redhat.com> writes:

Show 11 quoted lines

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

Below you mention you found it from test failures. Nice to know that you weren't actually using it ;-)

Still, I would welcome second and third set of eyeballs to see if this is a dead code that the "compiler" is complaining about. If so, we can remove that unused code instead of fixing it.

Show 16 quoted lines

> What I meant by 'does not build' is that the warnings that were added
> in the newest Perl release populate the cvs.log when running the test
> suite.
> This causes some tests from t9402-git-cvsserver-refs.sh to fail, which
> then fails the whole build in Fedora.
> Tests that are affected are t9402.30, t9402.31, t9402.32, t9402.34.
>
>
>> >> 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.
>
> I'll rephrase the commit message to meet this requirement.

Also please update the earlier part of the log message to clarify what the end-user observable symptoms are (e.g. "gives warnings before doing its thing? or errors out and does not run? or something else?").

Thanks.