cvsimport still not working with cvsnt

cvsimport still not working with cvsnt

[Guy Rouillier]

I'm going to try sending this blind, as the mailing list has sent me the 
promised authorization key after 24 hrs.

I'm brand new to git.  We'll be moving over from CVS, so I imported a 
small part of our CVS repository to start learning git.  We use the 
CVSNT server, and git-cvsimport was failing with "I HATE YOU".  I 
finally found the problems, both of which were reported in 2008 here:

http://kerneltrap.org/mailarchive/git/2008/3/13/1157364

However, these changes do not appear in the version 1.7.2.2 that Gentoo 
supplies.  I checked the 1.7.3-rc2 source and the changes are not in 
there either.

I do see one possible issue with the supplied modifications.  At work, 
we upgraded from CVS to CVSNT.  So, my home directory has both .cvspass 
(from the original CVS) and .cvs/cvspass (after the conversion to 
CVSNT.)  Sloppy housekeeping on my part, I admit, but probably not 
uncommon.  The supplied patch would pick up the original CVS file and 
would fail.  (BTW, this is true only of the git-cvsimport.perl script 
itself; cvsps must shell out to the installed CVS client (in my case, 
cvsnt), because when I invoked that manually, it worked.)

So, I would advise checking to see if both files exist, and if so exit 
with an error.  Unless cvsimport wants to get real fancy and shell out 
to the installed cvs client to try to figure out what is installed, 
there is no way to tell which cvspass file is actively being used.  I 
don't recommend trying to figure this out, as the user's intent is unclear.

-- 
Guy Rouillier

Re: cvsimport still not working with cvsnt

[Jonathan Nieder]

(+cc: Emil, some cvsimport people)

Guy Rouillier wrote:

> I'm going to try sending this blind, as the mailing list has sent me
> the promised authorization key after 24 hrs.

No problem.  Actually a subscription is not required --- the
convention on this list is to always reply-to-all.

> I finally found the problems, both of which were reported in 2008
> here:
>
> http://kerneltrap.org/mailarchive/git/2008/3/13/1157364

Seems to have received no replies[1].

> I do see one possible issue with the supplied modifications.  At
> work, we upgraded from CVS to CVSNT.  So, my home directory has both
> .cvspass (from the original CVS) and .cvs/cvspass (after the
> conversion to CVSNT.)  Sloppy housekeeping on my part, I admit, but
> probably not uncommon.  The supplied patch would pick up the
> original CVS file and would fail.  (BTW, this is true only of the
> git-cvsimport.perl script itself; cvsps must shell out to the
> installed CVS client (in my case, cvsnt), because when I invoked
> that manually, it worked.)
> 
> So, I would advise checking to see if both files exist, and if so
> exit with an error.  Unless cvsimport wants to get real fancy and
> shell out to the installed cvs client to try to figure out what is
> installed, there is no way to tell which cvspass file is actively
> being used.  I don't recommend trying to figure this out, as the
> user's intent is unclear.

Thanks, sounds sane to me.  Care to write a patch?

Regards,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/77109

Re: cvsimport still not working with cvsnt

[Emil Medve]

Hello Guy,


On 12/20/10 15:36, Jonathan Nieder wrote:
> (+cc: Emil, some cvsimport people)
> 
> Guy Rouillier wrote:

Sometimes, on some particularly nasty CVS repos, I noticed better
results when using http://cvs2svn.tigris.org

>> I'm going to try sending this blind, as the mailing list has sent me
>> the promised authorization key after 24 hrs.
> 
> No problem.  Actually a subscription is not required --- the
> convention on this list is to always reply-to-all.
> 
>> I finally found the problems, both of which were reported in 2008
>> here:
>>
>> http://kerneltrap.org/mailarchive/git/2008/3/13/1157364
> 
> Seems to have received no replies[1].

I don't remember why, but that patch didn't get enough interest

>> I do see one possible issue with the supplied modifications.  At
>> work, we upgraded from CVS to CVSNT.  So, my home directory has both
>> .cvspass (from the original CVS) and .cvs/cvspass (after the
>> conversion to CVSNT.)  Sloppy housekeeping on my part, I admit, but
>> probably not uncommon.  The supplied patch would pick up the
>> original CVS file and would fail.  (BTW, this is true only of the
>> git-cvsimport.perl script itself; cvsps must shell out to the
>> installed CVS client (in my case, cvsnt), because when I invoked
>> that manually, it worked.)
>>
>> So, I would advise checking to see if both files exist, and if so
>> exit with an error.  Unless cvsimport wants to get real fancy and
>> shell out to the installed cvs client to try to figure out what is
>> installed, there is no way to tell which cvspass file is actively
>> being used.  I don't recommend trying to figure this out, as the
>> user's intent is unclear.
> 
> Thanks, sounds sane to me.  Care to write a patch?

If you care enough about this scenario, how about search for the
relevant <CVSROOT, password> in both files. If you find just one pair or
if you find a pair in both files and they are "equal" then just use it.
If you find two pairs, one in each file, use the one from the file with
a newer modified time-stamp. In a migration scenario such as this, you'd
imaging the "old" file will get stale after a while. Not perfect, but
some informational messages in case of a duplicate would help the user
clarify their intentions

Additionally/Alternatively just add a command line parameter to allow
the user to explicitly specify a cvspass file


Cheers,
Emil.

Re: cvsimport still not working with cvsnt

[Guy Rouillier]

On 12/21/2010 5:09 PM, Emil Medve wrote:
> Hello Guy,
>
>
> On 12/20/10 15:36, Jonathan Nieder wrote:
>> (+cc: Emil, some cvsimport people)
>>
>> Guy Rouillier wrote:
>
> Sometimes, on some particularly nasty CVS repos, I noticed better
> results when using http://cvs2svn.tigris.org
>
>>> I'm going to try sending this blind, as the mailing list has sent me
>>> the promised authorization key after 24 hrs.
>>
>> No problem.  Actually a subscription is not required --- the
>> convention on this list is to always reply-to-all.
>>
>>> I finally found the problems, both of which were reported in 2008
>>> here:
>>>
>>> http://kerneltrap.org/mailarchive/git/2008/3/13/1157364
>>
>> Seems to have received no replies[1].
>
> I don't remember why, but that patch didn't get enough interest
>
>>> I do see one possible issue with the supplied modifications.  At
>>> work, we upgraded from CVS to CVSNT.  So, my home directory has both
>>> .cvspass (from the original CVS) and .cvs/cvspass (after the
>>> conversion to CVSNT.)  Sloppy housekeeping on my part, I admit, but
>>> probably not uncommon.  The supplied patch would pick up the
>>> original CVS file and would fail.  (BTW, this is true only of the
>>> git-cvsimport.perl script itself; cvsps must shell out to the
>>> installed CVS client (in my case, cvsnt), because when I invoked
>>> that manually, it worked.)
>>>
>>> So, I would advise checking to see if both files exist, and if so
>>> exit with an error.  Unless cvsimport wants to get real fancy and
>>> shell out to the installed cvs client to try to figure out what is
>>> installed, there is no way to tell which cvspass file is actively
>>> being used.  I don't recommend trying to figure this out, as the
>>> user's intent is unclear.
>>
>> Thanks, sounds sane to me.  Care to write a patch?
>
> If you care enough about this scenario, how about search for the
> relevant<CVSROOT, password>  in both files. If you find just one pair or
> if you find a pair in both files and they are "equal" then just use it.
> If you find two pairs, one in each file, use the one from the file with
> a newer modified time-stamp. In a migration scenario such as this, you'd
> imaging the "old" file will get stale after a while. Not perfect, but
> some informational messages in case of a duplicate would help the user
> clarify their intentions
>
> Additionally/Alternatively just add a command line parameter to allow
> the user to explicitly specify a cvspass file

Emil and Jonathan, thanks for the feedback.  Perl is not my strong 
point, but I'll take a crack at it over the upcoming holidays.  I'm 
inclined not to get too fancy and try to second-guess the user's 
environment.  Perhaps he has both cvs and cvsnt installed for some 
reason (testing one, using the other for regular work); perhaps a tool 
installed one or the other and he doesn't even know he has them both. Etc.

So, at most I can see, as Emil suggested, seeing if the entry exists in 
both files and is the same in both.  If so, or if the entry is only in 
one of them, then just use the entry.  However, if the entry is in both 
files and is different, I'd prefer to just exit with an error and have 
the user clarify his environment.

-- 
Guy Rouillier

Re: cvsimport still not working with cvsnt

[Guy Rouillier]

On 12/22/2010 12:43 AM, Guy Rouillier wrote:
>
> Emil and Jonathan, thanks for the feedback. Perl is not my strong point,
> but I'll take a crack at it over the upcoming holidays. I'm inclined not
> to get too fancy and try to second-guess the user's environment. Perhaps
> he has both cvs and cvsnt installed for some reason (testing one, using
> the other for regular work); perhaps a tool installed one or the other
> and he doesn't even know he has them both. Etc.
>
> So, at most I can see, as Emil suggested, seeing if the entry exists in
> both files and is the same in both. If so, or if the entry is only in
> one of them, then just use the entry. However, if the entry is in both
> files and is different, I'd prefer to just exit with an error and have
> the user clarify his environment.

Here is my patch for accomplishing the above.  As this is my first time
submitting a patch, please let me know the correct procedure if
submitting a diff here is not appropriate.  Thanks.

--- git-cvsimport.org	2011-01-09 03:52:39.000000000 -0500
+++ git-cvsimport.cvsnt	2011-01-10 01:42:29.000000000 -0500
@@ -260,6 +260,8 @@
  		if ($pass) {
  			$pass = $self->_scramble($pass);
  		} else {
+			# First try the original CVS location.
+
  			open(H,$ENV{'HOME'}."/.cvspass") and do {
  				# :pserver:cvs@mea.tmt.tele.fi:/cvsroot/zmailer Ah<Z
  				while (<H>) {
@@ -272,7 +274,30 @@
  					}
  				}
  			};
-			$pass = "A" unless $pass;
+
+			# Now try the CVSNT location.
+
+			open(H,$ENV{'HOME'}."/.cvs/cvspass") and do {
+				# :pserver:cvs@mea.tmt.tele.fi:/cvsroot/zmailer Ah<Z
+				while (<H>) {
+					chomp;
+					s/^\/\d+\s+//;
+					my ($w,$p) = split(/=/,$_,2);
+					if ($w eq $rr or $w eq $rr2) {
+						my $cvsntpass = $p;
+
+						if (!$pass) {
+							$pass = $cvsntpass;
+						} elsif ($pass ne $cvsntpass) {
+							die("CVSROOT found in both CVS and CVSNT cvspass files, passwords do not match\n");
+						}
+						last;
+					}
+				}
+			};
+
+
+			die("Password not found for CVSROOT: $opt_d\n") unless $pass;
  		}

  		my ($s, $rep);



-- 
Guy Rouillier

Re: cvsimport still not working with cvsnt

[Martin Langhoff]

On Mon, Jan 10, 2011 at 2:33 AM, Guy Rouillier <guyr@burntmail.com> wrote:
> Here is my patch for accomplishing the above.  As this is my first time
> submitting a patch, please let me know the correct procedure if
> submitting a diff here is not appropriate.  Thanks.

The concept of what the patch is doing is good, but I'd recommend

@cvspasslocations = ($ENV{'HOME'}."/cvspass", $ENV{'HOME'}."/.cvs/cvspass")

foreach $cvspass (@cvspasslocations) {
   open(...

and forgo the "matching" test.

cheers,


m
-- 
 martin@laptop.org -- School Server Architect
 - ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff

Re: cvsimport still not working with cvsnt

[Guy Rouillier]

On 1/10/2011 10:38 AM, Martin Langhoff wrote:
> On Mon, Jan 10, 2011 at 2:33 AM, Guy Rouillier<guyr@burntmail.com>  wrote:
>> Here is my patch for accomplishing the above.  As this is my first time
>> submitting a patch, please let me know the correct procedure if
>> submitting a diff here is not appropriate.  Thanks.
>
> The concept of what the patch is doing is good, but I'd recommend
>
> @cvspasslocations = ($ENV{'HOME'}."/cvspass", $ENV{'HOME'}."/.cvs/cvspass")
>
> foreach $cvspass (@cvspasslocations) {
>     open(...
>
> and forgo the "matching" test.
>

Martin, thanks for the reply.  Have you had a chance to read the entire 
thread?  The matching test was suggested by Emil.

This is my first patch submission.  What is the process for reaching 
consensus?

-- 
Guy Rouillier

Re: cvsimport still not working with cvsnt

[Jonathan Nieder]

Guy Rouillier wrote:

> Martin, thanks for the reply.  Have you had a chance to read the
> entire thread?  The matching test was suggested by Emil.

To summarize, Emil originally (2008)[1] suggested only checking
~/.cvs/cvspass when ~/.cvspass fails to open.  There was no response
at the time, perhaps because nobody interested saw the message.

Guy, two years later[2], wrote:

| I do see one possible issue with the supplied modifications.  At work, 
| we upgraded from CVS to CVSNT.  So, my home directory has both
| .cvspass (from the original CVS) and .cvs/cvspass (after the conversion to 
| CVSNT.)  Sloppy housekeeping on my part, I admit, but probably not 
| uncommon.  The supplied patch would pick up the original CVS file and 
| would fail.  (BTW, this is true only of the git-cvsimport.perl script 

and recommended erroring out if both files exist to make this easier
to diagnose.

Emil's advice: if this is an important use case to you, maybe it would
be served better by looking at both files?

> This is my first patch submission.  What is the process for reaching
> consensus?

See Documentation/SubmittingPatches, "An ideal patch flow".

My take: you learn what you can from others' advice, but ultimately
the idea is to just make those changes that make the patch better
(where better can mean featureful or simpler and more maintainable ---
this is not meant to be an excuse for overengineering).  In most cases
apparent conflicts are not real conflicts at all but signs of distinct
design goals to be balanced or reconciled.

Hope that helps,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/77109
[2] http://thread.gmane.org/gmane.comp.version-control.git/163979

Re: cvsimport still not working with cvsnt

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> and recommended erroring out if both files exist to make this easier
> to diagnose.
>
> Emil's advice: if this is an important use case to you, maybe it would
> be served better by looking at both files?

Thanks for summarizing two-year's worth of discussion ;-)

Trying both, one after another, in the order that likely favors newer one
over the older one, is a very valid option but is appropriate only under a
very narrow condition.  Picking a wrong one must reliably, silently _and_
quickly fail, and fail without any side effect.  The one in .cvspass may
identify you as a different user from the user .cvs/cvspass identifies you
as, and the two users may have different capabilities or default server
side preferences--in such a case, both may succeed, but in a different and
unexpected way [*1*].

As the general principle, in a "we see two, and we cannot tell which one
the user wants to use" situation like this, I tend to prefer erroring out
to _force_ the user to fix the configuration once and for all.

Unless the "try both" approach is reasonable, we could implement "we read
from one and when we find one we stop, otherwise we read from the other"
and document the order, but it is probably less friendly than the above
two options.


[Footnote]

*1* I know .cvspass is a bad example for this, as it records the password
for the <pserver you are trying to connect to, your user on the server>
tuple.  Once you get in, what you can do is exactly the same because you
are authenticating as the same user, but think of a case like .ssh/config
that is indexed by "Host" and allows you to use "User" to 

Re: cvsimport still not working with cvsnt

[Guy Rouillier]

Sorry for the delay in following up, work has been busy.

On 1/14/2011 4:49 PM, Junio C Hamano wrote:
> As the general principle, in a "we see two, and we cannot tell which one
> the user wants to use" situation like this, I tend to prefer erroring out
> to _force_ the user to fix the configuration once and for all.

That was my original inclination.  As no other opinions have been posted 
since your message, here is my amended patch, incorporating Martin's 
ideas and dieing if the script finds both CVS and CVSNT password files.
I don't know why diff got tripped up on brace indentation; I used only
tabs and everything looks fine in vi.

--- git-cvsimport.org   2011-01-09 03:52:39.000000000 -0500
+++ git-cvsimport.perl  2011-01-30 00:59:29.000000000 -0500
@@ -260,19 +260,27 @@
                if ($pass) {
                        $pass = $self->_scramble($pass);
                } else {
-                       open(H,$ENV{'HOME'}."/.cvspass") and do {
+                       my @cvspasslocations = ($ENV{'HOME'}."/.cvspass", $ENV{'HOME'}."/.cvs/cvspass");
+                       my $filecount = 0;
+                       foreach my $cvspass (@cvspasslocations) {
+
+                               open(H, $cvspass) and do {
                                # :pserver:cvs@mea.tmt.tele.fi:/cvsroot/zmailer Ah<Z
+                                       $filecount++;
                                while (<H>) {
                                        chomp;
                                        s/^\/\d+\s+//;
-                                       my ($w,$p) = split(/\s/,$_,2);
+                                               my ($w,$p) = split(/[\s=]/,$_,2);
                                        if ($w eq $rr or $w eq $rr2) {
                                                $pass = $p;
                                                last;
                                        }
                                }
                        };
-                       $pass = "A" unless $pass;
+                       }
+                       
+                       die("Two CVS password files found: @cvspasslocations, please remove one") if $filecount > 1;
+                       die("Password not found for CVSROOT: $opt_d\n") unless $pass;
                }

                my ($s, $rep);


-- 
Guy Rouillier

Re: cvsimport still not working with cvsnt

[Martin Langhoff]

On Sat, Jan 29, 2011 at 11:33 PM, Guy Rouillier <guyr@burntmail.com> wrote:
> That was my original inclination.  As no other opinions have been posted
> since your message, here is my amended patch, incorporating Martin's
> ideas and dieing if the script finds both CVS and CVSNT password files.

ACK! Thanks!


m
-- 
 martin@laptop.org -- Software Architect - OLPC
 - ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff

Re: cvsimport still not working with cvsnt

[Junio C Hamano]

Martin Langhoff <martin@laptop.org> writes:

> On Sat, Jan 29, 2011 at 11:33 PM, Guy Rouillier <guyr@burntmail.com> wrote:
>> That was my original inclination.  As no other opinions have been posted
>> since your message, here is my amended patch, incorporating Martin's
>> ideas and dieing if the script finds both CVS and CVSNT password files.
>
> ACK! Thanks!

Can somebody resubmit an appliable patch with a proper commit message that
describes the problem and the solution please.

Thanks.

Re: cvsimport still not working with cvsnt

[Guy Rouillier]

On 2/10/2011 5:01 PM, Junio C Hamano wrote:
> Martin Langhoff <martin@laptop.org> writes:
> 
>> On Sat, Jan 29, 2011 at 11:33 PM, Guy Rouillier<guyr@burntmail.com>  wrote:
>>> That was my original inclination.  As no other opinions have been posted
>>> since your message, here is my amended patch, incorporating Martin's
>>> ideas and dieing if the script finds both CVS and CVSNT password files.
>>
>> ACK! Thanks!
> 
> Can somebody resubmit an appliable patch with a proper commit message that
> describes the problem and the solution please.
> 
> Thanks.

Junio, sorry for the delay in response.  I'm new to all this and I thought 
perhaps one of the listed committers had to submit the official patch.  
Perhaps they do.  I followed the directions in SubmittingPatches.  The result 
is below.  Please let me know if I need to do something differently.  I 
didn't simply send the result of format-match because that would have started 
a different message thread.  Thanks.

>From d3ae7d304ee2b89740225b0433bf7d7e07248f59 Mon Sep 17 00:00:00 2001
From: Guy Rouillier <guyr@burntmail.com>
Date: Fri, 18 Feb 2011 00:53:10 -0500
Subject: [PATCH] Look for password in both CVS and CVSNT password files.

The existing code looks for the CVS reposity password only in
the CVS password file in HOME/.cvspass. Accommodate the CVS
alternative CVSNT by also looking in HOME/.cvs/cvspass.  Die
if both files are found, and ask the user to remove one.

Signed-off-by: Guy Rouillier <guyr@burntmail.com>
---
 git-cvsimport.perl |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 8e683e5..76b4765 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -259,19 +259,27 @@ 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 @cvspasslocations = ($ENV{'HOME'}."/.cvspass", $ENV{'HOME'}."/.cvs/cvspass");
+			my $filecount = 0;
+			foreach my $cvspass (@cvspasslocations) {
+
+				open(H, $cvspass) and do {
+					# :pserver:cvs@mea.tmt.tele.fi:/cvsroot/zmailer Ah<Z
+					$filecount++;
+					while (<H>) {
+						chomp;
+						s/^\/\d+\s+//;
+						my ($w,$p) = split(/[\s=]/,$_,2);
+						if ($w eq $rr or $w eq $rr2) {
+							$pass = $p;
+							last;
+						}
 					}
-				}
-			};
-			$pass = "A" unless $pass;
+				};
+			}
+
+			die("Two CVS password files found: @cvspasslocations, please remove one") if $filecount > 1;
+			die("Password not found for CVSROOT: $opt_d\n") unless $pass;
 		}

 		my ($s, $rep);
--
1.7.3.4



-- 
Guy Rouillier

Re: cvsimport still not working with cvsnt

[Junio C Hamano]

Guy Rouillier <guyr@burntmail.com> writes:

> ...  I'm new to all this and I thought 
> perhaps one of the listed committers had to submit the official patch.  

There is no _listed committers_ ;-)  I was hoping either you as the original
author of the patch or Martin as the area expert would respond, but as
long as the result looks correct and explained well, it doesn't matter
either way.

Just one hopefully final question.

After stripping "/<version number><space>" from the beginning of the line
in order to treat newer .cvspass file format and the original file format
the same way, the code splits the remainder into two fields (cvsroot and
lightly-scrambled password).  It used to split only at a whitespace, which
seems to be in line with the source of CVS 1.12.13 I looked at (it is in
password_entry_parseline() function, src/login.c).  You new code however
also allows '=' to be a delimiter to be used for this split.

Is this change intentional?  If so please explain why it is necessary in
the commit log message.

Re: cvsimport still not working with cvsnt

[Guy Rouillier]

On 2/18/2011 1:34 PM, Junio C Hamano wrote:
> Guy Rouillier <guyr@burntmail.com> writes:
> 
>> ...  I'm new to all this and I thought 
>> perhaps one of the listed committers had to submit the official patch.  
> 
> There is no _listed committers_ ;-)  I was hoping either you as the original
> author of the patch or Martin as the area expert would respond, but as
> long as the result looks correct and explained well, it doesn't matter
> either way.
> 
> Just one hopefully final question.
> 
> After stripping "/<version number><space>" from the beginning of the line
> in order to treat newer .cvspass file format and the original file format
> the same way, the code splits the remainder into two fields (cvsroot and
> lightly-scrambled password).  It used to split only at a whitespace, which
> seems to be in line with the source of CVS 1.12.13 I looked at (it is in
> password_entry_parseline() function, src/login.c).  You new code however
> also allows '=' to be a delimiter to be used for this split.
> 
> Is this change intentional?  If so please explain why it is necessary in
> the commit log message.

Thanks to everyone here for the gracious patience with newcomers.

Yes, the change is intentional.  I've added an additional commit comment to
explain why.

>From 0fdfbdc0dbd0a0280d987640890f7b5ff566d6ef Mon Sep 17 00:00:00 2001
From: Guy Rouillier <guyr@burntmail.com>
Date: Sat, 19 Feb 2011 01:56:15 -0500
Subject: [PATCH] Look for password in both CVS and CVSNT password files.

The existing code looks for the CVS reposity password only in
the CVS password file in HOME/.cvspass. Accommodate the CVS
alternative CVSNT by also looking in HOME/.cvs/cvspass.  Die
if both files are found, and ask the user to remove one.

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.

Signed-off-by: Guy Rouillier <guyr@burntmail.com>
---
 git-cvsimport.perl |   32 ++++++++++++++++++++------------
 1 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 8e683e5..76b4765 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -259,19 +259,27 @@ 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 @cvspasslocations = ($ENV{'HOME'}."/.cvspass", $ENV{'HOME'}."/.cvs/cvspass");
+			my $filecount = 0;
+			foreach my $cvspass (@cvspasslocations) {
+
+				open(H, $cvspass) and do {
+					# :pserver:cvs@mea.tmt.tele.fi:/cvsroot/zmailer Ah<Z
+					$filecount++;
+					while (<H>) {
+						chomp;
+						s/^\/\d+\s+//;
+						my ($w,$p) = split(/[\s=]/,$_,2);
+						if ($w eq $rr or $w eq $rr2) {
+							$pass = $p;
+							last;
+						}
 					}
-				}
-			};
-			$pass = "A" unless $pass;
+				};
+			}
+
+			die("Two CVS password files found: @cvspasslocations, please remove one") if $filecount > 1;
+			die("Password not found for CVSROOT: $opt_d\n") unless $pass;
 		}

 		my ($s, $rep);
--
1.7.4.rc1.5.ge17aa

-- 
Guy Rouillier

Re: cvsimport still not working with cvsnt

[Junio C Hamano]

Guy Rouillier <guyr@burntmail.com> 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);

Re: cvsimport still not working with cvsnt

[Guy Rouillier]

On 2/20/2011 2:21 AM, Junio C Hamano wrote:
> Guy Rouillier <guyr@burntmail.com> 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?

...

> 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

Re: cvsimport still not working with cvsnt

[Junio C Hamano]

Guy Rouillier <guyr@burntmail.com> writes:

> On 2/20/2011 2:21 AM, Junio C Hamano wrote:
> ...
>> 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.

Re: cvsimport still not working with cvsnt

[Junio C Hamano]

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

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

Even though I don't deeply care about what CVSNT does, but I am somewhat
curious why this "change SP to =" was done when CVSNT writes out its
$HOME/.cvs/cvspass file.

Does anybody know why?  Only to make things incompatible, perhaps? ;-)

  http://www.selenic.com/pipermail/mercurial/2009-April/025095.html

seems to indicate that somebody next door had a similar experience on
exactly the same issue.

Re: cvsimport still not working with cvsnt

[Martin Langhoff]

On Tue, Feb 22, 2011 at 6:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Even though I don't deeply care about what CVSNT does...
...
> Does anybody know why?  Only to make things incompatible, perhaps? ;-)

A brief googling around shows that it also stores it in the Windows registry.

Should we support that too...? ;-)


m
-- 
 martin@laptop.org -- Software Architect - OLPC
 - ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff

Re: cvsimport still not working with cvsnt

[Guy Rouillier]

On 2/22/2011 6:50 PM, Martin Langhoff wrote:
> On Tue, Feb 22, 2011 at 6:08 PM, Junio C Hamano<gitster@pobox.com>  wrote:
>> Even though I don't deeply care about what CVSNT does...
> ..
>> Does anybody know why?  Only to make things incompatible, perhaps? ;-)
>
> A brief googling around shows that it also stores it in the Windows registry.
>
> Should we support that too...? ;-)

One thing at a time, Martin :).  After I get this patch through, I want 
to start working on getting the rest of the Perl script to run under 
Windows.  I was almost there; the biggest issue is that Perl 
implementations (ActiveState, Strawberry) for Windows don't support the 
list form of open.  I converted most of them successfully, but got stuck 
on one so decided to submit this patch first.  Thank goodness I did this 
separately :).

To answer Junio's question, I'm looking at the CVSNT code now 
(GlobalSettings.cpp, if anyone is interested.)  The password is stored 
in a general fashion like any other user-specified value.  So, the 
authors elected to use a properties file format of key=value.  That is 
as valid a format as any other.

-- 
Guy Rouillier

Re: cvsimport still not working with cvsnt

[Junio C Hamano]

Martin Langhoff <martin@laptop.org> writes:

> On Tue, Feb 22, 2011 at 6:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Even though I don't deeply care about what CVSNT does...
> ...
>> Does anybody know why?  Only to make things incompatible, perhaps? ;-)
>
> A brief googling around shows that it also stores it in the Windows registry.

Yes, I saw that too.  I actually also got the impression that registry is
the primary location for cvsnt (hence I suspect .cvs/cvspass support might
be secondary and would not be surprised if it were sub-par).

Re: cvsimport still not working with cvsnt

[Junio C Hamano]

Guy Rouillier <guyr@burntmail.com> writes:

> To answer Junio's question, I'm looking at the CVSNT code now
> (GlobalSettings.cpp, if anyone is interested.)  The password is stored
> in a general fashion like any other user-specified value.  So, the
> authors elected to use a properties file format of key=value.  That is
> as valid a format as any other.

As you dug that far, could you find out what happens when cvsroot contains
an equal-sign character in its path component?

I am starting to suspect that we do need two separate codepaths, and we
would need to split out the logic to find matching password entry given a
cvsroot value into a separate function to keep our sanity after all.

Re: cvsimport still not working with cvsnt

[Guy Rouillier]

On 2/22/2011 7:45 PM, Junio C Hamano wrote:
> Guy Rouillier<guyr@burntmail.com>  writes:
>
>> To answer Junio's question, I'm looking at the CVSNT code now
>> (GlobalSettings.cpp, if anyone is interested.)  The password is stored
>> in a general fashion like any other user-specified value.  So, the
>> authors elected to use a properties file format of key=value.  That is
>> as valid a format as any other.
>
> As you dug that far, could you find out what happens when cvsroot contains
> an equal-sign character in its path component?
>
> I am starting to suspect that we do need two separate codepaths, and we
> would need to split out the logic to find matching password entry given a
> cvsroot value into a separate function to keep our sanity after all.
>

I'll take a look.  I spent a short amount of time with Google looking 
for "cvsroot valid characters" but didn't find anything authoritative. 
Note that this issue is not unique to CVSNT.  What does CVS do with 
CVSROOT containing a space character?

-- 
Guy Rouillier

Re: cvsimport still not working with cvsnt

[Junio C Hamano]

Guy Rouillier <guyr@burntmail.com> writes:

> ... Note that this issue is not unique to CVSNT.  What does
> CVS do with CVSROOT containing a space character?

IIRC, the comparison is done against canonicalized cvsroot string, so that
you can try to connect to :pserver:Xz.Com:/path/to/repo even after you ran
"cvs -d :pserver:xz.com:/path/to/repo login" and I wouldn't be surprised
if the canonicalization involved quoting SP.  Since August 2009 nobody has
complained with the current code that doesn't do any canonicalization, and
I take that as a sign that nobody sane so far used a cvsroot with a space
in it ;-).  But that doesn't mean nobody sane has been using a cvsroot
with an equal sign in it, so we would need to at least avoid splitting at
an equal sign when reading from .cvsroot.

It probably is a good idea to port the cvsroot canonicalization code to
cvsimport in any case.

Re: cvsimport still not working with cvsnt

[Guy Rouillier]

On 2/22/2011 7:42 PM, Junio C Hamano wrote:
> Martin Langhoff<martin@laptop.org>  writes:
>
>> On Tue, Feb 22, 2011 at 6:08 PM, Junio C Hamano<gitster@pobox.com>  wrote:
>>> Even though I don't deeply care about what CVSNT does...
>> ...
>>> Does anybody know why?  Only to make things incompatible, perhaps? ;-)
>>
>> A brief googling around shows that it also stores it in the Windows registry.
>
> Yes, I saw that too.  I actually also got the impression that registry is
> the primary location for cvsnt (hence I suspect .cvs/cvspass support might
> be secondary and would not be surprised if it were sub-par).

There may perhaps be a misunderstanding of CVSNT.  CVSNT is a 
multi-platform client and server.  Both parts can run on many platforms, 
including Windows, Linux, and Solaris.  I don't use Macs so don't know 
about them.

Use of HOME/.cvs/cvspass is not secondary or sub-par.  On any platform 
other than Windows, HOME/.cvs/cvspass is the standard place that CVSNT 
stores repository passwords.  And on Windows, you can optionally tell it 
to store repository passwords in HOME/.cvs/cvspass instead of the 
registry.  I have my Windows configured that way for consistency with my 
numerous Linux accounts.

The whole reason I resurrected this 2 year old topic is that we are 
trying to migrate from CVSNT on *Linux* to git.

Thanks.

-- 
Guy Rouillier

Re: cvsimport still not working with cvsnt

[Guy Rouillier]

On 2/23/2011 12:24 AM, Junio C Hamano wrote:
> Guy Rouillier<guyr@burntmail.com>  writes:
>
>> ... Note that this issue is not unique to CVSNT.  What does CVS do
>> with CVSROOT containing a space character?
>
> IIRC, the comparison is done against canonicalized cvsroot string, so
> that you can try to connect to :pserver:Xz.Com:/path/to/repo even
> after you ran "cvs -d :pserver:xz.com:/path/to/repo login" and I
> wouldn't be surprised if the canonicalization involved quoting SP.
> Since August 2009 nobody has complained with the current code that
> doesn't do any canonicalization, and I take that as a sign that
> nobody sane so far used a cvsroot with a space in it ;-).  But that
> doesn't mean nobody sane has been using a cvsroot with an equal sign
> in it, so we would need to at least avoid splitting at an equal sign
> when reading from .cvsroot.
>
> It probably is a good idea to port the cvsroot canonicalization code
> to cvsimport in any case.

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:

guyr@gentoo-vm /data $ cvs -d 
":pserver:guyr@gentoo-vm:2401:/data/cvs\=repo/cvsroot" login
Logging in to :pserver:guyr@gentoo-vm:2401:/data/cvs\=repo/cvsroot
CVS Password:
Empty password used - try 'cvs login' with a real password
cvs [login aborted]: /data/cvs\=repo/cvsroot: no such repository

I tried as many permutations as I could think of, escaping the equal 
sign, not escaping it, etc.  None of them worked.  I did verify my 
environment before running this test by setting up a repository without 
the equal sign in the name, and everything works fine.

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.

-- 
Guy Rouillier

Re: cvsimport still not working with cvsnt

[Junio C Hamano]

Guy Rouillier <guyr@burntmail.com> 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?

Re: cvsimport still not working with cvsnt

[Guy Rouillier]

On 2/27/2011 3:26 AM, Junio C Hamano wrote:
> Guy Rouillier<guyr@burntmail.com>  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?

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 <guyr@burntmail.com>
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(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index bbf327f..abf5759 100755
--- a/git-cvsimport.perl
+++ 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@mea.tmt.tele.fi:/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;
+				}
+			}
+		}
+		close($fh);
+	}
+	return $pass;
+}
+
 sub conn {
 	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") 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);
--
1.7.4.rc1.5.ge17aa

-- 
Guy Rouillier

Re: cvsimport still not working with cvsnt

[Jonathan Nieder]

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@mea.tmt.tele.fi:/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.

Regards,
Jonathan

Re: cvsimport still not working with cvsnt

[Guy Rouillier]

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@mea.tmt.tele.fi:/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@burntmail.com>
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@burntmail.com>
---
 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@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,23 @@ 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("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

Re: cvsimport still not working with cvsnt

[Junio C Hamano]

Thanks, will queue.