Merge lp://staging/~luoyonggang/bzr-svn/perfect-layout into lp://staging/bzr-svn/1.1

Hi,

On 14/06/11 12:57, Yonggang Luo wrote:
> Yonggang Luo has proposed merging lp:~luoyonggang/bzr-svn/perfect-layout into lp:bzr-svn.
>
> Requested reviews:
> Jelmer Vernooij (jelmer)
>
> For more details, see:
> https://code.launchpad.net/~luoyonggang/bzr-svn/perfect-layout/+merge/64530
>
> There is indent fixup.
>
> Mainly focused on get perfect working with bzr-svn's layout!
In general, please submit merge proposals for specific changes. That
makes it possible to merge those specific changes without having to
reject the entire MP (like this one).

You seem to be deleting a few things that are there for good reasons.
Please keep the following around:

* The --layout option in svn-import - removing it breaks existing users
who rely on it, or might have scripts that rely on it
* Guessing of layouts in "bzr svn-import". It's there for a reason - to
prevent users from shooting themselves in the foot.

I don't see why prefix isn't relevant for PerfectLayout; shouldn't users
be able to fetch just a part of the repository?

Please don't call your custom layout "perfect". It may be perfect for
you, it's not a good fit for everybody and that name doesn't really
explain how it works. What about "copytracking" ?

As we discussed earlier, please don't register your layout in the layout
registry as it needs to have the LogWalker passed in. Instead, please
add a configuration option for it, look for that option in
Repository.get_layout() and return PerfectLayout(self._log) from there.

Cheers,

Jelmer

2011/6/14 Jelmer Vernooij <email address hidden>

> Hi,
>
> On 14/06/11 12:57, Yonggang Luo wrote:
> > Yonggang Luo has proposed merging lp:~luoyonggang/bzr-svn/perfect-layout
> into lp:bzr-svn.
> >
> > Requested reviews:
> > Jelmer Vernooij (jelmer)
> >
> > For more details, see:
> >
> https://code.launchpad.net/~luoyonggang/bzr-svn/perfect-layout/+merge/64530
> >
> > There is indent fixup.
> >
> > Mainly focused on get perfect working with bzr-svn's layout!
> In general, please submit merge proposals for specific changes. That
> makes it possible to merge those specific changes without having to
> reject the entire MP (like this one).
>
I don't know how to do that.

>
> You seem to be deleting a few things that are there for good reasons.
> Please keep the following around:
>
> * The --layout option in svn-import - removing it breaks existing users
> who rely on it, or might have scripts that rely on it
> * Guessing of layouts in "bzr svn-import". It's there for a reason - to
> prevent users from shooting themselves in the foot.
>
Did not affecting of bzr svn-import . I've tested.
Why I ignore prefix? I am not ignore of it, just because of PerfectLayout
need using of it
PerfectLayout need manage of prefix by its self.

>
> I don't see why prefix isn't relevant for PerfectLayout; shouldn't users
> be able to fetch just a part of the repository?
>
> Please don't call your custom layout "perfect". It may be perfect for
> you, it's not a good fit for everybody and that name doesn't really
> explain how it works. What about "copytracking" ?
>
Err.. you should tell me earlier, such as the last discussion!

> As we discussed earlier, please don't register your layout in the layout
>
NO。。。it's not working, when I was trying, all kinds of problem is appeared..

> registry as it needs to have the LogWalker passed in. Instead, please
> add a configuration option for it, look for that option in
> Repository.get_layout() and return PerfectLayout(self._log) from there.
>
> Cheers,
>
> Jelmer
>
> --
> https://code.launchpad.net/~luoyonggang/bzr-svn/perfect-layout/+merge/64530
> You are the owner of lp:~luoyonggang/bzr-svn/perfect-layout.
>

--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

On 14/06/11 13:43, Yonggang Luo wrote:
> 2011/6/14 Jelmer Vernooij <email address hidden>
>
>
>> In general, please submit merge proposals for specific changes. That
>> makes it possible to merge those specific changes without having to
>> reject the entire MP (like this one).
>>
> I don't know how to do that.
I mean create different branches and propose them for merging against
lp:bzr separately.
>> You seem to be deleting a few things that are there for good reasons.
>> Please keep the following around:
>>
>> * The --layout option in svn-import - removing it breaks existing users
>> who rely on it, or might have scripts that rely on it
>> * Guessing of layouts in "bzr svn-import". It's there for a reason - to
>> prevent users from shooting themselves in the foot.
>>
> Did not affecting of bzr svn-import . I've tested.
> Why I ignore prefix? I am not ignore of it, just because of PerfectLayout
> need using of it
> PerfectLayout need manage of prefix by its self.
prefix is used to restrict which paths are imported. If I specify:

bzr svn-import svn://svn.llvm.org/llvm-gcc and svn://svn.llvm.org/ is
the repository root I expect only the llvm-gcc part of the repository to
be imported, and not any of the other projects. Ignoring prefix means
all projects in the repository will be imported.

>> I don't see why prefix isn't relevant for PerfectLayout; shouldn't users
>> be able to fetch just a part of the repository?
>>
>> Please don't call your custom layout "perfect". It may be perfect for
>> you, it's not a good fit for everybody and that name doesn't really
>> explain how it works. What about "copytracking" ?
>>
> Err.. you should tell me earlier, such as the last discussion!
? We never discussed the name of your layout before as far as I
remember, and we never looked at any code.

>> As we discussed earlier, please don't register your layout in the layout
>>
> NO。。。it's not working, when I was trying, all kinds of problem is appeared..
Can you be more specific, what exactly didn't work?

Cheers,

Jelmer

2011/6/14 Jelmer Vernooij <email address hidden>

> On 14/06/11 13:43, Yonggang Luo wrote:
> > 2011/6/14 Jelmer Vernooij <email address hidden>
> >
> >
> >> In general, please submit merge proposals for specific changes. That
> >> makes it possible to merge those specific changes without having to
> >> reject the entire MP (like this one).
> >>
> > I don't know how to do that.
> I mean create different branches and propose them for merging against
> lp:bzr separately.
>

That's too much complicated. It's just leading the development progress too
depression.
Is there any other way?

>> You seem to be deleting a few things that are there for good reasons.
> >> Please keep the following around:
> >>
> >> * The --layout option in svn-import - removing it breaks existing users
> >> who rely on it, or might have scripts that rely on it
> >> * Guessing of layouts in "bzr svn-import". It's there for a reason - to
> >> prevent users from shooting themselves in the foot.
> >>
> > Did not affecting of bzr svn-import . I've tested.
> > Why I ignore prefix? I am not ignore of it, just because of PerfectLayout
> > need using of it
> > PerfectLayout need manage of prefix by its self.
> prefix is used to restrict which paths are imported. If I specify:
>
> bzr svn-import svn://svn.llvm.org/llvm-gcc and svn://svn.llvm.org/ is
> the repository root I expect only the llvm-gcc part of the repository to
> be imported, and not any of the other projects. Ignoring prefix means
> all projects in the repository will be imported.
>
>
> >> I don't see why prefix isn't relevant for PerfectLayout; shouldn't users
> >> be able to fetch just a part of the repository?
> >>
> >> Please don't call your custom layout "perfect". It may be perfect for
> >> you, it's not a good fit for everybody and that name doesn't really
> >> explain how it works. What about "copytracking" ?
> >>
> > Err.. you should tell me earlier, such as the last discussion!
> ? We never discussed the name of your layout before as far as I
> remember, and we never looked at any code.
>
> You mentioned PerfectLayout at freenode, so I was think you were accepted
of it.

> >> As we discussed earlier, please don't register your layout in the
> layout
> >>
> > NO。。。it's not working, when I was trying, all kinds of problem is
> appeared..
> Can you be more specific, what exactly didn't work?
>
> The code in commands.py that I removed.

> Cheers,
>
> Jelmer
>
> --
> https://code.launchpad.net/~luoyonggang/bzr-svn/perfect-layout/+merge/64530
> You are the owner of lp:~luoyonggang/bzr-svn/perfect-layout.
>

--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

On 14/06/11 14:07, Yonggang Luo wrote:
> 2011/6/14 Jelmer Vernooij <email address hidden>
>
>> On 14/06/11 13:43, Yonggang Luo wrote:
>>> 2011/6/14 Jelmer Vernooij <email address hidden>
>>>
>>>
>>>> In general, please submit merge proposals for specific changes. That
>>>> makes it possible to merge those specific changes without having to
>>>> reject the entire MP (like this one).
>>>>
>>> I don't know how to do that.
>> I mean create different branches and propose them for merging against
>> lp:bzr separately.
>>
> That's too much complicated. It's just leading the development progress too
> depression.
> Is there any other way?
It's fine with me if you want to submit bigger branches, I was just
suggesting smaller branches since they can be landed independently.
>>>> I don't see why prefix isn't relevant for PerfectLayout; shouldn't users
>>>> be able to fetch just a part of the repository?
>>>>
>>>> Please don't call your custom layout "perfect". It may be perfect for
>>>> you, it's not a good fit for everybody and that name doesn't really
>>>> explain how it works. What about "copytracking" ?
>>>>
>>> Err.. you should tell me earlier, such as the last discussion!
>> ? We never discussed the name of your layout before as far as I
>> remember, and we never looked at any code.

> You mentioned PerfectLayout at freenode, so I was think you were accepted
> of it.
I mean we didn't explicitly discuss the name - I don't like it for the
reasons specified earlier.

Cheers,

Jelmer

Another problem, because of repo already have an function named with
get_layout
Why another get_layout resident at command.py?

I think they must be at the same place. It's easier to management.

--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

Why this error arising, it's looks weird.. I have no idea on this, on
another computer, it's works fine.
ssh -v luoyonggang@<email address hidden>
OpenSSH_5.8p1, OpenSSL 0.9.8r 8 Feb 2011
debug1: Reading configuration data /cygdrive/d/CI/docs/home/lyg/.ssh/config
debug1: Applying options for bazaar.launchpad.net
debug1: Connecting to bazaar.launchpad.net [91.189.90.11] port 22.
debug1: Connection established.
debug1: permanently_set_uid: 0/0
debug1: identity file /cygdrive/d/CI/docs/home/lyg/.ssh/id_rsa type 1
debug1: identity file /cygdrive/d/CI/docs/home/lyg/.ssh/id_rsa-cert type -1
debug1: Remote protocol version 2.0, remote software version Twisted
debug1: no match: Twisted
debug1: Enabling compatibility mode for protocol 2.0
      4 [main] ssh 5584 exception::handle: Exception:
STATUS_ACCESS_VIOLATION
   3075 [main] ssh 5584 open_stackdumpfile: Dumping stack trace to
ssh.exe.stack
dump

On 14/06/11 15:58, Yonggang Luo wrote:
> Another problem, because of repo already have an function named with
> get_layout
> Why another get_layout resident at command.py?
I don't think that's a problem, they have a different scope. get_layout
in commands.py could be renamed to get_layout_by_name or something like
that, but I don't see any reason to do so.
>
> I think they must be at the same place. It's easier to management.
They do different things; one is a UI layer helper for getting a layout by name. The other is for finding the layout to use for a repository.

Cheers,

Jelmer

On 14/06/11 15:59, Yonggang Luo wrote:
> Why this error arising, it's looks weird.. I have no idea on this, on
> another computer, it's works fine.
> ssh -v luoyonggang@<email address hidden>
> OpenSSH_5.8p1, OpenSSL 0.9.8r 8 Feb 2011
> debug1: Reading configuration data /cygdrive/d/CI/docs/home/lyg/.ssh/config
> debug1: Applying options for bazaar.launchpad.net
> debug1: Connecting to bazaar.launchpad.net [91.189.90.11] port 22.
> debug1: Connection established.
> debug1: permanently_set_uid: 0/0
> debug1: identity file /cygdrive/d/CI/docs/home/lyg/.ssh/id_rsa type 1
> debug1: identity file /cygdrive/d/CI/docs/home/lyg/.ssh/id_rsa-cert type -1
> debug1: Remote protocol version 2.0, remote software version Twisted
> debug1: no match: Twisted
> debug1: Enabling compatibility mode for protocol 2.0
> 4 [main] ssh 5584 exception::handle: Exception:
> STATUS_ACCESS_VIOLATION
> 3075 [main] ssh 5584 open_stackdumpfile: Dumping stack trace to
> ssh.exe.stack
> dump
Looks like a segfaulting ssh client; the cygwin bug tracker is probably
a more appropriate place for that.

Cheers,

jelmer

>
>
> > debug1: Enabling compatibility mode for protocol 2.0
> > 4 [main] ssh 5584 exception::handle: Exception:
> > STATUS_ACCESS_VIOLATION
> > 3075 [main] ssh 5584 open_stackdumpfile: Dumping stack trace to
> > ssh.exe.stack
> > dump
> Looks like a segfaulting ssh client; the cygwin bug tracker is probably
> a more appropriate place for that.
>
> Thanks, by using of putty, its working, ssh.exe still not working, I found
that problem appeared before under cygwin mailling list. but they didn't
resolved at last:)

--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

2011/6/14 Jelmer Vernooij <email address hidden>

> On 14/06/11 15:58, Yonggang Luo wrote:
> > Another problem, because of repo already have an function named with
> > get_layout
> > Why another get_layout resident at command.py?
> I don't think that's a problem, they have a different scope. get_layout
> in commands.py could be renamed to get_layout_by_name or something like
> that, but I don't see any reason to do so.
> >
> > I think they must be at the same place. It's easier to management.
> They do different things; one is a UI layer helper for getting a layout by
> name. The other is for finding the layout to use for a repository.
>
> I know they were different things, I means the UI only provide the
layoutname, and the creating of Layout will at the single point. so we can
separate it up. Right place do the right things, UI's main function is
providing the parameters, and these parameters are used by repository:)

> Cheers,
>
> Jelmer
>
>
> --
> https://code.launchpad.net/~luoyonggang/bzr-svn/perfect-layout/+merge/64530
> You are the owner of lp:~luoyonggang/bzr-svn/perfect-layout.
>

--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

On 14/06/11 16:36, Yonggang Luo wrote:
> 2011/6/14 Jelmer Vernooij <email address hidden>
>
>> On 14/06/11 15:58, Yonggang Luo wrote:
>>> Another problem, because of repo already have an function named with
>>> get_layout
>>> Why another get_layout resident at command.py?
>> I don't think that's a problem, they have a different scope. get_layout
>> in commands.py could be renamed to get_layout_by_name or something like
>> that, but I don't see any reason to do so.
>>> I think they must be at the same place. It's easier to management.
>> They do different things; one is a UI layer helper for getting a layout by
>> name. The other is for finding the layout to use for a repository.

> I know they were different things, I means the UI only provide the
> layoutname, and the creating of Layout will at the single point. so we can
> separate it up. Right place do the right things, UI's main function is
> providing the parameters, and these parameters are used by repository:)
That's already the case - the actual looking up is happening with this
single line of code in both cases:

ret = layout_registry.get(layoutname)()

The UI helper function has some logic to cope with the fact that command
line options can be unicode (whereas layout names are strictly ascii at
the moment), and it has some code to display a command line user error
if the specific layout was not found.

Cheers,

Jelmer

2011/6/14 Jelmer Vernooij <email address hidden>

> On 14/06/11 16:36, Yonggang Luo wrote:
> > 2011/6/14 Jelmer Vernooij <email address hidden>
> >
> >> On 14/06/11 15:58, Yonggang Luo wrote:
> >>> Another problem, because of repo already have an function named with
> >>> get_layout
> >>> Why another get_layout resident at command.py?
> >> I don't think that's a problem, they have a different scope. get_layout
> >> in commands.py could be renamed to get_layout_by_name or something like
> >> that, but I don't see any reason to do so.
> >>> I think they must be at the same place. It's easier to management.
> >> They do different things; one is a UI layer helper for getting a layout
> by
> >> name. The other is for finding the layout to use for a repository.
>
> > I know they were different things, I means the UI only provide the
> > layoutname, and the creating of Layout will at the single point. so we
> can
> > separate it up. Right place do the right things, UI's main function is
> > providing the parameters, and these parameters are used by repository:)
> That's already the case - the actual looking up is happening with this
> single line of code in both cases:
>
> ret = layout_registry.get(layoutname)()
>
> That's why I mentioned to merge this two function, it's implement the same
function at two different place and get the things
to be complicated? Don't you realized of this?

Compare the codebase bzr-svn with hgsubversion, one is 600kb+, one is 150kb,
I don't think bzr-svn implement much more function than hgsubversion, but
why the codebase is so large? Even though the framework is better, but the
codebase is really redundant， this is one of the example, I was try to let
you realize of it, but you just ignore of that.
Python is a script language, but you using it like a c language. That's I
want to say.

Sorry for the critical, but that's an fact.

> The UI helper function has some logic to cope with the fact that command
> line options can be unicode (whereas layout names are strictly ascii at
> the moment), and it has some code to display a command line user error
> if the specific layout was not found.
>
> That's what I was accepted, but I think should implement an function
get_layoutname, but not get_layout, because get_layout is the working of
repository... Do don't implement the same function here and there. that's
make no sense.

> Cheers,
>
> Jelmer
>
>
>
> --
> https://code.launchpad.net/~luoyonggang/bzr-svn/perfect-layout/+merge/64530
> You are the owner of lp:~luoyonggang/bzr-svn/perfect-layout.
>

--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

On 14/06/11 17:31, Yonggang Luo wrote:
> 2011/6/14 Jelmer Vernooij <email address hidden>
>
>> On 14/06/11 16:36, Yonggang Luo wrote:
>>> 2011/6/14 Jelmer Vernooij <email address hidden>
>>>
>>>> On 14/06/11 15:58, Yonggang Luo wrote:
>>>>> Another problem, because of repo already have an function named with
>>>>> get_layout
>>>>> Why another get_layout resident at command.py?
>>>> I don't think that's a problem, they have a different scope. get_layout
>>>> in commands.py could be renamed to get_layout_by_name or something like
>>>> that, but I don't see any reason to do so.
>>>>> I think they must be at the same place. It's easier to management.
>>>> They do different things; one is a UI layer helper for getting a layout
>> by
>>>> name. The other is for finding the layout to use for a repository.
>>> I know they were different things, I means the UI only provide the
>>> layoutname, and the creating of Layout will at the single point. so we
>> can
>>> separate it up. Right place do the right things, UI's main function is
>>> providing the parameters, and these parameters are used by repository:)
>> That's already the case - the actual looking up is happening with this
>> single line of code in both cases:
>>
>> ret = layout_registry.get(layoutname)()
>>

> That's why I mentioned to merge this two function, it's implement the same
> function at two different place and get the things
> to be complicated? Don't you realized of this?
These two functions do really different things. One could be called
get_layout_for_repository and the other could be called
get_layout_by_name. If you think they can be merged, I'm happy to review
a merge proposal to that extend, but they only really have one line in
common (the one I mentioned earlier). I'm not sure how that could be
factored out.

>
> Compare the codebase bzr-svn with hgsubversion, one is 600kb+, one is 150kb,
> I don't think bzr-svn implement much more function than hgsubversion, but
> why the codebase is so large? Even though the framework is better, but the
> codebase is really redundant， this is one of the example, I was try to let
> you realize of it, but you just ignore of that.
> Python is a script language, but you using it like a c language. That's I
> want to say.
Unlike hgsubversion, bzr-svn allows random access to svn repositories,
it doesn't just allow pushing and pulling. It also supports
roundtripping push (push that preserves all metadata). I'm not sure if
that qualifies the larger size of the project, but that's definitely one
of the reasons.

I'm happy to merge branches that remove redundancy where it is present.
However, so far you haven't mentioned anything that would actually
simplify the code base *without* throwing away existing features.

How is the codebase C like?

Cheers,

jelmer

Marking as rejected for the moment, please change back to "needs review" when applicable.