Re: [PATCH] git-svn: find-rev and rebase for SVN::Mirror repositories
- From
- Eric Wong <normalperson@yhbt.net>
- Date
- Jul 14, 2008, 09:12 UTC
- Message-ID
- <20080714091243.GA27794@untitled>
- In-Reply-To
- <7bf6f1d20807081908kdf9f615taa532ae579b457d7@mail.gmail.com>
João Abecasis <joao@abecasis.name> wrote:
Show 16 quoted lines
> find-rev and rebase error out on svm because git-svn doesn't trace the original > svn revision numbers back to git commits. The updated test case, included in the > patch, shows the issue and passes with the rest of the patch applied. > > This fixes Git::SVN::find_by_url to find branches based on the svm:source URL, > where useSvmProps is set. Also makes sure cmd_find_rev and working_head_info use > the information they have to correctly track the source repository. This is > enough to get find-rev and rebase working. > > Signed-off-by: João Abecasis <joao@abecasis.name> > --- > Incidentally, I've tried submitting these fixes before, but failed to > get much attention, so I could be doing something wrong... Any input > on the patch or my approach to this list is appreciated. This time I > reworded the commit message and added Eric Wong to the CC list, since > he seems to be Ack'ing git-svn patches.
Hi João,
Sorry, I haven't been following the mailing list much lately, and even keeping up with my inbox is getting to be a chore :(
The commit message should be wrapped at 72 characters or less. Some further notes below (mostly whitespace issues, but one regression).
Show 5 quoted lines
> sub read_all_remotes {
> my $r = {};
> + my $usesvmprops = eval { command_oneline(qw/config --bool
> + svn.useSvmProps/) };
> + $usesvmprops = $usesvmprops eq 'true' if $usesvmprops;Always use tabs for indentation (but spaces for alignment is alright).
I'd also rather not propagate the crazy alllowercasewithoutunderscores convention of git-config into code, either. $uses_svm_props is much easier to parse when I'm half-awake :)
Show 30 quoted lines
> foreach (grep { s/^svn-remote\.// } command(qw/config -l/)) {
> if (m!^(.+)\.fetch=\s*(.*)\s*:\s*refs/remotes/(.+)\s*$!) {
> my ($remote, $local_ref, $remote_ref) = ($1, $2, $3);
> $local_ref =~ s{^/}{};
> $r->{$remote}->{fetch}->{$local_ref} = $remote_ref;
> + $r->{$remote}->{svm} = {} if $usesvmprops;
> + } elsif (m!^(.+)\.usesvmprops=\s*(.*)\s*$!) {
> + $r->{$1}->{svm} = {};
> } elsif (m!^(.+)\.url=\s*(.*)\s*$!) {
> $r->{$1}->{url} = $2;
> } elsif (m!^(.+)\.(branches|tags)=
> @@ -1437,6 +1443,21 @@ sub read_all_remotes {
> }
> }
> }
> +
> + map {
> + if (defined $r->{$_}->{svm}) {
> + my $svm;
> + eval {
> + my $section = "svn-remote.$_";
> + $svm = {
> + source => tmp_config('--get', "$section.svm-source"),
> + replace => tmp_config('--get', "$section.svm-replace"),
> + }
> + };
> + $r->{$_}->{svm} = $svm;
> + }
> + } keys %$r;
> +Please wrap all code at 80-columns.
Show 20 quoted lines
> @@ -1563,13 +1584,21 @@ sub find_by_url { # repos_root and, path are optional
> }
> my $p = $path;
> my $rwr = rewrite_root({repo_id => $repo_id});
> + my $svm = $remotes->{$repo_id}->{svm}
> + if defined $remotes->{$repo_id}->{svm};
> unless (defined $p) {
> $p = $full_url;
> my $z = $u;
> + my $prefix = '';
> if ($rwr) {
> $z = $rwr;
> + } elsif (defined $svm) {
> + $z = $svm->{source};
> + $prefix = $svm->{replace};
> + $prefix =~ s#^\Q$u\E(?:/|$)##;
> + $prefix =~ s#/$##;
> }
> - $p =~ s#^\Q$z\E(?:/|$)## or next;
> + $p =~ s#^\Q$z\E(?=/|$)#$prefix# or next;This broke t9100 for me at: 'able to dcommit to a subdirectory' Changing the ?= back to ?: works.
Thanks for the patch, please let us know if that change is OK.
-- Eric Wong