Fix: Correct global flock fallbacks (#1834)

bit-team / backintime

Back In Time - An easy-to-use backup tool for GNU/Linux using rsync in the back

https://backintime.readthedocs.io

GNU General Public License v2.0

2.09k stars 204 forks source link

Fix: Correct global flock fallbacks (#1834) #1835

Closed NickNackGus closed 3 months ago

NickNackGus commented 3 months ago

Prevents a crash if XDG_RUNTIME_DIR is not set
Default to ~/.cache instead of ~/cache
return None explicitly instead of implicitly (pylint R1710 inconsistent return statements)

Fix #1834

buhtz commented 3 months ago

Hello Timothy, On behalf of the team, a big thank you for reporting the bug, investigating it and also contribute fix. Thank you for taking the time to improve Back In Time. We appreciate it.

Your PR looks awesome on a first view. I will review it as soon as possible. I will do some more testing. I need to investigate if this affects Debian, too. I am assuming not but if so it would block the current version 1.5.2 arriving in Debian and the upcoming Ubuntu release (Debian Import Freeze is in some days on 15th August).

Best, Christian

buhtz commented 3 months ago

I can confirm that this will fix #1834

NickNackGus commented 3 months ago

Done - though None isn't strictly required in all cases. pylint was only complaining about it where a method returned non-None in some cases, and didn't specify a return value in others.

buhtz commented 3 months ago

Currently all PRs are blocked until Ubunuts "Debian Import Freeze" on 15th August.

Until then maybe one of my team mates can also have a look on this PR.

NickNackGus commented 3 months ago

One thing this PR doesn't address - if there's an exception around here in the code, there's no further information in the logs to help debug. I'm not sure how best to catch and log such issues so that they're easier to debug in the future. Putting a try/catch for anything that inherits from Exception would work, but also add a warning in pylint for catching too general an exception, and would need to be after the logging code was loaded at any rate.

buhtz commented 3 months ago

I don't see a need for more Exception handling. The method _can_use_file() does most of it and also catch a broad Exception in the end. Catch exception you do expect. Don't catch everything. It was right that my code crashed because it was crap and not robust enough.

BIT has a lot edge cases I often don't have in mind. Because of that PR I extended the release process docu about what to consider when doing manual tests.