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

[Todd Zullinger <tmz@pobox.com>]

Jeff King wrote:
> On Thu, May 22, 2025 at 08:55:56AM -0700, Junio C Hamano wrote:
> 
>> Ondrej Pohorelsky <opohorel@redhat.com> 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.
> 
> I agree that the code does not appear to be called, and doing this:
> 
> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index a4e1bad33c..cc891eba67 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -5009,6 +5009,7 @@ sub escapeRefName
>      #   = "_-xx-" Where "xx" is the hexadecimal representation of the
>      #     desired ASCII character byte. (for anything else)
>  
> +    die "foo";
>      if(! $refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/)
>      {
>          $refName=~s/_-/_-u--/g;
> 
> still lets t9402 pass. I suspect the issue is that perl complains to
> stderr while parsing the file (polluting the log), not when actually
> running the code.

Just for curiosity, the only commit found with escapeRefName
is when it was added:

    $ git log -G '\bescapeRefName\b' -- git-cvsserver.perl
    commit 51a7e6dbc9
    Author: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
    Date:   Sat Oct 13 23:42:26 2012 -0600

	cvsserver: define a tag name character escape mechanism
	
	CVS tags are officially only allowed to use [-_0-9A-Za-f].  Git
	refs commonly uses other characters, especially [./].  Such characters
	need to be escaped from CVS in order to be referenced.
	
	This just defines functions to escape/unescape names.  The functions
	are not used yet.
	
	Signed-off-by: Matthew Ogilvie <mmogilvi_git@miniinfo.net>
	Signed-off-by: Junio C Hamano <gitster@pobox.com>

A subsequent commit, 658b57ad52 (cvsserver: add misc commit
lookup, file meta data, and file listing functions,
2012-10-13), made use of unescapeRefName; escapeRefName
seems to have _never_ been used.

-- 
Todd