[PATCH 1/2] git-svn: Fix merge detecting with rewrite-root

[PATCH 1/2] git-svn: Fix merge detecting with rewrite-root

[Tuomas Suutari]

Detecting of merges from svn:mergeinfo or svk merge tickets failed
with rewrite-root option. This fixes it.

Signed-off-by: Tuomas Suutari <tuomas.suutari@gmail.com>
---
Hi again,

now I found another problem while importing SVN repo with git-svn.

To speed-up the import, I copied the SVN repo with rsync to localhost
and used file:// URL for the import, but because I want to be able to
track the history with svn+ssh:// later, I used the rewrite-root
option. That seemed to break the merge detecting.

With this patch the merge detecting works also with rewrite-root
option, but since there are no comments why the $self->rewrite_root
was used in the first place, I have no idea, if this is the right
thing to do.

 git-svn.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 265852f..1cbddca 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2993,7 +2993,7 @@ sub find_extra_svk_parents {
 	for my $ticket ( @tickets ) {
 		my ($uuid, $path, $rev) = split /:/, $ticket;
 		if ( $uuid eq $self->ra_uuid ) {
-			my $url = $self->rewrite_root || $self->{url};
+			my $url = $self->{url};
 			my $repos_root = $url;
 			my $branch_from = $path;
 			$branch_from =~ s{^/}{};
@@ -3201,7 +3201,7 @@ sub find_extra_svn_parents {
 	# are now marked as merge, we can add the tip as a parent.
 	my @merges = split "\n", $mergeinfo;
 	my @merge_tips;
-	my $url = $self->rewrite_root || $self->{url};
+	my $url = $self->{url};
 	my $uuid = $self->ra_uuid;
 	my %ranges;
 	for my $merge ( @merges ) {
-- 
1.7.0.2.ged48

[PATCH 2/2] t9150,t9151: Add rewrite-root option to init

[Tuomas Suutari]

The rewrite-root option seems to be a bit problematic with merge
detecting, so it's better to have a merge detecting test with it
turned on.

Signed-off-by: Tuomas Suutari <tuomas.suutari@gmail.com>
---

This is mainly for supporting the first patch. Try applying this
before the first patch to see the problem it addresses.

After this is applied, then there are no merge detecting tests without
rewrite-root. Could this be a problem?

 t/t9150-svk-mergetickets.sh |    1 +
 t/t9151-svn-mergeinfo.sh    |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/t/t9150-svk-mergetickets.sh b/t/t9150-svk-mergetickets.sh
index 5358142..24c2421 100755
--- a/t/t9150-svk-mergetickets.sh
+++ b/t/t9150-svk-mergetickets.sh
@@ -11,6 +11,7 @@ test_expect_success 'load svk depot' "
 	svnadmin load -q '$rawsvnrepo' \
 	  < '$TEST_DIRECTORY/t9150/svk-merge.dump' &&
 	git svn init --minimize-url -R svkmerge \
+	  --rewrite-root=http://svn.example.org \
 	  -T trunk -b branches '$svnrepo' &&
 	git svn fetch --all
 	"
diff --git a/t/t9151-svn-mergeinfo.sh b/t/t9151-svn-mergeinfo.sh
index 3569c62..c415775 100755
--- a/t/t9151-svn-mergeinfo.sh
+++ b/t/t9151-svn-mergeinfo.sh
@@ -11,6 +11,7 @@ test_expect_success 'load svn dump' "
 	svnadmin load -q '$rawsvnrepo' \
 	  < '$TEST_DIRECTORY/t9151/svn-mergeinfo.dump' &&
 	git svn init --minimize-url -R svnmerge \
+	  --rewrite-root=http://svn.example.org \
 	  -T trunk -b branches '$svnrepo' &&
 	git svn fetch --all
 	"
-- 
1.7.0.2.ged48

Re: [PATCH 1/2] git-svn: Fix merge detecting with rewrite-root

[Eric Wong]

Tuomas Suutari <tuomas.suutari@gmail.com> wrote:
> Detecting of merges from svn:mergeinfo or svk merge tickets failed
> with rewrite-root option. This fixes it.
> 
> Signed-off-by: Tuomas Suutari <tuomas.suutari@gmail.com>
> ---
> Hi again,
> 
> now I found another problem while importing SVN repo with git-svn.
> 
> To speed-up the import, I copied the SVN repo with rsync to localhost
> and used file:// URL for the import, but because I want to be able to
> track the history with svn+ssh:// later, I used the rewrite-root
> option. That seemed to break the merge detecting.
> 
> With this patch the merge detecting works also with rewrite-root
> option, but since there are no comments why the $self->rewrite_root
> was used in the first place, I have no idea, if this is the right
> thing to do.

Hi Tuomas,

I'm not sure why rewrite_root is used here, either.  Ignoring it
seems correct but I'll wait for Sam to chime in.

>  git-svn.perl |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index 265852f..1cbddca 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -2993,7 +2993,7 @@ sub find_extra_svk_parents {
>  	for my $ticket ( @tickets ) {
>  		my ($uuid, $path, $rev) = split /:/, $ticket;
>  		if ( $uuid eq $self->ra_uuid ) {
> -			my $url = $self->rewrite_root || $self->{url};
> +			my $url = $self->{url};
>  			my $repos_root = $url;
>  			my $branch_from = $path;
>  			$branch_from =~ s{^/}{};
> @@ -3201,7 +3201,7 @@ sub find_extra_svn_parents {
>  	# are now marked as merge, we can add the tip as a parent.
>  	my @merges = split "\n", $mergeinfo;
>  	my @merge_tips;
> -	my $url = $self->rewrite_root || $self->{url};
> +	my $url = $self->{url};
>  	my $uuid = $self->ra_uuid;
>  	my %ranges;
>  	for my $merge ( @merges ) {
> -- 
> 1.7.0.2.ged48

Re: [PATCH 1/2] git-svn: Fix merge detecting with rewrite-root

[Tuomas Suutari]

Hi Sam,

Could you please provide your feedback on this?

Thanks!

On Fri 2010-02-26 11:43:17 Eric Wong wrote:
> Tuomas Suutari <tuomas.suutari@gmail.com> wrote:
> > Detecting of merges from svn:mergeinfo or svk merge tickets failed
> > with rewrite-root option. This fixes it.
> >
> > Signed-off-by: Tuomas Suutari <tuomas.suutari@gmail.com>
> > ---
> > Hi again,
> >
> > now I found another problem while importing SVN repo with git-svn.
> >
> > To speed-up the import, I copied the SVN repo with rsync to localhost
> > and used file:// URL for the import, but because I want to be able to
> > track the history with svn+ssh:// later, I used the rewrite-root
> > option. That seemed to break the merge detecting.
> >
> > With this patch the merge detecting works also with rewrite-root
> > option, but since there are no comments why the $self->rewrite_root
> > was used in the first place, I have no idea, if this is the right
> > thing to do.
> 
> Hi Tuomas,
> 
> I'm not sure why rewrite_root is used here, either.  Ignoring it
> seems correct but I'll wait for Sam to chime in.
> 
> >  git-svn.perl |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/git-svn.perl b/git-svn.perl
> > index 265852f..1cbddca 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -2993,7 +2993,7 @@ sub find_extra_svk_parents {
> >  	for my $ticket ( @tickets ) {
> >  		my ($uuid, $path, $rev) = split /:/, $ticket;
> >  		if ( $uuid eq $self->ra_uuid ) {
> > -			my $url = $self->rewrite_root || $self->{url};
> > +			my $url = $self->{url};
> >  			my $repos_root = $url;
> >  			my $branch_from = $path;
> >  			$branch_from =~ s{^/}{};
> > @@ -3201,7 +3201,7 @@ sub find_extra_svn_parents {
> >  	# are now marked as merge, we can add the tip as a parent.
> >  	my @merges = split "\n", $mergeinfo;
> >  	my @merge_tips;
> > -	my $url = $self->rewrite_root || $self->{url};
> > +	my $url = $self->{url};
> >  	my $uuid = $self->ra_uuid;
> >  	my %ranges;
> >  	for my $merge ( @merges ) {
> 

Re: [PATCH 1/2] git-svn: Fix merge detecting with rewrite-root

[Sam Vilain]

I may have been cargo culting nearby similar code blocks; treat it as
suspicious.

The idea with the rewrite_root support is for SVK and/or svnsync; these
first copy revisions to a local repository, which you then point git-svn
at.  I wrote a long piece on this, on this (quite dated now) guide to
converting from SVK to git: of particular relevance is this section:
http://utsl.gen.nz/talks/git-svn/intro.html#howto-fetch-convert

So.  I predict that if this is not done correctly, then any SVK or
svnsync mirrors, when imported via git-svn, will not correctly detect
standard SVN merge attributes present in the original repository.  This
should be relatively easy to construct a test case for, using a similar
style to the t9110, t9111, t9150 and t9151 tests.  ie, make a script
which constructs a repository in the desired format, and then test that
it imports as it should.

That's a lot of work to go through however for a class of users which
may not even exist.  So, in the meantime it may be more appropriate to
go with the simpler approach which does not try to handle this case
correctly, until someone wants to do the above work.

Does that help?
Sam 


On Sat, 2010-03-13 at 08:54 +0200, Tuomas Suutari wrote:
> Hi Sam,
> 
> Could you please provide your feedback on this?
> 
> Thanks!
> 
> On Fri 2010-02-26 11:43:17 Eric Wong wrote:
> > Tuomas Suutari <tuomas.suutari@gmail.com> wrote:
> > > Detecting of merges from svn:mergeinfo or svk merge tickets failed
> > > with rewrite-root option. This fixes it.
> > >
> > > Signed-off-by: Tuomas Suutari <tuomas.suutari@gmail.com>
> > > ---
> > > Hi again,
> > >
> > > now I found another problem while importing SVN repo with git-svn.
> > >
> > > To speed-up the import, I copied the SVN repo with rsync to localhost
> > > and used file:// URL for the import, but because I want to be able to
> > > track the history with svn+ssh:// later, I used the rewrite-root
> > > option. That seemed to break the merge detecting.
> > >
> > > With this patch the merge detecting works also with rewrite-root
> > > option, but since there are no comments why the $self->rewrite_root
> > > was used in the first place, I have no idea, if this is the right
> > > thing to do.
> > 
> > Hi Tuomas,
> > 
> > I'm not sure why rewrite_root is used here, either.  Ignoring it
> > seems correct but I'll wait for Sam to chime in.
> > 
> > >  git-svn.perl |    4 ++--
> > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/git-svn.perl b/git-svn.perl
> > > index 265852f..1cbddca 100755
> > > --- a/git-svn.perl
> > > +++ b/git-svn.perl
> > > @@ -2993,7 +2993,7 @@ sub find_extra_svk_parents {
> > >  	for my $ticket ( @tickets ) {
> > >  		my ($uuid, $path, $rev) = split /:/, $ticket;
> > >  		if ( $uuid eq $self->ra_uuid ) {
> > > -			my $url = $self->rewrite_root || $self->{url};
> > > +			my $url = $self->{url};
> > >  			my $repos_root = $url;
> > >  			my $branch_from = $path;
> > >  			$branch_from =~ s{^/}{};
> > > @@ -3201,7 +3201,7 @@ sub find_extra_svn_parents {
> > >  	# are now marked as merge, we can add the tip as a parent.
> > >  	my @merges = split "\n", $mergeinfo;
> > >  	my @merge_tips;
> > > -	my $url = $self->rewrite_root || $self->{url};
> > > +	my $url = $self->{url};
> > >  	my $uuid = $self->ra_uuid;
> > >  	my %ranges;
> > >  	for my $merge ( @merges ) {
> > 

Re: [PATCH 1/2] git-svn: Fix merge detecting with rewrite-root

[Eric Wong]

Tuomas Suutari <tuomas.suutari@gmail.com> wrote:
> Detecting of merges from svn:mergeinfo or svk merge tickets failed
> with rewrite-root option. This fixes it.
> 
> Signed-off-by: Tuomas Suutari <tuomas.suutari@gmail.com>

> With this patch the merge detecting works also with rewrite-root
> option, but since there are no comments why the $self->rewrite_root
> was used in the first place, I have no idea, if this is the right
> thing to do.

Thanks Tuomas, and Sam for the clarification,

This series acked and pushed out to git://git.bogomips.org/git-svn

-- 
Eric Wong

Re: [PATCH 1/2] git-svn: Fix merge detecting with rewrite-root

[Junio C Hamano]

Thanks; pulled.