ftp: clean up error handling

We don't want to abort on errors since this is an interactive
tool; while at it, also clean up duplicate error reporting on
errors in 'ls' and weird bytes formatting on errors.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Reviewed-by: Rob Browning <rlb@defaultvalue.org>
[rlb@defaultvalue.org: let unexpected exceptions propgate]
[rlb@defaultvalue.org: use str for exception messages]
[rlb@defaultvalue.org: render paths via path_msg()]
[rlb@defaultvalue.org: add match_rx_grp and test for expected output]
Signed-off-by: Rob Browning <rlb@defaultvalue.org>
Tested-by: Rob Browning <rlb@defaultvalue.org>

diff --git a/lib/bup/cmd/ftp.py b/lib/bup/cmd/ftp.py
index 9a33754c039bac2b3ef656698f00c082367e8414..bd3bd6991bd5de08d05a7a5d53e818a6a59e90fe 100644 (file)
--- a/lib/bup/cmd/ftp.py
+++ b/lib/bup/cmd/ftp.py
@@ -16,6 +16,10 @@ from bup.repo import LocalRepo
 
 repo = None
 
+
+class CommandError(Exception):
+    pass
+
 class OptionError(Exception):
     pass
 
@@ -25,7 +29,6 @@ def do_ls(repo, pwd, args, out):
     try:
         opt = ls.opts_from_cmdline(args, onabort=OptionError, pwd=pwd_str)
     except OptionError as e:
-        log('error: %s' % e)
         return None
     return ls.within_repo(repo, opt, out, pwd_str)
 
@@ -117,6 +120,9 @@ def inputiter(f, pwd, out):
         for line in f:
             yield line
 
+def rpath_msg(res):
+    """Return a path_msg for the resolved path res."""
+    return path_msg(b'/'.join(name for name, item in res))
 
 def present_interface(stdin, out, extra, repo):
     pwd = vfs.resolve(repo, b'/')
@@ -150,11 +156,9 @@ def present_interface(stdin, out, extra, repo):
                     res = vfs.resolve(repo, parm, parent=np)
                     _, leaf_item = res[-1]
                     if not leaf_item:
-                        raise Exception('%s does not exist'
-                                        % path_msg(b'/'.join(name for name, item
-                                                             in res)))
+                        raise CommandError('path does not exist: ' + rpath_msg(res))
                     if not stat.S_ISDIR(vfs.item_mode(leaf_item)):
-                        raise Exception('%s is not a directory' % path_msg(parm))
+                        raise CommandError('path is not a directory: ' + path_msg(parm))
                     np = res
                 pwd = np
             elif cmd == b'pwd':
@@ -167,23 +171,20 @@ def present_interface(stdin, out, extra, repo):
                     res = vfs.resolve(repo, parm, parent=pwd)
                     _, leaf_item = res[-1]
                     if not leaf_item:
-                        raise Exception('%s does not exist' %
-                                        path_msg(b'/'.join(name for name, item
-                                                           in res)))
+                        raise CommandError('path does not exist: ' + rpath_msg(res))
                     with vfs.fopen(repo, leaf_item) as srcfile:
                         write_to_file(srcfile, out)
                 out.flush()
             elif cmd == b'get':
                 if len(words) not in [2,3]:
-                    raise Exception('Usage: get <filename> [localname]')
+                    raise CommandError('Usage: get <filename> [localname]')
                 rname = words[1]
                 (dir,base) = os.path.split(rname)
                 lname = len(words) > 2 and words[2] or base
                 res = vfs.resolve(repo, rname, parent=pwd)
                 _, leaf_item = res[-1]
                 if not leaf_item:
-                    raise Exception('%s does not exist' %
-                                    path_msg(b'/'.join(name for name, item in res)))
+                    raise CommandError('path does not exist: ' + rpath_msg(res))
                 with vfs.fopen(repo, leaf_item) as srcfile:
                     with open(lname, 'wb') as destfile:
                         log('Saving %s\n' % path_msg(lname))
@@ -195,7 +196,7 @@ def present_interface(stdin, out, extra, repo):
                     res = vfs.resolve(repo, dir, parent=pwd)
                     _, dir_item = res[-1]
                     if not dir_item:
-                        raise Exception('%s does not exist' % path_msg(dir))
+                        raise CommandError('path does not exist: ' + path_msg(dir))
                     for name, item in vfs.contents(repo, dir_item):
                         if name == b'.':
                             continue
@@ -204,9 +205,8 @@ def present_interface(stdin, out, extra, repo):
                                 deref = vfs.resolve(repo, name, parent=res)
                                 deref_name, deref_item = deref[-1]
                                 if not deref_item:
-                                    raise Exception('%s does not exist' %
-                                                    path_msg('/'.join(name for name, item
-                                                                      in deref)))
+                                    raise CommandError('path does not exist: '
+                                                       + rpath_msg(res))
                                 item = deref_item
                             with vfs.fopen(repo, item) as srcfile:
                                 with open(name, 'wb') as destfile:
@@ -218,10 +218,11 @@ def present_interface(stdin, out, extra, repo):
             elif cmd in (b'quit', b'exit', b'bye'):
                 break
             else:
-                raise Exception('no such command %r' % cmd)
-        except Exception as e:
-            log('error: %s\n' % e)
-            raise
+                raise CommandError('no such command: '
+                                   + cmd.encode(errors='backslashreplace'))
+        except CommandError as ex:
+            out.write(b'error: %s\n' % str(ex).encode(errors='backslashreplace'))
+            out.flush()
 
 def main(argv):
     global repo

diff --git a/test/ext/test_ftp.py b/test/ext/test_ftp.py
index d6bb273710fc3c3e88cc6c297fc1ee48c5342371..3fb6467a3c66f3823e95215801797e6b2e238a0a 100644 (file)
--- a/test/ext/test_ftp.py
+++ b/test/ext/test_ftp.py
@@ -1,8 +1,8 @@
 
-from __future__ import absolute_import, print_function
 from os import chdir, mkdir, symlink, unlink
 from subprocess import PIPE
 from time import localtime, strftime, tzset
+import re
 
 from bup.compat import environ
 from bup.helpers import unlink as unlink_if_exists
@@ -20,13 +20,18 @@ def bup(*args, **kwargs):
 def jl(*lines):
     return b''.join(line + b'\n' for line in lines)
 
+def match_rx_grp(rx, expected, src):
+    match = re.fullmatch(rx, src)
+    wvpass(match, 're.fullmatch(%r, %r)' % (rx, src))
+    if not match:
+        return
+    wvpasseq(expected, match.groups())
+
 environ[b'GIT_AUTHOR_NAME'] = b'bup test'
 environ[b'GIT_COMMITTER_NAME'] = b'bup test'
 environ[b'GIT_AUTHOR_EMAIL'] = b'bup@a425bc70a02811e49bdf73ee56450e6f'
 environ[b'GIT_COMMITTER_EMAIL'] = b'bup@a425bc70a02811e49bdf73ee56450e6f'
 
-import subprocess
-
 def test_ftp(tmpdir):
     environ[b'BUP_DIR'] = tmpdir + b'/repo'
     environ[b'GIT_DIR'] = tmpdir + b'/repo'
@@ -70,12 +75,14 @@ def test_ftp(tmpdir):
              bup(b'ftp', input=jl(b'cd src/latest/dir-symlink', b'pwd')).out)
     wvpasseq(b'/src/%s/dir\n' % save_name,
              bup(b'ftp', input=jl(b'cd src latest dir-symlink', b'pwd')).out)
-    wvpassne(0, bup(b'ftp',
-                    input=jl(b'cd src/latest/bad-symlink', b'pwd'),
-                    check=False, stdout=None).rc)
-    wvpassne(0, bup(b'ftp',
-                    input=jl(b'cd src/latest/not-there', b'pwd'),
-                    check=False, stdout=None).rc)
+
+    match_rx_grp(br'(error: path does not exist: /src/)[0-9-]+(/not-there\n/\n)',
+                 (b'error: path does not exist: /src/', b'/not-there\n/\n'),
+                 bup(b'ftp', input=jl(b'cd src/latest/bad-symlink', b'pwd')).out)
+
+    match_rx_grp(br'(error: path does not exist: /src/)[0-9-]+(/not-there\n/\n)',
+                 (b'error: path does not exist: /src/', b'/not-there\n/\n'),
+                 bup(b'ftp', input=jl(b'cd src/latest/not-there', b'pwd')).out)
 
     wvstart('ls')
     # FIXME: elaborate
@@ -99,13 +106,15 @@ def test_ftp(tmpdir):
     with open(b'dest', 'rb') as f:
         wvpasseq(b'excitement!\n', f.read())
     unlink(b'dest')
-    wvpassne(0, bup(b'ftp',
-                    input=jl(b'get src/latest/bad-symlink dest'),
-                    check=False, stdout=None).rc)
-    wvpassne(0, bup(b'ftp',
-                    input=jl(b'get src/latest/not-there'),
-                    check=False, stdout=None).rc)
-    
+
+    match_rx_grp(br'(error: path does not exist: /src/)[0-9-]+(/not-there\n)',
+                 (b'error: path does not exist: /src/', b'/not-there\n'),
+                 bup(b'ftp', input=jl(b'get src/latest/bad-symlink dest')).out)
+
+    match_rx_grp(br'(error: path does not exist: /src/)[0-9-]+(/not-there\n)',
+                 (b'error: path does not exist: /src/', b'/not-there\n'),
+                 bup(b'ftp', input=jl(b'get src/latest/not-there dest')).out)
+
     wvstart('mget')
     unlink_if_exists(b'file-1')
     bup(b'ftp', input=jl(b'mget src/latest/file-1'))
@@ -122,8 +131,5 @@ def test_ftp(tmpdir):
     bup(b'ftp', input=jl(b'mget src/latest/file-symlink'))
     with open(b'file-symlink', 'rb') as f:
         wvpasseq(b'excitement!\n', f.read())
-    wvpassne(0, bup(b'ftp',
-                    input=jl(b'mget src/latest/bad-symlink dest'),
-                    check=False, stdout=None).rc)
     # bup mget currently always does pattern matching
     bup(b'ftp', input=b'mget src/latest/not-there\n')