[PATCH v3] git-svn: Make following parents atomic

[Deskin Miller <deskinm@umich.edu>]

find_parent_branch generates branch@rev type branches when one has to
look back through SVN history to properly get the history for a branch
copied from somewhere not already being tracked by git-svn.  If in the
process of fetching this history, git-svn is interrupted, then when one
fetches again, it will use whatever was last fetched as the parent
commit and fail to fetch any more history which it didn't get to before
being terminated.  This is especially troubling in that different
git-svn copies of the same SVN repository can end up with different
commit sha1s, incorrectly showing the history as divergent and
precluding easy collaboration using git push and fetch.

To fix this, when we initialise the Git::SVN object $gs to search for
and perhaps fetch history, we check if there are any commits in SVN in
the range between the current revision $gs is at, and the top revision
for which we were asked to fill history.  If there are commits we're
missing in that range, we continue the fetch from the current revision
to the top, properly getting all history before using it as the parent
for the branch we're trying to create.

Signed-off-by: Deskin Miller <deskinm@umich.edu>
---
Re-roll based on Junio's comments on the testcase, plus fix some minor
whitespace lunacy I left in the testcase.

On Mon, Dec 08, 2008 at 12:33:22AM -0800, Junio C Hamano wrote:
> Deskin Miller <deskinm@umich.edu> writes:
> 
> > +test_expect_success "follow-parent is atomic" '
> > +	cd wc &&
> > +	[...]
> > +	cd .. &&
> 
> If you need to chdir around inside a test, please do that in a subshell,
> so that after failing any command in between, the next test will not start
> in an unexpected directory.

Good point; this even bit me once while I worked on this testcase, and I
still didn't take the hint (yikes!)
 
> > +	(svn cp -m "early stunk flunked too" \
> > +		"$svnrepo"/stunk@17 "$svnrepo"/flunked ||
> > +	svn cp -m "early stunk flunked too" \
> > +		-r17 "$svnrepo"/stunk "$svnrepo"/flunked) &&
> 
> On the other hand, I do not see a need for this portion to be in a
> subshell.  Wouldn't a normal statement grouping with {} work just as well?

It does work as well; I just didn't know about the { cmd-list; }
construction.

Thanks for the comments,

Deskin Miller

 git-svn.perl                     |   16 ++++++++++---
 t/t9104-git-svn-follow-parent.sh |   42 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 56238da..25ed2f4 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2318,12 +2318,20 @@ sub find_parent_branch {
 		$gs = Git::SVN->init($u, $p, $repo_id, $ref_id, 1);
 	}
 	my ($r0, $parent) = $gs->find_rev_before($r, 1);
-	if (!defined $r0 || !defined $parent) {
-		my ($base, $head) = parse_revision_argument(0, $r);
-		if ($base <= $r) {
+	{
+		my ($base, $head);
+		if (!defined $r0 || !defined $parent) {
+			($base, $head) = parse_revision_argument(0, $r);
+		} else {
+			if ($r0 < $r) {
+				$gs->ra->get_log([$gs->{path}], $r0 + 1, $r, 1,
+					0, 1, sub { $base = $_[1] - 1 });
+			}
+		}
+		if (defined $base && $base <= $r) {
 			$gs->fetch($base, $r);
 		}
-		($r0, $parent) = $gs->last_rev_commit;
+		($r0, $parent) = $gs->find_rev_before($r, 1);
 	}
 	if (defined $r0 && defined $parent) {
 		print STDERR "Found branch parent: ($self->{ref_id}) $parent\n";
diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
index 4d964e2..d80ea64 100755
--- a/t/t9104-git-svn-follow-parent.sh
+++ b/t/t9104-git-svn-follow-parent.sh
@@ -149,6 +149,48 @@ test_expect_success "track initial change if it was only made to parent" '
 	     "`git rev-parse r9270-d~1`"
 	'
 
+test_expect_success "follow-parent is atomic" '
+	(
+		cd wc &&
+		svn up &&
+		svn mkdir stunk &&
+		echo "trunk stunk" > stunk/readme &&
+		svn add stunk/readme &&
+		svn ci -m "trunk stunk" &&
+		echo "stunk like junk" >> stunk/readme &&
+		svn ci -m "really stunk" &&
+		echo "stink stank stunk" >> stunk/readme &&
+		svn ci -m "even the grinch agrees"
+	) &&
+	svn copy -m "stunk flunked" "$svnrepo"/stunk "$svnrepo"/flunk &&
+	{ svn cp -m "early stunk flunked too" \
+		"$svnrepo"/stunk@17 "$svnrepo"/flunked ||
+	svn cp -m "early stunk flunked too" \
+		-r17 "$svnrepo"/stunk "$svnrepo"/flunked; } &&
+	git svn init --minimize-url -i stunk "$svnrepo"/stunk &&
+	git svn fetch -i stunk &&
+	git update-ref refs/remotes/flunk@18 refs/remotes/stunk~2 &&
+	git update-ref -d refs/remotes/stunk &&
+	git config --unset svn-remote.svn.fetch stunk &&
+	mkdir -p "$GIT_DIR"/svn/flunk@18 &&
+	rev_map=$(cd "$GIT_DIR"/svn/stunk && ls .rev_map*) &&
+	dd if="$GIT_DIR"/svn/stunk/$rev_map \
+	   of="$GIT_DIR"/svn/flunk@18/$rev_map bs=24 count=1 &&
+	rm -rf "$GIT_DIR"/svn/stunk &&
+	git svn init --minimize-url -i flunk "$svnrepo"/flunk &&
+	git svn fetch -i flunk &&
+	git svn init --minimize-url -i stunk "$svnrepo"/stunk &&
+	git svn fetch -i stunk &&
+	git svn init --minimize-url -i flunked "$svnrepo"/flunked &&
+	git svn fetch -i flunked
+	test "`git rev-parse --verify refs/remotes/flunk@18`" \
+	   = "`git rev-parse --verify refs/remotes/stunk`" &&
+	test "`git rev-parse --verify refs/remotes/flunk~1`" \
+	   = "`git rev-parse --verify refs/remotes/stunk`" &&
+	test "`git rev-parse --verify refs/remotes/flunked~1`" \
+	   = "`git rev-parse --verify refs/remotes/stunk~1`"
+	'
+
 test_expect_success "track multi-parent paths" '
 	svn cp -m "resurrect /glob" "$svnrepo"/r9270 "$svnrepo"/glob &&
 	git-svn multi-fetch &&
-- 
1.6.1.rc2