threads / patch / 10230

patchgit-cvsserver: added support for update -p

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

## tl;dr

13 messages between Oct 10, 2007 and Oct 11, 2007. Diffs are folded; open one to read it.

replies: 12people: 5as markdown or json

Jan Wielemaker· Oct 10, 2007, 11:16 UTC · lore

[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(-)
Show changes to git-cvsserver.perl +15 −0
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
Johannes Schindelin· Oct 10, 2007, 13:47 UTC · re: Jan Wielemaker · lore

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

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

Jan Wielemaker· Oct 10, 2007, 14:26 UTC · re: Johannes Schindelin · lore

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

On Wednesday 10 October 2007 15:47, Johannes Schindelin wrote:
Show 16 quoted lines
> 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
Jan Wielemaker· Oct 10, 2007, 17:27 UTC · re: Johannes Schindelin · lore

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

> 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.
Johannes Schindelin· Oct 10, 2007, 19:27 UTC · re: Jan Wielemaker · lore

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

Hi,
On Wed, 10 Oct 2007, Jan Wielemaker wrote:
Show 8 quoted lines
> > 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

Frank Lichtenheld· Oct 10, 2007, 20:00 UTC · re: Jan Wielemaker · lore

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

On Wed, Oct 10, 2007 at 01:16:03PM +0200, Jan Wielemaker wrote:
Show 14 quoted lines
> +	# 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/
Andreas Ericsson· Oct 11, 2007, 08:45 UTC · re: Frank Lichtenheld · lore

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

Frank Lichtenheld wrote:
Show 20 quoted lines
> 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
Frank Lichtenheld· Oct 11, 2007, 16:36 UTC · re: Jan Wielemaker · lore

[PATCH] cvsserver: added support for update -p

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.
Show changes to 2 files +55 −0

git-cvsserver.perl, t/t9400-git-cvsserver-server.sh

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
Jan Wielemaker· Oct 11, 2007, 16:52 UTC · re: Frank Lichtenheld · lore

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

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
Frank Lichtenheld· Oct 11, 2007, 17:29 UTC · re: Jan Wielemaker · lore

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

On Thu, Oct 11, 2007 at 06:52:32PM +0200, Jan Wielemaker wrote:
Show 10 quoted lines
> 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/
Johannes Schindelin· Oct 11, 2007, 20:59 UTC · re: Frank Lichtenheld · lore

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

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

Frank Lichtenheld· Oct 11, 2007, 21:07 UTC · re: Johannes Schindelin · lore

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

On Thu, Oct 11, 2007 at 09:59:28PM +0100, Johannes Schindelin wrote:
Show 6 quoted lines
> 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/

← back to recent threads