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

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

[Ondřej Pohořelský via GitGitGadget]

From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <opohorel@redhat.com>

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 (jplesnik@redhat.com)

[0] https://metacpan.org/release/ETHER/perl-5.41.12/view/pod/perl5414delta.pod#New-Warnings

Signed-off-by: Ondřej Pohořelský <opohorel@redhat.com>
---
    cvsserver: avoid precedence problem between ! and %s

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1925%2Fopohorel%2Fcvsserver_parentheses-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1925/opohorel/cvsserver_parentheses-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1925

 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;

base-commit: cb96e1697ad6e54d11fc920c95f82977f8e438f8
-- 
gitgitgadget

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

[Kristoffer Haugsbakk]

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?= <opohorel@redhat.com>
>
> 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 (jplesnik@redhat.com)

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

    Helped-by: Jitka Plesnikova <jplesnik@redhat.com>
    Signed-off-by: Ondřej Pohořelský <opohorel@redhat.com>

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

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

[Ondřej Pohořelský via GitGitGadget]

From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <opohorel@redhat.com>

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 <jplesnik@redhat.com>
Suggested-by: Jitka Plesnikova <jplesnik@redhat.com>
Signed-off-by: Ondřej Pohořelský <opohorel@redhat.com>
---
    cvsserver: avoid precedence problem between ! and %s
    
    cc: "Kristoffer Haugsbakk" kristofferhaugsbakk@fastmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1925%2Fopohorel%2Fcvsserver_parentheses-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1925/opohorel/cvsserver_parentheses-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1925

Range-diff vs v1:

 1:  42c03c4b044 ! 1:  a15f924657c cvsserver: avoid precedence problem between ! and %s
     @@ Commit message
      
          Added parentheses avoid this issue.
      
     -    Full credit for finding the issue and coming up with the fix goes to
     -    Jitka Plesnikova (jplesnik@redhat.com)
     -
          [0] https://metacpan.org/release/ETHER/perl-5.41.12/view/pod/perl5414delta.pod#New-Warnings
      
     +    Reported-by: Jitka Plesnikova <jplesnik@redhat.com>
     +    Suggested-by: Jitka Plesnikova <jplesnik@redhat.com>
          Signed-off-by: Ondřej Pohořelský <opohorel@redhat.com>
      
       ## git-cvsserver.perl ##


 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;

base-commit: cb96e1697ad6e54d11fc920c95f82977f8e438f8
-- 
gitgitgadget

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

[Junio C Hamano]

"Ondřej Pohořelský via GitGitGadget" <gitgitgadget@gmail.com>
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

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

[Junio C Hamano]

"Ondřej Pohořelský via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <opohorel@redhat.com>
>
> 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 <jplesnik@redhat.com>
> Suggested-by: Jitka Plesnikova <jplesnik@redhat.com>
> Signed-off-by: Ondřej Pohořelský <opohorel@redhat.com>
> ---
>  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.

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

[Junio C Hamano]

"Ondřej Pohořelský via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <opohorel@redhat.com>
>
> 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.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Ondřej Pohořelský via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <opohorel@redhat.com>
>>
>> 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.

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 989 bytes --]

On 2025-05-21 at 14:58:07, Junio C Hamano wrote:
> "Ondřej Pohořelský via GitGitGadget" <gitgitgadget@gmail.com>
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]

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

[Ondrej Pohorelsky]

On Wed, May 21, 2025 at 7:03 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Ondřej Pohořelský via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> >
> >> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <opohorel@redhat.com>
> >>
> >> 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 <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Ondřej Pohořelský via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> >
> >> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <opohorel@redhat.com>
> >>
> >> 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

opohorel@redhat.com

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

[Ondrej Pohorelsky]

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
<sandals@crustytoothpaste.net> wrote:
>
> On 2025-05-21 at 14:58:07, Junio C Hamano wrote:
> > "Ondřej Pohořelský via GitGitGadget" <gitgitgadget@gmail.com>
> > 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

opohorel@redhat.com

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

[Ondřej Pohořelský via GitGitGadget]

From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <opohorel@redhat.com>

With perl-5.41.4 and newer, test t9402-git-cvsserver-refs.sh
(specifically t9402.30, t9402.31, t9402.32, t9402.34) fails, because
of the new warnings[0] populating cvs.log.

Use the 'does not match' operator '!~' directly to express the
negated pattern match, resolving the precedence issue.

[0] https://metacpan.org/release/ETHER/perl-5.41.12/view/pod/perl5414delta.pod#New-Warnings

Reported-by: Jitka Plesnikova <jplesnik@redhat.com>
Suggested-by: Jitka Plesnikova <jplesnik@redhat.com>
Signed-off-by: Ondřej Pohořelský <opohorel@redhat.com>
---
    cvsserver: avoid precedence problem between ! and %s
    
    cc: "Kristoffer Haugsbakk" kristofferhaugsbakk@fastmail.com cc: "brian
    m. carlson" sandals@crustytoothpaste.net

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1925%2Fopohorel%2Fcvsserver_parentheses-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1925/opohorel/cvsserver_parentheses-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1925

Range-diff vs v2:

 1:  a15f924657c ! 1:  b7563182492 cvsserver: avoid precedence problem between ! and %s
     @@ Metadata
       ## Commit message ##
          cvsserver: avoid precedence problem between ! and %s
      
     -    With perl-5.41.4 and newer, git-cvsserver fails to build because of
     -    possible precedence problem[0]
     +    With perl-5.41.4 and newer, test t9402-git-cvsserver-refs.sh
     +    (specifically t9402.30, t9402.31, t9402.32, t9402.34) fails, because
     +    of the new warnings[0] populating cvs.log.
      
     -    Added parentheses avoid this issue.
     +    Use the 'does not match' operator '!~' directly to express the
     +    negated pattern match, resolving the precedence issue.
      
          [0] https://metacpan.org/release/ETHER/perl-5.41.12/view/pod/perl5414delta.pod#New-Warnings
      
     @@ git-cvsserver.perl: sub escapeRefName
           #     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]*)*$/))
     ++    if ($refName !~ /^[1-9][0-9]*(\.[1-9][0-9]*)*$/)
           {
               $refName=~s/_-/_-u--/g;
               $refName=~s/\./_-p-/g;


 git-cvsserver.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index a4e1bad33ca..7ccd720019b 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;

base-commit: cb96e1697ad6e54d11fc920c95f82977f8e438f8
-- 
gitgitgadget

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

[Junio C Hamano]

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.

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

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

[Jeff King]

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.

-Peff

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

[Todd Zullinger]

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

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

[Junio C Hamano]

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.

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

[Matthew Ogilvie]

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

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

[Junio C Hamano]

Matthew Ogilvie <mmogilvi+git@zoho.com> 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.)

;-)

[PATCH v4] cvsserver: remove unused escapeRefName function

[Ondřej Pohořelský via GitGitGadget]

From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <opohorel@redhat.com>

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.

Reported-by: Jitka Plesnikova <jplesnik@redhat.com>
Signed-off-by: Ondřej Pohořelský <opohorel@redhat.com>
---
    cvsserver: avoid precedence problem between ! and %s
    
    cc: "Kristoffer Haugsbakk" kristofferhaugsbakk@fastmail.com cc: "brian
    m. carlson" sandals@crustytoothpaste.net cc: Jeff King peff@peff.net cc:
    Todd Zullinger tmz@pobox.com cc: Matthew Ogilvie mmogilvi+git@zoho.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1925%2Fopohorel%2Fcvsserver_parentheses-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1925/opohorel/cvsserver_parentheses-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1925

Range-diff vs v3:

 1:  b7563182492 ! 1:  ce853594ceb cvsserver: avoid precedence problem between ! and %s
     @@ Metadata
      Author: Ondřej Pohořelský <opohorel@redhat.com>
      
       ## Commit message ##
     -    cvsserver: avoid precedence problem between ! and %s
     +    cvsserver: remove unused escapeRefName function
      
     -    With perl-5.41.4 and newer, test t9402-git-cvsserver-refs.sh
     -    (specifically t9402.30, t9402.31, t9402.32, t9402.34) fails, because
     -    of the new warnings[0] populating cvs.log.
     +    Function 'escapeRefName' introduced in 51a7e6dbc9 has never been used.
      
     -    Use the 'does not match' operator '!~' directly to express the
     -    negated pattern match, resolving the precedence issue.
     +    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.
      
     -    [0] https://metacpan.org/release/ETHER/perl-5.41.12/view/pod/perl5414delta.pod#New-Warnings
     +    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 <jplesnik@redhat.com>
     -    Suggested-by: Jitka Plesnikova <jplesnik@redhat.com>
          Signed-off-by: Ondřej Pohořelský <opohorel@redhat.com>
      
       ## git-cvsserver.perl ##
     +@@ git-cvsserver.perl: sub gethistorydense
     +     return $result;
     + }
     + 
     +-=head2 escapeRefName
     ++=head2 unescapeRefName
     + 
     +-Apply an escape mechanism to compensate for characters that
     ++Undo an escape mechanism to compensate for characters that
     + git ref names can have that CVS tags can not.
     + 
     + =cut
     +-sub escapeRefName
     ++sub unescapeRefName
     + {
     +     my($self,$refName)=@_;
     + 
      @@ git-cvsserver.perl: 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;
     +-    {
     +-        $refName=~s/_-/_-u--/g;
     +-        $refName=~s/\./_-p-/g;
     +-        $refName=~s%/%_-s-%g;
     +-        $refName=~s/[^-_a-zA-Z0-9]/sprintf("_-%02x-",$1)/eg;
     +-    }
     +-}
     +-
     +-=head2 unescapeRefName
     +-
     +-Undo an escape mechanism to compensate for characters that
     +-git ref names can have that CVS tags can not.
     +-
     +-=cut
     +-sub unescapeRefName
     +-{
     +-    my($self,$refName)=@_;
     +-
     +-    # see escapeRefName() for description of escape mechanism.
     +-
     +     $refName=~s/_-([spu]|[0-9a-f][0-9a-f])-/unescapeRefNameChar($1)/eg;
     + 
     +     # allowed tag names


 git-cvsserver.perl | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index a4e1bad33ca..d8d5422cbca 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -4986,13 +4986,13 @@ sub gethistorydense
     return $result;
 }
 
-=head2 escapeRefName
+=head2 unescapeRefName
 
-Apply an escape mechanism to compensate for characters that
+Undo an escape mechanism to compensate for characters that
 git ref names can have that CVS tags can not.
 
 =cut
-sub escapeRefName
+sub unescapeRefName
 {
     my($self,$refName)=@_;
 
@@ -5009,27 +5009,6 @@ 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]*)*$/)
-    {
-        $refName=~s/_-/_-u--/g;
-        $refName=~s/\./_-p-/g;
-        $refName=~s%/%_-s-%g;
-        $refName=~s/[^-_a-zA-Z0-9]/sprintf("_-%02x-",$1)/eg;
-    }
-}
-
-=head2 unescapeRefName
-
-Undo an escape mechanism to compensate for characters that
-git ref names can have that CVS tags can not.
-
-=cut
-sub unescapeRefName
-{
-    my($self,$refName)=@_;
-
-    # see escapeRefName() for description of escape mechanism.
-
     $refName=~s/_-([spu]|[0-9a-f][0-9a-f])-/unescapeRefNameChar($1)/eg;
 
     # allowed tag names

base-commit: cb96e1697ad6e54d11fc920c95f82977f8e438f8
-- 
gitgitgadget

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

[Ondrej Pohorelsky]

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 <gitster@pobox.com> wrote:
>
> Matthew Ogilvie <mmogilvi+git@zoho.com> 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

opohorel@redhat.com

Re: [PATCH v4] cvsserver: remove unused escapeRefName function

[Junio C Hamano]

"Ondřej Pohořelský via GitGitGadget" <gitgitgadget@gmail.com>
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".

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

[Junio C Hamano]

Ondrej Pohorelsky <opohorel@redhat.com> 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.