cvsserver: avoid precedence problem between ! and %s

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

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

git-cvsserver.perl +0 -0

index a4e1bad33ca..076c10cb2c2 100755

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.

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

Quoted 2 lines · writes:

From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <[email protected]>

Quoted 2 lines

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?

Quoted 1 line

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.

Quoted 5 lines · Junio C Hamano

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

Quoted 2 lines · writes:

From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <[email protected]>

Quoted 7 lines

Quoted 2 lines

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?

Quoted 6 lines

Quoted 1 line

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.

Quoted 5 lines · Junio C Hamano

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

Quoted 2 lines · writes:

From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <[email protected]>

Quoted 7 lines

Quoted 2 lines

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?

Quoted 6 lines

Quoted 1 line

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.

Quoted 11 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

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.

Quoted 11 lines · Ondrej Pohorelsky

Quoted 11 lines

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.

Quoted 14 lines · Junio C Hamano

Quoted 11 lines · Ondrej Pohorelsky

Quoted 11 lines

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?

Quoted 6 lines

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:

git-cvsserver.perl +1 -0

index a4e1bad33c..cc891eba67 100755
@@ -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.

Quoted 17 lines · Todd Zullinger

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.

Quoted 7 lines

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.

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

Quoted 8 lines · Matthew Ogilvie

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

;-)

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

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.

git-cvsserver.perl +0 -0

     {
         $refName=~s/_-/_-u--/g;
         $refName=~s/\./_-p-/g;

base-commit: cb96e1697ad6e54d11fc920c95f82977f8e438f8

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

Quoted 8 lines · writes:

git-cvsserver.perl +0 -0

index a4e1bad33ca..076c10cb2c2 100755
@@ -5009,7 +5009,7 @@ sub escapeRefName
     #   = "_-xx-" Where "xx" is the hexadecimal representation of the
     #     desired ASCII character byte. (for anything else)
 

git-cvsserver.perl sub escapeRefName · line 5012

Quoted 2 lines

git-cvsserver.perl +1 -1

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

Quoted 11 lines · Junio C Hamano

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

Quoted 8 lines · writes:

git-cvsserver.perl +0 -0

index a4e1bad33ca..076c10cb2c2 100755
@@ -5009,7 +5009,7 @@ sub escapeRefName
     #   = "_-xx-" Where "xx" is the hexadecimal representation of the
     #     desired ASCII character byte. (for anything else)
 

Quoted 5 lines

git-cvsserver.perl sub escapeRefName · line 5012

Quoted 2 lines

git-cvsserver.perl +1 -1

-    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