mr.developer icon indicating copy to clipboard operation
mr.developer copied to clipboard

[FIX] Propagate exception when git url does not match

Open StefanRijnhart opened this issue 5 years ago • 4 comments

I changed the remote of my git source hoping that mr.developer would reset the code as always-checkout was set to force but it silently continued, leaving the old code in place.

Apparently, the git code raises GitError in most cases but in some cases, such as the case of having to update an unknown branch, it exits with a non zero value. When that happens, queued log messages are discarded, and in non-threading mode, builtout halts without an explanation. What is worse is that in threading mode, buildout continues and eventually reports success.

Replacing the sys.exit by raising a GitError fixes the logging but the exceptions still had to be propagated to the main level in threading mode.

The unknown branch was only encountered by accident. The earlier error of the non-matching remote url was not even considered a fatal error. I'm proposing to do so, at least when always-checkout is set to force.

StefanRijnhart avatar Mar 15 '21 19:03 StefanRijnhart

sys.exit(1) is found elsewhere in git.py. Should I replace all occurrences?

StefanRijnhart avatar Mar 15 '21 19:03 StefanRijnhart

I only glanced at this and the general idea to replace sys.exit makes sense. I'm unsure about the way the exception is propagated though.

It also seems like this broke Python >= 3.6

fschulze avatar Mar 16 '21 08:03 fschulze

Thank you for your considerations! I will replace the other occurrences of sys.exit(1) then. As for propagating the exception, I think patching it on the thread is perfectly fine (although I updated the PR this morning to sanitize the threads afterwards). It is a cheapo version of https://stackoverflow.com/a/31614591, but if you like that version better I can refactor accordingly.

Tests are fixed here: https://github.com/fschulze/mr.developer/pull/205

StefanRijnhart avatar Mar 16 '21 10:03 StefanRijnhart

The exception propagation now makes sense to me.

Could you try and add a test for the missing branch case?

fschulze avatar Mar 20 '21 12:03 fschulze