Feature request: git-svn dcommit should send deltas upstream

Feature request: git-svn dcommit should send deltas upstream

[Florian Weimer]

Looking at my network traffic and the Perl code, it seems to me that
git-svn fails to create a diff (delta) before sending data to the
server.  As a result, a few changes in a multi-megabyte file lead to a
large upload (similar to the situation with CVS).  git-svn should be
able to compute this diff in all cases because it has got an up-to-date
copy of the current revision in the Subversion repository.

As far as I can tell, this can't be fixed with a one-liner; some handles
need to be passed down to the code that actually handles the upload.

Re: Feature request: git-svn dcommit should send deltas upstream

[Eric Wong]

Florian Weimer <fw@deneb.enyo.de> wrote:
> Looking at my network traffic and the Perl code, it seems to me that
> git-svn fails to create a diff (delta) before sending data to the
> server.  As a result, a few changes in a multi-megabyte file lead to a
> large upload (similar to the situation with CVS).  git-svn should be
> able to compute this diff in all cases because it has got an up-to-date
> copy of the current revision in the Subversion repository.
> 
> As far as I can tell, this can't be fixed with a one-liner; some handles
> need to be passed down to the code that actually handles the upload.

Odd.  Can you verify that svn(1) does not send full files in this case,
too?

It's been too long since I've looked at the SVN TxDelta API, but I
thought SVN::TxDelta::apply would take care of the delta computation for
us...

-- 
Eric Wong

Re: Feature request: git-svn dcommit should send deltas upstream

[Florian Weimer]

* Eric Wong:

> Florian Weimer <fw@deneb.enyo.de> wrote:
>> Looking at my network traffic and the Perl code, it seems to me that
>> git-svn fails to create a diff (delta) before sending data to the
>> server.  As a result, a few changes in a multi-megabyte file lead to a
>> large upload (similar to the situation with CVS).  git-svn should be
>> able to compute this diff in all cases because it has got an up-to-date
>> copy of the current revision in the Subversion repository.
>> 
>> As far as I can tell, this can't be fixed with a one-liner; some handles
>> need to be passed down to the code that actually handles the upload.
>
> Odd.  Can you verify that svn(1) does not send full files in this case,
> too?

These two are pcap files of single-line edits to the same file:

-rw-r--r-- 1 root root 1.3M 2008-08-29 11:54 /tmp/git
-rw-r--r-- 1 root root  40K 2008-08-29 11:53 /tmp/svn

> It's been too long since I've looked at the SVN TxDelta API, but I
> thought SVN::TxDelta::apply would take care of the delta computation for
> us...

SVN::Git::Editor::M does not seem to make use of the base text.

Re: Feature request: git-svn dcommit should send deltas upstream

[Florian Weimer]

* Florian Weimer:

>> It's been too long since I've looked at the SVN TxDelta API, but I
>> thought SVN::TxDelta::apply would take care of the delta computation for
>> us...
>
> SVN::Git::Editor::M does not seem to make use of the base text.

Here's an attempt at delta generation.  I don't know if it is entirely
correct, but it does work in the sense that it passes "make test" and
the test cases I reported eariler.

Sorry for the format.  Has anybody got some Emacs code to submit diffs
using Gnus?

commit c94c8b36b41bac20ebf952244675507d2fb927cb
Author: Florian Weimer <fw@deneb.enyo.de>
Date:   Sun Aug 31 17:05:09 2008 +0200

    git-svn: extract base blob in generate_diff
    
    We need the base blob to compute a delta to be sent to the server.
    
    Signed-off-by: Florian Weimer <fw@deneb.enyo.de>

diff --git a/git-svn.perl b/git-svn.perl
index 7a1d26d..0479f41 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3380,11 +3380,12 @@ sub generate_diff {
 	while (<$diff_fh>) {
 		chomp $_; # this gets rid of the trailing "\0"
 		if ($state eq 'meta' && /^:(\d{6})\s(\d{6})\s
-					$::sha1\s($::sha1)\s
+					($::sha1)\s($::sha1)\s
 					([MTCRAD])\d*$/xo) {
 			push @mods, {	mode_a => $1, mode_b => $2,
-					sha1_b => $3, chg => $4 };
-			if ($4 =~ /^(?:C|R)$/) {
+					sha1_a => $3, sha1_b => $4,
+					chg => $5 };
+			if ($5 =~ /^(?:C|R)$/) {
 				$state = 'file_a';
 			} else {
 				$state = 'file_b';

commit 020f941b8b606ed3dc2d323194ab11e019380b03
Author: Florian Weimer <fw@deneb.enyo.de>
Date:   Sun Aug 31 17:45:04 2008 +0200

    git-svn: Introduce SVN::Git::Editor::_chg_file_get_blob
    
    Signed-off-by: Florian Weimer <fw@deneb.enyo.de>

diff --git a/git-svn.perl b/git-svn.perl
index 0479f41..2c3e13f 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3663,28 +3663,35 @@ sub change_file_prop {
 	$self->SUPER::change_file_prop($fbat, $pname, $pval, $self->{pool});
 }
 
-sub chg_file {
-	my ($self, $fbat, $m) = @_;
-	if ($m->{mode_b} =~ /755$/ && $m->{mode_a} !~ /755$/) {
-		$self->change_file_prop($fbat,'svn:executable','*');
-	} elsif ($m->{mode_b} !~ /755$/ && $m->{mode_a} =~ /755$/) {
-		$self->change_file_prop($fbat,'svn:executable',undef);
-	}
-	my $fh = Git::temp_acquire('git_blob');
-	if ($m->{mode_b} =~ /^120/) {
+sub _chg_file_get_blob ($$$$) {
+	my ($self, $fbat, $m, $which) = @_;
+	my $fh = Git::temp_acquire("git_blob_$which");
+	if ($m->{"mode_$which"} =~ /^120/) {
 		print $fh 'link ' or croak $!;
 		$self->change_file_prop($fbat,'svn:special','*');
-	} elsif ($m->{mode_a} =~ /^120/ && $m->{mode_b} !~ /^120/) {
+	} elsif ($m->{mode_a} =~ /^120/ && $m->{"mode_$which"} !~ /^120/) {
 		$self->change_file_prop($fbat,'svn:special',undef);
 	}
-	my $size = $::_repository->cat_blob($m->{sha1_b}, $fh);
-	croak "Failed to read object $m->{sha1_b}" if ($size < 0);
+	my $blob = $m->{"sha1_$which"};
+	return ($fh,) if ($blob =~ /^0{40}$/);
+	my $size = $::_repository->cat_blob($blob, $fh);
+	croak "Failed to read object $blob" if ($size < 0);
 	$fh->flush == 0 or croak $!;
 	seek $fh, 0, 0 or croak $!;
 
 	my $exp = ::md5sum($fh);
 	seek $fh, 0, 0 or croak $!;
+	return ($fh, $exp);
+}
 
+sub chg_file {
+	my ($self, $fbat, $m) = @_;
+	if ($m->{mode_b} =~ /755$/ && $m->{mode_a} !~ /755$/) {
+		$self->change_file_prop($fbat,'svn:executable','*');
+	} elsif ($m->{mode_b} !~ /755$/ && $m->{mode_a} =~ /755$/) {
+		$self->change_file_prop($fbat,'svn:executable',undef);
+	}
+	my ($fh, $exp) = _chg_file_get_blob $self, $fbat, $m, 'b';
 	my $pool = SVN::Pool->new;
 	my $atd = $self->apply_textdelta($fbat, undef, $pool);
 	my $got = SVN::TxDelta::send_stream($fh, @$atd, $pool);

commit 846d32cbe8283ffdf4ad5ddb2905dfbc30eace27
Author: Florian Weimer <fw@deneb.enyo.de>
Date:   Sun Aug 31 17:47:09 2008 +0200

    git-svn: Send deltas during commits
    
    Signed-off-by: Florian Weimer <fw@deneb.enyo.de>

diff --git a/git-svn.perl b/git-svn.perl
index 2c3e13f..fdf4e4a 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3691,12 +3691,20 @@ sub chg_file {
 	} elsif ($m->{mode_b} !~ /755$/ && $m->{mode_a} =~ /755$/) {
 		$self->change_file_prop($fbat,'svn:executable',undef);
 	}
-	my ($fh, $exp) = _chg_file_get_blob $self, $fbat, $m, 'b';
+	my ($fh_a, $exp_a) = _chg_file_get_blob $self, $fbat, $m, 'a';
+	my ($fh_b, $exp_b) = _chg_file_get_blob $self, $fbat, $m, 'b';
 	my $pool = SVN::Pool->new;
-	my $atd = $self->apply_textdelta($fbat, undef, $pool);
-	my $got = SVN::TxDelta::send_stream($fh, @$atd, $pool);
-	die "Checksum mismatch\nexpected: $exp\ngot: $got\n" if ($got ne $exp);
-	Git::temp_release($fh, 1);
+	my $atd = $self->apply_textdelta($fbat, $exp_a, $pool);
+	if (-s $fh_a) {
+		my $txstream = SVN::TxDelta::new ($fh_a, $fh_b, $pool);
+		SVN::TxDelta::send_txstream($txstream, @$atd, $pool);
+	} else {
+		my $got = SVN::TxDelta::send_stream($fh_b, @$atd, $pool);
+		die "Checksum mismatch\nexpected: $exp_b\ngot: $got\n"
+		    if ($got ne $exp_b);
+	}
+	Git::temp_release($fh_b, 1);
+	Git::temp_release($fh_a, 1);
 	$pool->clear;
 }
 

Re: Feature request: git-svn dcommit should send deltas upstream

[Junio C Hamano]

Florian Weimer <fw@deneb.enyo.de> writes:

> * Florian Weimer:
>
>>> It's been too long since I've looked at the SVN TxDelta API, but I
>>> thought SVN::TxDelta::apply would take care of the delta computation for
>>> us...
>>
>> SVN::Git::Editor::M does not seem to make use of the base text.
>
> Here's an attempt at delta generation.  I don't know if it is entirely
> correct, but it does work in the sense that it passes "make test" and
> the test cases I reported eariler.
>
> Sorry for the format.  Has anybody got some Emacs code to submit diffs
> using Gnus?

What's wrong with using "C-x C-i" in the message mode to read format-patch
output in (alternatively "C-u M-! git format-patch --stdout -1 $commit"
in the message mode), move "Subject: " up and remove the remainder of the
fake headers?

Re: Feature request: git-svn dcommit should send deltas upstream

[Florian Weimer]

* Junio C. Hamano:

>> Sorry for the format.  Has anybody got some Emacs code to submit diffs
>> using Gnus?
>
> What's wrong with using "C-x C-i" in the message mode to read format-patch
> output in (alternatively "C-u M-! git format-patch --stdout -1 $commit"
> in the message mode), move "Subject: " up and remove the remainder of the
> fake headers?

For single patches, it's okay.  Using this approach, proper threading of
a series of patches is rather error-prone, though.

Re: Feature request: git-svn dcommit should send deltas upstream

[Eric Wong]

Florian Weimer <fw@deneb.enyo.de> wrote:
> * Florian Weimer:
> 
> >> It's been too long since I've looked at the SVN TxDelta API, but I
> >> thought SVN::TxDelta::apply would take care of the delta computation for
> >> us...
> >
> > SVN::Git::Editor::M does not seem to make use of the base text.
> 
> Here's an attempt at delta generation.  I don't know if it is entirely
> correct, but it does work in the sense that it passes "make test" and
> the test cases I reported eariler.

Thanks Florian.

Works for me, here.  I've also tested this editing an audio file and
transmitting it over the wire without any trouble.  I've added one more
patch on top of your series to check the (undef) return value of
send_txstream.

Acked and pushed out to git://git.bogomips.org/git-svn.git along
with Thomas's other patch.

-- 
Eric Wong

Re: Feature request: git-svn dcommit should send deltas upstream

[Florian Weimer]

* Eric Wong:

> Works for me, here.  I've also tested this editing an audio file and
> transmitting it over the wire without any trouble.  I've added one more
> patch on top of your series to check the (undef) return value of
> send_txstream.

I assumed that send_txstream croaks when it encounters an error.
Looking at the SWIG-generated sources, this seems to be the case.