[PATCH] cvsserver: fix legacy cvs client and branch rev issues

[PATCH] cvsserver: fix legacy cvs client and branch rev issues

[Dirk Koopman]

Early cvs clients don't cause state->{args} to be initialised,
so force this to occur.
Some revision checking code assumes that revisions will be
recognisably numeric to perl, Branches are not, because they
have more decimal points (eg 1.2.3.4 instead of just 1.2).
---
 git-cvsserver.perl |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 5cbf27e..0a4b75e 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -1813,11 +1813,14 @@ sub req_annotate
 # the second is $state->{files} which is everything after it.
 sub argsplit
 {
+    $state->{args} = [];        # need this here because later code depends on it
+                                # and for some reason earlier versions of CVS don't
+                                # satisfy the next condition on plain 'cvs update'
+
     return unless( defined($state->{arguments}) and ref $state->{arguments} eq "ARRAY" );
 
     my $type = shift;
 
-    $state->{args} = [];
     $state->{files} = [];
     $state->{opt} = {};
 
@@ -1906,11 +1909,13 @@ sub argsfromdir
 
     # push added files
     foreach my $file (keys %{$state->{entries}}) {
-	if ( exists $state->{entries}{$file}{revision} &&
-		$state->{entries}{$file}{revision} == 0 )
-	{
-	    push @gethead, { name => $file, filehash => 'added' };
-	}
+        # remember that revisions could be on branches 1.2.3.4[.5.6..]
+        # not just a recogisable "numeric" 1.2
+        if ( exists $state->{entries}{$file}{revision} &&
+             !$state->{entries}{$file}{revision} )
+        {
+            push @gethead, { name => $file, filehash => 'added' };
+        }
     }
 
     if ( scalar(@{$state->{args}}) == 1 )
-- 
1.5.2.1

Re: [PATCH] cvsserver: fix legacy cvs client and branch rev issues

[Frank Lichtenheld]

Hi.

On Sat, Jun 16, 2007 at 07:50:06PM +0100, Dirk Koopman wrote:
> Early cvs clients don't cause state->{args} to be initialised,
> so force this to occur.
> Some revision checking code assumes that revisions will be
> recognisably numeric to perl, Branches are not, because they
> have more decimal points (eg 1.2.3.4 instead of just 1.2). 
> ---
>  git-cvsserver.perl |   17 +++++++++++------
>  1 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index 5cbf27e..0a4b75e 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -1813,11 +1813,14 @@ sub req_annotate
>  # the second is $state->{files} which is everything after it.
>  sub argsplit
>  {
> +    $state->{args} = [];        # need this here because later code depends on it
> +                                # and for some reason earlier versions of CVS don't
> +                                # satisfy the next condition on plain 'cvs update'
> +
>      return unless( defined($state->{arguments}) and ref $state->{arguments} eq "ARRAY" );
>  
>      my $type = shift;
>  
> -    $state->{args} = [];
>      $state->{files} = [];
>      $state->{opt} = {};
>  

I just would move all the initializations up there. And I think the
comment is really unnecessary. Will prepare a replacement patch.

> @@ -1906,11 +1909,13 @@ sub argsfromdir
>  
>      # push added files
>      foreach my $file (keys %{$state->{entries}}) {
> -	if ( exists $state->{entries}{$file}{revision} &&
> -		$state->{entries}{$file}{revision} == 0 )
> -	{
> -	    push @gethead, { name => $file, filehash => 'added' };
> -	}
> +        # remember that revisions could be on branches 1.2.3.4[.5.6..]
> +        # not just a recogisable "numeric" 1.2
> +        if ( exists $state->{entries}{$file}{revision} &&
> +             !$state->{entries}{$file}{revision} )
> +        {
> +            push @gethead, { name => $file, filehash => 'added' };
> +        }
>      }
>  
>      if ( scalar(@{$state->{args}}) == 1 )

Hmm, I don't see how you could have a problem with that since cvsserver
doesn't support branches and never generates any revision numbers in
that format?

There is probably much more code out there in cvsserver that does assume
that revision is always a simple integer.

And again that comment is a but much IMHO.

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [PATCH] cvsserver: fix legacy cvs client and branch rev issues

[Dirk Koopman]

Frank Lichtenheld wrote:
> Hi.
> 
> On Sat, Jun 16, 2007 at 07:50:06PM +0100, Dirk Koopman wrote:
>> Early cvs clients don't cause state->{args} to be initialised,
>> so force this to occur.
>> Some revision checking code assumes that revisions will be
>> recognisably numeric to perl, Branches are not, because they
>> have more decimal points (eg 1.2.3.4 instead of just 1.2). 

<snip>

> 
> Hmm, I don't see how you could have a problem with that since cvsserver
> doesn't support branches and never generates any revision numbers in
> that format?
> 
> There is probably much more code out there in cvsserver that does assume
> that revision is always a simple integer.
> 
> And again that comment is a but much IMHO.
> 

The specific issue that I was trying to solve is that I have (in CVS 
terms) a main line (git head: master) and an active CVS development 
branch and git head (called SR [for the sake of argument]).

I have imported both into git using cvsimport. For compatibility (and 
windows users) I need a anonymous, read only, :pserver: CVS 
implementation that can serve either head.

The version numbers in the CVS import on branch SR are standard CVS 
single level branch 1.2.3.4. Doing a 'cvs update' on this branch was 
causing all sorts of warnings about 1.2.3.4 not being numeric on that 
test. After changing the test, the warnings have gone away and it all 
still seems to work.

Having said that, I haven't worked out where cvsserver is getting those 
version numbers from in the first place, but it obviously knows that it 
is dealing with a branch sufficient to work well enough for my needs.

Of course, quite what happens when the branch merges back and people 
want to 'cvs update -A', I shall leave for the future...

Groetjes  Dirk

Re: [PATCH] cvsserver: fix legacy cvs client and branch rev issues

[Frank Lichtenheld]

On Sun, Jun 17, 2007 at 10:10:51AM +0100, Dirk Koopman wrote:
> Frank Lichtenheld wrote:
> >On Sat, Jun 16, 2007 at 07:50:06PM +0100, Dirk Koopman wrote:
> >Hmm, I don't see how you could have a problem with that since cvsserver
> >doesn't support branches and never generates any revision numbers in
> >that format?
> >
> >There is probably much more code out there in cvsserver that does assume
> >that revision is always a simple integer.

Let me rephrase that (after actually looking through the code):
All of the revision handling code assumes that.

> The specific issue that I was trying to solve is that I have (in CVS 
> terms) a main line (git head: master) and an active CVS development 
> branch and git head (called SR [for the sake of argument]).
> 
> I have imported both into git using cvsimport. For compatibility (and 
> windows users) I need a anonymous, read only, :pserver: CVS 
> implementation that can serve either head.
> 
> The version numbers in the CVS import on branch SR are standard CVS 
> single level branch 1.2.3.4. Doing a 'cvs update' on this branch was 
> causing all sorts of warnings about 1.2.3.4 not being numeric on that 
> test. After changing the test, the warnings have gone away and it all 
> still seems to work.
>
> Having said that, I haven't worked out where cvsserver is getting those 
> version numbers from in the first place, but it obviously knows that it 
> is dealing with a branch sufficient to work well enough for my needs.

Hmm, so you did the cvs update in an old working copy of the original
CVS repository? Then CVS sent those version numbers from the CVS/Entries
file to the server, cvsserver certainly never generates numbers like
that. And I would be very suprised if you could do anything remotely
useful with abusing the old working copy this way... The revision
numbers that cvsserver assigns to the files of the main branch might
be almost always identical to the ones they had in CVS before the
import, but the ones for branches will definetly not be.

> Of course, quite what happens when the branch merges back and people 
> want to 'cvs update -A', I shall leave for the future...

I don't think that cvsserver actually cares about what the client sends
as sticky tags/dates/..., so it might not actually change anything
whether you use -A or not (pure speculation on my part here).

Summary: You're (ab)using cvsserver in very interesting ways that are not
really beeing thought of in the current design/implementation. There'll
be dragons ;)

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [PATCH] cvsserver: fix legacy cvs client and branch rev issues

[Dirk Koopman]

Frank Lichtenheld wrote:

> 
> Summary: You're (ab)using cvsserver in very interesting ways that are not
> really beeing thought of in the current design/implementation. There'll
> be dragons ;)
> 

Hmm... I think that is becoming clear. The trouble is that I am not at 
all certain that what I am doing is particularly unusual. After all, 
using git, the whole point is that working on branches or the main line 
should easy and cheap!

If it were me, I might have been inclined to always set Repository to 
'master' (or even to the name of the repository with .git removed), then 
git checkout <tag> <file> each file, one at a time, using the (<tag> || 
'master') from each Entry that is sent. So with no tag, you get the 
master copy, otherwise the <tag>ged copy - this all assuming that the 
git repo is set up correctly.

But as I am CVS read only, what is there does for me so I am not 
complaining :-) The two people that can also commit can start to use git 
and send me patches... Do them good :-)

Dirk

Re: [PATCH] cvsserver: fix legacy cvs client and branch rev issues

[Frank Lichtenheld]

On Sun, Jun 17, 2007 at 05:53:27PM +0100, Dirk Koopman wrote:
> Frank Lichtenheld wrote:
> >Summary: You're (ab)using cvsserver in very interesting ways that are not
> >really beeing thought of in the current design/implementation. There'll
> >be dragons ;)
> >
> 
> Hmm... I think that is becoming clear. The trouble is that I am not at 
> all certain that what I am doing is particularly unusual. After all, 
> using git, the whole point is that working on branches or the main line 
> should easy and cheap!

Sure, it is a know limitation of cvsserver. But it is not trivially
to remove. Patches welcome ;)

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [PATCH] cvsserver: fix legacy cvs client and branch rev issues

[Martin Langhoff]

On 6/17/07, Frank Lichtenheld <frank@lichtenheld.de> wrote:
> On Sun, Jun 17, 2007 at 10:10:51AM +0100, Dirk Koopman wrote:
> > Frank Lichtenheld wrote:
> > >On Sat, Jun 16, 2007 at 07:50:06PM +0100, Dirk Koopman wrote:
> > >Hmm, I don't see how you could have a problem with that since cvsserver
> > >doesn't support branches and never generates any revision numbers in
> > >that format?
> > >
> > >There is probably much more code out there in cvsserver that does assume
> > >that revision is always a simple integer.
>
> Let me rephrase that (after actually looking through the code):
> All of the revision handling code assumes that.

Exactly. cvsserver emulates CVS on a single HEAD, that's why you use
the headname as the 'module' parameter you pass to CVS when doing a
checkout.

...

> Hmm, so you did the cvs update in an old working copy of the original
> CVS repository? Then CVS sent those version numbers from the CVS/Entries
> file to the server, cvsserver certainly never generates numbers like
> that. And I would be very suprised if you could do anything remotely
> useful with abusing the old working copy this way...

Agreed - that's not really supported.

Now, I'd _love_ to have a bit of time to implement CVS-style branch
support to cvsserver (so a check for valid version numbers that have
more dots would be a good thing), but it's hard hard hard, specially
because there are many ambiguities to resolve. It would enormously
useful to have branch support together with support for a bit of
"version skew" so that you can replace a real CVS server with
cvsserver and have people continue using the old cvs checkouts --
because the file versions and branches match.

As things stand, I want to say thanks to Frank for giving cvsserver
some love :-)

cheers,


martin

[PATCH] cvsserver: always initialize state in argsplit()

[Frank Lichtenheld]

Other code assumes that this is initialized, so do it
even if there were no arguments given.

Signed-off-by: Dirk Koopman <djk@tobit.co.uk>
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
---
 git-cvsserver.perl |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

 Hrm, sorry to Dirk for the double mail. This time actually
 send to the list and not to git@localhost ...

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 5cbf27e..10aba50 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -1813,14 +1813,14 @@ sub req_annotate
 # the second is $state->{files} which is everything after it.
 sub argsplit
 {
-    return unless( defined($state->{arguments}) and ref $state->{arguments} eq "ARRAY" );
-
-    my $type = shift;
-
     $state->{args} = [];
     $state->{files} = [];
     $state->{opt} = {};
 
+    return unless( defined($state->{arguments}) and ref $state->{arguments} eq "ARRAY" );
+
+    my $type = shift;
+
     if ( defined($type) )
     {
         my $opt = {};
-- 
1.5.2.1