Guy Rouillier [off-list ref] writes:
The two clients use a different delimiter to separate the CVS
repository name from the user password. The original CVS
client separates the two entries with a space character, while
CVSNT separates them with an equal (=) character. Hence,
the regular expression used to split these two tokens is
altered to accept either delimiter.
That sounds like a wrong approach. If there are two clients, one reads
from one location with one syntax, and the other one reads from another
different location with a different syntax, shouldn't the code using the
original syntax when reading the original file, and the other syntax when
reading the file for the other client?
I personally don't even like the sloppiness of the original code before
your patch that discards the version information ("/<digits>") and hopes
the file format stays the same for some time to come, but "one uses space
and the other uses equal, so lets mix them up and split at space-or-equal
when we know we are reading from the file that uses space (iow the one we
know we shouldn't be splitting at equal)" is making it even worse.
In practice, I would imagine that the cvsroot part wouldn't contain an
equal sign, so this looser regexp would not hurt in the real life, but it
does feel yucky.
Here is a totally untested patch. I think the original code used
$pass="A" as a fall-back when it didn't find any password entry, and I
tried to retain that instead of dying. Also this does not error out if
you merely have two cvspass files, as long as you do not have the wanted
entry for both of them.
git-cvsimport.perl | 52 ++++++++++++++++++++++++++++++++++++++++------------
1 files changed, 40 insertions(+), 12 deletions(-)
diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 8e683e5..0a25926 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@mea.tmt.tele.fi:/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,22 @@ sub conn {
if ($pass) {
$pass = $self->_scramble($pass);
} else {
- open(H,$ENV{'HOME'}."/.cvspass") and do {
- # :pserver:cvs@mea.tmt.tele.fi:/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("More than one cvs password files have ".
+ "entries for CVSROOT $opt_d: @loc");
+ } elsif (!$pass) {
+ $pass = "A";
+ }
}
my ($s, $rep);On 2/20/2011 2:21 AM, Junio C Hamano wrote:
Guy Rouillier [off-list ref] writes:
quoted
The two clients use a different delimiter to separate the CVS
repository name from the user password. The original CVS
client separates the two entries with a space character, while
CVSNT separates them with an equal (=) character. Hence,
the regular expression used to split these two tokens is
altered to accept either delimiter.
That sounds like a wrong approach. If there are two clients, one reads
from one location with one syntax, and the other one reads from another
different location with a different syntax, shouldn't the code using the
original syntax when reading the original file, and the other syntax when
reading the file for the other client?
...
In practice, I would imagine that the cvsroot part wouldn't contain an
equal sign, so this looser regexp would not hurt in the real life, but it
does feel yucky.
Well, this is the important point. I did think of these aspects when
writing the code. Sure, writing more precise code is possible, but the
results are the same in either case. If you look back at the version I
wrote in response to Emil's post, I did have two entirely separate
sections for CVS and CVSNT, and I used only one delimiter in each.
Martin then suggested I combine the two sections into one, so while
following that suggestion I had to alter the regular expression.
Here is a totally untested patch. I think the original code used
$pass="A" as a fall-back when it didn't find any password entry, and I
tried to retain that instead of dying. Also this does not error out if
you merely have two cvspass files, as long as you do not have the wanted
entry for both of them.
I'll take a look at the patch later this week when I have some time. I
purposely took out the code the set the password to "A" if the CVS
repository is not located in the password file. I was surprised to
see that in the original code. I can't think of any situation
where silently making up a password is a good idea.
Finally, after our last round of discussions, I thought we all agreed
not to try to do any matching on the contents of the two password files.
I originally implemented a comparison of the contents of the two files,
but you pointed out there are many possible permutations involving
entries found in both files. So I went back to my earlier approach
of just erroring out if both files are found.
--
Guy Rouillier
Guy Rouillier [off-list ref] writes:
On 2/20/2011 2:21 AM, Junio C Hamano wrote:
...
quoted
In practice, I would imagine that the cvsroot part wouldn't contain an
equal sign, so this looser regexp would not hurt in the real life, but it
does feel yucky.
Well, this is the important point. I did think of these aspects when
writing the code. Sure, writing more precise code is possible, but the
results are the same in either case.
It is probably unlikely to see a SP in the pathname, but I do not think it
is reasonable to introduce a regression to forbid '=' in the pathname to
the repository, which we have been supporting since August 2009, when we
know the patch as-is will regress the use case, and especially when we
already know a way to code not to regress is not too complex.
The "substitute with 'A' when missing" comes from e481b1d (cvs: initialize
empty password, 2009-09-17); it makes me worried that the patch is
removing the support, _unless_ that commit by Clemens was addressing a
problem that does not exist (and if so, I'd like to see a sentence or two in
the commit log to explain why it is a sane thing to do to remove it).
Thanks.