Comment 1 for bug 1982534

Some comments and questions on SRU review:

This is a bit special since it's not the usual "backport a fix" type SRU, so I thought that reviewing the code changes themselves seemed more important than usual in this case. So here are my code review comments:

Why are you calling subprocess.Popen() instead of subprocess.check_call() or similar? To use Popen() directly, you also need to call wait() on the output of subprocess.Popen() really and then check the result code. It'll work if you don't, but any problems will fail silently and won't be logged, so as written this seems buggy to me.

On check_call() vs. call(), I suggest check_call() since you're already catching, logging and then ignoring errors, and a failure would then be logged appropriately.

Presumably this script needs to work with the Python version shipped in Bionic? I checked and that seems to be 3.6, and https://docs.python.org/3/library/subprocess.html#subprocess.check_call says check_call() was introduced prior to 3.3, so I think you're good there.

Why are you invoking the rm command instead of calling os.unlink()?

Note that there are cases where two files are the same but not a symlink. A hard link is one, and that would be fine in this case. Another case that would not be fine is a bind mount. You might conclude that this case isn't reasonable to support; I'll leave that decision to you.

However, if you aren't checking hashes following review changes, then please update the bug description so that you're no longer saying that you are :)