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

[Aaron Bentley]

Support appenddir configuration policy. (Aaron Bentley)

Hi all,

This branch introduces a new configuration policy, "appenddir". This new
policy is similar to appendpath, except that it appends only the last path
element to the value. Here's an example configuration:

[/home/abentley/bzr]
public_branch = lp:~abentley/bzr
public_branch:policy = appenddir

This means that the public branch for "/home/abentley/bzr/foo" will be
"lp:~abentley/bzr/foo", as with appendpath. But the public branch of
"/home/abentley/bzr/bar/foo" will *also* be "lp:~abentley/bzr/foo", whereas
appendpath would make it "lp:~abentley/bzr/bar/foo". While the appendpath
behaviour may be useful for some, it produces too-long branch names in many
cases.

My motivating case is "bzr-pipeline". When a pipeline is created with
"reconfigure-pipeline", the branch is converted into a lightweight checkout,
with branches at ".bzr/pipes/". If the push location had been set with
appendpath, a push location of "lp:~abentley/bzr/foo" would become
"lp:~abentley/bzr/.bzr/pipes/foo", which is undesirable. With appenddir, it
would retain the value "lp:~abentley/bzr/foo". I believe bzr-colo has the same
issue.

While the new policy is syntactically compatible with existing config files,
current versions of bzr treat unknown configuration policies as internal
errors:
*** Bazaar has encountered an internal error. This probably indicates a
    bug in Bazaar. You can help us fix it by filing a bug report at
        https://bugs.launchpad.net/bzr/+filebug
    including this traceback and a description of the problem.

I would be happy to create a bugfix for earlier bzrs so that they emit a
user-friendly error when they encounter unknown configuration policies.

[Vincent Ladeuil (vila)]

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

review: Needs Information

[Aaron Bentley (abentley)]

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

[lp:~abentley/bzr/appenddir]

5668. By Aaron Bentley on 2011-02-17

Saner checking for valid store locations.

[Vincent Ladeuil (vila)]

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.

review: Abstain

[Vincent Ladeuil (vila)]

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

[Vincent Ladeuil (vila)]

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

Unmerged revisions

5668. By Aaron Bentley on 2011-02-17

Saner checking for valid store locations.

5667. By Aaron Bentley on 2011-02-16

Support appenddir configuration policy.

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2011-01-20 04:44:14 +0000
+++ bzrlib/config.py	2011-02-17 16:40:48 +0000
@@ -109,25 +109,23 @@
 POLICY_NONE = 0
 POLICY_NORECURSE = 1
 POLICY_APPENDPATH = 2
+POLICY_APPENDDIR = 5
 
 _policy_name = {
     POLICY_NONE: None,
     POLICY_NORECURSE: 'norecurse',
     POLICY_APPENDPATH: 'appendpath',
-    }
-_policy_value = {
-    None: POLICY_NONE,
-    'none': POLICY_NONE,
-    'norecurse': POLICY_NORECURSE,
-    'appendpath': POLICY_APPENDPATH,
-    }
-
+    POLICY_APPENDDIR: 'appenddir',
+    }
+_policy_value = dict((v, k) for k, v in _policy_name.iteritems())
+_policy_value['none'] = POLICY_NONE
 
 STORE_LOCATION = POLICY_NONE
 STORE_LOCATION_NORECURSE = POLICY_NORECURSE
 STORE_LOCATION_APPENDPATH = POLICY_APPENDPATH
 STORE_BRANCH = 3
 STORE_GLOBAL = 4
+STORE_LOCATION_APPENDDIR = POLICY_APPENDDIR
 
 _ConfigObj = None
 def ConfigObj(*args, **kwargs):
@@ -561,6 +559,10 @@
                 if extra_path:
                     value = urlutils.join(value, extra_path)
                 return value
+            elif policy == POLICY_APPENDDIR:
+                if extra_path:
+                    value = urlutils.join(value, urlutils.basename(extra_path))
+                return value
             else:
                 raise AssertionError('Unexpected config policy %r' % policy)
         else:
@@ -931,7 +933,8 @@
         """Save option and its value in the configuration."""
         if store not in [STORE_LOCATION,
                          STORE_LOCATION_NORECURSE,
-                         STORE_LOCATION_APPENDPATH]:
+                         STORE_LOCATION_APPENDPATH,
+                         STORE_LOCATION_APPENDDIR]:
             raise ValueError('bad storage policy %r for %r' %
                 (store, option))
         self.reload()

=== modified file 'bzrlib/help_topics/en/configuration.txt'
--- bzrlib/help_topics/en/configuration.txt	2011-01-16 01:12:01 +0000
+++ bzrlib/help_topics/en/configuration.txt	2011-02-17 16:40:48 +0000
@@ -276,6 +276,8 @@
  appendpath:
    for contained locations, any additional path components are
    appended to the value.
+ appenddir:
+   for contained locations, the last path component is appended to the value.
 
 Policies are specified by keys with names of the form "$var:policy".
 For example, to define the push location for a tree of branches, the
@@ -286,7 +288,21 @@
   push_location:policy = appendpath
 
 With this configuration, the push location for ``/top/location/branch1``
-would be ``sftp://example.com/location/branch1``.
+would be ``sftp://example.com/location/branch1``, and the push location for
+``top/location/subdir/branch2`` would be
+``sftp://example.com/location/subdir/branch2``.
+
+If we want to avoid including subdirectories, we can use the appenddir policy
+instead::
+
+  [/top/location]
+  push_location = sftp://example.com/location
+  push_location:policy = appenddir
+
+With this configuration, the push location for ``/top/location/branch1``
+would be ``sftp://example.com/location/branch1``, and the push location for
+``top/location/subdir/branch2`` would be
+``sftp://example.com/location/branch2``.
 
 
 The main configuration file, bazaar.conf

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2011-01-20 04:44:14 +0000
+++ bzrlib/tests/test_config.py	2011-02-17 16:40:48 +0000
@@ -33,7 +33,6 @@
     errors,
     osutils,
     mail_client,
-    mergetools,
     ui,
     urlutils,
     tests,
@@ -1216,6 +1215,13 @@
         self.assertEqual('normal',
                          self.my_config.get_user_option('appendpath_option'))
 
+    def test_get_user_option_appenddir(self):
+        foo_config = config.LocationConfig('/foo')
+        foo_config.set_user_option('bar', 'baz',
+                                   config.STORE_LOCATION_APPENDDIR)
+        qux_config = config.LocationConfig('/foo/bar/qux')
+        self.assertEqual('baz/qux', qux_config.get_user_option('bar'))
+
     def test_get_user_option_norecurse(self):
         self.get_branch_config('http://www.example.com')
         self.assertEqual('norecurse',

=== modified file 'doc/developers/contribution-quickstart.txt'
--- doc/developers/contribution-quickstart.txt	2010-12-02 10:41:05 +0000
+++ doc/developers/contribution-quickstart.txt	2011-02-17 16:40:48 +0000
@@ -58,9 +58,9 @@
 
     [/home/USER/bzr]
     push_location = lp:~LAUNCHPAD_USER/bzr/
-    push_location:policy = appendpath
+    push_location:policy = appenddir
     public_branch = http://bazaar.launchpad.net/~LAUNCHPAD_USER/bzr/
-    public_branch:policy = appendpath
+    public_branch:policy = appenddir
 
 with your local and Launchpad usernames inserted.
 

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-02-16 17:20:10 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-02-17 16:40:48 +0000
@@ -26,6 +26,10 @@
 * External merge tools can now be configured in bazaar.conf. See
   ``bzr help configuration`` for more information.  (Gordon Tyler, #489915)
 
+* New "appenddir" configuration policy appends only the last path element to a
+  config value, providing better support for colocated branches and
+  bzr-pipeline. (Aaron Bentley)
+
 Improvements
 ************
 
@@ -38,7 +42,7 @@
 
 * Branching, merging and pulling a branch now copies revisions named in
   tags, not just the tag metadata.  (Andrew Bennetts, #309682)
-  
+
 * ``bzr cat-revision`` no longer requires a working tree. (Jelmer Vernooij, #704405)
 
 Bug Fixes