Re: [PATCH 2/5] svn-fe: allow svnadmin instead of svnrdump in svn helper

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Jul 6, 2011, 11:11 UTC
Message-ID: <CALkWK0=UKvvVVeRKcMq1L5ywV0WcAn4BNvEeQuTTZtZAmubanA@mail.gmail.com>
In-Reply-To: <CA+gfSn-mX-Uny1dy=1Xwo42r_pTYosWJibNGafV=sLrsmc-e+A@mail.gmail.com>

Hi again,

Dmitry Ivankov writes:

Show 15 quoted lines

> On Wed, Jul 6, 2011 at 4:12 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
>> svnrdump's output is not identical to svnadmin's output.  You'd
>> perhaps want to document the differences, and the impact they might
>> have?
> Hm, I don't see much differences.
> Data format is the same SVN-fs-dump-format-version: 3. In my case I
> observe following differences:
> - svnadmin writes sha1 checksums along with md5 ones
> - hashtable dumps can have different elements dump order
> - svnrdump sets Prop-delta: true for empty props
> - text deltas can be encoded differently (does it affect svn:ann?)
> My svnadmin is version 1.6.16 (r1073529)
> svnrdump is from r1135490
>
> Anything else I'm missing in a simple dump of a repository root?

Yes, those are the differences. Both of us know this, but an new reader might naively assume that they are exactly the same -- that's why I suggested that you document the fact somewhere. Once you've documented it, it's clear that these subtle differences have no impact on how svn-fe deals with the streams.

Show 11 quoted lines

>> I saw this '--username', '--password' in other patches too.  I'm
>> probably missing context, but I'm curious to know why this is required
>> and what you're planning to do to fix it in the future.
> For repos with no read restrictions this isn't necessary, like those
> in the test script supplied.
> It's a placeholder for quick local patch if anyone wants to test a
> repo with permissions.
> In future it should be allowed to configure credentials in
> corresponding remote section of git.config and
> pass all these options to helpers. Using ~/.subversion/ configuration
> is a valid option too.

I see. However, I think you should just have one dedicated test for testing credentials -- once that passes, you don't need to complicate the other tests with information about credentials. Try to test only one aspect in each test; the report summarizing failed tests will make more sense then.

Thanks.

-- Ram