Nathan Hartman wrote on Wed, Oct 23, 2019 at 11:48:28 -0400:
> On Sun, Oct 20, 2019 at 2:16 AM Yasuhito FUTATSUKI <futatuki_at_poem.co.jp>
> wrote:
> > With this approach, I think the script should exit with code
> > other than 0 if an exception raised, to notify error occured to hook
> > scripts. Python script exits with code 1 if the script is termitated
> > by unhandled exceptions, so it can notify on current implementation.
> > Hook scripts can also watch stderr to log what happens on mailer.py,
> > etc., though our post-*.tmpl don't do it.
>
>
> I think the attached patch accomplishes that. (I'm not putting it inline
> because it's 200 lines long! Most of them due to indent changes.)
In such cases, it's helpful to attach a secondary diff, generated with «svn
diff -x-wp», for review purposes.
> I'd appreciate a review!
>
> The changes are pretty basic:
> * A new exception class is added: MessageSendFailure.
> * The contents of SMTPOutput.finish() are wrapped in try..except. We
> catch any smtplib.SMTPException, print an error to stderr, and raise
> MessageSendFailure.
> * In all classes' generate(), we wrap the body of the for-loop in a
> try..except. We catch MessageSendFailure and increment a counter;
> generate() returns the value of the counter.
> * In main, the return value of messenger.generate() is returned to
> main's caller.
> * In the __main__ program, the call to svn.core.run_app() is wrapped
> with sys.exit().
What is the functional change? A log message should describe the functional
change and the the reasons therefor; it shouldn't simply narrate the diff. Your
last bullet is a good counter-example.
As Yasuhito said, unhandled exceptions already cause a non-zero exit code, so
I'm guessing the point here is that SMTPException during the first delivery no
longer prevent second (and any subsequent) deliveries from being attempted? In
other words, in what case does the patch have a user-visible effect?
> Index: tools/hook-scripts/mailer/mailer.py
> ===================================================================
> --- tools/hook-scripts/mailer/mailer.py (revision 1868639)
[ I am quoting the -x-wp diff, rather than the one Nathan sent. ]
> @@ -294,6 +295,9 @@ class SMTPOutput(MailedOutput):
> self.cfg.general.smtp_password)
> server.sendmail(self.from_addr, self.to_addrs, self.buffer.getvalue())
> server.quit()
> + except smtplib.SMTPException as detail:
> + sys.stderr.write("SMTP error occurred: %s" % detail);
Error messages should be signed:
sys.stderr.write("mailer.py: SMTP error occurred: %s" % (detail,))
# ^^^^^^^^^^^
> @@ -1455,8 +1476,8 @@ if the property was added, modified or deleted, re
> if not os.path.exists(config_fname):
> raise MissingConfig(config_fname)
>
> - svn.core.run_app(main, cmd, config_fname, repos_dir,
> - sys.argv[3:3+expected_args])
> + sys.exit(svn.core.run_app(main, cmd, config_fname, repos_dir,
> + sys.argv[3:3+expected_args]))
If run_app returns 256, the exit code will be zero. I suggest to do
ret = svn.core.run_app(…)
sys.exit(1 if ret else 0)
I did not review the core functionality changes due to lack of time, sorry.
Cheers,
Daniel
Received on 2019-10-24 04:37:30 CEST