* git-svn can lose changes silently
@ 2006-11-09 0:34 Steven Grimm
2006-11-09 7:33 ` Seth Falcon
2006-11-09 9:19 ` [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn Eric Wong
0 siblings, 2 replies; 9+ messages in thread
From: Steven Grimm @ 2006-11-09 0:34 UTC (permalink / raw)
To: git
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?
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: git-svn can lose changes silently 2006-11-09 0:34 git-svn can lose changes silently Steven Grimm @ 2006-11-09 7:33 ` Seth Falcon 2006-11-09 9:19 ` [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn Eric Wong 1 sibling, 0 replies; 9+ messages in thread From: Seth Falcon @ 2006-11-09 7:33 UTC (permalink / raw) To: git 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. ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn 2006-11-09 0:34 git-svn can lose changes silently Steven Grimm 2006-11-09 7:33 ` Seth Falcon @ 2006-11-09 9:19 ` Eric Wong 2006-11-09 10:00 ` Junio C Hamano 1 sibling, 1 reply; 9+ messages in thread From: Eric Wong @ 2006-11-09 9:19 UTC (permalink / raw) To: Steven Grimm; +Cc: git, Junio C Hamano 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 -- ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn 2006-11-09 9:19 ` [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn Eric Wong @ 2006-11-09 10:00 ` Junio C Hamano 2006-11-09 17:42 ` Seth Falcon 2006-11-09 19:30 ` Steven Grimm 0 siblings, 2 replies; 9+ messages in thread From: Junio C Hamano @ 2006-11-09 10:00 UTC (permalink / raw) To: Eric Wong, Steven Grimm; +Cc: git 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn 2006-11-09 10:00 ` Junio C Hamano @ 2006-11-09 17:42 ` Seth Falcon 2006-11-09 19:22 ` Eric Wong 2006-11-09 19:30 ` Steven Grimm 1 sibling, 1 reply; 9+ messages in thread From: Seth Falcon @ 2006-11-09 17:42 UTC (permalink / raw) To: git 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, ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn 2006-11-09 17:42 ` Seth Falcon @ 2006-11-09 19:22 ` Eric Wong 0 siblings, 0 replies; 9+ messages in thread From: Eric Wong @ 2006-11-09 19:22 UTC (permalink / raw) To: Seth Falcon; +Cc: git 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. -- ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn 2006-11-09 10:00 ` Junio C Hamano 2006-11-09 17:42 ` Seth Falcon @ 2006-11-09 19:30 ` Steven Grimm 2006-11-09 20:47 ` Eric Wong 1 sibling, 1 reply; 9+ messages in thread From: Steven Grimm @ 2006-11-09 19:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: Eric Wong, git 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn 2006-11-09 19:30 ` Steven Grimm @ 2006-11-09 20:47 ` Eric Wong 2006-11-09 22:37 ` Steven Grimm 0 siblings, 1 reply; 9+ messages in thread From: Eric Wong @ 2006-11-09 20:47 UTC (permalink / raw) To: Steven Grimm; +Cc: Junio C Hamano, git 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. -- ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn 2006-11-09 20:47 ` Eric Wong @ 2006-11-09 22:37 ` Steven Grimm 0 siblings, 0 replies; 9+ messages in thread From: Steven Grimm @ 2006-11-09 22:37 UTC (permalink / raw) To: Eric Wong; +Cc: Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2006-11-09 22:37 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-11-09 0:34 git-svn can lose changes silently Steven Grimm 2006-11-09 7:33 ` Seth Falcon 2006-11-09 9:19 ` [PATCH] git-svn: fix dcommit losing changes when out-of-date from svn Eric Wong 2006-11-09 10:00 ` Junio C Hamano 2006-11-09 17:42 ` Seth Falcon 2006-11-09 19:22 ` Eric Wong 2006-11-09 19:30 ` Steven Grimm 2006-11-09 20:47 ` Eric Wong 2006-11-09 22:37 ` Steven Grimm
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).