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

[Matthew Ogilvie <mmogilvi+git@zoho.com>]

On Thu, May 22, 2025 at 11:51:25AM -0700, Junio C Hamano wrote:
> Todd Zullinger <tmz@pobox.com> writes:
> 
> > 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.
> 
> OK, so we can safely remove it, it seems ;-)  I wonder what, if any,
> the unescaping side is unescaping, if we are not doing the escaping.
> 
> Thanks for digging.

FYI:

One intent is that the user might do the escaping manually, if
they need to refer to a git refspec that is not legal in CVS.
For example, "cvs update -r pu_-s-mo_-s-experiment1" instead of
"cvs update -r pu/mo/experiment1".  To some extent the function
could be considered a form of documentation of how you would do
this manually.

Also, the fact escapeRefName() isn't called suggests that there
might be other bugs.  There is a test case in t9402 that
tests arguments "-r heads/b1" with a comment that "This is not
really legal CVS, but it seems to work anyway".  I haven't fully
tracked it down, but I suspect that might end up putting a
literal "heads/b1" in the CVS sandbox's "CVS/Entries" file.  If so,
that is invalid, because Entries uses slash for its own field
separator.  If we added more tests immediately after it
*without* a different "-r" (which is very high priority
when resolving which version to update to), they would likely fail.
It might make sense to put an escapeRefName(unescapeRefName()) nested
call somewhere to protect against things like this test case...

However, despite writing and (incompletely) testing this code, I
have never *really* used it, and probably never will.  So I'm not
in a hurry to try to test or fix it further...

(For that matter, has anyone ever heard of anyone actually using
git-cvsserver at all?  I think I would be surprised if there was anyone
using it, especially so many years after CVS stopped being maintained
at all.)

        - Matthew Ogilvie