CVS import

CVS import

[Ferry Huberts (Pelagic)]

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

Re: CVS import [SOLVED]

[Ferry Huberts (Pelagic)]

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?

Ferry


On Mon, February 16, 2009 10:17, Ferry Huberts (Pelagic) wrote:
> 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
>

Re: CVS import [SOLVED]

[Johannes Schindelin]

Hi,

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

Ciao,
Dscho

Re: CVS import [SOLVED]

[Johannes Schindelin]

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

Re: CVS import [SOLVED]

[Ferry Huberts (Pelagic)]

Johannes Schindelin wrote:
> 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...
>   
I'm willing to give it a stab but I'm not versed on Perl at all. C and 
Java I can do without breaking a sweat though.
Isn't it a better idea to have the original authors do this? They 
understand the code.
Also, doing this would constitute my first patch to git. I'm unfamiliar 
with its codebase and the requirements of the people that contribute to 
it. Willing to learn though :-)
My patches would then probably need some review and would take a bit 
longer to develop. If that's acceptable then I'm willing to try.

Ferry

Re: CVS import [SOLVED]

[Johannes Schindelin]

Hi,

On Mon, 16 Feb 2009, Ferry Huberts (Pelagic) wrote:

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

Well, the tests and the man page changes I asked you to do don't require 
Perl..

> Isn't it a better idea to have the original authors do this? They 
> understand the code.

No.  This is Open Source, and it is not their itch.

> Also, doing this would constitute my first patch to git. I'm unfamiliar 
> with its codebase and the requirements of the people that contribute to 
> it. Willing to learn though :-)

That's the best way to start.

> My patches would then probably need some review and would take a bit 
> longer to develop. If that's acceptable then I'm willing to try.

No problem, I am here (if you want to cope with me, that is).

Ciao,
Dscho

Re: CVS import [SOLVED]

[Ferry Huberts (Pelagic)]

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.
Which leaves me with some questions of what to do exactly:

 autocrlf safecrlf
1 false    false
2 false     warn
3 false     true
4 input    false
5 input     warn
6 input     true
7 true     false
8 true      warn
9 true      true


1- git ignores the safecrlf flag; obviously acceptable
2- git ignores the safecrlf flag; so acceptable
3- git ignores the safecrlf flag, so acceptable
4- seems acceptable to me
5- seems acceptable to me
6- unacceptable
7- seems acceptable to me
8- seems acceptable to me
9- unacceptable

So, 6 and 9 (safecrlf==true) are definitely unacceptable. 1-3 are definitely acceptable.
How about the others?
Should these produce warnings?
Should the user use a 'force' option to make the import work (and acknowledge that he's calling for trouble)
Should we enforce some setting? Which flags/setting?

Input appreciated :-)

Ferry

Re: CVS import [SOLVED]

[Johannes Schindelin]

Hi,

On Mon, 16 Feb 2009, Ferry Huberts (Pelagic) wrote:

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

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.

Ciao,
Dscho

Re: CVS import [SOLVED]

[Ferry Huberts (Pelagic)]

Ok.

I tested all combinations of autocrlf and safecrlf on an artificial cvs
repository with only a dos text file and a Unix text file. Here are my results.


 autocrlf safecrlf
1 false    false
2 false     warn
3 false     true
4 input    false
5 input     warn
6 input     true
7 true     false
8 true      warn
9 true      true


1- correct import, no warnings
2- correct import, no warnings
3- correct import, no warnings
4- correct import, no warnings
5- correct import, warning on the dos text file
6- correct import, no warnings
7- correct import, no warnings
8- correct import, no warnings
9- fail:

Initialized empty Git repository in /data/home.f9/ferry/testarea/cvsimport/wc.git.true.true/.git/
Running cvsps...
cvs_direct initialized to CVSROOT /data/home.f9/ferry/testarea/cvsimport/cvs
cvs rlog: Logging master
* UNKNOWN LINE * Branches:
Fetching dos.txt   v 1.1
New dos.txt: 25 bytes
Fetching unix.txt   v 1.1
New unix.txt: 22 bytes
fatal: LF would be replaced by CRLF in /tmp/gitcvs.RT1XN8
Use of uninitialized value $sha in scalar chomp at /usr/bin/git-cvsimport line 928.
Use of uninitialized value in concatenation (.) or string at /usr/bin/git-cvsimport line 674, <CVS> line 14.
fatal: malformed index info 100666 	unix.txt
unable to write to git-update-index:  at /usr/bin/git-cvsimport line 679, <CVS> line 14.


So 9 crashes while 6 does not. Apparently the artificial repo with the 2 text files
doesn't give enough coverage: my problem was with 6.

It seems that the import script does not detect a fatal from git. It seems to
me that it does not check the return code because it tries to continue.
Must be here (from line 923):

print "".($init ? "New" : "Update")." $fn: $size bytes\n" if $opt_v;
my $pid = open(my $F, '-|');
die $! unless defined $pid;
if (!$pid) {
	exec("git-hash-object", "-w", $tmpname)
	or die "Cannot create object: $!\n";
}
my $sha = <$F>;
chomp $sha;
close $F;


I think the culprit here is git-hash-object. Either it does return a non-zero
exit code or cvsimport does not see the exit code correctly.
I've traced it in the code to the file convert.c, function
static void check_safe_crlf(const char *path, int action,
                            struct text_stat *stats, enum safe_crlf checksafe)

It does do a 'die' which will exit with code 128, but apparently isn't picked
up by perl. I'm stuck now as I don't know Perl well enough.


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.

Re: CVS import [SOLVED]

[Johannes Schindelin]

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

Ciao,
Dscho

Re: CVS import [SOLVED]

[Ferry Huberts (Pelagic)]

Johannes Schindelin wrote:
> 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)?
>
>   
as far as i could tell (but i looked at the checkout, not the 
repository) it means that the files were imported 1:1. no modifications.
(which is actually a bit weird as for some of the cases the autocrlf 
option was set to true or input)

Ferry

Re: CVS import [SOLVED]

[Jeff King]

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

Re: CVS import [SOLVED]

[Ferry Huberts (Pelagic)]

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)

Ferry

On Fri, February 20, 2009 16:28, Jeff King wrote:
> 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
>

autocrlf=input and safecrlf (was Re: CVS import [SOLVED])

[Jeff King]

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

Re: autocrlf=input and safecrlf (was Re: CVS import [SOLVED])

[Ferry Huberts (Pelagic)]

Jeff King wrote:
> 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
>   
yes that was a typo.
> 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 meant that I would add a patch that makes sure that a new repository 
is created with that option set to 'off' and that an existing repository 
would be checked for that option set to 'off'. I suggested to _also_ add 
remarks about this in the man page of cvsimport. Johannes already 
suggested a patch but that was for the autocrlf option (trivially 
converted to the safecrlf option)
> 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'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.
Your remark about corrupting your data is a bit strong for my taste. 
Corruption from one point of view, making sure that everybody handles 
the same content from another :-)

Ferry

Re: autocrlf=input and safecrlf (was Re: CVS import [SOLVED])

[Jeff King]

On Sat, Feb 21, 2009 at 12:24:11AM +0100, Ferry Huberts (Pelagic) wrote:

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

OK, so there is some value to that combination, then, I suppose. It
seems like there must be some easier and more obvious way to say "reject
all CRLFs", but I can't think of one besides setting up a hook (which
would work at commit time, not add time).

> Your remark about corrupting your data is a bit strong for my taste.  
> Corruption from one point of view, making sure that everybody handles the 
> same content from another :-)

I'm not sure you understood what I meant. What I meant is that for some
set of data, applying CRLF->LF conversion is lossy, and will permanently
destroy the ability to restore the original data. For example, arbitrary
binary data which contains both CRLF and LF will have all CRLF become
LF, but you don't know which of the resulting LFs were originally CRLFs,
and which were just LFs. The data is corrupted, there is no way to get
back the original, and this is what CRLF is about protecting.

However, that safecrlf check is implemented by saying "with the current
autocrlf settings, would checkin and checkout get the same file?". In
the case of autocrlf=true, that that exactly prevents the data above
from being corrupted. But with autocrlf=input, it prevents _any_ CRs
from being converted, since checkout will not convert them back. So even
though your data is not irretrievable (the transformation _is_
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 safecrlf=true
into their ~/.gitconfig and thinks they are safe.

-Peff

Re: autocrlf=input and safecrlf (was Re: CVS import [SOLVED])

[Ferry Huberts (Pelagic)]

Jeff King wrote:
> On Sat, Feb 21, 2009 at 12:24:11AM +0100, Ferry Huberts (Pelagic) wrote:
>
>>> 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.
>
> OK, so there is some value to that combination, then, I suppose. It seems like there must be some easier and more obvious way to say "reject all CRLFs", but I can't think of one besides setting up
a hook (which would work at commit time, not add time).
>
>> Your remark about corrupting your data is a bit strong for my taste.   Corruption from one point of view, making sure that everybody handles the  same content from another :-)
>
> I'm not sure you understood what I meant. What I meant is that for some set of data, applying CRLF->LF conversion is lossy, and will permanently destroy the ability to restore the original data.
For example, arbitrary binary data which contains both CRLF and LF will have all CRLF become LF, but you don't know which of the resulting LFs were originally CRLFs, and which were just LFs. The
data is corrupted, there is no way to get back the original, and this is what CRLF is about protecting.
>
> However, that safecrlf check is implemented by saying "with the current autocrlf settings, would checkin and checkout get the same file?". In the case of autocrlf=true, that that exactly prevents
the data above from being corrupted. But with autocrlf=input, it prevents _any_ CRs from being converted, since checkout will not convert them back. So even though your data is not irretrievable
(the transformation _is_
> 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
safecrlf=true into their ~/.gitconfig and thinks they are safe.
>
> -Peff

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.

Ferry

Re: autocrlf=input and safecrlf (was Re: CVS import [SOLVED])

[Jeff King]

[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

Re: autocrlf=input and safecrlf (was Re: CVS import [SOLVED])

[Ferry Huberts (Pelagic)]

On Mon, February 23, 2009 07:56, Jeff King wrote:
> [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
>

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.

I started these discussions to narrow down on what we should actually patch.
Now it appears that we should do what Johannes originally proposed. Always
nice to be right, isn't it Johannes? :-)

Ferry

Re: autocrlf=input and safecrlf (was Re: CVS import [SOLVED])

[Jeff King]

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.

-Peff

Re: autocrlf=input and safecrlf (was Re: CVS import [SOLVED])

[Ferry Huberts (Pelagic)]

On Mon, February 23, 2009 08:10, Jeff King wrote:
> 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.

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

Ferry

Re: autocrlf=input and safecrlf (was Re: CVS import [SOLVED])

[Jeff King]

On Mon, Feb 23, 2009 at 08:29:57AM +0100, Ferry Huberts (Pelagic) wrote:

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

OK, sorry I missed that before.

It actually works fine with CRLF files. It breaks on _LF_ files.  Look
again at the output you posted, which shows it barfing while working on
unix.txt.

This is the flip-side of the CRLF and autocrlf=input problem; safecrlf
is protecting us from the case where the file would change on checkout,
in addition to when it would actually be corrupted.

But both of those checks (CRLF on autocrlf=input and safecrlf=true, and
LF on autocrlf=output/true and safecrlf=true) aren't useful to us here;
we are not coming from the working tree to git, and worried about
getting back. We are munging input coming from cvs, and whatever gets
put into the working tree is fine (as long as it is not binary
corruption).

So I think the right solution is a relaxed safecrlf mode that protects
against corruption, but not these other cases. And then git-cvsimport
should use that.

In the meantime, detecting the situation is not a bad idea.

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

-Peff

Re: autocrlf=input and safecrlf (was Re: CVS import [SOLVED])

[Ferry Huberts (Pelagic)]

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

Re: autocrlf=input and safecrlf (was Re: CVS import [SOLVED])

[Jeff King]

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.

-Peff

Re: autocrlf=input and safecrlf (was Re: CVS import [SOLVED])

[Ferry Huberts (Pelagic)]

On Wed, February 25, 2009 07:56, Jeff King wrote:
> 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?
>

the option would be setup for the import repository only, not global nor system

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

Ferry

Re: autocrlf=input and safecrlf (was Re: CVS import [SOLVED])

[Jeff King]

On Wed, Feb 25, 2009 at 09:03:00AM +0100, Ferry Huberts (Pelagic) wrote:

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

OK, that sounds reasonable to me.

-Peff