CVS import

> Hi list,
>
> when I try to import a CVS repo with:
>
> git cvsimport -v -i \
>   -d :pserver:anonymous@javagroups.cvs.sourceforge.net:/cvsroot/javagroups \
>   JGroups
>
> I'm getting the errors:
>
> New tests/other/org/jgroups/tests/adapttcp/Test.java: 5914 bytes
> Use of uninitialized value in concatenation (.) or string at /usr/bin/git-cvsimport line 674, <CVS> line 652.
> Use of uninitialized value in concatenation (.) or string at /usr/bin/git-cvsimport line 674, <CVS> line 652.
> fatal: malformed index info 100666 	src/org/jgroups/util/RWLock.java
> unable to write to git-update-index:  at /usr/bin/git-cvsimport line 679, <CVS> line 652.
>
>
> I've seen this before when trying to import other repositories.
> And since I'm not good with Perl I was wondering whether this sounds familiar
> and if there's a fix for it.
>
> I'm on Fedora 10 with the following packages:
> git.x86_64                                                      1.6.0.6-1.fc10
> git-all.x86_64                                                  1.6.0.6-1.fc10
> git-arch.x86_64                                                 1.6.0.6-1.fc10
> git-cvs.x86_64                                                  1.6.0.6-1.fc10
> git-daemon.x86_64                                               1.6.0.6-1.fc10
> git-email.x86_64                                                1.6.0.6-1.fc10
> git-gui.x86_64                                                  1.6.0.6-1.fc10
> git-svn.x86_64                                                  1.6.0.6-1.fc10
> gitk.x86_64                                                     1.6.0.6-1.fc10
> gitosis.noarch                                                  0.2-6.20080825git.fc10
> gitweb.x86_64                                                   1.6.0.6-1.fc10
>
>
>
> Ferry
>

> I solved it:
> 
> it has to do with the
> core.autocrlf=input
> core.safecrlf=true
> 
> settings I had in my global config.

> On Mon, 16 Feb 2009, Ferry Huberts (Pelagic) wrote:
> 
> > I solved it:
> > 
> > it has to do with the
> > core.autocrlf=input
> > core.safecrlf=true
> > 
> > settings I had in my global config.
> 
> Thanks!
> 
> > Maybe the manual page should warn against having these defined?
> 
> Maybe it should be solved differently?  As cvsimport needs to operate with 
> autocrlf=false, it seems, it could set that variable when it creates a 
> repository, and check the variable otherwise (erroring out if it is set 
> inappropriately)?

> Hi,
>
> On Mon, 16 Feb 2009, Johannes Schindelin wrote:
>
>   
>> On Mon, 16 Feb 2009, Ferry Huberts (Pelagic) wrote:
>>
>>     
>>> I solved it:
>>>
>>> it has to do with the
>>> core.autocrlf=input
>>> core.safecrlf=true
>>>
>>> settings I had in my global config.
>>>       
>> Thanks!
>>
>>     
>>> Maybe the manual page should warn against having these defined?
>>>       
>> Maybe it should be solved differently?  As cvsimport needs to operate with 
>> autocrlf=false, it seems, it could set that variable when it creates a 
>> repository, and check the variable otherwise (erroring out if it is set 
>> inappropriately)?
>>     
>
> IOW something like this:
>
> -- snip --
>  git-cvsimport.perl |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/git-cvsimport.perl b/git-cvsimport.perl
> index e439202..a27cc94 100755
> --- a/git-cvsimport.perl
> +++ b/git-cvsimport.perl
> @@ -562,12 +562,16 @@ my %index; # holds filenames of one index per branch
>  unless (-d $git_dir) {
>  	system("git-init");
>  	die "Cannot init the GIT db at $git_tree: $?\n" if $?;
> +	system("git-config core.autocrlf false");
> +	die "Cannot set core.autocrlf false" if $?;
>  	system("git-read-tree");
>  	die "Cannot init an empty tree: $?\n" if $?;
>  
>  	$last_branch = $opt_o;
>  	$orig_branch = "";
>  } else {
> +	die "Cannot operate with core.autocrlf other than 'false'"
> +		if (`git-config --bool core.autocrlf` =~ /true|input/);
>  	open(F, "git-symbolic-ref HEAD |") or
>  		die "Cannot run git-symbolic-ref: $!\n";
>  	chomp ($last_branch = <F>);
> -- snap --
>
> If you could add a test to t9600 and a few words to the man page, that 
> would be awesome.
>
> Ciao,
> Dscho
>
> P.S.: I think the same strategy should be applied to git-svn...
>   

> Johannes Schindelin wrote:
>
> > On Mon, 16 Feb 2009, Johannes Schindelin wrote:
> >
> >   
> > > On Mon, 16 Feb 2009, Ferry Huberts (Pelagic) wrote:
> > >
> > >     
> > > > I solved it:
> > > >
> > > > it has to do with the
> > > > core.autocrlf=input
> > > > core.safecrlf=true
> > > >
> > > > settings I had in my global config.
> > > >       
> > > Thanks!
> > >
> > >     
> > > > Maybe the manual page should warn against having these defined?
> > > >       
> > > Maybe it should be solved differently?  As cvsimport needs to operate with
> > > autocrlf=false, it seems, it could set that variable when it creates a
> > > repository, and check the variable otherwise (erroring out if it is set
> > > inappropriately)?
> > >     
> >
> > IOW something like this:
> >
> > -- snip --
> >  git-cvsimport.perl |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/git-cvsimport.perl b/git-cvsimport.perl
> > index e439202..a27cc94 100755
> > --- a/git-cvsimport.perl
> > +++ b/git-cvsimport.perl
> > @@ -562,12 +562,16 @@ my %index; # holds filenames of one index per branch
> >  unless (-d $git_dir) {
> >   system("git-init");
> >   die "Cannot init the GIT db at $git_tree: $?\n" if $?;
> > +	system("git-config core.autocrlf false");
> > +	die "Cannot set core.autocrlf false" if $?;
> >   system("git-read-tree");
> >   die "Cannot init an empty tree: $?\n" if $?;
> >  
> >   $last_branch = $opt_o;
> >   $orig_branch = "";
> > } else {
> > +	die "Cannot operate with core.autocrlf other than 'false'"
> > +		if (`git-config --bool core.autocrlf` =~ /true|input/);
> >   open(F, "git-symbolic-ref HEAD |") or
> >   	die "Cannot run git-symbolic-ref: $!\n";
> > 	chomp ($last_branch = <F>);
> > -- snap --
> >
> > If you could add a test to t9600 and a few words to the man page, that would
> > be awesome.
> >
> > Ciao,
> > Dscho
> >
> > P.S.: I think the same strategy should be applied to git-svn...
> >   
> I'm willing to give it a stab but I'm not versed on Perl at all.

> I solved it:
>
> it has to do with the
> core.autocrlf=input
> core.safecrlf=true
>
> settings I had in my global config.
> Maybe the manual page should warn against having these defined?
>

> On Mon, February 16, 2009 14:20, Ferry Huberts (Pelagic) wrote:
> > I solved it:
> >
> > it has to do with the
> > core.autocrlf=input
> > core.safecrlf=true
> >
> > settings I had in my global config.
> > Maybe the manual page should warn against having these defined?
> >
> 
> I'm working on it now, and did some more testing: it's actually the 
> safecrlf setting, not the autocrlf option.

> Hi,
>
> On Tue, 17 Feb 2009, Ferry Huberts (Pelagic) wrote:
>
>   
>> 1- correct import, no warnings
>>     
>
> When you say "correct import", do you mean that the import worked, or that 
> the imported file is actually bytewise identical to what is stored in the 
> CVS _repository_ (as opposed to the working directory)?
>
>   

> > I'm working on it now, and did some more testing: it's actually the 
> > safecrlf setting, not the autocrlf option.
> 
> Oh.  That probably means that cvsimport gets confused by the extra 
> warnings.
> 
> However, I think it is not correct to run cvsimport with autocrlf set to 
> anything than false anyway (and safecrlf would not trigger then, right?).
> 
> So IMHO the solution is still to force autocrlf off.

> On Mon, Feb 16, 2009 at 09:59:29PM +0100, Johannes Schindelin wrote:
>
>> > I'm working on it now, and did some more testing: it's actually the
>> > safecrlf setting, not the autocrlf option.
>>
>> Oh.  That probably means that cvsimport gets confused by the extra
>> warnings.
>>
>> However, I think it is not correct to run cvsimport with autocrlf set to
>> anything than false anyway (and safecrlf would not trigger then, right?).
>>
>> So IMHO the solution is still to force autocrlf off.
>
> I don't think that's right. What is happening is that git-hash-object is
> barfing, and git-cvsimport is not properly detecting the error.
> something like this (untested) would make that better:
>
> diff --git a/git-cvsimport.perl b/git-cvsimport.perl
> index e439202..65e7990 100755
> --- a/git-cvsimport.perl
> +++ b/git-cvsimport.perl
> @@ -926,6 +926,7 @@ while (<CVS>) {
>  			my $sha = <$F>;
>  			chomp $sha;
>  			close $F;
> +			$? and die "hash-object reported failure";
>  			my $mode = pmode($cvs->{'mode'});
>  			push(@new,[$mode, $sha, $fn]); # may be resurrected!
>  		}
>
> But the problem is not autocrlf. It is that the combination of "autocrlf
> = input" and "safecrlf" is nonsensical. Just try this:
>
>   $ git init
>   $ git config core.autocrlf input
>   $ git config core.safecrlf true
>   $ printf 'DOS\r\n' >file
>   $ git add file
>   fatal: CRLF would be replaced by LF in file.
>
> which makes sense. SafeCRLF is about making sure that the file will be
> the same on checkin and checkout. But it won't, because we are only
> doing CRLF conversion half the time.
>
> So the best workaround is disabling safecrlf, which makes no sense with
> his autocrlf setting. But I also think safecrlf could be smarter by
> treating autocrlf=input as autocrlf=true. That is, we don't care if in
> our _particular_ config it will come out the same; we care about whether
> one could, if so inclined, get the CRLF's back to create a byte-for-byte
> identical object.
>
> -Peff
>

> I replied in the thread with something comparable:
> http://article.gmane.org/gmane.comp.version-control.git/110358
> 
> My suggestion is make sure that safecrlf is set to false (see the end
> part of the mail)

> Back to the issue:
> I think requiring autocrlf = false is too strict.  Requiring autocrlf
> = false should be enough. That combined with a bit of text in the
> manual page about these settings: autocrlf = false is strongly
> recommended. Also, safecrlf is required to be set to false.

> On Fri, Feb 20, 2009 at 05:25:43PM +0100, Ferry Huberts (Pelagic) wrote:
>
>   
>> I replied in the thread with something comparable:
>> http://article.gmane.org/gmane.comp.version-control.git/110358
>>
>> My suggestion is make sure that safecrlf is set to false (see the end
>> part of the mail)
>>     
>
> Oh, sorry, I missed that bit. You said:
>
>   
>> Back to the issue:
>> I think requiring autocrlf = false is too strict.  Requiring autocrlf
>> = false should be enough. That combined with a bit of text in the
>> manual page about these settings: autocrlf = false is strongly
>> recommended. Also, safecrlf is required to be set to false.
>>     
>
> Assuming there is a typo and you meant to say "Requiring safecrlf =
> false should be enough", then yes, I agree. But if you are recommending
>   

> to put that into the "git cvsimport" manpage, I'm not sure that makes
> sense. Setting autocrlf to input and turning on safecrlf breaks much
> more than that; you can't add any file that has a CRLF in it.  So such a
> warning should probably go in the config description for those options.
>
>   

> I still think safecrlf could probably be made more useful in this case
> to differentiate between "this will corrupt your data if you do a
> checkout with your current config settings" and "this will corrupt your
> data forever".  But I am not a user of either config variable, so maybe
> there is some subtlety I'm missing.
>
> -Peff
>   

>> I still think safecrlf could probably be made more useful in this case
>> to differentiate between "this will corrupt your data if you do a
>> checkout with your current config settings" and "this will corrupt your
>> data forever".  But I am not a user of either config variable, so maybe
>> there is some subtlety I'm missing.
>
> I'm a user of these options myself. I maintain several large repositories 
> that contain data that is used both on Unix and Windows platforms and that 
> have the autocrlf=input and safecrlf=true. This makes sure that everything 
> is in Unix format.

> reversible, you just don't have it enabled), safecrlf is still
> triggering and refusing the content.
>
> And I was suggesting that it might be useful to distinguish between those two situations. Because right now, with autocrlf=input you have two choices:
>
>   - safecrlf=false, in which you will corrupt mixed CRLF/LF data without
>     any warning
>
>   - safecrlf=true, in which case you are not allowed to check in CR at
>     all
>
> But there is no choice for "protect me from actual corruption, but convert text files (i.e., all CRLF)".
>
> I am a bit concerned about a proposal to set safecrlf=false in all cvsimported repositories.  You are turning off the protection against corrupting binary files.  _Even if_ the person has put

> > I am a bit concerned about a proposal to set safecrlf=false in all
> > cvsimported repositories.  You are turning off the protection
> > against corrupting binary files.  _Even if_ the person has put
> > safecrlf=true into their ~/.gitconfig and thinks they are safe.
> 
> Ok.  I follow and agree. Full circle :-) We're back to Johannes'
> proposal: make sure that autocrlf is set to false. Agree? If so, then
> I'll try to whip up a patch.

> [Can you please wrap your lines? They seem to be about 200 characters,
>  and then rewrap the quoted text, but without putting a '>' marker.
>  I had a very hard time figuring out what was quoted and what was not.]
>
> On Mon, Feb 23, 2009 at 07:50:48AM +0100, Ferry Huberts (Pelagic) wrote:
>
>> > I am a bit concerned about a proposal to set safecrlf=false in all
>> > cvsimported repositories.  You are turning off the protection
>> > against corrupting binary files.  _Even if_ the person has put
>> > safecrlf=true into their ~/.gitconfig and thinks they are safe.
>>
>> Ok.  I follow and agree. Full circle :-) We're back to Johannes'
>> proposal: make sure that autocrlf is set to false. Agree? If so, then
>> I'll try to whip up a patch.
>
> But won't that now import CRLF's into your new git repo? Especially on
> Windows, where (IIRC) cvs gives you files with CRLF by default?
>
> -Peff
>

> > But won't that now import CRLF's into your new git repo? Especially on
> > Windows, where (IIRC) cvs gives you files with CRLF by default?
> 
> Yes it would. But sadly that's the only way to make sure that the import
> will work (without serious manual intervention).
> I found this out the hard way.

> On Mon, Feb 23, 2009 at 08:09:03AM +0100, Ferry Huberts (Pelagic) wrote:
>
>> > But won't that now import CRLF's into your new git repo? Especially on
>> > Windows, where (IIRC) cvs gives you files with CRLF by default?
>>
>> Yes it would. But sadly that's the only way to make sure that the import
>> will work (without serious manual intervention).
>> I found this out the hard way.
>
> Wouldn't setting autocrlf=true, safecrlf=true do the import you want?
> Then you could reset autocrlf to input after import but before checkout
> time.

> > Wouldn't setting autocrlf=true, safecrlf=true do the import you want?
> > Then you could reset autocrlf to input after import but before checkout
> > time.
> 
> No. As I demonstrated in my testing setup the combination of autocrlf=true
> and safecrlf=true ALWAYS makes the import NOT work (for repositories that
> have CRLF files). In my own experience I also found that the combination

> of autocrlf=input and safecrlf=true ALWAYS makes the import NOT work for
> PRATICAL repositories. That lead me to the conclusion to require
> safecrlf=false. From the discussion and arguments from you it appeared
> that that wouldn't be enough. Therefore I think that we have to require
> autocrlf=false (which makes git ignore the safecrlf setting).

> > So yes, in some sense it is safecrlf that is broken. I'm just concerned
> > about tweaking the user's options behind their back. The import can
> > happen differently than they expected no matter which of safecrlf or
> > autocrlf you tweak. So I think you are better off to complain and die.
> 
> The plan was:
> - when creating a new git repo for cvs import: setup safecrlf=false
> - when importing into an existing repo: check whether the safecrlf
>   setting is set to false and crash and burn when not :-)
>   (complain before going up in flames)

> On Tue, Feb 24, 2009 at 10:25:12AM +0100, Ferry Huberts (Pelagic) wrote:
>
>> > So yes, in some sense it is safecrlf that is broken. I'm just concerned
>> > about tweaking the user's options behind their back. The import can
>> > happen differently than they expected no matter which of safecrlf or
>> > autocrlf you tweak. So I think you are better off to complain and die.
>>
>> The plan was:
>> - when creating a new git repo for cvs import: setup safecrlf=false
>> - when importing into an existing repo: check whether the safecrlf
>>   setting is set to false and crash and burn when not :-)
>>   (complain before going up in flames)
>
> Why is it OK to silently change the settings in the first case, but not
> the second? Don't both have the potential to screw up the user's import?
>

> Also, are settings going to be unset after the first import? If so, then
> further incremental imports will fail as described in your second case.
> But if not, then safecrlf is turned off for that repo, even for
> non-cvsimport commands, overriding anything in the user's ~/.gitconfig.
> For somebody doing a one-shot import, they are paying that price without
> any benefit.
>

> > Also, are settings going to be unset after the first import? If so, then
> > further incremental imports will fail as described in your second case.
> > But if not, then safecrlf is turned off for that repo, even for
> > non-cvsimport commands, overriding anything in the user's ~/.gitconfig.
> > For somebody doing a one-shot import, they are paying that price without
> > any benefit.
> >
> this actually makes sense to me. I was only thinking about the continuous
> import use-case. In that light it would be better to just complain and die
> in the script. I guess I'll just implement that in the patch then.