From 1c968112d2bfb1d93c7009a42c518de5309dd75c Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Sun, 24 Jan 2021 21:35:34 +0100 Subject: [PATCH] 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 Reviewed-by: Rob Browning [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 Tested-by: Rob Browning --- lib/bup/cmd/ftp.py | 39 ++++++++++++++++++++------------------- test/ext/test_ftp.py | 44 +++++++++++++++++++++++++------------------- 2 files changed, 45 insertions(+), 38 deletions(-) diff --git a/lib/bup/cmd/ftp.py b/lib/bup/cmd/ftp.py index 9a33754..bd3bd69 100644 --- 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 [localname]') + raise CommandError('Usage: get [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 d6bb273..3fb6467 100644 --- 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') -- 2.39.2