Re: [PATCH] git-svn: find-rev and rebase for SVN::Mirror repositories

["João Abecasis" <joao@abecasis.name>]

Hi Eric,

Thanks for your reply and comments.

Eric Wong <normalperson@yhbt.net> wrote:
> João Abecasis <joao@abecasis.name> wrote:
>> 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 :(

I can relate to that ;-)

> The commit message should be wrapped at 72 characters or less.
> Some further notes below (mostly whitespace issues, but one
> regression).

Noted.

>>  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 :)

Done!

>>       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.

Okidoki. I just wasn't sure how wide was a tab ;-)

>> @@ -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.

Duh! Missed that. I had to make a couple of tweaks after the tests
were working, because I had some another test case that was breaking
somewhere. Anyway, my other test case (which is the one where I need
it to work) works without that change after all.

> Thanks for the patch, please let us know if that change is OK.

Let's go with that, since it works. If I do find a test case where it
breaks we'll fix it later.

Updated patch coming up.

Cheers,


João