threads / patch / 8617

patchcvsserver: fix legacy cvs client and branch rev issues

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

## tl;dr

8 messages between Jun 16, 2007 and Jun 17, 2007. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Dirk Koopman· Jun 16, 2007, 18:50 UTC · lore
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(-)
Show changes to git-cvsserver.perl +11 −6
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
Frank Lichtenheld· Jun 17, 2007, 08:19 UTC · re: Dirk Koopman · lore

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

Hi.
On Sat, Jun 16, 2007 at 07:50:06PM +0100, Dirk Koopman wrote:
Show 29 quoted lines
> 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.

Show 19 quoted lines
> @@ -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/
Dirk Koopman· Jun 17, 2007, 09:10 UTC · re: Frank Lichtenheld · lore

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

Frank Lichtenheld wrote:
Show 8 quoted lines
> 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>
Show 10 quoted lines
> 
> 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
Frank Lichtenheld· Jun 17, 2007, 10:37 UTC · re: Dirk Koopman · lore

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

On Sun, Jun 17, 2007 at 10:10:51AM +0100, Dirk Koopman wrote:
Show 8 quoted lines
> 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.

Show 17 quoted lines
> 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/
Dirk Koopman· Jun 17, 2007, 16:53 UTC · re: Frank Lichtenheld · lore

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

Frank Lichtenheld wrote:
Show 5 quoted lines
> 
> 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
Frank Lichtenheld· Jun 17, 2007, 17:20 UTC · re: Dirk Koopman · lore

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

On Sun, Jun 17, 2007 at 05:53:27PM +0100, Dirk Koopman wrote:
Show 10 quoted lines
> 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/
Martin Langhoff· Jun 17, 2007, 21:27 UTC · re: Frank Lichtenheld · lore

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

On 6/17/07, Frank Lichtenheld <frank@lichtenheld.de> wrote:
Show 12 quoted lines
> 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.

...
Show 5 quoted lines
> 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
Frank Lichtenheld· Jun 17, 2007, 08:31 UTC · re: Dirk Koopman · lore

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

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 ...
Show changes to git-cvsserver.perl +4 −4
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

← back to recent threads