[PATCH] Add -k kill keyword expansion option to git-cvsimport

[PATCH] Add -k kill keyword expansion option to git-cvsimport

[Martin Langhoff]

[PATCH] Add -k kill keyword expansion option to git-cvsimport

Early versions of git-cvsimport defaulted to using preexisting keyword
expansion settings. This change preserves compatibility with existing cvs
imports and allows new repository migrations to kill keyword expansion.

Should improve our chances of detecting merges and reduce imported
repository size.

Signed-off: Martin Langhoff <martin.langhoff@gmail.com>
---

 Documentation/git-cvsimport-script.txt |    7 ++++++-
 git-cvsimport-script                   |   10 ++++++----
 2 files changed, 12 insertions(+), 5 deletions(-)

f0b33908e48ac75a1e3f08a3e3159de3a9a371d9
diff --git a/Documentation/git-cvsimport-script.txt
b/Documentation/git-cvsimport-script.txt
--- a/Documentation/git-cvsimport-script.txt
+++ b/Documentation/git-cvsimport-script.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 --------
 'git-cvsimport-script' [ -o <branch-for-HEAD> ] [ -h ] [ -v ]
                       [ -d <CVSROOT> ] [ -p <options-for-cvsps> ]
-                       [ -C <GIT_repository> ] [ -i ] [ <CVS_module> ]
+                       [ -C <GIT_repository> ] [ -i ] [ -k ] [ <CVS_module> ]


 DESCRIPTION
@@ -34,6 +34,11 @@ OPTIONS
       ensures the working directory and cache remain untouched and will
       not create them if they do not exist.

+-k::
+       Kill keywords: will extract files with -ko from the CVS archive
+       to avoid noisy changesets. Highly recommended, but off by default
+       to preserve compatibility with early imported trees.
+
 -o <branch-for-HEAD>::
       The 'HEAD' branch from CVS is imported to the 'origin' branch within
       the git repository, as 'HEAD' already has a special meaning for git.
diff --git a/git-cvsimport-script b/git-cvsimport-script
--- a/git-cvsimport-script
+++ b/git-cvsimport-script
@@ -35,12 +35,12 @@ sub usage() {
 Usage: ${\basename $0}     # fetch/update GIT from CVS
       [ -o branch-for-HEAD ] [ -h ] [ -v ] [ -d CVSROOT ]
       [ -p opts-for-cvsps ] [ -C GIT_repository ] [ -z fuzz ]
-       [ -i ] [ CVS_module ]
+       [ -i ] [ -k ] [ CVS_module ]
 END
       exit(1);
 }

-getopts("hivo:d:p:C:z:") or usage();
+getopts("hivko:d:p:C:z:") or usage();
 usage if $opt_h;

 @ARGV <= 1 or usage();
@@ -218,8 +218,10 @@ sub _file {
       my($self,$fn,$rev) = @_;
       $self->{'socketo'}->write("Argument -N\n") or return undef;
       $self->{'socketo'}->write("Argument -P\n") or return undef;
-       # $self->{'socketo'}->write("Argument -ko\n") or return undef;
-       # -ko: Linus' version doesn't use it
+       # -ko: Linus' version doesn't use it - defaults to off
+       if ($opt_k) {
+           $self->{'socketo'}->write("Argument -ko\n") or return undef;
+       }
       $self->{'socketo'}->write("Argument -r\n") or return undef;
       $self->{'socketo'}->write("Argument $rev\n") or return undef;
       $self->{'socketo'}->write("Argument --\n") or return undef;

Re: [PATCH] Add -k kill keyword expansion option to git-cvsimport

[Martin Langhoff]

On 8/15/05, Martin Langhoff <martin.langhoff@gmail.com> wrote:
> [PATCH] Add -k kill keyword expansion option to git-cvsimport

 Bad patch! Please ignore while I fix and resend... 

apologies.

martin

[PATCH] Add -k kill keyword expansion option to git-cvsimport

[Martin Langhoff]

Early versions of git-cvsimport defaulted to using preexisting keyword
expansion settings. This change preserves compatibility with existing cvs
imports and allows new repository migrations to kill keyword expansion.

Should improve our chances of detecting merges and reduce imported
repository size.

Signed-off: Martin Langhoff <martin.langhoff@gmail.com>
---

 Documentation/git-cvsimport-script.txt |    7 ++++++-
 git-cvsimport-script                   |   12 +++++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

68d02ed3485e389315f33ab6387c0f1fc028b255
diff --git a/Documentation/git-cvsimport-script.txt
b/Documentation/git-cvsimport-script.txt
--- a/Documentation/git-cvsimport-script.txt
+++ b/Documentation/git-cvsimport-script.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 --------
 'git-cvsimport-script' [ -o <branch-for-HEAD> ] [ -h ] [ -v ]
 			[ -d <CVSROOT> ] [ -p <options-for-cvsps> ]
-			[ -C <GIT_repository> ] [ -i ] [ <CVS_module> ]
+			[ -C <GIT_repository> ] [ -i ] [ -k ] [ <CVS_module> ]
 
 
 DESCRIPTION
@@ -34,6 +34,11 @@ OPTIONS
 	ensures the working directory and cache remain untouched and will
 	not create them if they do not exist.
 
+-k::
+	Kill keywords: will extract files with -ko from the CVS archive
+	to avoid noisy changesets. Highly recommended, but off by default
+	to preserve compatibility with early imported trees. 
+
 -o <branch-for-HEAD>::
 	The 'HEAD' branch from CVS is imported to the 'origin' branch within
 	the git repository, as 'HEAD' already has a special meaning for git.
diff --git a/git-cvsimport-script b/git-cvsimport-script
--- a/git-cvsimport-script
+++ b/git-cvsimport-script
@@ -28,19 +28,19 @@ use POSIX qw(strftime dup2);
 $SIG{'PIPE'}="IGNORE";
 $ENV{'TZ'}="UTC";
 
-our($opt_h,$opt_o,$opt_v,$opt_d,$opt_p,$opt_C,$opt_z,$opt_i);
+our($opt_h,$opt_o,$opt_v,$opt_k,$opt_d,$opt_p,$opt_C,$opt_z,$opt_i);
 
 sub usage() {
 	print STDERR <<END;
 Usage: ${\basename $0}     # fetch/update GIT from CVS
        [ -o branch-for-HEAD ] [ -h ] [ -v ] [ -d CVSROOT ]
        [ -p opts-for-cvsps ] [ -C GIT_repository ] [ -z fuzz ]
-       [ -i ] [ CVS_module ]
+       [ -i ] [ -k ] [ CVS_module ]
 END
 	exit(1);
 }
 
-getopts("hivo:d:p:C:z:") or usage();
+getopts("hivko:d:p:C:z:") or usage();
 usage if $opt_h;
 
 @ARGV <= 1 or usage();
@@ -218,8 +218,10 @@ sub _file {
 	my($self,$fn,$rev) = @_;
 	$self->{'socketo'}->write("Argument -N\n") or return undef;
 	$self->{'socketo'}->write("Argument -P\n") or return undef;
-	# $self->{'socketo'}->write("Argument -ko\n") or return undef;
-	# -ko: Linus' version doesn't use it
+	# -ko: Linus' version doesn't use it - defaults to off
+	if ($opt_k) {
+	    $self->{'socketo'}->write("Argument -ko\n") or return undef;
+	}
 	$self->{'socketo'}->write("Argument -r\n") or return undef;
 	$self->{'socketo'}->write("Argument $rev\n") or return undef;
 	$self->{'socketo'}->write("Argument --\n") or return undef;

Re: [PATCH] Add -k kill keyword expansion option to git-cvsimport

[Junio C Hamano]

Martin Langhoff <martin.langhoff@gmail.com> writes:

> [PATCH] Add -k kill keyword expansion option to git-cvsimport
>
> Early versions of git-cvsimport defaulted to using preexisting keyword
> expansion settings. This change preserves compatibility with existing cvs
> imports and allows new repository migrations to kill keyword expansion.
>
> Should improve our chances of detecting merges and reduce imported
> repository size.

The discussion between you and Linus since you brought this up
has kept me wondering if -ko is the only thing people may want
to do, or sometimes -kk or even -kb or -kv make sense for some
others, in which case instead of a -k option that does not allow
anything but -ko, making it take an optional single letter
o/k/b/v might might more sense.  A single -k defaulting to -ko
is fine by me if you did so, because I think that is the most
useful and usual mode of operation while converting to GIT
repository.

Thoughts?

Re: [PATCH] Add -k kill keyword expansion option to git-cvsimport

[Martin Langhoff]

On 8/15/05, Junio C Hamano <junkio@cox.net> wrote:
> The discussion between you and Linus since you brought this up
> has kept me wondering if -ko is the only thing people may want
> to do, or sometimes -kk or even -kb or -kv make sense for some

The git-cvsimport script requests the full file at a given revision
straight from the cvs server daemon it has instantiated. So I suspect
we are mixing up cvs client flags with protocol flags. Hmm, reading up
on it  ( http://www.elegosoft.com/cvs/cvsclient.html#SEC9 ) the flags
are carried through, and it sounds like it's pretty broken.

Still, it clearly leaves handling of newlines to the client. The
difference between -kb and -ko (we are using -ko ATM) is the newline
handling if you are using standard cvs clients. With git-cvsimport, we
are doing the unix thing, which happens to be right thing to do.

Perhaps repos created with early versions of CVSNT are broken in this
regard, but tough.

I think -kv is just the wrong thing to do if you are migrating to git.
Anyway, this script has so far followed cvs's own default... which is
-kv, and I am generally unwilling to break backwards compatibility.
Though we could make it default to 'on' and provide '-K' for those
masochistic enough to want it.

People with repos where cvswrappers was set to mark files as -kb or
-ko are safe from all this pain and tears.

cheers,


martin

Re: [PATCH] Add -k kill keyword expansion option to git-cvsimport

[Junio C Hamano]

Martin Langhoff <martin.langhoff@gmail.com> writes:

> I think -kv is just the wrong thing to do if you are migrating to git.
> Anyway, this script has so far followed cvs's own default... which is
> -kv, and I am generally unwilling to break backwards compatibility.

Isn't cvs default -kkv?

Re: [PATCH] Add -k kill keyword expansion option to git-cvsimport

[Martin Langhoff]

On 8/15/05, Junio C Hamano <junkio@cox.net> wrote:
> Isn't cvs default -kkv?

You're right, default is -kkv (expand keyword/value every time) and
not -kv (expand keyword/value only if previously unexpanded).

There's something else in the -kb / -ko distinction according to the
protocol description I linked before. Using -kb changes the way the
file is sent, and this may well break our protocol handling, which is
currently quite happy to deal with the file as a series of lines.

OTOH I am not sure if it breaks when dealing with true binary stuff
like images, compressed data, etc.

cheers,


martin

Re: [PATCH] Add -k kill keyword expansion option to git-cvsimport

[Junio C Hamano]

Martin Langhoff <martin.langhoff@gmail.com> writes:

>> ..., in which case instead of a -k option that does not allow
>> anything but -ko, making it take an optional single letter
>> o/k/b/v might might more sense.  A single -k defaulting to -ko
>> is fine by me if you did so, because I think that is the most
>> useful and usual mode of operation while converting to GIT
>> repository.

> Anyway, this script has so far followed cvs's own default... which is
> -kv, and I am generally unwilling to break backwards compatibility.

I realize what I wrote was prone to be misunderstood.  What I
meant about "single -k defaulting to -ko" was this:

    cvsimport without -k<any> => cvs default
    cvsimport -k == cvsimport -ko => use cvs -ko
    cvsimoprt -kv => use cvs -kv

Re: [PATCH] Add -k kill keyword expansion option to git-cvsimport

[Junio C Hamano]

Martin Langhoff <martin.langhoff@gmail.com> writes:

> Do you want just -kv or you'd like to handle all the modes?

No, I do not.

I was just wondering if we are limiting options for people who
want to convert their own CVS repositories by always using
either -kkv or -ko and nothing else.  Your simply saying "I do
not think so, -ko is what makes the most sense, and any other
option does not make any sense, but we used to do -kkv so let's
leave that as the default and have a -k option that does -ko" is
enough for me.

It is getting late for me so I'll merge it and push it out
tomorrow when I find time; it will be my day-job day.

Re: [PATCH] Add -k kill keyword expansion option to git-cvsimport

[Martin Langhoff]

On 8/15/05, Junio C Hamano <junkio@cox.net> wrote:
> I was just wondering if we are limiting options for people who
> want to convert their own CVS repositories by always using
> either -kkv or -ko and nothing else. 

I think the other modes are relevant in different scenarios. -kv is
only meaningful as file mode over the life of the file in the repo.
-kk is only meaningful when calling cvs update with -j -j parameters
or cvs diff, and is effectively a synonim of -ko.

In the position we are, getting file/revisions out of a repo, there
are 2 possible files we can get: the one that you'll get with -kkv and
the one you'll get with -ko/-kb. -kb/-ko should give us exactly the
same file, modulo bugs.

I suspect that in practice -kb is more reliable when it comes to
binary files. But to support that the _files() method will need to
handle a slightly different protocol mode on the socket, and I rather
not mess with it unless I can prove its broken. Talking with cvs
servers on the socket is not my idea of fun, and there's all sorts of
version-specific oddities.

cheers,


martin

Re: [PATCH] Add -k kill keyword expansion option to git-cvsimport

[Martin Langhoff]

On 8/15/05, Martin Langhoff <martin.langhoff@gmail.com> wrote:
> I think the other modes are relevant in different scenarios. -kv is
> only meaningful as file mode over the life of the file in the repo.
> -kk is only meaningful when calling cvs update with -j -j parameters
> or cvs diff, and is effectively a synonim of -ko.
> 
> In the position we are, getting file/revisions out of a repo, there
> are 2 possible files we can get: the one that you'll get with -kkv and
> the one you'll get with -ko/-kb. -kb/-ko should give us exactly the
> same file, modulo bugs.

After a few more trial runs, it ends up being that -kb and -ko drop
the ball in some instances, and the most reliable flag to send is -kk.
Don't ask me how or why.

So this patch is obsolete too. 



martin

Re: [PATCH] Add -k kill keyword expansion option to git-cvsimport

[Junio C Hamano]

Martin Langhoff <martin.langhoff@gmail.com> writes:

> So this patch is obsolete too. 

I take it to mean that it should be dropped and replaced with
the one you sent today with -kk change.

However, the -kk change one is a corrupted patch and does not
apply.  Your MUA ate leading whitespaces, perhaps.

I have already slurped in other two patches to cvsimport in the
proposed updates branch, so could you kindly proofread them (I
am no expert on cvs networking protocol issues) and rebase the
-kk patch, and send it without whitespace corruption this time
around please?

-jc

Re: [PATCH] Add -k kill keyword expansion option to git-cvsimport

[Martin Langhoff]

On 8/16/05, Junio C Hamano <junkio@cox.net> wrote:
> However, the -kk change one is a corrupted patch and does not
> apply.  Your MUA ate leading whitespaces, perhaps.

I stupidly did a forward. Rebased to your current pu branch and sent.
From now on I'll be sending straight from cmdline.
 
> I have already slurped in other two patches to cvsimport in the
> proposed updates branch, so could you kindly proofread them (I
> am no expert on cvs networking protocol issues)

Proofread. I don't claim to have understood 100% but it made sense.
Most importantly, it imported several repos perfectly.

cheers,


martin