Ondrej Pohorelsky <opohorel@xxxxxxxxxx> writes: >> 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. > 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.
