Regression: git-svn clone failure

Regression: git-svn clone failure

[Andrew Myrick]

[Resending because I forgot to make the message plain text]

I was testing the latest changes to git-svn pushed to Eric's repo (git://git.bogomips.org/git-svn) by cloning a few other projects that I work on, and one of those clones failed where it had succeeded with git 1.6.5.  The error message I received is:

W:svn cherry-pick ignored (/branches/BranchA:3933-3950) - missing 1 commit(s) (eg 3fc50d3a7e0f555547ab34bb570db47ce71e1abb)
W:svn cherry-pick ignored (/branches/BranchB:3951-3970) - missing 1 commit(s) (eg 3beb9f2fde0a91aa0e8097e05f9054b23b221daf)
W:svn cherry-pick ignored (/branches/BranchC:3971-3985) - missing 1 commit(s) (eg a7ae202254604f8a78cca391be36c58efc79eb20)
Found merge parent (svn:mergeinfo prop): 8b2cf9e9250b5ff1fe47c68215d0a178cfe35a3b
Found merge parent (svn:mergeinfo prop): 59f8c571ae77885469bb31f007b0048ee7812e07
fatal: ambiguous argument '0..1': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions
rev-list -1 0..1: command returned error: 128

At this point, the clone got stuck in a loop and I had to kill it.

Note that all of the projects I cloned had "svn cherry-pick ignored" warnings sprinkled throughout the fetch logs; I'm not sure how much they matter.  It comes from find_extra_svn_parents(), which I would guess is a best-effort algorithm, and any failures to detect extra parents aren't anything to worry about.

Does anyone have suggestions on how I can debug this?  If you want to poke around, I can't provide access to the repository, but I can run commands and relay (sanitized) output if it will aid in debugging.

-Andrew

Re: Regression: git-svn clone failure

[Eric Wong]

Andrew Myrick <amyrick@apple.com> wrote:
> [Resending because I forgot to make the message plain text]
> 
> I was testing the latest changes to git-svn pushed to Eric's repo
> (git://git.bogomips.org/git-svn) by cloning a few other projects that
> I work on, and one of those clones failed where it had succeeded with
> git 1.6.5.  The error message I received is:
> 
> W:svn cherry-pick ignored (/branches/BranchA:3933-3950) - missing 1 commit(s) (eg 3fc50d3a7e0f555547ab34bb570db47ce71e1abb)
> W:svn cherry-pick ignored (/branches/BranchB:3951-3970) - missing 1 commit(s) (eg 3beb9f2fde0a91aa0e8097e05f9054b23b221daf)
> W:svn cherry-pick ignored (/branches/BranchC:3971-3985) - missing 1 commit(s) (eg a7ae202254604f8a78cca391be36c58efc79eb20)
> Found merge parent (svn:mergeinfo prop): 8b2cf9e9250b5ff1fe47c68215d0a178cfe35a3b
> Found merge parent (svn:mergeinfo prop): 59f8c571ae77885469bb31f007b0048ee7812e07
> fatal: ambiguous argument '0..1': unknown revision or path not in the working tree.
> Use '--' to separate paths from revisions
> rev-list -1 0..1: command returned error: 128

Hi Andrew,

That looks like a simple error, does the following patch help?

diff --git a/git-svn.perl b/git-svn.perl
index 3670960..dba0d12 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3163,7 +3163,8 @@ sub find_extra_svn_parents {
 				next unless $new_parents[$i];
 				next unless $new_parents[$j];
 				my $revs = command_oneline(
-					"rev-list", "-1", "$i..$j",
+					"rev-list", "-1",
+					"$new_parents[$i]..$new_parents[$j]",
 				       );
 				if ( !$revs ) {
 					undef($new_parents[$i]);


Unfortunately I don't know my way around the rest of this code well
so I shall defer to Sam if it's something else...
-- 
Eric Wong

Re: Regression: git-svn clone failure

[Andrew Myrick]

On Dec 22, 2009, at 11:21 AM, Eric Wong wrote:
> That looks like a simple error, does the following patch help?
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index 3670960..dba0d12 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -3163,7 +3163,8 @@ sub find_extra_svn_parents {
> 				next unless $new_parents[$i];
> 				next unless $new_parents[$j];
> 				my $revs = command_oneline(
> -					"rev-list", "-1", "$i..$j",
> +					"rev-list", "-1",
> +					"$new_parents[$i]..$new_parents[$j]",
> 				       );
> 				if ( !$revs ) {
> 					undef($new_parents[$i]);
> 
> 

Worked like a charm; the fetch is proceeding now.  Thanks, Eric!

Do you know what the "svn cherry-pick ignored" warnings mean, and if it's something I should be concerned about?  This particular project is missing up to 65 commits at some revisions.

-Andrew

Re: Regression: git-svn clone failure

[Eric Wong]

Andrew Myrick <amyrick@apple.com> wrote:
> On Dec 22, 2009, at 11:21 AM, Eric Wong wrote:
> > That looks like a simple error, does the following patch help?

<snip>

> Worked like a charm; the fetch is proceeding now.  Thanks, Eric!

Awesome, tanks for the feedback, Andrew.

I've pushed out a proper commit to git://git.bogomips.org/git-svn
for Junio (which also contains the previous pull request).

Andrew Myrick (1):
      git-svn: Remove obsolete MAXPARENT check

Eric Wong (3):
      git svn: fix --revision when fetching deleted paths
      update release notes for git svn in 1.6.6
      git svn: lookup new parents correctly from svn:mergeinfo

Sam Vilain (5):
      git-svn: expand the svn mergeinfo test suite, highlighting some failures
      git-svn: memoize conversion of SVN merge ticket info to git commit ranges
      git-svn: fix some mistakes with interpreting SVN mergeinfo commit ranges
      git-svn: exclude already merged tips using one rev-list call
      git-svn: detect cherry-picks correctly.

> Do you know what the "svn cherry-pick ignored" warnings mean, and if
> it's something I should be concerned about?  This particular project
> is missing up to 65 commits at some revisions.

Definitely a question for Sam :)

-- 
Eric Wong

Re: Regression: git-svn clone failure

[Sam Vilain]

On Tue, 2009-12-22 at 11:38 -0800, Andrew Myrick wrote:
> Worked like a charm; the fetch is proceeding now.  Thanks, Eric!
> 
> Do you know what the "svn cherry-pick ignored" warnings mean, and if it's
>  something I should be concerned about?  This particular project is missing
>  up to 65 commits at some revisions.

With git, merge parent relationships imply (conceptually, anyway) that
all of the changes reachable from that branch are included in the
commit.  If someone is doing cherry-picking, then they are specifically
excluding some commits, so adding a merge parent to that branch isn't
right.  This is what the warning is saying.  It's happening every commit
because that section of code doesn't know whether a mergeinfo record is
new or not.

This wasn't happening with the old code, because it was simply not
detecting them correctly and adding merge parents anyway.

However in the case that someone is merging from another branch, merging
most commits, and only skipping a few, then it may make sense to record
it as a real merge.  Here we start getting into non-deterministic
conversion; I had to do this for perl.git, because the merge records
weren't reliable.  Basically I had the script, when the amount of merged
records was within a certain window, prompt me to ask me whether - based
on the change comment and outstanding files to merge - whether it should
be recorded as a real merge or not.  Then, depending on which option I
picked, it would write out to the commit message a note of which files
were not *actually* merged in that commit.

Something like the below change might be the right thing for you, it
might not - before using it, make sure you keep a complete copy of your
git-svn clone so you can restart if required.  Run it for a bit and
inspect the results.  Basically considers a 90% merge "good enough" and
records the differences in the log.  It could be possible to record the
cherry-pick information in the commit message, too - but we'd need to
also know which merge records were *added* in the current commit.
Actually, knowing that would make the whole thing much faster anyway, so
perhaps we need to bite the bullet and record it somewhere in the
metadata.

Anyway, this change may work - it doesn't break the test suite so that's
a good sign.  But hopefully it should give you an idea of the direction
things could have to take.  Perhaps you can see why I built a
high-performance fastimport importer for perl.git...

Subject: [PATCH] git-svn: consider 90% of a branch cherry picked to be a merge

Be slightly fuzzy when deciding if a branch is a merge or a cherry pick; in
some instances this might indicate intentionally skipping changes as not
required, as if they had performed a real merge and then skipped those
files.

Signed-off-by: Sam Vilain <sam@vilain.net>
---
 git-svn.perl |   31 ++++++++++++++++++++++++++-----
 1 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index f06e535..3064504 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2562,6 +2562,10 @@ sub do_git_commit {
 	unless ($self->no_metadata) {
 		print $msg_fh "\ngit-svn-id: $log_entry->{metadata}\n"
 		              or croak $!;
+		if ($log_entry->{merge_notes}) {
+			print $msg_fh "\ngit-svn-merge: $log_entry->{merge_notes}\n"
+		              or croak $!;
+		}
 	}
 	$msg_fh->flush == 0 or croak $!;
 	close $msg_fh or croak $!;
@@ -3027,10 +3031,11 @@ sub check_cherry_pick {
 	my @ranges = @_;
 	my %commits = map { $_ => 1 }
 		_rev_list("--no-merges", $tip, "--not", $base);
+	my $before = keys %commits;
 	for my $range ( @ranges ) {
 		delete @commits{_rev_list($range)};
 	}
-	return (keys %commits);
+	return ($before, keys %commits);
 }
 
 BEGIN {
@@ -3103,6 +3108,8 @@ sub find_extra_svn_parents {
 	my %excluded = map { $_ => 1 }
 		parents_exclude($parents, grep { defined } @merge_tips);
 
+	my @merge_warnings;
+
 	# check merge tips for new parents
 	my @new_parents;
 	for my $merge_tip ( @merge_tips ) {
@@ -3118,14 +3125,25 @@ sub find_extra_svn_parents {
 		       );
 
 		# double check that there are no missing non-merge commits
-		my (@incomplete) = check_cherry_pick(
+		my ($total, @incomplete) = check_cherry_pick(
 			$merge_base, $merge_tip,
 			@$ranges,
 		       );
 
-		if ( @incomplete ) {
+		if ( @incomplete and @incomplete > ($total*0.10) ) {
 			warn "W:svn cherry-pick ignored ($spec) - missing "
-				.@incomplete." commit(s) (eg $incomplete[0])\n";
+				.@incomplete."/$total commit(s) (eg $incomplete[0])\n";
+			# XXX - can't do this, it will appear every time;
+			# we need to know this record was added this commit
+			#push @merge_warnings, "picked: ". join(" ",
+			#     map { my $x=$_; $x=~
+			#	s{([a-f0-9]{12})[a-f0-9]+}{$1}g } @$ranges)
+		} elsif ( @incomplete ) {
+			warn "W:treating svn cherry-pick as merge "
+				.@incomplete."/$total commit(s) included\n";
+			push @merge_warnings, "skipped: ".
+				join(" ", map { substr $_, 0, 12 } @incomplete)
+				.")";
 		} else {
 			warn
 				"Found merge parent (svn:mergeinfo prop): ",
@@ -3151,6 +3169,7 @@ sub find_extra_svn_parents {
 		}
 	}
 	push @$parents, grep { defined } @new_parents;
+	return ( @merge_warnings ? join("; ", @merge_warnings) : undef );
 }
 
 sub make_log_entry {
@@ -3159,6 +3178,7 @@ sub make_log_entry {
 
 	my @parents = @$parents;
 	my $ps = $ed->{path_strip} || "";
+	my $merge_notes;
 	for my $path ( grep { m/$ps/ } %{$ed->{dir_prop}} ) {
 		my $props = $ed->{dir_prop}{$path};
 		if ( $props->{"svk:merge"} ) {
@@ -3166,7 +3186,7 @@ sub make_log_entry {
 				($ed, $props->{"svk:merge"}, \@parents);
 		}
 		if ( $props->{"svn:mergeinfo"} ) {
-			$self->find_extra_svn_parents
+			$merge_notes = $self->find_extra_svn_parents
 				($ed,
 				 $props->{"svn:mergeinfo"},
 				 \@parents);
@@ -3269,6 +3289,7 @@ sub make_log_entry {
 	$log_entry{email} = $email;
 	$log_entry{commit_name} = $commit_name;
 	$log_entry{commit_email} = $commit_email;
+	$log_entry{merge_notes} = $merge_notes;
 	\%log_entry;
 }
 
-- 
1.6.3.3

Re: Regression: git-svn clone failure

[Junio C Hamano]

Sam Vilain <sam@vilain.net> writes:

> With git, merge parent relationships imply (conceptually, anyway) that
> all of the changes reachable from that branch are included in the
> commit.  If someone is doing cherry-picking, then they are specifically
> excluding some commits, so adding a merge parent to that branch isn't
> right.  This is what the warning is saying.  It's happening every commit
> because that section of code doesn't know whether a mergeinfo record is
> new or not.
> ...
> Subject: [PATCH] git-svn: consider 90% of a branch cherry picked to be a merge
>
> Be slightly fuzzy when deciding if a branch is a merge or a cherry pick; in
> some instances this might indicate intentionally skipping changes as not
> required, as if they had performed a real merge and then skipped those
> files.
>
> Signed-off-by: Sam Vilain <sam@vilain.net>

If I were _using_ git-svn (or any other tool), I would rather be forced to
see overlapping changes from both branches to sort out the conflict myself
when I merge such a cherry-picked history, rather than an automated but
unreliable operation that drops changes randomly, still records that
everything from the branch is now merged, and reports "everything is
peachy".

That sounds horrible, as you cannot trust your merges anymore.  I hope I
am mis-interpreting what you wrote above.

Re: Regression: git-svn clone failure

[Sam Vilain]

Junio C Hamano wrote:
> Sam Vilain <sam@vilain.net> writes:
>   
>> With git, merge parent relationships imply (conceptually, anyway) that all of the changes reachable from that branch are included in the commit.  If someone is doing cherry-picking, then they are specifically excluding some commits, so adding a merge parent to that branch isn't right.
>> ...
>> Subject: [PATCH] git-svn: consider 90% of a branch cherry picked to be a merge
>>
>> Be slightly fuzzy when deciding if a branch is a merge or a cherry pick; ... might ... if ...
>>
>> Signed-off-by: Sam Vilain <sam@vilain.net>
>>     
>
> If I were _using_ git-svn (or any other tool), I would rather be forced to ... sort out the conflict myself ... rather than an automated but unreliable operation that drops changes randomly, ... and reports "everything is peachy".
>
> That sounds horrible, as you cannot trust your merges anymore.  I hope I
> am mis-interpreting what you wrote above.
>   

Welcome to the world of SVN, Junio. It's a world of sunshine and
happiness, pain and despair.

Sam.

Re: Regression: git-svn clone failure

[Andrew Myrick]

On Dec 22, 2009, at 1:13 PM, Sam Vilain wrote:

> On Tue, 2009-12-22 at 11:38 -0800, Andrew Myrick wrote:
>> Worked like a charm; the fetch is proceeding now.  Thanks, Eric!
>> 
>> Do you know what the "svn cherry-pick ignored" warnings mean, and if it's
>> something I should be concerned about?  This particular project is missing
>> up to 65 commits at some revisions.
> 
> With git, merge parent relationships imply (conceptually, anyway) that
> all of the changes reachable from that branch are included in the
> commit.  If someone is doing cherry-picking, then they are specifically
> excluding some commits, so adding a merge parent to that branch isn't
> right.  This is what the warning is saying.  It's happening every commit
> because that section of code doesn't know whether a mergeinfo record is
> new or not.
> 
> This wasn't happening with the old code, because it was simply not
> detecting them correctly and adding merge parents anyway.

This makes perfect sense now.  Thank you for clarifying.  Unfortunately, I don't think the patch you provided will help my particular problem.  Allow me to elaborate.

As I mentioned before, my project's integration model is to create a separate branch for every change.  Specifically, we create a branch from a recent internal tag.  So, the model for a simple bug fix looks something like this:

            F---G  branch1
          /           \
        D  tag1  \    E  tag2
       /                 \  /
A---B                 C  trunk

Revision B on trunk was tagged with tag1.  A bug was found in that version, so a branch was created from tag1, a fix was committed to the branch, and then the branch was merged back to trunk.  Finally, trunk is tagged with tag2.

The "missing commit" messages show up when git svn fetch is fetching revision C.  It warns the there is a cherry-pick from branch1, and states that commits D and F are missing.  These commits are just copies, however; there is no code change.  The svn:mergeinfo property on trunk also only points at commit G.  Should git-svn be ignoring commits D and F, which are copy operations, not code changes?

Also of note is that we very, very rarely cherry-pick commits, and never directly from a branch to trunk.  Branches are always integrated back to trunk in their entirety.

-Andrew

Re: Regression: git-svn clone failure

[Sam Vilain]

Andrew Myrick wrote:
> This makes perfect sense now.  Thank you for clarifying.  Unfortunately, I don't think the patch you provided will help my particular problem.  Allow me to elaborate.
>
> As I mentioned before, my project's integration model is to create a separate branch for every change.  Specifically, we create a branch from a recent internal tag.  So, the model for a simple bug fix looks something like this:
>
>             F---G  branch1
>           /           \
>         D  tag1  \    E  tag2
>        /                 \  /
> A---B                 C  trunk
>
> Revision B on trunk was tagged with tag1.  A bug was found in that version, so a branch was created from tag1, a fix was committed to the branch, and then the branch was merged back to trunk.  Finally, trunk is tagged with tag2.
>
> The "missing commit" messages show up when git svn fetch is fetching revision C.  It warns the there is a cherry-pick from branch1, and states that commits D and F are missing.  These commits are just copies, however; there is no code change.  The svn:mergeinfo property on trunk also only points at commit G.  Should git-svn be ignoring commits D and F, which are copy operations, not code changes?
>
> Also of note is that we very, very rarely cherry-pick commits, and never directly from a branch to trunk.  Branches are always integrated back to trunk in their entirety.
>   

Ok. Yes, I can see that. I guess what the code needs to do then is
figure out if the missing changes didn't touch the tree, and exclude
them if that happens.

in the check_cherry_pick function, try putting at the end something like;

for my $commit (keys %commits) {
if (has_no_changes($commit)) {
delete $commits{$commit};
}
}

Before the return. has_no_changes should be:

sub has_no_changes {
my $commit = shift;
# merges should always have no changes, but more
# importantly $commit~1 won't be defined for them, so
# don't proceed if that is the case.
my $num_parents = split / /, command_oneline(
qw(rev-list --parents -1 -m), $commit,
);
return 0 if $num_parents > 1;
return (command_oneline("rev-parse", "$commit^{tree}")
eq command_oneline("rev-parse", "$commit~1^{tree}"));
}

has_no_changes should also be memoized. Cherry picking a single commit
from a large unrelated branch will be slow (using cat-file --batch could
help here, but that's not something I can hack out off the cuff like
this), the first time, then it will remember whether particular commits
have changes or not.

Good luck,
Sam

Re: [spf:guess] Re: Regression: git-svn clone failure

[Sam Vilain]

On Tue, 2009-12-22 at 11:21 -0800, Eric Wong wrote:
> That looks like a simple error, does the following patch help?
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index 3670960..dba0d12 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -3163,7 +3163,8 @@ sub find_extra_svn_parents {
>  				next unless $new_parents[$i];
>  				next unless $new_parents[$j];
>  				my $revs = command_oneline(
> -					"rev-list", "-1", "$i..$j",
> +					"rev-list", "-1",
> +					"$new_parents[$i]..$new_parents[$j]",
>  				       );

Yes, that is the intent.

Hrm, I'd have thought my test would have stepped over that code when it
merged in a branch which merged two an svn branch which included a merge
of another svn branch.  Obviously not!  I'll cook something up to cover
that..

Sam.