Merge lp:~abentley/bzr/config-branchname into lp:bzr

[Aaron Bentley]

Support branchname config variable.

This branch adds a "branchname" automatic config variable, which is the name of a native colocated branch, as specified by the "branch=" portion of its url. This allows the user to configure urls to reflect the branch name, just as they would use relpath or basename to configure other types of branches.

I have used it like so, in order to push this branch:
[/home/abentley/hacking/bzr]
push_location = bzr+ssh://bazaar.launchpad.net/~abentley/bzr/{branchname}

This is an itch that I am scratching. For me, the biggest advantage of bzr-colo colocation is that, since it uses standard branches, it can automatically configure the push and public locations. With this change, native colo branches will have parity in terms of configuration.

[James Westby (james-w)]

On Mon, 18 Jun 2012 14:00:27 -0000, Aaron Bentley <email address hidden> wrote:
> +Another such option is ``branchname``, which refers to the name of a colocated
> +branch. It can be used like this::

Is it just colocated branches, or does it work for all local branches?
It seems like this could work for pipelines with much the same
configuration if it does?

Thanks,

James

[Aaron Bentley (abentley)]

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

On 12-06-18 10:28 AM, James Westby wrote:
> On Mon, 18 Jun 2012 14:00:27 -0000, Aaron Bentley
> <email address hidden> wrote:
>> +Another such option is ``branchname``, which refers to the name
>> of a colocated +branch. It can be used like this::
>
> Is it just colocated branches, or does it work for all local
> branches?

No, it only works for branches which have names specified in their url
via ",branch=$NAME". Only colocated branches have URLs like that.
But I don't think they are required to be local.

> It seems like this could work for pipelines with much the same
> configuration if it does?

For bzr-colo style pipelines, the "basename" config variable will work
just as well. But pipelines can also be used with native-colocated
branches.

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

iEYEARECAAYFAk/fPPoACgkQ0F+nu1YWqI2zHwCeLpkIo+iTs1kBIiM3261w5+l3
3YUAoIcg/aupDuQQNYkzSmd5SI4v2PV+
=3pFD
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

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

On 6/18/2012 4:40 PM, Aaron Bentley wrote:
> On 12-06-18 10:28 AM, James Westby wrote:
>> On Mon, 18 Jun 2012 14:00:27 -0000, Aaron Bentley
>> <email address hidden> wrote:
>>> +Another such option is ``branchname``, which refers to the
>>> name of a colocated +branch. It can be used like this::
>
>> Is it just colocated branches, or does it work for all local
>> branches?
>
> No, it only works for branches which have names specified in their
> url via ",branch=$NAME". Only colocated branches have URLs like
> that. But I don't think they are required to be local.
>
>> It seems like this could work for pipelines with much the same
>> configuration if it does?
>
> For bzr-colo style pipelines, the "basename" config variable will
> work just as well. But pipelines can also be used with
> native-colocated branches.
>
> Aaron

Would it be possible to have 'nick' available as a config variable
rather than 'branchname'? It seems that having to write things as
(basename or branchname) and not having a universal value is a bit odd.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk/gh0AACgkQJdeBCYSNAAPaZQCgyB5ZRRYA+KDbx5jI7tDI19JB
a7IAnRqkPwxChQJhABs642462wOxftzC
=XsVC
-----END PGP SIGNATURE-----

[Aaron Bentley (abentley)]

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

> Would it be possible to have 'nick' available as a config variable
> rather than 'branchname'? It seems that having to write things as
> (basename or branchname) and not having a universal value is a bit
> odd.

I agree that it would be nice to have a single thing that worked for
all of them.

It's possible to use 'nick', but that's expensive, changes the
locations.conf paradigm and introduces new failure modes.

It's expensive because of explicit nicks. It requires opening a
branch, and it can cause network traffic.

locations.conf has always configured locations, not branches, and this
would change that paradigm.

It would mean that locations.conf would need to handle branch-opening
failures, something it's never considered before.

Maybe we should drop the concept of explicit nicks. I think they're
pretty rarely used. If nicks could be calculated from URL alone, all
the above issues go away.

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

iEYEARECAAYFAk/gjgIACgkQ0F+nu1YWqI1pHQCePUU3JoBb8yFBACOx0ON/EM92
8eAAn19kIIMLT+/f1uIyaKGeuQTPHi0m
=iq4E
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

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

On 6/19/2012 4:37 PM, Aaron Bentley wrote:
>> Would it be possible to have 'nick' available as a config
>> variable rather than 'branchname'? It seems that having to write
>> things as (basename or branchname) and not having a universal
>> value is a bit odd.
>
> I agree that it would be nice to have a single thing that worked
> for all of them.
>
> It's possible to use 'nick', but that's expensive, changes the
> locations.conf paradigm and introduces new failure modes.
>
> It's expensive because of explicit nicks. It requires opening a
> branch, and it can cause network traffic.
>
> locations.conf has always configured locations, not branches, and
> this would change that paradigm.
>
> It would mean that locations.conf would need to handle
> branch-opening failures, something it's never considered before.
>
> Maybe we should drop the concept of explicit nicks. I think
> they're pretty rarely used. If nicks could be calculated from URL
> alone, all the above issues go away.
>
> Aaron

We could call it 'branchshortname' or something along those lines,
that would be essentially (basename or branchname).

I would be happy enough to have 'branchname' that worked along those
lines. (if ,branch=? then ?, else basename)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk/gkIMACgkQJdeBCYSNAAPJ9ACfd49jxU2EdK7E/IKKctYc9+Aa
kc4AoK3LAhELgZ5soq771eyD+jixmPVO
=YUXp
-----END PGP SIGNATURE-----

[Aaron Bentley (abentley)]

I've made the changes you requested.

[John A Meinel (jameinel)]

Offhand it feels like we would need to unescape the basename if we are unescaping the branch portion. However your test seems to clearly state that isn't the case. That indicates we prob have an API issue (similar commands return different types, eg escaped URLs vs Unicode). But that isn't something you need to fix in this branch.

The doc seems a bit incomplete as it only indicates branchname works with Colo branches.

review: Approve

[Aaron Bentley (abentley)]

> Offhand it feels like we would need to unescape the basename if we are
> unescaping the branch portion. However your test seems to clearly state that
> isn't the case. That indicates we prob have an API issue (similar commands
> return different types, eg escaped URLs vs Unicode). But that isn't something
> you need to fix in this branch.

We don't need to unescape the basename because for file:/// URLs, self.location is set to a filesystem path. This matches the behaviour of our existing config mechamism. However, I've realized it's really a bug, because it means that unescaping is not done for other URL types. I've filed bug #1015570 about this.

> The doc seems a bit incomplete as it only indicates branchname works with Colo
> branches.

I added this to the docs: For non-colocated branches, it behaves like basename.

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2012-05-16 08:54:21 +0000
+++ bzrlib/config.py	2012-06-20 15:19:22 +0000
@@ -3444,11 +3444,14 @@
 
 class LocationSection(Section):
 
-    def __init__(self, section, extra_path):
+    def __init__(self, section, extra_path, branch_name=None):
         super(LocationSection, self).__init__(section.id, section.options)
         self.extra_path = extra_path
+        if branch_name is None:
+            branch_name = ''
         self.locals = {'relpath': extra_path,
-                       'basename': urlutils.basename(extra_path)}
+                       'basename': urlutils.basename(extra_path),
+                       'branchname': branch_name}
 
     def get(self, name, default=None, expand=True):
         value = super(LocationSection, self).get(name, default)
@@ -3524,9 +3527,15 @@
 
     def __init__(self, store, location):
         super(LocationMatcher, self).__init__(store)
+        url, params = urlutils.split_segment_parameters(location)
         if location.startswith('file://'):
             location = urlutils.local_path_from_url(location)
         self.location = location
+        branch_name = params.get('branch')
+        if branch_name is None:
+            self.branch_name = urlutils.basename(self.location)
+        else:
+            self.branch_name = urlutils.unescape(branch_name)
 
     def _get_matching_sections(self):
         """Get all sections matching ``location``."""
@@ -3558,8 +3567,9 @@
             while True:
                 section = iter_all_sections.next()
                 if section_id == section.id:
-                    matching_sections.append(
-                        (length, LocationSection(section, extra_path)))
+                    section = LocationSection(section, extra_path,
+                                              self.branch_name)
+                    matching_sections.append((length, section))
                     break
         return matching_sections
 

=== modified file 'bzrlib/help_topics/en/configuration.txt'
--- bzrlib/help_topics/en/configuration.txt	2011-11-14 16:36:31 +0000
+++ bzrlib/help_topics/en/configuration.txt	2012-06-20 15:19:22 +0000
@@ -333,6 +333,18 @@
 itself is defined as the relative path between the section name and the
 location it matches.
 
+Another such option is ``branchname``, which refers to the name of a colocated
+branch.  For non-colocated branches, it behaves like basename.  It can be used
+like this::
+
+  [/home/vila/src/bzr/bugs]
+  mypush = lp:~vila/bzr/{branchname}
+
+When used with a colocated branch named ``832013-expand-in-stack``, we'll get::
+
+  bzr config mypush
+  lp:~vila/bzr/832013-expand-in-stack
+
 When an option is local to a Section, it cannot be referred to from option
 values in any other section from the same ``Store`` nor from any other
 ``Store``.

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2012-06-18 11:43:07 +0000
+++ bzrlib/tests/test_config.py	2012-06-20 15:19:22 +0000
@@ -17,6 +17,7 @@
 """Tests for finding and reading the bzr config file[s]."""
 # import system imports here
 from cStringIO import StringIO
+from textwrap import dedent
 import os
 import sys
 import threading
@@ -3418,6 +3419,28 @@
         matcher = config.LocationMatcher(store, expected_url)
         self.assertEquals(expected_location, matcher.location)
 
+    def test_branch_name_colo(self):
+        store = self.get_store(self)
+        store._load_from_string(dedent("""\
+            [/]
+            push_location=my{branchname}
+        """))
+        matcher = config.LocationMatcher(store, 'file:///,branch=example%3c')
+        self.assertEqual('example<', matcher.branch_name)
+        ((_, section),) = matcher.get_sections()
+        self.assertEqual('example<', section.locals['branchname'])
+
+    def test_branch_name_basename(self):
+        store = self.get_store(self)
+        store._load_from_string(dedent("""\
+            [/]
+            push_location=my{branchname}
+        """))
+        matcher = config.LocationMatcher(store, 'file:///parent/example%3c')
+        self.assertEqual('example<', matcher.branch_name)
+        ((_, section),) = matcher.get_sections()
+        self.assertEqual('example<', section.locals['branchname'])
+
 
 class TestStartingPathMatcher(TestStore):