From: Junio C Hamano <hidden> Date: 2016-06-15 22:50:40
Guy Rouillier [off-list ref] writes:
As I suspected after reading how the cvspass file is read and written,
CVSNT doesn't work with repositories with an equal sign in the
repository name. You can init it fine, and you can set up a password
for it. But if you try to login things go very wrong:
...
Since CVSNT can't handle a repository with an equal sign in its name,
I say we don't worry about this. I say the same about the original
CVS with a repository name with embedded spaces. We certainly don't
want to try to solve problems the original product doesn't solve.
Thanks; your observation matches my earlier suspicion. So in short:
- CVSNT does not work with repository path with an '=' in it, but does work
with ones with a SP in it; and
- CVS has trouble with repository path with a SP in it, but works with
ones with an '=' in it just fine.
Have I summarized it correctly?
So I agree that cvsimport should not worry about supporting repository
path with an '=' in it, but we do need to make sure we work with one with
a SP in it, when we are reading from cvspass file for CVSNT.
Similarly when we are reading from cvspass file for CVS, we should make
sure we don't break with repository path with an '=' in it.
Do we already have such a solution in the thread? Can somebody conclude
the discussion with a final, tested and applyable patch, please?
From: Guy Rouillier <hidden> Date: 2016-06-15 22:51:07
On 2/27/2011 3:26 AM, Junio C Hamano wrote:
Guy Rouillier[off-list ref] writes:
quoted
As I suspected after reading how the cvspass file is read and written,
CVSNT doesn't work with repositories with an equal sign in the
repository name. You can init it fine, and you can set up a password
for it. But if you try to login things go very wrong:
...
Since CVSNT can't handle a repository with an equal sign in its name,
I say we don't worry about this. I say the same about the original
CVS with a repository name with embedded spaces. We certainly don't
want to try to solve problems the original product doesn't solve.
Thanks; your observation matches my earlier suspicion. So in short:
- CVSNT does not work with repository path with an '=' in it, but does work
with ones with a SP in it; and
- CVS has trouble with repository path with a SP in it, but works with
ones with an '=' in it just fine.
Have I summarized it correctly?
So I agree that cvsimport should not worry about supporting repository
path with an '=' in it, but we do need to make sure we work with one with
a SP in it, when we are reading from cvspass file for CVSNT.
Similarly when we are reading from cvspass file for CVS, we should make
sure we don't break with repository path with an '=' in it.
Do we already have such a solution in the thread? Can somebody conclude
the discussion with a final, tested and applyable patch, please?
I've integrated the untested version you submitted several iterations ago,
and tested it with CVSNT. Unfortunately, I don't have a CVS repo to test
with, so if anyone else watching this thread has access to CVS, it would
be good if they can test with CVS. Here is my hopefully final revision.
Note that I've left this test in, although I still think it is a bad idea:
elsif (!$pass) {
$pass = "A";
}
I looked back at the revisions around 9/17/2009. That revision puts
this test back in because two revisions earlier on 8/14/2009 took it out.
But that doesn't explain why it was put in there in the first
place. I still say a better idea, if we don't want to allow an empty
password, is to error out rather than silently set a bogus password.
---
From bea4854fca07ff3a7034a04ad2f163701f9f581c Mon Sep 17 00:00:00 2001
From: Guy Rouillier <redacted>
Date: Fri, 29 Apr 2011 00:01:52 -0400
Subject: [PATCH] Look for password in both CVS and CVSNT password files.
In conn, if password is not passed on command line, look for a password
entry in both the CVS password file and the CVSNT password file. If only
one file is found and the requested repository is in that file, or if both
files are found but the requested repository is found in only one file, use
the password from the single file containing the repository entry. If both
files are found and the requested repository is found in both files, then
produce an error message.
The CVS password file separates tokens with a space character, while
the CVSNT password file separates tokens with an equal (=) character.
Add a sub find_password_entry that accepts the password file name
and a delimiter to eliminate code duplication.
---
git-cvsimport.perl | 52 ++++++++++++++++++++++++++++++++++++++++------------
1 files changed, 40 insertions(+), 12 deletions(-)
@@ -227,6 +227,30 @@ sub new {return$self;}+subfind_password_entry{+my($cvspass,@cvsroot)=@_;+my($file,$delim)=@$cvspass;+my$pass;+local($_);++if(open(my$fh,$file)){+# :pserver:cvs@mea.tmt.tele.fi:/cvsroot/zmailer Ah<Z+while(<$fh>){+chomp;+s/^\/\d+\s+//;+my($w,$p)=split($delim,$_,2);+formy$cvsroot(@cvsroot){+if($weq$cvsroot){+$pass=$p;+last;+}+}+}+close($fh);+}+return$pass;+}+subconn{my$self=shift;my$repo=$self->{'fullrep'};
@@ -259,19 +283,23 @@ sub conn {if($pass){$pass=$self->_scramble($pass);}else{-open(H,$ENV{'HOME'}."/.cvspass")anddo{-# :pserver:cvs@mea.tmt.tele.fi:/cvsroot/zmailer Ah<Z-while(<H>){-chomp;-s/^\/\d+\s+//;-my($w,$p)=split(/\s/,$_,2);-if($weq$rror$weq$rr2){-$pass=$p;-last;-}+my@cvspass=([$ENV{'HOME'}."/.cvspass",qr/\s/],+[$ENV{'HOME'}."/.cvs/cvspass",qr/=/]);+my@loc=();+foreachmy$cvspass(@cvspass){+my$p=find_password_entry($cvspass,$rr,$rr2);+if($p){+push@loc,$cvspass->[0];+$pass=$p;}-};-$pass="A"unless$pass;+}++if(1<@loc){+die("More than one cvs password files have ".+"entries for CVSROOT $opt_d: @loc");+}elsif(!$pass){+$pass="A";+}}my($s,$rep);--
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:07
Guy Rouillier wrote:
Note that I've left this test in, although I still think it is a bad idea:
elsif (!$pass) {
$pass = "A";
}
[...]
But that doesn't explain why it was put in there in the first
place. I still say a better idea, if we don't want to allow an empty
password, is to error out rather than silently set a bogus password.
It might be a good idea after all to do something else in that case
(as a separate patch :)), but it would require a little investigation.
Isn't the convention in CVS for anonymous pserver access to accept an
arbitrary password?
The CVS password file separates tokens with a space character, while
the CVSNT password file separates tokens with an equal (=) character.
Add a sub find_password_entry that accepts the password file name
and a delimiter to eliminate code duplication.
---
Sounds sensible to my untrained ears. Sign-off?
[...]
quoted hunk
+++ b/git-cvsimport.perl
@@ -227,6 +227,30 @@ sub new {return$self;}+subfind_password_entry{+my($cvspass,@cvsroot)=@_;+my($file,$delim)=@$cvspass;+my$pass;+local($_);++if(open(my$fh,$file)){+# :pserver:cvs@mea.tmt.tele.fi:/cvsroot/zmailer Ah<Z+while(<$fh>){+chomp;+s/^\/\d+\s+//;+my($w,$p)=split($delim,$_,2);+formy$cvsroot(@cvsroot){+if($weq$cvsroot){+$pass=$p;+last;
In the old code, this "last" applied to the while loop, while in the
new code it applies to the for loop. Intentional?
[...]
+ if (1 < @loc) {
+ die("More than one cvs password files have ".
+ "entries for CVSROOT $opt_d: @loc");
Grammar nit: "More than one" is singular (weird, eh?). It might
be clearer to say:
"Multiple cvs password files have " .
"entries for CVSROOT $opt_d: @loc"
(or "Both cvs password files").
Thanks again, and hope that helps.
Regards,
Jonathan
From: Guy Rouillier <hidden> Date: 2016-06-15 22:51:08
On 4/29/2011 6:27 PM, Jonathan Nieder wrote:
Guy Rouillier wrote:
quoted
Note that I've left this test in, although I still think it is a bad idea:
elsif (!$pass) {
$pass = "A";
}
[...]
quoted
But that doesn't explain why it was put in there in the first
place. I still say a better idea, if we don't want to allow an empty
password, is to error out rather than silently set a bogus password.
It might be a good idea after all to do something else in that case
(as a separate patch :)), but it would require a little investigation.
Isn't the convention in CVS for anonymous pserver access to accept an
arbitrary password?
quoted
The CVS password file separates tokens with a space character, while
the CVSNT password file separates tokens with an equal (=) character.
Add a sub find_password_entry that accepts the password file name
and a delimiter to eliminate code duplication.
---
Sounds sensible to my untrained ears. Sign-off?
[...]
quoted
+++ b/git-cvsimport.perl
@@ -227,6 +227,30 @@ sub new {return$self;}+subfind_password_entry{+my($cvspass,@cvsroot)=@_;+my($file,$delim)=@$cvspass;+my$pass;+local($_);++if(open(my$fh,$file)){+# :pserver:cvs@mea.tmt.tele.fi:/cvsroot/zmailer Ah<Z+while(<$fh>){+chomp;+s/^\/\d+\s+//;+my($w,$p)=split($delim,$_,2);+formy$cvsroot(@cvsroot){+if($weq$cvsroot){+$pass=$p;+last;
In the old code, this "last" applied to the while loop, while in the
new code it applies to the for loop. Intentional?
[...]
quoted
+ if (1< @loc) {
+ die("More than one cvs password files have ".
+ "entries for CVSROOT $opt_d: @loc");
Grammar nit: "More than one" is singular (weird, eh?). It might
be clearer to say:
"Multiple cvs password files have " .
"entries for CVSROOT $opt_d: @loc"
(or "Both cvs password files").
Thanks again, and hope that helps.
Jonathan, thanks for reading carefully. I hadn't looked at this in a
couple months because I've been busy at work, and Perl is not my strong
point. I had removed the last label because I didn't think it was
necessary, but you point out that it is.
I concur with addressing that default CVS password with a different
patch. That logic has been in the code since the original Perl version,
so perhaps no one has really looked into CVS password requirements in
any detail.
Here is hopefully my final version:
---
From a96233ab1112748338e6445ed1e4a5f0e8c1213b Mon Sep 17 00:00:00 2001
From: Guy Rouillier <redacted>
Date: Sun, 1 May 2011 01:23:44 -0400
Subject: [PATCH] Look for password in both CVS and CVSNT password files.
In conn, if password is not passed on command line, look for a password
entry in both the CVS password file and the CVSNT password file. If only
one file is found and the requested repository is in that file, or if both
files are found but the requested repository is found in only one file, use
the password from the single file containing the repository entry. If both
files are found and the requested repository is found in both files, then
produce an error message.
The CVS password file separates tokens with a space character, while
the CVSNT password file separates tokens with an equal (=) character.
Add a sub find_password_entry that accepts the password file name
and a delimiter to eliminate code duplication.
Signed-off-by: Guy Rouillier <redacted>
---
git-cvsimport.perl | 53 ++++++++++++++++++++++++++++++++++++++++-----------
1 files changed, 41 insertions(+), 12 deletions(-)
@@ -227,6 +227,31 @@ sub new {return$self;}+subfind_password_entry{+my($cvspass,@cvsroot)=@_;+my($file,$delim)=@$cvspass;+my$pass;+local($_);++if(open(my$fh,$file)){+# :pserver:cvs@mea.tmt.tele.fi:/cvsroot/zmailer Ah<Z+CVSPASSFILE:+while(<$fh>){+chomp;+s/^\/\d+\s+//;+my($w,$p)=split($delim,$_,2);+formy$cvsroot(@cvsroot){+if($weq$cvsroot){+$pass=$p;+lastCVSPASSFILE;+}+}+}+close($fh);+}+return$pass;+}+subconn{my$self=shift;my$repo=$self->{'fullrep'};
@@ -259,19 +284,23 @@ sub conn {if($pass){$pass=$self->_scramble($pass);}else{-open(H,$ENV{'HOME'}."/.cvspass")anddo{-# :pserver:cvs@mea.tmt.tele.fi:/cvsroot/zmailer Ah<Z-while(<H>){-chomp;-s/^\/\d+\s+//;-my($w,$p)=split(/\s/,$_,2);-if($weq$rror$weq$rr2){-$pass=$p;-last;-}+my@cvspass=([$ENV{'HOME'}."/.cvspass",qr/\s/],+[$ENV{'HOME'}."/.cvs/cvspass",qr/=/]);+my@loc=();+foreachmy$cvspass(@cvspass){+my$p=find_password_entry($cvspass,$rr,$rr2);+if($p){+push@loc,$cvspass->[0];+$pass=$p;}-};-$pass="A"unless$pass;+}++if(1<@loc){+die("Multiple cvs password files have ".+"entries for CVSROOT $opt_d: @loc");+}elsif(!$pass){+$pass="A";+}}my($s,$rep);--