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

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>
Signed-off-by: Junio C Hamano <junkio@cox.net>

diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt
index 450ff1f85b5f6ed0cca552bebb0ca1a279788ae5..a764d1f8ee52742b85ce8edff22814a32be82dfb 100644 (file)
--- 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 4a56f1871a6e185c4504d2464f264a8d8adc124b..80b7b87f0f4f1933e551439ca4fb2cd68981a029 100755 (executable)
--- 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 f994b72f80eb9cbc6617dc1d3c9e6bbe63319b7b..746c8277d06a7d9d147df16d6c16efce65bf34a2 100755 (executable)
--- 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 (executable)
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