bup: breaks if PYTHONOPTIMIZE is set (reported in Debian)

[Jon Dowland]

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=668997

"bup uses the Python assert statement for regular operations, not just
for additional sanity checks that go beyond what should be done during
normal operation (i.e. "debugging assertions"). Python disables assert
when running in optimised mode [1,2], e.g. by running with -O [3] or
setting the PYTHONOPTIMIZE environment variable [4]."

[Niklas Hambüchen]

From http://docs.python.org/reference/simple_stmts.html#assert

"In the current implementation, the built-in variable __debug__ is True
under normal circumstances, False when optimization is requested"

"Assignments to __debug__ are illegal. The value for the built-in
variable is determined when the interpreter starts."

Doesn't look like something one should build control flow on top of,
especially as there is no guarantee that non-cPython interpreters will
behave the same.

[Gabriel Filion]

So basically, it makes bup "vulnerable" to receiving crap and crapping
out with it.

Maybe there's just a couple of asserts that should be converted to if ->
fatal(). Especially ones that verify user input (like option strings).

--
Gabriel Filion

[Aaron M. Ucko]

Gabriel Filion <gabriel...@gmx.net> writes:

> Maybe there's just a couple of asserts that should be converted to if ->
> fatal(). Especially ones that verify user input (like option strings).

As I understand it, the issue is more that some assertions have side
effects which are problematic to skip. I see two in lib/bup/git.py:

962: assert(self.p.poll() == None)
988: assert(self.p.stdout.readline() == '\n')

--
Aaron M. Ucko, KB1CJC (amu at alum.mit.edu, ucko at debian.org)
http://www.mit.edu/~amu/ | http://stuff.mit.edu/cgi/finger/?a...@monk.mit.edu

[Rob Browning]

Fix a number of places where bup's assertions had material
side-effects, or where other code expected to see the AssertionError,
neither of which happen when optimization is enabled.

Reported-by: Jon Dowland <jm...@debian.org>
Signed-off-by: Rob Browning <r...@defaultvalue.org>
---

Proposed for inclusion in master, maybe 0.25.

cmd/midx-cmd.py | 3 ++-
lib/bup/client.py | 3 ++-
lib/bup/git.py | 6 ++++--
lib/bup/helpers.py | 3 ++-
lib/bup/t/tclient.py | 2 +-
5 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/cmd/midx-cmd.py b/cmd/midx-cmd.py
index 1243c8d..62011e4 100755
--- a/cmd/midx-cmd.py
+++ b/cmd/midx-cmd.py
@@ -135,7 +135,8 @@ def _do_midx(outdir, outfilename, infilenames, prefixstr):
print p.idxnames
assert(len(p) == total)
for pe, e in p, git.idxmerge(inp, final_progress=False):
- assert(i == pi.next())
+ pin = pi.next()
+ assert(i == pin)
assert(p.exists(i))

return total, outfilename
diff --git a/lib/bup/client.py b/lib/bup/client.py
index c2c83e2..7f9334b 100644
--- a/lib/bup/client.py
+++ b/lib/bup/client.py
@@ -40,7 +40,8 @@ def parse_remote(remote):
url_match = re.match(
'%s(?:%s%s)?%s' % (protocol, host, port, path), remote, re.I)
if url_match:
- assert(url_match.group(1) in ('ssh', 'bup', 'file'))
+ if not url_match.group(1) in ('ssh', 'bup', 'file'):
+ raise ClientError, 'unexpected protocol: %s' % url_match.group(1)
return url_match.group(1,3,4,5)
else:
rs = remote.split(':', 1)
diff --git a/lib/bup/git.py b/lib/bup/git.py
index ad4668b..403b969 100644
--- a/lib/bup/git.py
+++ b/lib/bup/git.py
@@ -962,7 +962,8 @@ class CatPipe:
if not self.p or self.p.poll() != None:
self._restart()
assert(self.p)
- assert(self.p.poll() == None)
+ poll_result = self.p.poll()
+ assert(poll_result == None)
if self.inprogress:
log('_fast_get: opening %r while %r is open\n'
% (id, self.inprogress))
@@ -988,7 +989,8 @@ class CatPipe:
yield type
for blob in it:
yield blob
- assert(self.p.stdout.readline() == '\n')
+ readline_result = self.p.stdout.readline()
+ assert(readline_result == '\n')
self.inprogress = None
except Exception, e:
it.abort()
diff --git a/lib/bup/helpers.py b/lib/bup/helpers.py
index d3eea37..5c46839 100644
--- a/lib/bup/helpers.py
+++ b/lib/bup/helpers.py
@@ -723,7 +723,8 @@ def path_components(path):
full_path_to_name). Path must start with '/'.
Example:
'/home/foo' -> [('', '/'), ('home', '/home'), ('foo', '/home/foo')]"""
- assert(path.startswith('/'))
+ if not path.startswith('/'):
+ raise Exception, 'path must start with "/": %s' % path
# Since we assume path startswith('/'), we can skip the first element.
result = [('', '/')]
norm_path = os.path.abspath(path)
diff --git a/lib/bup/t/tclient.py b/lib/bup/t/tclient.py
index 85dd8b2..78d845c 100644
--- a/lib/bup/t/tclient.py
+++ b/lib/bup/t/tclient.py
@@ -144,5 +144,5 @@ def test_remote_parsing():
try:
client.parse_remote('http://asdf.com/bup')
WVFAIL()
- except AssertionError:
+ except client.ClientError:
WVPASS()
--
1.7.10.4

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> Fix a number of places where bup's assertions had material
> side-effects, or where other code expected to see the AssertionError,
> neither of which happen when optimization is enabled.

Pushed to master.

Just a note -- since we seem to be very short on reviewers, I may start
pushing some patches unreviewed, in the interest of finishing 0.25.

Please let me know if this causes any trouble.

Thanks
--
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4