On 4/29/2011 6:27 PM, Jonathan Nieder wrote:
> 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?
>
> [...]
>> +++ b/git-cvsimport.perl
>> @@ -227,6 +227,30 @@ sub new {
>> return $self;
>> }
>>
>> +sub find_password_entry {
>> + my ($cvspass, @cvsroot) = @_;
>> + my ($file, $delim) = @$cvspass;
>> + my $pass;
>> + local ($_);
>> +
>> + if (open(my $fh, $file)) {
>> + # :pserver:cvs@xxxxxxxxxxxxxxx:/cvsroot/zmailer Ah<Z
>> + while (<$fh>) {
>> + chomp;
>> + s/^\/\d+\s+//;
>> + my ($w, $p) = split($delim,$_,2);
>> + for my $cvsroot (@cvsroot) {
>> + if ($w eq $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.
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 <guyr@xxxxxxxxxxxxx>
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 <guyr@xxxxxxxxxxxxx>
---
git-cvsimport.perl | 53 ++++++++++++++++++++++++++++++++++++++++-----------
1 files changed, 41 insertions(+), 12 deletions(-)
diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index bbf327f..a01b73d 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -227,6 +227,31 @@ sub new {
return $self;
}
+sub find_password_entry {
+ my ($cvspass, @cvsroot) = @_;
+ my ($file, $delim) = @$cvspass;
+ my $pass;
+ local ($_);
+
+ if (open(my $fh, $file)) {
+ # :pserver:cvs@xxxxxxxxxxxxxxx:/cvsroot/zmailer Ah<Z
+ CVSPASSFILE:
+ while (<$fh>) {
+ chomp;
+ s/^\/\d+\s+//;
+ my ($w, $p) = split($delim,$_,2);
+ for my $cvsroot (@cvsroot) {
+ if ($w eq $cvsroot) {
+ $pass = $p;
+ last CVSPASSFILE;
+ }
+ }
+ }
+ close($fh);
+ }
+ return $pass;
+}
+
sub conn {
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") and do {
- # :pserver:cvs@xxxxxxxxxxxxxxx:/cvsroot/zmailer Ah<Z
- while (<H>) {
- chomp;
- s/^\/\d+\s+//;
- my ($w,$p) = split(/\s/,$_,2);
- if ($w eq $rr or $w eq $rr2) {
- $pass = $p;
- last;
- }
+ my @cvspass = ([$ENV{'HOME'}."/.cvspass", qr/\s/],
+ [$ENV{'HOME'}."/.cvs/cvspass", qr/=/]);
+ my @loc = ();
+ foreach my $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);
--
1.7.5.134.gbea48
--
Guy Rouillier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html