Re: [PATCH v2] cvsserver: avoid precedence problem between ! and %s
- From
- Matthew Ogilvie <mmogilvi+git@zoho.com>
- Date
- May 23, 2025, 04:47 UTC
- Message-ID
- <aC_90R3ohRRBVIV7@comcast.net>
- In-Reply-To
- <xmqqtt5c5viq.fsf@gitster.g>
On Thu, May 22, 2025 at 11:51:25AM -0700, Junio C Hamano wrote:
Show 31 quoted lines
> 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