git-svn: fix dcommit clobbering upstream when committing multiple changes

Although dcommit could detect if the first commit in the series
would conflict with the HEAD revision in SVN, it could not
detect conflicts in further commits it made.

Now we rebase each uncommitted change after each revision is
committed to SVN to ensure that we are up-to-date.  git-rebase
will bail out on conflict errors if our next change cannot be
applied and committed to SVN cleanly, preventing accidental
clobbering of changes on the SVN-side.

--no-rebase users will have trouble with this, and are thus
warned if they are committing more than one commit.  Fixing this
for (hopefully uncommon) --no-rebase users would be more complex
and will probably happen at a later date.

Thanks to David Watson for finding this and the original test.

Signed-off-by: Junio C Hamano <gitster@pobox.com>

2 files changed, 62 insertions, 28 deletions

diff --git a/git-svn.perl b/git-svn.perl
index 98218dabd9..d3c8cd0b8e 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -384,6 +384,12 @@ sub cmd_dcommit {
 	}
 	my $last_rev;
 	my ($linear_refs, $parents) = linearize_history($gs, \@refs);
+	if ($_no_rebase && scalar(@$linear_refs) > 1) {
+		warn "Attempting to commit more than one change while ",
+		     "--no-rebase is enabled.\n",
+		     "If these changes depend on each other, re-running ",
+		     "without --no-rebase will be required."
+	}
 	foreach my $d (@$linear_refs) {
 		unless (defined $last_rev) {
 			(undef, $last_rev, undef) = cmt_metadata("$d~1");
@@ -395,6 +401,7 @@ sub cmd_dcommit {
 		if ($_dry_run) {
 			print "diff-tree $d~1 $d\n";
 		} else {
+			my $cmt_rev;
 			my %ed_opts = ( r => $last_rev,
 			                log => get_commit_entry($d)->{log},
 			                ra => Git::SVN::Ra->new($gs->full_url),
@@ -402,42 +409,39 @@ sub cmd_dcommit {
 			                tree_b => $d,
 			                editor_cb => sub {
 			                       print "Committed r$_[0]\n";
-			                       $last_rev = $_[0]; },
+			                       $cmt_rev = $_[0];
+			                },
 			                svn_path => '');
 			if (!SVN::Git::Editor->new(\%ed_opts)->apply_diff) {
 				print "No changes\n$d~1 == $d\n";
 			} elsif ($parents->{$d} && @{$parents->{$d}}) {
-				$gs->{inject_parents_dcommit}->{$last_rev} =
+				$gs->{inject_parents_dcommit}->{$cmt_rev} =
 				                               $parents->{$d};
 			}
+			$_fetch_all ? $gs->fetch_all : $gs->fetch;
+			next if $_no_rebase;
+
+			# we always want to rebase against the current HEAD,
+			# not any head that was passed to us
+			my @diff = command('diff-tree', 'HEAD',
+			                   $gs->refname, '--');
+			my @finish;
+			if (@diff) {
+				@finish = rebase_cmd();
+				print STDERR "W: HEAD and ", $gs->refname,
+				             " differ, using @finish:\n",
+				             "@diff";
+			} else {
+				print "No changes between current HEAD and ",
+				      $gs->refname,
+				      "\nResetting to the latest ",
+				      $gs->refname, "\n";
+				@finish = qw/reset --mixed/;
+			}
+			command_noisy(@finish, $gs->refname);
+			$last_rev = $cmt_rev;
 		}
 	}
-	return if $_dry_run;
-	unless ($gs) {
-		warn "Could not determine fetch information for $url\n",
-		     "Will not attempt to fetch and rebase commits.\n",
-		     "This probably means you have useSvmProps and should\n",
-		     "now resync your SVN::Mirror repository.\n";
-		return;
-	}
-	$_fetch_all ? $gs->fetch_all : $gs->fetch;
-	unless ($_no_rebase) {
-		# we always want to rebase against the current HEAD, not any
-		# head that was passed to us
-		my @diff = command('diff-tree', 'HEAD', $gs->refname, '--');
-		my @finish;
-		if (@diff) {
-			@finish = rebase_cmd();
-			print STDERR "W: HEAD and ", $gs->refname, " differ, ",
-				     "using @finish:\n", "@diff";
-		} else {
-			print "No changes between current HEAD and ",
-			      $gs->refname, "\nResetting to the latest ",
-			      $gs->refname, "\n";
-			@finish = qw/reset --mixed/;
-		}
-		command_noisy(@finish, $gs->refname);
-	}
 }
 
 sub cmd_find_rev {
diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh
index 6f132f2419..79b7968eaf 100755
--- a/t/t9106-git-svn-commit-diff-clobber.sh
+++ b/t/t9106-git-svn-commit-diff-clobber.sh
@@ -66,4 +66,34 @@ test_expect_success 'dcommit does the svn equivalent of an index merge' "
 	git-svn dcommit
 	"
 
+test_expect_success 'commit another change from svn side' "
+	svn co $svnrepo t.svn &&
+	cd t.svn &&
+		echo third line from svn >> file &&
+		poke file &&
+		svn commit -m 'third line from svn' &&
+	cd .. &&
+	rm -rf t.svn
+	"
+
+test_expect_failure 'multiple dcommit from git-svn will not clobber svn' "
+	git reset --hard refs/remotes/git-svn &&
+	echo new file >> new-file &&
+	git update-index --add new-file &&
+	git commit -a -m 'new file' &&
+	echo clobber > file &&
+	git commit -a -m 'clobber' &&
+	git svn dcommit
+	" || true
+
+
+test_expect_success 'check that rebase really failed' 'test -d .dotest'
+
+test_expect_success 'resolve, continue the rebase and dcommit' "
+	echo clobber and I really mean it > file &&
+	git update-index file &&
+	git rebase --continue &&
+	git svn dcommit
+	"
+
 test_done