git-svn can lose changes silently

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?

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.

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

index 450ff1f..a764d1f 100644
@@ -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

index 4a56f18..80b7b87 100755
@@ -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 #########################

index f994b72..746c827 100755
@@ -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
 	"

1.4.4.rc1.g40e94

-- 

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.

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

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.

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

-Steve

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

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,

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.

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].

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:

index 4a56f18..daa1764 100755
@@ -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.