Re: cvsimport still not working with cvsnt

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 29, 2011, 22:27 UTC
Message-ID: <20110429222729.GB5916@elie>
In-Reply-To: <4DBA3E14.7090602@burntmail.com>

Guy Rouillier wrote:

Show 5 quoted lines

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

Show 5 quoted lines

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

[...]

Show 21 quoted lines

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