>>>>> "dmarker" == David Marker <David.Marker at sun.com> writes:
dmarker> Take a look, unless anybody really hates the concept we will
dmarker> start by using this to update hg.os.o
I like the concept. Here are some comments and questions about the
script itself (which overall looks good):
> Although it could be used for most hooks, any "pre" hook is
> altered.
What does this mean?
> In addition to passing on "KWARGS" the following environment
> variables are also passed on:
[...]
> TIP [*] (the tip node at the time bghook() was called)
Nit: something less generic (e.g, HGTIP) would probably be better.
> # And it would probably be a bad thing if the background process could
> # do a rollback.
s/probably//
> if script.endswith("pyc"):
> script = script[:-1] # remove that.
The "remove that" comment is unclear. I'd change it to "remove final
'c'" or "s/pyc/py/".
> # If a problem occurred, go back through {REPO}/.hg/hgrc and
> # handle each [atjobhook] one by one.
What's "atjobhook"? Is it a historical name for "background hook"?
> It is not recommended to run this standalone at all.
> This script needs a standalone mode for it to re-run itslef
> and thus get "off process" so hg doesn't wait.
Typo: "itslef" -> "itself".
I found the word "needs" to be confusing here. It appears to be an
explanation for why there's a main method, but it could be read as a
to-do item.
I'd change this text to something like
This script should not normally be run directly. Rather, it should
only be invoked by the bghook method, which should only be invoked
as a Mercurial hook.
> rc = True # True means repo.hook() failed.
[...]
> rc = repo.hook(hooktype, True, **kwargs)
[...]
> if ok is False or rc is True:
External hooks return an integer exit status, not a boolean true/false.
I would juse use
if not okay or rc:
> ebuffer = ui.popbuffer()
> for line in ebuffer:
> wstream.write(line)
> raise BackgroundException
Possible improvement: some way to capture the contents of ebuffer in the
event that something isn't working right, but nothing is being flagged
as an error or exception.
> except AssertionError:
> # These are a subclass of Exception, but have no useful
> # information and its hard to tell that is what happened
> # it isn't handled separately.
> wstream.write("ERROR: hook tripped over assertion\n")
> ok = False
This doesn't make sense to me. I thought AssertionError included the
failed assertion; it seems like we should print that.
> # Otherwise if all was well, no mail need be sent.
> # m.subject(m.msg["Subject"].replace("FAILED", "TESTING"))
> # m.send() # XXX just for testing
Possible improvement: some way to enable the mail without having to edit
the script.
cheers,
mike
