[PATCH 6/6] ftp: clean up error handling

[Rob Browning]

From: Johannes Berg <joha...@sipsolutions.net>

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 <joha...@sipsolutions.net>
Reviewed-by: Rob Browning <r...@defaultvalue.org>
[r...@defaultvalue.org: let unexpected exceptions propgate]
[r...@defaultvalue.org: use str for exception messages]
[r...@defaultvalue.org: render paths via path_msg()]
[r...@defaultvalue.org: add match_rx_grp and test for expected output]
Signed-off-by: Rob Browning <r...@defaultvalue.org>
Tested-by: Rob Browning <r...@defaultvalue.org>
---

Pushed.

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 9a33754c..bd3bd699 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 <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 d6bb2737..3fb6467a 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.30.2