Skip to content

cvsserver: avoid precedence problem between ! and %s #1925

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

opohorel
Copy link

@opohorel opohorel commented May 21, 2025

cc: "Kristoffer Haugsbakk" [email protected]
cc: "brian m. carlson" [email protected]
cc: Jeff King [email protected]
cc: Todd Zullinger [email protected]
cc: Matthew Ogilvie [email protected]

@opohorel
Copy link
Author

/submit

Copy link

gitgitgadget bot commented May 21, 2025

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1925/opohorel/cvsserver_parentheses-v1

To fetch this version to local tag pr-1925/opohorel/cvsserver_parentheses-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1925/opohorel/cvsserver_parentheses-v1

Copy link

gitgitgadget bot commented May 21, 2025

On the Git mailing list, "Kristoffer Haugsbakk" wrote (reply to this):

Hi

On Wed, May 21, 2025, at 09:45, Ondřej Pohořelský via GitGitGadget wrote:
> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <[email protected]>
>
> With perl-5.41.4 and newer, git-cvsserver fails to build because of
> possible precedence problem[0]
>
> Added parentheses avoid this issue.
>
> Full credit for finding the issue and coming up with the fix goes to
> Jitka Plesnikova ([email protected])

You can mention the person in the trailer section above your signoff.  For example:

    Helped-by: Jitka Plesnikova <[email protected]>
    Signed-off-by: Ondřej Pohořelský <[email protected]>

Or choose one of the other common ones (from `Documentation/SubmittingPatches`):

    If you like, you can put extra trailers at the end:

    . `Reported-by:` is used to credit someone who found the bug that
      the patch attempts to fix.
    . `Acked-by:` says that the person who is more familiar with the area
      the patch attempts to modify liked the patch.
    . `Reviewed-by:`, unlike the other trailers, can only be offered by the
      reviewers themselves when they are completely satisfied with the
      patch after a detailed analysis.
    . `Tested-by:` is used to indicate that the person applied the patch
      and found it to have the desired effect.
    . `Co-authored-by:` is used to indicate that people exchanged drafts
       of a patch before submitting it.
    . `Helped-by:` is used to credit someone who suggested ideas for
      changes without providing the precise changes in patch form.
    . `Mentored-by:` is used to credit someone with helping develop a
      patch as part of a mentorship program (e.g., GSoC or Outreachy).
    . `Suggested-by:` is used to credit someone with suggesting the idea
      for a patch.

-- 
Kristoffer Haugsbakk

Copy link

gitgitgadget bot commented May 21, 2025

User "Kristoffer Haugsbakk" <[email protected]> has been added to the cc: list.

@opohorel opohorel force-pushed the cvsserver_parentheses branch from 42c03c4 to a15f924 Compare May 21, 2025 08:18
@opohorel
Copy link
Author

/submit

Copy link

gitgitgadget bot commented May 21, 2025

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1925/opohorel/cvsserver_parentheses-v2

To fetch this version to local tag pr-1925/opohorel/cvsserver_parentheses-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1925/opohorel/cvsserver_parentheses-v2

Copy link

gitgitgadget bot commented May 21, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Ondřej Pohořelský via GitGitGadget" <[email protected]>
writes:

> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index a4e1bad33ca..076c10cb2c2 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -5009,7 +5009,7 @@ sub escapeRefName
>      #   = "_-xx-" Where "xx" is the hexadecimal representation of the
>      #     desired ASCII character byte. (for anything else)
>  
> -    if(! $refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/)
> +    if(! ($refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/))

Interesting.  Shouldn't it be using !~ instead if it wants to assert
that the refname does not match the pattern?



>      {
>          $refName=~s/_-/_-u--/g;
>          $refName=~s/\./_-p-/g;
>
> base-commit: cb96e1697ad6e54d11fc920c95f82977f8e438f8

Copy link

gitgitgadget bot commented May 21, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Ondřej Pohořelský via GitGitGadget" <[email protected]>
writes:

> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <[email protected]>
>
> With perl-5.41.4 and newer, git-cvsserver fails to build because of
> possible precedence problem[0]
>
> Added parentheses avoid this issue.
>
> [0] https://metacpan.org/release/ETHER/perl-5.41.12/view/pod/perl5414delta.pod#New-Warnings
>
> Reported-by: Jitka Plesnikova <[email protected]>
> Suggested-by: Jitka Plesnikova <[email protected]>
> Signed-off-by: Ondřej Pohořelský <[email protected]>
> ---
>  git-cvsserver.perl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index a4e1bad33ca..076c10cb2c2 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -5009,7 +5009,7 @@ sub escapeRefName
>      #   = "_-xx-" Where "xx" is the hexadecimal representation of the
>      #     desired ASCII character byte. (for anything else)
>  
> -    if(! $refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/)
> +    if(! ($refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/))
>      {
>          $refName=~s/_-/_-u--/g;
>          $refName=~s/\./_-p-/g;

Thanks, will queue.

Copy link

gitgitgadget bot commented May 21, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Ondřej Pohořelský via GitGitGadget" <[email protected]>
writes:

> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <[email protected]>
>
> With perl-5.41.4 and newer, git-cvsserver fails to build because of
> possible precedence problem[0]

What is the exact symptom?  As Perl is not a language to compile and
run separately, "fails to build" does not look like what exactly is
going on.  "gives a warning and then refuses to run"?  "gives a warning
before running"?  Something else?

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

Copy link

gitgitgadget bot commented May 21, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <[email protected]> writes:

> "Ondřej Pohořelský via GitGitGadget" <[email protected]>
> writes:
>
>> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <[email protected]>
>>
>> With perl-5.41.4 and newer, git-cvsserver fails to build because of
>> possible precedence problem[0]
>
> What is the exact symptom?  As Perl is not a language to compile and
> run separately, "fails to build" does not look like what exactly is
> going on.  "gives a warning and then refuses to run"?  "gives a warning
> before running"?  Something else?

Stepping back a bit, the original that the new warning complains
about is this:

-    if(! $refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/)

And the complaint is that due to operator binding precedence, this
does

    (!$refname) =~ /pattern/

Unless the behaviour has changed as well as warning, which is highly
unlikely, doesn't it mean that the code was wrong, with or without
the warning, all along?  The intent of the code was to see if the
refname conformed to dotted decimal, and if it does not, the refname
gets munged in the block guarded by that if (condition).  But the
condition was a total nonsense.  !$refname would most likely to be
an empty string (unless $refname contains '0' or an empty string),
which would not match the /^[1-9][0-9]*(\.[1-9][0-9]*)*$/ pattern,
so we probably have always munged the $refName in escapeRefName sub.

Which I do not see used anywhere in the program, though.  There is a
call to unescapeRefname method, but it seems to me that
escapeRefName is never called.

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?

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

Copy link

gitgitgadget bot commented May 21, 2025

This patch series was integrated into seen via git@8c262cd.

@gitgitgadget gitgitgadget bot added the seen label May 21, 2025
Copy link

gitgitgadget bot commented May 21, 2025

On the Git mailing list, "brian m. carlson" wrote (reply to this):

On 2025-05-21 at 14:58:07, Junio C Hamano wrote:
> "Ondřej Pohořelský via GitGitGadget" <[email protected]>
> writes:
> 
> > diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> > index a4e1bad33ca..076c10cb2c2 100755
> > --- a/git-cvsserver.perl
> > +++ b/git-cvsserver.perl
> > @@ -5009,7 +5009,7 @@ sub escapeRefName
> >      #   = "_-xx-" Where "xx" is the hexadecimal representation of the
> >      #     desired ASCII character byte. (for anything else)
> >  
> > -    if(! $refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/)
> > +    if(! ($refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/))
> 
> Interesting.  Shouldn't it be using !~ instead if it wants to assert
> that the refname does not match the pattern?

Yes, it should.  It's likely the reason this is getting a warning is
that `!` is higher precedence than `=~` and `!~` (see `man perlop`) and
switching to `!~` is the customary way of writing this.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

Copy link

gitgitgadget bot commented May 21, 2025

User "brian m. carlson" <[email protected]> has been added to the cc: list.

Copy link

gitgitgadget bot commented May 22, 2025

On the Git mailing list, Ondrej Pohorelsky wrote (reply to this):

On Wed, May 21, 2025 at 7:03 PM Junio C Hamano <[email protected]> wrote:
>
> Junio C Hamano <[email protected]> writes:
>
> > "Ondřej Pohořelský via GitGitGadget" <[email protected]>
> > writes:
> >
> >> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <[email protected]>
> >>
> >> With perl-5.41.4 and newer, git-cvsserver fails to build because of
> >> possible precedence problem[0]
> >
> > What is the exact symptom?  As Perl is not a language to compile and
> > run separately, "fails to build" does not look like what exactly is
> > going on.  "gives a warning and then refuses to run"?  "gives a warning
> > before running"?  Something else?
>
> Stepping back a bit, the original that the new warning complains
> about is this:
>
> -    if(! $refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/)
>
> And the complaint is that due to operator binding precedence, this
> does
>
>     (!$refname) =~ /pattern/
>
> Unless the behaviour has changed as well as warning, which is highly
> unlikely, doesn't it mean that the code was wrong, with or without
> the warning, all along?  The intent of the code was to see if the
> refname conformed to dotted decimal, and if it does not, the refname
> gets munged in the block guarded by that if (condition).  But the
> condition was a total nonsense.  !$refname would most likely to be
> an empty string (unless $refname contains '0' or an empty string),
> which would not match the /^[1-9][0-9]*(\.[1-9][0-9]*)*$/ pattern,
> so we probably have always munged the $refName in escapeRefName sub.
>
> Which I do not see used anywhere in the program, though.  There is a
> call to unescapeRefname method, but it seems to me that
> escapeRefName is never called.
>
> 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?
>


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.


On Wed, May 21, 2025 at 7:03 PM Junio C Hamano <[email protected]> wrote:
>
> Junio C Hamano <[email protected]> writes:
>
> > "Ondřej Pohořelský via GitGitGadget" <[email protected]>
> > writes:
> >
> >> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <[email protected]>
> >>
> >> With perl-5.41.4 and newer, git-cvsserver fails to build because of
> >> possible precedence problem[0]
> >
> > What is the exact symptom?  As Perl is not a language to compile and
> > run separately, "fails to build" does not look like what exactly is
> > going on.  "gives a warning and then refuses to run"?  "gives a warning
> > before running"?  Something else?
>
> Stepping back a bit, the original that the new warning complains
> about is this:
>
> -    if(! $refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/)
>
> And the complaint is that due to operator binding precedence, this
> does
>
>     (!$refname) =~ /pattern/
>
> Unless the behaviour has changed as well as warning, which is highly
> unlikely, doesn't it mean that the code was wrong, with or without
> the warning, all along?  The intent of the code was to see if the
> refname conformed to dotted decimal, and if it does not, the refname
> gets munged in the block guarded by that if (condition).  But the
> condition was a total nonsense.  !$refname would most likely to be
> an empty string (unless $refname contains '0' or an empty string),
> which would not match the /^[1-9][0-9]*(\.[1-9][0-9]*)*$/ pattern,
> so we probably have always munged the $refName in escapeRefName sub.
>
> Which I do not see used anywhere in the program, though.  There is a
> call to unescapeRefname method, but it seems to me that
> escapeRefName is never called.
>
> 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?
>
> >> 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.
>


-- 

Ondřej Pohořelský

Software Engineer

Red Hat

[email protected]

Copy link

gitgitgadget bot commented May 22, 2025

On the Git mailing list, Ondrej Pohorelsky wrote (reply to this):

Looking at the code, we were not exactly sure how the code should
work, so we picked the solution with the least impact that suppresses
the warning and doesn't break anything.
I'll change the commit to use `!~` instead.


On Wed, May 21, 2025 at 11:49 PM brian m. carlson
<[email protected]> wrote:
>
> On 2025-05-21 at 14:58:07, Junio C Hamano wrote:
> > "Ondřej Pohořelský via GitGitGadget" <[email protected]>
> > writes:
> >
> > > diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> > > index a4e1bad33ca..076c10cb2c2 100755
> > > --- a/git-cvsserver.perl
> > > +++ b/git-cvsserver.perl
> > > @@ -5009,7 +5009,7 @@ sub escapeRefName
> > >      #   = "_-xx-" Where "xx" is the hexadecimal representation of the
> > >      #     desired ASCII character byte. (for anything else)
> > >
> > > -    if(! $refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/)
> > > +    if(! ($refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/))
> >
> > Interesting.  Shouldn't it be using !~ instead if it wants to assert
> > that the refname does not match the pattern?
>
> Yes, it should.  It's likely the reason this is getting a warning is
> that `!` is higher precedence than `=~` and `!~` (see `man perlop`) and
> switching to `!~` is the customary way of writing this.
> --
> brian m. carlson (they/them)
> Toronto, Ontario, CA



-- 

Ondřej Pohořelský

Software Engineer

Red Hat

[email protected]

@opohorel opohorel force-pushed the cvsserver_parentheses branch from a15f924 to b756318 Compare May 22, 2025 10:41
@opohorel
Copy link
Author

/submit

Copy link

gitgitgadget bot commented May 22, 2025

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1925/opohorel/cvsserver_parentheses-v3

To fetch this version to local tag pr-1925/opohorel/cvsserver_parentheses-v3:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1925/opohorel/cvsserver_parentheses-v3

Copy link

gitgitgadget bot commented May 22, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

Ondrej Pohorelsky <[email protected]> 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.

Copy link

gitgitgadget bot commented May 22, 2025

On the Git mailing list, Jeff King wrote (reply to this):

On Thu, May 22, 2025 at 08:55:56AM -0700, Junio C Hamano wrote:

> Ondrej Pohorelsky <[email protected]> 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.

-Peff

Copy link

gitgitgadget bot commented May 22, 2025

User Jeff King <[email protected]> has been added to the cc: list.

Copy link

gitgitgadget bot commented May 22, 2025

On the Git mailing list, Todd Zullinger wrote (reply to this):

Jeff King wrote:
> On Thu, May 22, 2025 at 08:55:56AM -0700, Junio C Hamano wrote:
> 
>> Ondrej Pohorelsky <[email protected]> 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 <[email protected]>
    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 <[email protected]>
	Signed-off-by: Junio C Hamano <[email protected]>

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

Copy link

gitgitgadget bot commented May 22, 2025

User Todd Zullinger <[email protected]> has been added to the cc: list.

Copy link

gitgitgadget bot commented May 22, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

Todd Zullinger <[email protected]> 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 <[email protected]>
>     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 <[email protected]>
> 	Signed-off-by: Junio C Hamano <[email protected]>
>
> 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.

Copy link

gitgitgadget bot commented May 22, 2025

This patch series was integrated into seen via git@fce3750.

Copy link

gitgitgadget bot commented May 23, 2025

On the Git mailing list, Matthew Ogilvie wrote (reply to this):

On Thu, May 22, 2025 at 11:51:25AM -0700, Junio C Hamano wrote:
> Todd Zullinger <[email protected]> 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 <[email protected]>
> >     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 <[email protected]>
> > 	Signed-off-by: Junio C Hamano <[email protected]>
> >
> > 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

Copy link

gitgitgadget bot commented May 23, 2025

User Matthew Ogilvie <[email protected]> has been added to the cc: list.

Copy link

gitgitgadget bot commented May 23, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

Matthew Ogilvie <[email protected]> writes:

> 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.)

;-)

Copy link

gitgitgadget bot commented May 24, 2025

This branch is now known as op/cvsserver-perl-warning.

Copy link

gitgitgadget bot commented May 24, 2025

This patch series was integrated into seen via git@66bfb4a.

Copy link

gitgitgadget bot commented May 24, 2025

There was a status update in the "New Topics" section about the branch op/cvsserver-perl-warning on the Git mailing list:

Recent versions of Perl started warning against "! A =~ /pattern/"
which does not negate the result of the matching.
source: <[email protected]>

Function 'escapeRefName' introduced in 51a7e6d has never been used.

Despite being dead code, changes in Perl 5.41.4 exposed precedence
warning withing its logic, which then caused test failures in t9402 by
logging the warnings to stderr while parsing the code. The affected
tests are t9402.30, t9402.31, t9402.32 and t9402.34.

Remove this unused function to simplify the codebase and stop the
warnings and test failures. Its corresponding unescapeRefName function,
which remains in use, has had its comments updated.

Reported-by: Jitka Plesnikova <[email protected]>
Signed-off-by: Ondřej Pohořelský <[email protected]>
@opohorel opohorel force-pushed the cvsserver_parentheses branch from b756318 to ce85359 Compare May 26, 2025 13:29
@opohorel
Copy link
Author

/submit

Copy link

gitgitgadget bot commented May 26, 2025

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1925/opohorel/cvsserver_parentheses-v4

To fetch this version to local tag pr-1925/opohorel/cvsserver_parentheses-v4:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1925/opohorel/cvsserver_parentheses-v4

Copy link

gitgitgadget bot commented May 26, 2025

On the Git mailing list, Ondrej Pohorelsky wrote (reply to this):

I've just submitted v4, which removes the 'escapeRefName' function, so
we avoid the warnings and test failures when we build with new Perl
releases.
I think the next step would be to remove whole git-cvsserver as was
said earlier. I'll take a look what it is going to take and submit a
patch with the removal later, if that's ok


On Fri, May 23, 2025 at 5:48 PM Junio C Hamano <[email protected]> wrote:
>
> Matthew Ogilvie <[email protected]> writes:
>
> > 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.)
>
> ;-)
>


-- 

Ondřej Pohořelský

Software Engineer

Red Hat

[email protected]

Copy link

gitgitgadget bot commented May 27, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Ondřej Pohořelský via GitGitGadget" <[email protected]>
writes:

> Function 'escapeRefName' introduced in 51a7e6dbc9 has never been used.
>
> Despite being dead code, changes in Perl 5.41.4 exposed precedence
> warning withing its logic, which then caused test failures in t9402 by
> logging the warnings to stderr while parsing the code. The affected
> tests are t9402.30, t9402.31, t9402.32 and t9402.34.
>
> Remove this unused function to simplify the codebase and stop the
> warnings and test failures. Its corresponding unescapeRefName function,
> which remains in use, has had its comments updated.


Very clearly explained and looking good.  Thanks.

Will queue with "withing" -> "within".

Copy link

gitgitgadget bot commented May 27, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

Ondrej Pohorelsky <[email protected]> writes:

> I've just submitted v4, which removes the 'escapeRefName' function, so
> we avoid the warnings and test failures when we build with new Perl
> releases.

Great, thanks.

> I think the next step would be to remove whole git-cvsserver as was
> said earlier. I'll take a look what it is going to take and submit a
> patch with the removal later, if that's ok

It probably needs to follow the pattern established for all the
other recent topics that touched Documentation/BreakingChanges file.

Copy link

gitgitgadget bot commented May 27, 2025

This patch series was integrated into seen via git@1a4ec1d.

Copy link

gitgitgadget bot commented May 28, 2025

There was a status update in the "Cooking" section about the branch op/cvsserver-perl-warning on the Git mailing list:

Recent versions of Perl started warning against "! A =~ /pattern/"
which does not negate the result of the matching.  As it turns out
that the problematic function is not even called, it was removed.

Will merge to 'nexr'.
source: <[email protected]>

Copy link

gitgitgadget bot commented May 28, 2025

This patch series was integrated into seen via git@040a9be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant