Code review comment for lp://qastaging/~benoit.pierre/bzrtools/shell_improvements

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

Hi Benoit,

Thanks for your patch. I'm sorry about the delay responding.

In general, please submit changes separately. It wasn't easy for me to
review initially, which contributed to the delay.

 review resubmit

Benoit PIERRE wrote:
> Benoit PIERRE has proposed merging lp:~benoit.pierre/bzrtools/shell_improvements into lp:bzrtools.
>
> Requested reviews:
> Aaron Bentley (abentley)
>
> Various improvements for the shell command:
>
> - allow killing the current command line and start with a new one with ^C like
> most shells do (which also helps avoiding killing the shell by mistake when
> one's timing is off when trying to interrupt a command in progress), ^D can
> be used to exit when on an empty line

I think what you mean is that ^C no longer kills the shell permanently.
  Is that right? I don't see anything that specifically enables killing
commands.

> - catch shlex.split ValueError exceptions: the erroneous command can be edited
> again by using <up> to navigate the readline history

What can cause a ValueError?

> (fix https://bugs.launchpad.net/bzrtools/+bug/231020)

Btw, don't be shy to link your branch to the bugs it fixes. This will
show up in the merge proposal.

>
> - add global config aliases to the list of possible commands when completing
>
> - fix shell handling of alias so for example "s|grep pattern" correctly works
> if s is an alias
>
> - add wrapper for reconfigure/switch command so prompt gets updated to reflect
> the new tree
>
> (fix https://bugs.launchpad.net/bzrtools/+bug/231110)
>
>
>

> === modified file 'shell.py'
> --- shell.py 2008-02-13 04:58:32 +0000
> +++ shell.py 2009-08-02 13:35:17 +0000
> @@ -26,7 +26,7 @@
>
> from bzrlib import osutils
> from bzrlib.branch import Branch
> -from bzrlib.config import config_dir, ensure_config_dir_exists
> +from bzrlib.config import config_dir, ensure_config_dir_exists, GlobalConfig
> from bzrlib.commands import get_cmd_object, get_all_cmds, get_alias
> from bzrlib.errors import BzrError
> from bzrlib.workingtree import WorkingTree
> @@ -36,6 +36,7 @@
>
> SHELL_BLACKLIST = set(['rm', 'ls'])
> COMPLETION_BLACKLIST = set(['shell'])
> +COMPLEX_COMMAND_CHARS = ('|', '<', '>', '&', '*', '?')

I thought using a string was cute, but if we're going to use the correct
container type, we should use a set.

>
>
> class BlackListedCommand(BzrError):
> @@ -167,18 +168,37 @@
> def do_help(self, line):
> self.default("help "+line)
>
> + def exec_and_update_tree(self, line):
> + result = self.default(line)
> + try:
> + self.tree = WorkingTree.open_containing(".")[0]
> + except:
> + self.tree = None
> + return result
> +
> + def do_switch(self, line):
> + return self.exec_and_update_tree('switch ' + line)

This is okay, but I think it would be better to just put 'switch' and
'reconfigure' in a set like the COMPLETION_BLACKLIST, and then update
the tree when commands in that set are encountered.

> + def do_reconfigure(self, line):
> + return self.exec_and_update_tree('reconfigure ' + line)
> +
> def default(self, line):
> - args = shlex.split(line)
> - alias_args = get_alias(args[0])
> - if alias_args is not None:
> - args[0] = alias_args.pop(0)
> + try:
> + args = shlex.split(line)
> + except ValueError, e:
> + print 'ValueError:', e
> + return
> + alias_args = None

Why are you setting this here?

>
> commandname = args.pop(0)
> - for char in ('|', '<', '>'):
> + for char in COMPLEX_COMMAND_CHARS:
> commandname = commandname.split(char)[0]
> - if commandname[-1] in ('|', '<', '>'):
> + if commandname[-1] in COMPLEX_COMMAND_CHARS:
> commandname = commandname[:-1]

I don't see how this if commandname[-1] in COMPLEX_COMMAND_CHARS could
ever fire, but that's my fault, not yours.

> try:
> + alias_args = get_alias(commandname)
> + if alias_args is not None:
> + commandname = alias_args.pop(0)
> if commandname in SHELL_BLACKLIST:
> raise BlackListedCommand(commandname)
> cmd_obj = get_cmd_object(commandname)
> @@ -223,10 +243,13 @@
> def run_shell():
> try:
> prompt = PromptCmd()
> - try:
> - prompt.cmdloop()
> - finally:
> - prompt.write_history()
> + while True:
> + try:
> + prompt.cmdloop()
> + except KeyboardInterrupt:
> + print
> + finally:
> + prompt.write_history()

try/except/finally isn't legal in python2.4. Please change this to
try/try/except/finally.

> except StopIteration:
> pass
>
> @@ -280,6 +303,8 @@
>
>
> def iter_command_names(hidden=False):
> + for alias in GlobalConfig().get_aliases().keys():
> + yield alias
> for real_cmd_name, cmd_class in get_all_cmds():
> if not hidden and cmd_class.hidden:
> continue
> @@ -312,7 +337,7 @@
>
>
> def too_complicated(line):
> - for char in '|<>*?':
> + for char in COMPLEX_COMMAND_CHARS:
> if char in line:
> return True
> return False
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkqu7xUACgkQ0F+nu1YWqI3oJgCdGE/Xo/wzy0a82VnbHy6+7XNR
hkQAmwfV+qdFmozDuXwU57wWbIPgbc58
=FhR5
-----END PGP SIGNATURE-----