[PATCH] git-cvsserver: added support for update -p

[PATCH] git-cvsserver: added support for update -p

[Jan Wielemaker]

[PATCH] git-cvsserver: added support for update -p
---
Hi,

Someone in our team uses "cvs update -p [-r rev] file" (somehow invoked
through TortoiseCVS). The patch below provides that. I think it is fine,
except that I don't know with wich other flags -p can be combined and
therefore when exactly this should be tested. Figured out that normal
CVS sends the file line-by-line preceeded by "M " using strace on the
client to a real CVS server.

	Enjoy --- Jan

 git-cvsserver.perl |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 13dbd27..987f4d6 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -956,6 +956,21 @@ sub req_update
             $meta = $updater->getmeta($filename);
         }
 
+	# if we have a -p we should just send the file
+        if ( exists ( $state->{opt}{p} ) )
+	{
+	    if ( open my $fh, '-|', "git-cat-file", "blob", $meta->{filehash} )
+	    {   while ( <$fh> )
+		{ print "M " . $_;
+		}
+		close $fh or die ("Couldn't close filehandle for transmitfile(): $!");
+	    } else
+	    { die("Couldn't execute git-cat-file");
+	    }
+
+	    next;
+	}
+
 	if ( ! defined $meta )
 	{
 	    $meta = {
-- 
1.5.3.4

Re: [PATCH] git-cvsserver: added support for update -p

[Johannes Schindelin]

Hi,

On Wed, 10 Oct 2007, Jan Wielemaker wrote:

> [PATCH] git-cvsserver: added support for update -p
> ---

Proposed alternative for the commit message:

-- snip --
The cvs subcommand "update -p <file>" is frequently used to see the 
contents of a given file in HEAD, sort of our "git show <file>".  It
is not that hard to support it, so here it is.

Commit-message-proposed-by: Johannes Schindelin <johannes.schindelin.de>
Signed-off-by: Jan Wielemaker <jan@swi-prolog.org>
-- snap --

Remember: having such a commit message already at the beginning of your 
mail body makes it easier to everyone reading your email, for a small 
cost (time) of just one person (you).

Ciao,
Dscho

P.S.: Have not reviewed the patch at all, so cannot say anything about the 
merits of it; will leave it to djpig ;-)

Re: [PATCH] git-cvsserver: added support for update -p

[Jan Wielemaker]

On Wednesday 10 October 2007 15:47, Johannes Schindelin wrote:
> Hi,
>
> On Wed, 10 Oct 2007, Jan Wielemaker wrote:
> > [PATCH] git-cvsserver: added support for update -p
> > ---
>
> Proposed alternative for the commit message:
>
> -- snip --
> The cvs subcommand "update -p <file>" is frequently used to see the
> contents of a given file in HEAD, sort of our "git show <file>".  It
> is not that hard to support it, so here it is.
>
> Commit-message-proposed-by: Johannes Schindelin <johannes.schindelin.de>
> Signed-off-by: Jan Wielemaker <jan@swi-prolog.org>
> -- snap --

Ok. I'm still a guy of ChangeLog files, which you generally needed for
CVS to keep track of a large project :-) As the CVS commit message
aren't much good anyway, I kept them short. Also for my own project I'm
considering to replace these with larger commit messages and drop the
ChangeLog files.

> P.S.: Have not reviewed the patch at all, so cannot say anything about the
> merits of it; will leave it to djpig ;-)

Don't trust my Perl; its just copy and intelligent(-ish) paste :-) Works
for me though and this isn't very complicated. Is there a test suite for
git-cvsserver?

	Cheers --- Jan

Re: [PATCH] git-cvsserver: added support for update -p

[Johannes Schindelin]

Hi,

On Wed, 10 Oct 2007, Jan Wielemaker wrote:

> Is there a test suite for git-cvsserver?

Yes: t/t9400-git-cvsserver-server.sh

Hth,
Dscho

Re: [PATCH] git-cvsserver: added support for update -p

[Jan Wielemaker]

> On Wed, 10 Oct 2007, Jan Wielemaker wrote:
> > Is there a test suite for git-cvsserver?
>
> Yes: t/t9400-git-cvsserver-server.sh

Thanks.  B.t.w. from the main directory:

gollem (git) 21_> make check
for i in *.c; do 
sparse -g -O2 -Wall  -DSHA1_HEADER='<openssl/sha.h>' -DETC_GITCONFIG='"/home/jan/etc/gitconfig"' -DNO_STRLCPY -D__BIG_ENDIAN__ -D__powerpc__ 
$i || exit; done
/bin/sh: sparse: command not found
make: *** [check] Error 127

Dunno, but maybe something like this is more appropriate:

	echo "See t/README for testing GIT"

	Cheers --- Jan

P.s.	My modified version passes all tests.

Re: [PATCH] git-cvsserver: added support for update -p

[Johannes Schindelin]

Hi,

On Wed, 10 Oct 2007, Jan Wielemaker wrote:

> > On Wed, 10 Oct 2007, Jan Wielemaker wrote:
> > > Is there a test suite for git-cvsserver?
> >
> > Yes: t/t9400-git-cvsserver-server.sh
> 
> Thanks.  B.t.w. from the main directory:
> 
> gollem (git) 21_> make check

make check is to check with the static code analyzer "sparse".

To test, try "make test".  Since this is so commonly used to test 
packages (for example, the vast majority of Perl packages have it), I do 
not see the need to put a message pointing to "make test" in the "check" 
target.

Ciao,
Dscho

Re: [PATCH] git-cvsserver: added support for update -p

[Frank Lichtenheld]

On Wed, Oct 10, 2007 at 01:16:03PM +0200, Jan Wielemaker wrote:
> +	# if we have a -p we should just send the file
> +        if ( exists ( $state->{opt}{p} ) )
> +	{
> +	    if ( open my $fh, '-|', "git-cat-file", "blob", $meta->{filehash} )
> +	    {   while ( <$fh> )
> +		{ print "M " . $_;
> +		}
> +		close $fh or die ("Couldn't close filehandle for transmitfile(): $!");
> +	    } else
> +	    { die("Couldn't execute git-cat-file");
> +	    }
> +
> +	    next;
> +	}


There seems to be inconsistent whitespace in the patch.
And please never do that else\n{ again, it hurts my eye ;)

Will try to test (and write a testcase for) it tomorrow. 

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

Re: [PATCH] git-cvsserver: added support for update -p

[Andreas Ericsson]

Frank Lichtenheld wrote:
> On Wed, Oct 10, 2007 at 01:16:03PM +0200, Jan Wielemaker wrote:
>> +	# if we have a -p we should just send the file
>> +        if ( exists ( $state->{opt}{p} ) )
>> +	{
>> +	    if ( open my $fh, '-|', "git-cat-file", "blob", $meta->{filehash} )
>> +	    {   while ( <$fh> )
>> +		{ print "M " . $_;
>> +		}
>> +		close $fh or die ("Couldn't close filehandle for transmitfile(): $!");
>> +	    } else
>> +	    { die("Couldn't execute git-cat-file");
>> +	    }
>> +
>> +	    next;
>> +	}
> 
> 
> There seems to be inconsistent whitespace in the patch.
> And please never do that else\n{ again, it hurts my eye ;)
> 

That cuddled opening brace hurts mine more.

{ while()\n{ print()...

It's usually a good idea to pick some indentation style that at least *some* tool
can create, and when contributing to a project it's usually considered good form
to stick to the style already used.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

[PATCH] cvsserver: added support for update -p

[Frank Lichtenheld]

Based on a patch by Jan Wielemaker <jan@swi-prolog.org>.

Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
---
 git-cvsserver.perl              |   23 +++++++++++++++++++++++
 t/t9400-git-cvsserver-server.sh |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 0 deletions(-)

 Test cases added and fixed behaviour for non-existant files.

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 2e112fa..7374875 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -973,6 +973,29 @@ sub req_update
             $meta = $updater->getmeta($filename);
         }
 
+	# if we have a -p we should just send the file
+	if ( exists ( $state->{opt}{p} ) )
+	{
+	    if (! defined $meta)
+	    {
+		# non-existant files are ignored
+		print "E cvs update: nothing known about `$filename'\n";
+		next;
+	    }
+	    if ( open my $fh, '-|', "git-cat-file", "blob", $meta->{filehash} )
+	    {
+		while ( <$fh> )
+		{
+		    print "M $_";
+		}
+		close $fh or die ("Couldn't close filehandle: $!");
+	    } else {
+		die("Couldn't execute git-cat-file");
+	    }
+
+	    next;
+	}
+
 	if ( ! defined $meta )
 	{
 	    $meta = {
diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index ee58c0f..4f45578 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -446,6 +446,38 @@ test_expect_success 'cvs update (merge no-op)' \
     diff -q merge ../merge'
 
 cd "$WORKDIR"
+test_expect_success 'cvs update (-p)' \
+  'cd cvswork &&
+   GIT_CONFIG="$git_config" cvs -Q update -p merge non-existant testfile1 empty >log &&
+   cat merge testfile1 empty >../expected &&
+   diff -q log ../expected'
+
+cd "$WORKDIR"
+test_expect_success 'cvs update (-p -r)' \
+  'echo testfile1 >expected &&
+   for i in 1 2 3 4 5 6 7
+   do
+     echo Line $i >>expected
+   done &&
+   echo >>expected &&
+   cd cvswork &&
+   GIT_CONFIG="$git_config" cvs -Q update -p -r1.1 testfile1 merge empty >log &&
+   diff -q log ../expected'
+
+cd "$WORKDIR"
+test_expect_success 'cvs update (-p unclean and out-of-date)' \
+  'echo testfile2 >testfile2 &&
+   echo Line 10 >>merge &&
+   git add testfile2 merge &&
+   git commit -q -m "update -p" &&
+   git push gitcvs.git >/dev/null &&
+   cat testfile2 merge >expected &&
+   cd cvswork &&
+   echo "Line 10 workdir" >>merge
+   GIT_CONFIG="$git_config" cvs -Q update -p testfile2 merge >log &&
+   diff -q log ../expected'
+
+cd "$WORKDIR"
 cat <<EOF >list-modules-cmd
 Root $SERVERDIR
 Valid-responses ok error Valid-requests Force-gzip Referrer Redirect Checked-in New-entry Checksum Copy-file Updated Created Update-existing Merged Patched Rcs-diff Mode Mod-time Removed Remove-entry Set-static-directory Clear-static-directory Set-sticky Clear-sticky Edit-file Template Clear-template Notified Module-expansion Wrapper-rcsOption M Mbinary E F MT
-- 
1.5.3.4

Re: [PATCH] cvsserver: added support for update -p

[Jan Wielemaker]

On Thursday 11 October 2007 18:36, Frank Lichtenheld wrote:
> Based on a patch by Jan Wielemaker <jan@swi-prolog.org>.
>
> Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>

Thanks. You are a bigger Perl programmer than I :-) Are you also
interested in one that makes "cvs diff -c" work?  It works, but it
does not handle things like "cvs diff -C 5" and I'm a bit lost in
Perl-space ...  If someone knowing more about the server wants to
have a look, I'm happy to post the part I have.

	Cheers --- Jan

Re: [PATCH] cvsserver: added support for update -p

[Frank Lichtenheld]

On Thu, Oct 11, 2007 at 06:52:32PM +0200, Jan Wielemaker wrote:
> On Thursday 11 October 2007 18:36, Frank Lichtenheld wrote:
> > Based on a patch by Jan Wielemaker <jan@swi-prolog.org>.
> >
> > Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
> 
> Thanks. You are a bigger Perl programmer than I :-) Are you also
> interested in one that makes "cvs diff -c" work?  It works, but it
> does not handle things like "cvs diff -C 5" and I'm a bit lost in
> Perl-space ...  If someone knowing more about the server wants to
> have a look, I'm happy to post the part I have.

Hmm, the more half-patches you submit the more I'd rather prefer you
learning Perl ;) Or at least write your own testcases.

diff -c doesn't really interest me at all. So I'd really prefer you
doing the bulk of the work...

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

Re: [PATCH] cvsserver: added support for update -p

[Johannes Schindelin]

Hi,

On Thu, 11 Oct 2007, Frank Lichtenheld wrote:

> +	if ( exists ( $state->{opt}{p} ) )

I see you kept the coding style, which is not in agreement with the rest 
of git...  Intention or oversight?

Ciao,
Dscho

Re: [PATCH] cvsserver: added support for update -p

[Frank Lichtenheld]

On Thu, Oct 11, 2007 at 09:59:28PM +0100, Johannes Schindelin wrote:
> On Thu, 11 Oct 2007, Frank Lichtenheld wrote:
> 
> > +	if ( exists ( $state->{opt}{p} ) )
> 
> I see you kept the coding style, which is not in agreement with the rest 
> of git...  Intention or oversight?

It is in agreement with the rest of git-cvsserver. I really like the
style of the other perl stuff in git better, but I wasn't sure what
style takes precedence...

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