Re: [PATCH] Fix merge parent checking with svn.pushmergeinfo.

From: Jason Merrill <jason@redhat.com>
Date: Sep 15, 2017, 21:28 UTC
Message-ID: <CADzB+2mxyXcROYx72tac8cUxBMxi=ZxUNQYxpUR1CZ43e-j9gA@mail.gmail.com>
In-Reply-To: <20170915175248.GT27425@aiede.mtv.corp.google.com>

On Fri, Sep 15, 2017 at 1:52 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 16 quoted lines

> Hi,
>
> Jason Merrill wrote:
>
>> Subject: Fix merge parent checking with svn.pushmergeinfo.
>>
>> Without this fix, svn dcommit of a merge with svn.pushmergeinfo set would
>> get error messages like "merge parent <X> for <Y> is on branch
>> svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root
>> svn+ssh://jason@gcc.gnu.org/svn/gcc!"
>>
>> * git-svn.perl: Remove username from rooturl before comparing to branchurl.
>>
>> Signed-off-by: Jason Merrill <jason@redhat.com>
>
> Interesting.  Thanks for writing it.

Thanks for the review.

> Could there be a test for this to make sure this doesn't regress in
> the future?  See t/t9151-svn-mergeinfo.sh for some examples.

Hmm, I'm afraid figuring out how to write such a test would take longer than I can really spare for this issue. There don't seem to be any svn+ssh tests currently.

Show 9 quoted lines

> Nit: git doesn't use GNU-style changelogs, preferring to let the code
> speak for itself.  Maybe it would work better as the subject line?
> E.g. something like
>
>         git-svn: remove username from root before comparing to branch URL
>
>         Without this fix, ...
>
>         Signed-off-by: ...

How about this?

    git-svn: Fix svn.pushmergeinfo handling of svn+ssh usernames.

    Previously, svn dcommit of a merge with svn.pushmergeinfo set would
    get error messages like "merge parent <X> for <Y> is on branch
    svn+ssh://gcc.gnu.org/svn/gcc/trunk, which is not under the git-svn root
    svn+ssh://jason@gcc.gnu.org/svn/gcc!"

    So, let's call remove_username (as we do for svn info) before comparing
    rooturl to branchurl.

Show 16 quoted lines

>> ---
>>  git-svn.perl | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/git-svn.perl b/git-svn.perl
>> index fa42364785..1663612b1c 100755
>> --- a/git-svn.perl
>> +++ b/git-svn.perl
>> @@ -931,6 +931,7 @@ sub cmd_dcommit {
>>               # information from different SVN repos, and paths
>>               # which are not underneath this repository root.
>>               my $rooturl = $gs->repos_root;
>> +             Git::SVN::remove_username ($rooturl);
>
> style nit: Git doesn't include a space between function names and
> their argument list.

Fixed.

Show 10 quoted lines

> I wonder if it would make sense to rename the $rooturl variable
> since now it is not the unmodified root. E.g. how about
>
>                 my $expect_url = $gs->repos_root;
>                 Git::SVN::remove_username($expect_url);
>                 ...
>
>>               foreach my $d (@$linear_refs) {
>>                       my %parentshash;
>>                       read_commit_parents(\%parentshash, $d);

It isn't the unmodified root, but it is the effective root that is printed by svn info and used in branch URLs in git-svn-id, so it seems to me that the name $rooturl is still appropriate.

> The rest looks good.
>
> Thanks and hope that helps,
> Jonathan