[MERGE] Adds a new builtin command 'alias' to list, set and remove bzr aliases.

[Aaron Bentley]

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

bb:resubmit

Tim Penhey wrote:
> +class cmd_alias(Command):
> +    """Print the list of aliases set.
> +

^^^ Shouldn't this be something like "set/unset and display aliases?

> +    def run(self, name=None, remove=False):
> +        if remove:
> +            self.remove_alias(name)
> +        elif name is None:
> +            self.print_aliases()
> +        else:
> +            equal_pos = name.find('=')
> +            if equal_pos == -1:
> +                self.print_alias(name)
> +            else:
> +                self.set_alias(name[:equal_pos], name[equal_pos+1:])

^^^ I'd think name.split('=', 1) would be shorter, but to each their own.


> +    @display_command
> +    def remove_alias(self, alias_name):

^^^ This is not a display command; early termination may cause the
requested operation not to be performed.  No decorator, please.

> +        if alias_name is None:
> +            raise errors.BzrCommandError(
> +                'bzr alias --remove expects an alias to remove.')
> +        # If alias is not found, print something like:
> +        # unalias: foo: not found
> +        from bzrlib.commands import get_alias
> +        alias = get_alias(alias_name)
> +        if alias is None:
> +            print "bzr alias: %s: not found" % alias_name

^^^ Please use self.outf.write rather than print.  This ensures that the
correct encoding is used.  (This applies to every cmd_alias method.)

Also, remember it's easier to ask forgiveness than permission (EAFP).
Instead of checking whether the alias exists, just invoke unset_alias,
and handle the error it raises if there's no existing alias.

Also, it would have been better to raise a BzrCommandError, because you
should not exit with status=0 anyhow.

> +        else:
> +            c = config.GlobalConfig()
> +            c.unset_alias(alias_name)

^^^ It seems a bit odd to use commands.get_alias to find out whether an
alias exists, but use GlobalConfig.unset_alias if it does.

> +    @display_command
> +    def print_aliases(self):
> +        """Print out the defined aliases in a similar format to bash."""
> +        aliases = config.GlobalConfig().get_aliases()
> +        for key in sorted(aliases):
> +            print "bzr alias %s='%s'" % (key, aliases[key])

^^^ you could do for key, value in sorted(aliases.iteritems())

> +    @display_command
> +    def set_alias(self, alias_name, alias_commands):

^^^ not a display command

> === modified file 'bzrlib/config.py'
> --- bzrlib/config.py	2008-04-13 17:38:34 +0000
> +++ bzrlib/config.py	2008-04-25 11:30:09 +0000
> @@ -440,13 +440,38 @@
>  
>      def set_user_option(self, option, value):
>          """Save option and its value in the configuration."""
> +        self._set_option(option, value, 'DEFAULT')
> +
> +    def get_aliases(self):
> +        """Return the aliases section."""
> +        if 'ALIASES' in self._get_parser():
> +            return self._get_parser()['ALIASES']

get_parser reads a file off disk, so it makes sense to read it only once.

> +    def unset_alias(self, alias_name):
> +        """Unset an existing alias."""
> +        aliases = self._get_parser().get('ALIASES')
> +        assert aliases and alias_name in aliases, (
> +            "The alias should exist before this is called")

^^^ Please don't use asserts.  We are making them forbidden.  In fact,
you'll want to raise a specific error, so you can do EAFP.

> +    def _set_option(self, option, value, section):
>          # FIXME: RBC 20051029 This should refresh the parser and also take a
>          # file lock on bazaar.conf.
>          conf_dir = os.path.dirname(self._get_filename())
>          ensure_config_dir_exists(conf_dir)
> -        if 'DEFAULT' not in self._get_parser():
> -            self._get_parser()['DEFAULT'] = {}
> -        self._get_parser()['DEFAULT'][option] = value
> +        if section not in self._get_parser():
> +            self._get_parser()[section] = {}
> +        self._get_parser()[section][option] = value

I think this would be better as
parser = self._get_parser()
parser.setdefault(section, {})[option] = value

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

iD8DBQFIFlx40F+nu1YWqI0RApiAAJ9T+mWKM3Rd0HvFoe/FftQhvUZ1IACfeRG0
nI7Pjd8wD4XnfNjUdhwrUpE=
=W7j1
-----END PGP SIGNATURE-----