[PATCH] git-svn: Make following parents atomic

[PATCH] git-svn: Make following parents atomic

[Deskin Miller]

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>
---
Patch is based on maint.

This was a nasty bug that took some work to figure out; I knew two
git-svn copies had diverged, but though I could look at the commits
where they diverged there was no good way to figure out what git-svn had
been doing at that point.  I ended up writing a script to automate the
process and save information for me to analyse later; I'll be posting an
announcement with further explanation, but the repository is available
at

git://git.deskinm.fdns.net/git-svn-bugfix.git

Deskin Miller

 git-svn.perl                     |   14 +++++++++++---
 t/t9104-git-svn-follow-parent.sh |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 56238da..c53d864 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2318,9 +2318,17 @@ 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;
diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
index 4d964e2..8e7b95b 100755
--- a/t/t9104-git-svn-follow-parent.sh
+++ b/t/t9104-git-svn-follow-parent.sh
@@ -149,6 +149,39 @@ 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 &&
+	cd stunk &&
+	echo "trunk stunk" > readme &&
+	svn add readme &&
+	cd .. &&
+	svn ci -m "trunk stunk" &&
+	echo "stunk like junk" >> stunk/readme &&
+	svn ci -m "really stunk" &&
+	cd .. &&
+	svn copy -m "stunk flunked" "$svnrepo"/stunk "$svnrepo"/flunk &&
+	git svn init --minimize-url -i stunk "$svnrepo"/stunk &&
+	git svn fetch -i stunk &&
+	git update-ref refs/remotes/flunk@17 refs/remotes/stunk~1 &&
+	git update-ref -d refs/remotes/stunk &&
+	git config --unset svn-remote.svn.fetch stunk &&
+	mkdir -p "$GIT_DIR"/svn/flunk@17 &&
+	rev_map=$(cd "$GIT_DIR"/svn/stunk && ls .rev_map*) &&
+	dd if="$GIT_DIR"/svn/stunk/$rev_map \
+           of="$GIT_DIR"/svn/flunk@17/$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 &&
+	test "`git rev-parse --verify refs/remotes/flunk@17`" \
+           = "`git rev-parse --verify refs/remotes/stunk`" &&
+	test "`git rev-parse --verify refs/remotes/flunk~1`" \
+           = "`git rev-parse --verify refs/remotes/stunk`"
+	'
+
 test_expect_success "track multi-parent paths" '
 	svn cp -m "resurrect /glob" "$svnrepo"/r9270 "$svnrepo"/glob &&
 	git-svn multi-fetch &&
-- 
1.6.1.rc1.45.g123ed

[ANNOUNCE] git-svn-bugfix script (Re: [PATCH] git-svn: Make following parents atomic)

[Deskin Miller]

git-svn has some bugs where it won't create identical commits in
different git-svn copies of the same svn history, despite all relevant
configuration being identical; oftentimes, the copies will diverge from
each other at some point.  My theory for a long time was that
interrupting git svn fetch could cause this, and it turns out I was
right in one case, but since it's not something I could easily interrupt
my normal workflow with to do forensics when it occurred, I ended up
writing a script to repeatedly fetch from a certain svn repository, and
compare refs to a supposedly pristine fetch until the refs diverged or
one fetched all the svn history; then, rinse and repeat the process from
the beginning.  It's available at

git://git.deskinm.fdns.net/git-svn-bugfix.git

Using this script, r3924 of SVN's svn repository flagged one bug
repeatedly, for which I've posted a patch.  I'm posting the repo because
there are other places where history diverges that I've not had a chance
to debug yet, so others should feel free to use the script to find and
fix them.  If anyone feels inclined, I'll gladly take patches to the
script, but I don't really care to handle data or bug reports you
generate with it (at least not at this point); I can generate plenty of
data myself.

Deskin Miller

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

[Deskin Miller]

On Sun, Dec 07, 2008 at 11:06:10AM -0500, Deskin Miller wrote:
> [...]
>
> 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.

On looking at the patch again, I think I might have introduced a bug:
it'll take the most commit on the parent branch, even if it was branched
from an earlier point.  I'll spend more time looking at it and should
have a v2 in a day at most if I'm rigth, hopefully more like a few
hours.

Deskin Miller

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

[Deskin Miller]

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>
---
Fixes the bug I found after sending v1.  I squashed in a check for it
into the testcase; if it's preferable I can split it into its own
testcase (this one runs quite long as-is).

Deskin Miller

 git-svn.perl                     |   16 ++++++++++---
 t/t9104-git-svn-follow-parent.sh |   43 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 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..45138a2 100755
--- a/t/t9104-git-svn-follow-parent.sh
+++ b/t/t9104-git-svn-follow-parent.sh
@@ -149,6 +149,49 @@ 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 &&
+	cd stunk &&
+	echo "trunk stunk" > readme &&
+	svn add readme &&
+	cd .. &&
+	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" &&
+	cd .. &&
+	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.rc1.45.g123ed

Re: [PATCH v2] git-svn: Make following parents atomic

[Junio C Hamano]

Deskin Miller <deskinm@umich.edu> writes:

> +test_expect_success "follow-parent is atomic" '
> +	cd wc &&
> +	svn up &&
> +	svn mkdir stunk &&
> +	cd stunk &&
> +	echo "trunk stunk" > readme &&
> +	svn add readme &&
> +	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.

> +	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" &&
> +	cd .. &&
> +	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) &&

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?

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

[Deskin Miller]

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

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

[Eric Wong]

Deskin Miller <deskinm@umich.edu> wrote:
> 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>

Looks good Deskin, thanks

Acked-by: Eric Wong <normalperson@yhbt.net>

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

[Thomas Jarosch]

On Tuesday, 9. December 2008 00:35:23 you wrote:
> > 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>
>
> Looks good Deskin, thanks

This patch has a very nice side effect, it seems to fix a long standing 
problem with subversion imports. Here's the original report:
https://kerneltrap.org/mailarchive/git/2008/4/8/1377514/thread

Many of the 121 tags in my SVN tree were created by cvs2svn,
which often created tags by copying older revisions
of sub paths into the current tree. 

I've written a small script that checks out the same tag via git and SVN.
It runs a diff against those two trees and saves the result to a file
so I can manually check it. With git-svn from 1.6.0.5, the results are 
horrible: Over 30% of the tags didn't match the code in SVN.

With git-svn from 1.6.1rc3, my first two manual probes look very good.
Right now I'm reimporting the svn tree and will have the results
of the complete "checkout comparison" tomorrow.

Cheers,
Thomas

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

[Thomas Jarosch]

On Tuesday, 16. December 2008 14:22:44 Thomas Jarosch wrote:
> This patch has a very nice side effect, it seems to fix a long standing
> problem with subversion imports. Here's the original report:
> https://kerneltrap.org/mailarchive/git/2008/4/8/1377514/thread
>
> Many of the 121 tags in my SVN tree were created by cvs2svn,
> which often created tags by copying older revisions
> of sub paths into the current tree.
>
> I've written a small script that checks out the same tag via git and SVN.
> It runs a diff against those two trees and saves the result to a file
> so I can manually check it. With git-svn from 1.6.0.5, the results are
> horrible: Over 30% of the tags didn't match the code in SVN.
>
> With git-svn from 1.6.1rc3, my first two manual probes look very good.
> Right now I'm reimporting the svn tree and will have the results
> of the complete "checkout comparison" tomorrow.

Yipeee, our SVN repository is fully migrated to git and split into handy 3-5GB 
repositories. All the git tags match the code from the SVN tags,
so I guess this was a good stress test for git-svn 1.6.0.6 :-)

Cheers,
Thomas