Merge lp:~abentley/bzr/appenddir into lp:bzr

Thanks for working on this, this is clearly a sore point in out config handling.

I've got a couple of questions about the implementation:

- are you aware of the work in progress documented in configuration.txt in the lp:~bzr/bzr/devnotes ? The interpolation part especially may provide a more scalable means to achieve the same result.

- why did you chose 'appendir' instead of say 'nick' ? I'm well aware that they may not match but the later would also work for looms,

27 STORE_LOCATION = POLICY_NONE
28 STORE_LOCATION_NORECURSE = POLICY_NORECURSE
29 STORE_LOCATION_APPENDPATH = POLICY_APPENDPATH
30 STORE_BRANCH = 3
31 STORE_GLOBAL = 4
32 +STORE_LOCATION_APPENDDIR = POLICY_APPENDDIR

The overlapping between STORE_* and POLICY_* is already a concern in the current implementation, could you think of a better way to articulate their relationship ?

This shows even more in:

51 - if store not in [STORE_LOCATION,
52 - STORE_LOCATION_NORECURSE,
53 - STORE_LOCATION_APPENDPATH]:
54 + if store not in _policy_name:

Err, wait, checking 'store' in '_policy_name' ? This at least requires a comment...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11-02-17 04:47 AM, Vincent Ladeuil wrote:
> Review: Needs Information
> Thanks for working on this, this is clearly a sore point in out config handling.
>
> I've got a couple of questions about the implementation:
>
> - are you aware of the work in progress documented in configuration.txt in the lp:~bzr/bzr/devnotes ?

No. I'm glad that serious re-thinking is going on. I would rather not
have to wait for a ground-up rewrite, unless it is immanent.

> The interpolation part especially may provide a more scalable means to achieve the same result.
>
> - why did you chose 'appendir' instead of say 'nick' ?

Initially, I did choose 'nick', but locations.conf is not branch-aware,
and in fact is sometimes used without a branch. So making it
branch-aware didn't seem like a good fit.

> I'm well aware that they may not match but the later would also work for looms,

Supporting looms would be nice, but holds little value for me. I
haven't used looms since I wrote bzr-pipeline.

> 27 STORE_LOCATION = POLICY_NONE
> 28 STORE_LOCATION_NORECURSE = POLICY_NORECURSE
> 29 STORE_LOCATION_APPENDPATH = POLICY_APPENDPATH
> 30 STORE_BRANCH = 3
> 31 STORE_GLOBAL = 4
> 32 +STORE_LOCATION_APPENDDIR = POLICY_APPENDDIR
>
> The overlapping between STORE_* and POLICY_* is already a concern in the current implementation, could you think of a better way to articulate their relationship ?

It looks like inheritance to me-- All STORE values indicate which file
to store the value in, while the STORE_LOCATION ones also indicate a
policy. If I was doing a radical rewrite, I'd probably make them
classes with static/classmethods, e.g.

class ValueScope:

    @classmethod
    def store_value(cls, name, value):
        config = cls.get_config()
        cls._set_value(config, name, value)

    @staticmethod
    def _set_value(config, name, value)
        config.set_value(name, value)

class GlobalScope(ValueScope):

    @staticmethod
    def get_config():
        return GlobalConfig

class PlainLocationScope(ValueScope):

    @staticmethod
    def get_config():
        return LocationConfig

class AppendPathScope(PlainLocationScope):

    @staticmethod
    def _set_value(config, name, value):
        config.set_value(name, value, policy='appendpath')

A less-radical rewrite would be to make the STORE values tuples of
config, POLICY:

STORE_BRANCH = ('branch', POLICY_NONE)
STORE_LOCATION_APPENDPATH = ('location', POLICY_APPENDPATH)

No matter what, I don't think assigning integers to these things is
doing us any good. I'd love to see "STORE_BRANCH = 'branch'". That
would make debugging easier.

> This shows even more in:
>
> 51 - if store not in [STORE_LOCATION,
> 52 - STORE_LOCATION_NORECURSE,
> 53 - STORE_LOCATION_APPENDPATH]:
> 54 + if store not in _policy_name:
>
> Err, wait, checking 'store' in '_policy_name' ? This at least requires a comment...

Yesterday, it looked like a nice way to avoid repeating ourselves.
Today, it looks a bit broken.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk1dR4MACgkQ0F+nu1YWqI1IoA...

So, I've been working on this from a different angle:
- http://doc.bazaar.canonical.com/devnotes/configuration.html
- lp:~vila/bzr/new-config

This is a work in progress which still doesn't provide a working solution in core.

So basically I can't disapprove this proposal until I'm able to point to a working implementation but on the other hand, this proposal use a feature I'd like to get rid of (option and store policies) so approving it means I'll have to deprecate it in the future and provide a migration path.

On the third hand, I will need to provide a migration path for the existing policies which will require a yet *unknown* amount of work ;)

That being said:
- option expansion is (now) available and using {nickname} is very close to the 'appendir' policy,
- 'appenddir' fooled me (twice) as a policy name: it really add the *basename* of the branch/wt which indeed is a dir but I tend to think in terms of os.path API and read 'appenddir' as 'os.path.dirname'

Regarding policies, the plan is to define them once and only once for a given option, that means *outside* of the config files. This is my main issue with the current implementation.

Also, aren't there cases where 'appendir' would be empty and led to confusing errors ?

I also want to add that I *like* the feature even if I frown upon the implementation (while understanding that's the only one available ;).

@Aaron: What's the status on this ? I'll mark it wip to clear the queue but I'm open to discussion and don't want to mark it rejected (or should it be just abandoned ?).