review "assert" usage

[ThomasWaldmann]

See there for the problem description:

https://community.sonarsource.com/t/feature-python-assert-should-be-consider-harmful/38501

TL;DR: for now, do not run borg via python3 -O or with PYTHONOPTIMIZE set.

In the code, assert should be only used for:

• our test suite
• at places in production code where it doesn't really matter if the assert is executed or not. it must never be used if not executing the assert would influence correctness or security.
• if it matters, the assert should be replaced by if <condition>: raise SomeException

So, the task for borg master branch is to do a systematic review and fix all problematic asserts.

In case we find a lot of places to fix, a quick workaround for 1.4-maint branch could be to disallow running borg with assertions switched off, something like:

+try:
+    assert False
+except AssertionError:
+    pass  # OK
+else:
+    print("Borg requires working assertions. Please run Python without -O and/or unset PYTHONOPTIMIZE")
+    sys.exit(2)

Note: 2 is the classic error code for a fatal error, but borg 1.4.x also supports modern exit codes, so an appropriate one (2 or more specific) needs to be returned for that.

[120EE0980]

@ThomasWaldmann I found places where assert should be replaced eg : 1,2.....and many such places which need assert to be replaced.So I would first like to do a quick workaround for 1.4-maint branch. Is this file the entry point(where the workaround needs to be added) for borg??Or are there multiple entry points ?? After this, I would like to fix some problematic asserts.(but in some places I could not understand whether they are problematic or not, please help me in this :) )

[ThomasWaldmann]

The 2 places you found are to catch programming errors and are executed rather frequently.

Thus, such a programming error likely would be catched before a release is made.

Guess archiver.py is the best place to put a quick fix into 1.4.

[120EE0980]

@ThomasWaldmann I am working on replacing assert ..will soon open a PR.Do you also suggest that I make the quick fix into 1.4 in another PR right now or wait?

[ThomasWaldmann]

Yeah, you could do the quick fix for 1.4 first, then work on master, fixing the asserts.

[120EE0980]

@ThomasWaldmann After doing quick fix, when I ran tox -e py39, 1 test is failing..I am unable to debug. Please help.

[ThomasWaldmann]

You need to run pytest with -vv to see the full diff.

[120EE0980]

You need to run pytest with -vv to see the full diff.

[ThomasWaldmann]

Hmm, that does not look like it is related to the quick fix.

• can you reproduce the issue?
• is it only with py39?
• can you reproduce the issue without doing any own changes?

[120EE0980]

1. Yes, can reproduce the error(I ran pytest -vv multiple times to check whether the error is one time or not)
2. No, getting same error with py310 also
3. Yes, by removing changes I made as part of quickfix, still I was able to reproduce the same error.

[ThomasWaldmann]

OK, so:

• file a PR with your quickfix
• file a new issue about that test failure and give all the details, including your system's timezone.

[120EE0980]

I have checked all the specific error codes that borg provides but I found they are not suitable for this use case, so I think 2 is the best one. Also --show-rc does not log rc in this case as it exits before borg initialises.(so I guess can only manually find out rc using echo $?)