git-svn can lose changes silently

git-svn can lose changes silently

[Steven Grimm]

git-svn is happy to overwrite changes in the svn repository with no 
warnings. Didn't seem to be known behavior when I mentioned it in #git, 
so here's an example, starting completely from scratch to make it easier 
to reproduce. I'm using git-svn 1.4.3 and svn 1.2.3 on OS X.

First I create a new svn repository and stick a test file there:

% mkdir /tmp/svntest
% cd /tmp/svntest
% svnadmin create svn-repo
% svn co file:///tmp/svntest/svn-repo /tmp/svntest/svn-client
Checked out revision 0.
% cd /tmp/svntest/svn-client
% mkdir -p project/trunk
% echo "initial contents" > project/trunk/testfile
% svn add project/trunk/testfile
svn: 'project/trunk' is not a working copy
% svn add project
A         project
A         project/trunk
A         project/trunk/testfile
% svn commit -m "initial commit"
Adding         project
Adding         project/trunk
Adding         project/trunk/testfile
Transmitting file data .
Committed revision 1.

Now I clone that svn repository using git-svn and pull in the contents 
of the svn repo:

% cd /tmp/svntest
% git-svn init file:///tmp/svntest/svn-repo/project/trunk git-repo 
% cd git-repo
% git-svn fetch
        A       project/trunk/testfile
Committing initial tree 9ca0a5a8cb5ee41744aaf17f859e945f2ebaa7d4
r1 = 63c70a5e17ffb095a31e96b1a56612f1f8423202

Now I create a development branch and commit a change to the test file:

% git-checkout -b devel
% echo "a second line from the git side" >> testfile
% git-commit -a -m "git-side commit"

Now I go make a change on the svn side and commit it.

% cd /tmp/svntest/svn-client/project/trunk
% echo "a second line from the svn side" >> testfile
% svn commit -m "a second svn commit"
Sending        trunk/testfile
Transmitting file data .
Committed revision 2.

At this point the svn repository has the testfile with two lines: 
"initial contents" and "a second line from the svn side". Now, back on 
the git side, I commit my git-side change to svn (here's where the bug 
happens):

% cd /tmp/svntest/git-repo
% git-svn dcommit                     
diff-tree 679c0db253781216b9b72b51f2dfffec5711f1a3~1 
679c0db253781216b9b72b51f2dfffec5711f1a3
        M       testfile
Committed 3
        M       project/trunk/testfile
r2 = 7d7923588ffb41eb756959d71623581df9318603
        M       project/trunk/testfile
r3 = 25a5fefe01389260274bb2617bc36a2cce18f15d
No changes between current HEAD and refs/remotes/git-svn
Hard resetting to the latest refs/remotes/git-svn

Finally, I go back to the svn side and update from the repo:

% cd /tmp/svntest/svn-client
% svn up
U    project/trunk/testfile
Updated to revision 3.
% cat project/trunk/testfile
initial contents
a second line from the git side

The change I checked in from the svn side has vanished without a trace, 
no warning messages or anything.

It is probably not a feature that you can lose changes without knowing 
about it! Even if I'd run git-svn fetch before that commit, it still 
wouldn't help if the svn version of the file changed between the time I 
ran fetch and the time I ran dcommit, totally possible with a busy svn 
repository.

Opinions? Suggestions on fixing it? Do other people agree this is a bug?

Re: git-svn can lose changes silently

[Seth Falcon]

Steven Grimm <koreth@midwinter.com> writes:

> git-svn is happy to overwrite changes in the svn repository with no
> warnings. Didn't seem to be known behavior when I mentioned it in
> #git, so here's an example, starting completely from scratch to make
> it easier to reproduce. I'm using git-svn 1.4.3 and svn 1.2.3 on OS
> X.

I can reproduce your example and also see the same thing when
accessing an svn repository via https [I've never used the local
file:// mode and was leary of it].

> It is probably not a feature that you can lose changes without knowing
> about it! Even if I'd run git-svn fetch before that commit, it still
> wouldn't help if the svn version of the file changed between the time
> I ran fetch and the time I ran dcommit, totally possible with a busy
> svn repository.
>
> Opinions? Suggestions on fixing it? Do other people agree this is a
> bug?

I think this is a pretty big deal -- at least in terms of user
expectation.  The man page for 'git-svn commit' warns that upstream
changes will be lost.  But in suggesting to use 'git-svn dcommit'
instead, one might expect (hope?) this overwritting upstream changes
to not occur.

I'm embarassed to admit that I've been using git-svn for a few weeks
now and didn't test out these details.  

Here is a patch that I think improves matters a bit:

diff --git a/git-svn.perl b/git-svn.perl
index 4a56f18..daa1764 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -830,6 +830,17 @@ sub commit_diff {
        ($repo, $SVN_PATH) = repo_path_split($SVN_URL);
        $SVN_LOG ||= libsvn_connect($repo);
        $SVN ||= libsvn_connect($repo);
+
+        my ($r_last, $cmt_last) = svn_grab_base_rev();
+        my $fetched = fetch();
+       if ($r_last != $fetched->{revision}) {
+               print STDERR "There are new revisions that were fetched ",
+                               "and need to be merged (or acknowledged) ",
+                               "before committing.\n",
+                               "last rev: $r_last\n",
+                               " current: $fetched->{revision}\n";
+               exit 1;
+       }
        my @lock = $SVN::Core::VERSION ge '1.2.0' ? (undef, 0) : ();
        my $ed = SVN::Git::Editor->new({        r => $SVN->get_latest_revnum,
                                                ra => $SVN_LOG, c => $tb,

But for a possibly busy svn repository, the only real solution is one
that uses the svn libs in a way that behaves like the svn client in
terms of atomicity.

[PATCH] git-svn: fix dcommit losing changes when out-of-date from svn

[Eric Wong]

There was a bug in dcommit (and commit-diff) which caused deltas
to be generated against the latest version of the changed file
in a repository, and not the revision we are diffing (the tree)
against locally.

This bug can cause recent changes to the svn repository to be
silently clobbered by git-svn if our repository is out-of-date.

Thanks to Steven Grimm for noticing the bug.

The (few) people using the commit-diff command are now required
to use the -r/--revision argument.  dcommit usage is unchanged.

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 Documentation/git-svn.txt              |    1 +
 git-svn.perl                           |   29 +++++++++++-
 t/t9105-git-svn-commit-diff.sh         |    2 +-
 t/t9106-git-svn-commit-diff-clobber.sh |   74 ++++++++++++++++++++++++++++++++
 4 files changed, 102 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt
index 450ff1f..a764d1f 100644
--- a/Documentation/git-svn.txt
+++ b/Documentation/git-svn.txt
@@ -120,6 +120,7 @@ manually joining branches on commit.
 	URL of the target Subversion repository.  The final argument
 	(URL) may be omitted if you are working from a git-svn-aware
 	repository (that has been init-ed with git-svn).
+	The -r<revision> option is required for this.
 
 'graft-branches'::
 	This command attempts to detect merges/branches from already
diff --git a/git-svn.perl b/git-svn.perl
index 4a56f18..80b7b87 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -134,6 +134,7 @@ my %cmd = (
 	'commit-diff' => [ \&commit_diff, 'Commit a diff between two trees',
 			{ 'message|m=s' => \$_message,
 			  'file|F=s' => \$_file,
+			  'revision|r=s' => \$_revision,
 			%cmt_opts } ],
 	dcommit => [ \&dcommit, 'Commit several diffs to merge with upstream',
 			{ 'merge|m|M' => \$_merge,
@@ -586,11 +587,21 @@ sub commit_lib {
 sub dcommit {
 	my $gs = "refs/remotes/$GIT_SVN";
 	chomp(my @refs = safe_qx(qw/git-rev-list --no-merges/, "$gs..HEAD"));
+	my $last_rev;
 	foreach my $d (reverse @refs) {
+		unless (defined $last_rev) {
+			(undef, $last_rev, undef) = cmt_metadata("$d~1");
+			unless (defined $last_rev) {
+				die "Unable to extract revision information ",
+				    "from commit $d~1\n";
+			}
+		}
 		if ($_dry_run) {
 			print "diff-tree $d~1 $d\n";
 		} else {
-			commit_diff("$d~1", $d);
+			if (my $r = commit_diff("$d~1", $d, undef, $last_rev)) {
+				$last_rev = $r;
+			} # else: no changes, same $last_rev
 		}
 	}
 	return if $_dry_run;
@@ -814,6 +825,8 @@ sub commit_diff {
 		print STDERR "Needed URL or usable git-svn id command-line\n";
 		commit_diff_usage();
 	}
+	my $r = shift || $_revision;
+	die "-r|--revision is a required argument\n" unless (defined $r);
 	if (defined $_message && defined $_file) {
 		print STDERR "Both --message/-m and --file/-F specified ",
 				"for the commit message.\n",
@@ -830,13 +843,22 @@ sub commit_diff {
 	($repo, $SVN_PATH) = repo_path_split($SVN_URL);
 	$SVN_LOG ||= libsvn_connect($repo);
 	$SVN ||= libsvn_connect($repo);
+	if ($r eq 'HEAD') {
+		$r = $SVN->get_latest_revnum;
+	} elsif ($r !~ /^\d+$/) {
+		die "revision argument: $r not understood by git-svn\n";
+	}
 	my @lock = $SVN::Core::VERSION ge '1.2.0' ? (undef, 0) : ();
-	my $ed = SVN::Git::Editor->new({	r => $SVN->get_latest_revnum,
+	my $rev_committed;
+	my $ed = SVN::Git::Editor->new({	r => $r,
 						ra => $SVN_LOG, c => $tb,
 						svn_path => $SVN_PATH
 					},
 				$SVN->get_commit_editor($_message,
-					sub {print "Committed $_[0]\n"},@lock)
+					sub {
+						$rev_committed = $_[0];
+						print "Committed $_[0]\n";
+					}, @lock)
 				);
 	my $mods = libsvn_checkout_tree($ta, $tb, $ed);
 	if (@$mods == 0) {
@@ -846,6 +868,7 @@ sub commit_diff {
 		$ed->close_edit;
 	}
 	$_message = $_file = undef;
+	return $rev_committed;
 }
 
 ########################### utility functions #########################
diff --git a/t/t9105-git-svn-commit-diff.sh b/t/t9105-git-svn-commit-diff.sh
index f994b72..746c827 100755
--- a/t/t9105-git-svn-commit-diff.sh
+++ b/t/t9105-git-svn-commit-diff.sh
@@ -33,7 +33,7 @@ prev=`git rev-parse --verify HEAD^1`
 
 test_expect_success 'test the commit-diff command' "
 	test -n '$prev' && test -n '$head' &&
-	git-svn commit-diff '$prev' '$head' '$svnrepo' &&
+	git-svn commit-diff -r1 '$prev' '$head' '$svnrepo' &&
 	svn co $svnrepo wc &&
 	cmp readme wc/readme
 	"
diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh
new file mode 100755
index 0000000..58698b3
--- /dev/null
+++ b/t/t9106-git-svn-commit-diff-clobber.sh
@@ -0,0 +1,74 @@
+#!/bin/sh
+#
+# Copyright (c) 2006 Eric Wong
+test_description='git-svn commit-diff clobber'
+. ./lib-git-svn.sh
+
+if test -n "$GIT_SVN_NO_LIB" && test "$GIT_SVN_NO_LIB" -ne 0
+then
+	echo 'Skipping: commit-diff clobber needs SVN libraries'
+	test_done
+	exit 0
+fi
+
+test_expect_success 'initialize repo' "
+	mkdir import &&
+	cd import &&
+	echo initial > file &&
+	svn import -m 'initial' . $svnrepo &&
+	cd .. &&
+	echo initial > file &&
+	git update-index --add file &&
+	git commit -a -m 'initial'
+	"
+test_expect_success 'commit change from svn side' "
+	svn co $svnrepo t.svn &&
+	cd t.svn &&
+	echo second line from svn >> file &&
+	svn commit -m 'second line from svn' &&
+	cd .. &&
+	rm -rf t.svn
+	"
+
+test_expect_failure 'commit conflicting change from git' "
+	echo second line from git >> file &&
+	git commit -a -m 'second line from git' &&
+	git-svn commit-diff -r1 HEAD~1 HEAD $svnrepo
+	" || true
+
+test_expect_success 'commit complementing change from git' "
+	git reset --hard HEAD~1 &&
+	echo second line from svn >> file &&
+	git commit -a -m 'second line from svn' &&
+	echo third line from git >> file &&
+	git commit -a -m 'third line from git' &&
+	git-svn commit-diff -r2 HEAD~1 HEAD $svnrepo
+	"
+
+test_expect_failure 'dcommit fails to commit because of conflict' "
+	git-svn init $svnrepo &&
+	git-svn fetch &&
+	git reset --hard refs/remotes/git-svn &&
+	svn co $svnrepo t.svn &&
+	cd t.svn &&
+	echo fourth line from svn >> file &&
+	svn commit -m 'fourth line from svn' &&
+	cd .. &&
+	rm -rf t.svn &&
+	echo 'fourth line from git' >> file &&
+	git commit -a -m 'fourth line from git' &&
+	git-svn dcommit
+	" || true
+
+test_expect_success 'dcommit does the svn equivalent of an index merge' "
+	git reset --hard refs/remotes/git-svn &&
+	echo 'index merge' > file2 &&
+	git update-index --add file2 &&
+	git commit -a -m 'index merge' &&
+	echo 'more changes' >> file2 &&
+	git update-index file2 &&
+	git commit -a -m 'more changes' &&
+	git-svn dcommit
+	"
+
+test_done
-- 
1.4.4.rc1.g40e94

-- 

Re: [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> There was a bug in dcommit (and commit-diff) which caused deltas
> to be generated against the latest version of the changed file
> in a repository, and not the revision we are diffing (the tree)
> against locally.
>
> This bug can cause recent changes to the svn repository to be
> silently clobbered by git-svn if our repository is out-of-date.
>
> Thanks to Steven Grimm for noticing the bug.
>
> The (few) people using the commit-diff command are now required
> to use the -r/--revision argument.  dcommit usage is unchanged.
>
> Signed-off-by: Eric Wong <normalperson@yhbt.net>

Thanks both for your clear problem report and quick resolution
of the issue.

Steven, I do not interact with real svn repository myself so I
can only judge from the test in this patch and Steven's test
case, so it would be more assuring for me if you can confirm it
fixes the issue for you.

Re: [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn

[Seth Falcon]

Junio C Hamano <junkio@cox.net> writes:
> Eric Wong <normalperson@yhbt.net> writes:
>
>> There was a bug in dcommit (and commit-diff) which caused deltas
>> to be generated against the latest version of the changed file
>> in a repository, and not the revision we are diffing (the tree)
>> against locally.
>>
>> This bug can cause recent changes to the svn repository to be
>> silently clobbered by git-svn if our repository is out-of-date.

Eric, with this patch, is a dcommit operation as safe as a regular svn
commit from an svn working copy?  That is, the commit will abort if
the svn repository has changes that your git-svn/git repo hasn't yet
seen?  I'm pretty sure the answer is yes, but I'd like to be sure :-)

> Steven, I do not interact with real svn repository myself so I
> can only judge from the test in this patch and Steven's test
> case, so it would be more assuring for me if you can confirm it
> fixes the issue for you.

I'm not Steven, but I was reproducing the bug and with Eric's patch, I
get a nice error/abort when I try to dcommit when the svn repos has
relevant changes that I have not fetched yet.

Best,

Re: [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn

[Eric Wong]

Seth Falcon <sethfalcon@gmail.com> wrote:
> Junio C Hamano <junkio@cox.net> writes:
> > Eric Wong <normalperson@yhbt.net> writes:
> >
> >> There was a bug in dcommit (and commit-diff) which caused deltas
> >> to be generated against the latest version of the changed file
> >> in a repository, and not the revision we are diffing (the tree)
> >> against locally.
> >>
> >> This bug can cause recent changes to the svn repository to be
> >> silently clobbered by git-svn if our repository is out-of-date.
> 
> Eric, with this patch, is a dcommit operation as safe as a regular svn
> commit from an svn working copy?  That is, the commit will abort if
> the svn repository has changes that your git-svn/git repo hasn't yet
> seen?  I'm pretty sure the answer is yes, but I'd like to be sure :-)

Yes, this is as safe as a regular svn commit from a working copy.
Transactions will abort if there are conflicts in the files being
committed.

-- 

Re: [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn

[Steven Grimm]

Junio C Hamano wrote:
> Steven, I do not interact with real svn repository myself so I
> can only judge from the test in this patch and Steven's test
> case, so it would be more assuring for me if you can confirm it
> fixes the issue for you.
>   

It seems to; I can't make the problem happen any more. I am slightly 
concerned -- but I don't know libsvn well enough to say for sure -- that 
this doesn't actually *eliminate* the problem, but rather tightens the 
window of opportunity down to some very small amount of time. Which is 
certainly an improvement, of course!

Maybe only Eric can answer this, but from a cursory inspection, it 
doesn't look like it actually locks the modified files before generating 
the patch to apply. Is there still a possibility of losing a change that 
hits the svn repository in the middle of git-svn dcommit running? Or 
does locking happen implicitly somewhere I'm not seeing? (Again, I 
haven't combed the code deeply, so it's entirely possible I've missed 
something.)

-Steve

Re: [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn

[Eric Wong]

Steven Grimm <koreth@midwinter.com> wrote:
> Junio C Hamano wrote:
> >Steven, I do not interact with real svn repository myself so I
> >can only judge from the test in this patch and Steven's test
> >case, so it would be more assuring for me if you can confirm it
> >fixes the issue for you.
> >  
> 
> It seems to; I can't make the problem happen any more. I am slightly 
> concerned -- but I don't know libsvn well enough to say for sure -- that 
> this doesn't actually *eliminate* the problem, but rather tightens the 
> window of opportunity down to some very small amount of time. Which is 
> certainly an improvement, of course!
> 
> Maybe only Eric can answer this, but from a cursory inspection, it 
> doesn't look like it actually locks the modified files before generating 
> the patch to apply. Is there still a possibility of losing a change that 
> hits the svn repository in the middle of git-svn dcommit running? Or 
> does locking happen implicitly somewhere I'm not seeing? (Again, I 
> haven't combed the code deeply, so it's entirely possible I've missed 
> something.)

The commit runs as a transaction on the server-side, where all the real
locking occurs.  SVN supports user-side locking (svn lock), but it is
only advisory and can be taken/unlocked by other users
(svn lock/unlock --force).

When SVN::Git::Editor is instantiated in commit_diff() (line 853), the
'r' parameter passed to it is the revision we'll generate our diffs
against.  Before, we were generating diffs against the latest revision.

We generate diffs against 'r' in SVN::Git::Editor::M() (line 3339) and
SVN::Git::Editor::chg_file (line 3383) passing the $fbat baton object
(which represents the file at revision 'r') around.

-- 

Re: [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn

[Steven Grimm]

Eric Wong wrote:
> The commit runs as a transaction on the server-side, where all the real
> locking occurs.  SVN supports user-side locking (svn lock), but it is
> only advisory and can be taken/unlocked by other users
> (svn lock/unlock --force).
>   

Great, that sounds convincing to me. Thanks for such a quick turnaround 
on this fix!

-Steve