[PATCH] Implement printtime hook function

[PATCH] Implement printtime hook function

[Phil Pennock]

[-- Attachment #1: Type: text/plain, Size: 3815 bytes --]

A former colleague wanted to be able to have an arbitrary action taken
if a job takes too long to complete, which is almost what $REPORTTIME
gives us, but we can't have arbitrary actions taken.

So it seems like what's needed is a hook function (and array).

Below is a patch which I will *not* commit, because it's a first pass to
get feedback.  For one thing, it hooks into printtime so is also invoked
when the time pre-command modifier prints output.  This could be useful,
but might not be desired; I figured it would be best to pass the
printtime() `desc` parameter as the first parameter to the new hook, but
while that lets folks analyse and short-circuit I think it's putting the
onus in the wrong place.

I'm tempted to make this $REPORTTIME only, by amending printtime() to
take a new bool parameter and renaming the hook to reporttime().

Another issue is that reporttime() (shell-level function) becomes a new
magic function additionally called, and we don't have namespaces for
functions, but I think our normal approach is to accept that it _might_
conflict with scripts, right?

Then there's the parameters needed; I've opted to pass in the desc
(which for $REPORTTIME is the command invoked) in the first parameter
then elapsed/user/system/total time as subsequent parameters.  I half
think it might be worth putting a bunch of the various variables used
for $TIMEFMT into a hash, but if I do that I'm likely to want to add an
existshookfunc() check to reduce the work, or create a magic hash which
extracts values and make $TIMEFMT reduce to referencing that hash, or a
new print flag similar to -P which can be used to get the same
expansions as $TIMEFMT.

(Patch also fixes an unused variable complaint)

What do people think?  Is this generally useful enough, and should I
amend printtime?  How best to pass the various values or make them
available?

I *think* I favour:
 1: adding a new parameter to printtime();
 2: moving moving the printtime() expansion logic into a function which
    can be used by "print -T" or the current $TIMEFMT expansion; this
    involves making a new global which can hold the last child_times_t
    object around for implicit use;
 3: calling the hook "reporttime" and then the hook functions can just
    use "print -T" for any data they want.

I can add documentation when things are more firmed, then resend the
patch and go through the zsh/git/listid cycle.

Thoughts?

-Phil

diff --git a/Src/jobs.c b/Src/jobs.c
index 0dbb10b..2ca3bfe 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -645,11 +645,13 @@ static void
 printtime(struct timeval *real, child_times_t *ti, char *desc)
 {
     char *s;
+    LinkList funcargs;
     double elapsed_time, user_time, system_time;
 #ifdef HAVE_GETRUSAGE
     double total_time;
 #endif
     int percent, desclen;
+    char buf[20];
 
     if (!desc)
     {
@@ -830,6 +832,19 @@ printtime(struct timeval *real, child_times_t *ti, char *desc)
     unqueue_signals();
     putc('\n', stderr);
     fflush(stderr);
+
+    funcargs = newlinklist();
+    addlinknode(funcargs, "printtime");
+    addlinknode(funcargs, desc);
+    sprintf(buf, "%4.2f", elapsed_time);
+    addlinknode(funcargs, ztrdup(buf));
+    sprintf(buf, "%4.2f", user_time);
+    addlinknode(funcargs, ztrdup(buf));
+    sprintf(buf, "%4.2f", system_time);
+    addlinknode(funcargs, ztrdup(buf));
+    sprintf(buf, "%4.2f", total_time);
+    addlinknode(funcargs, ztrdup(buf));
+    callhookfunc("printtime", funcargs, 1, NULL);
 }
 
 /**/
@@ -1714,7 +1729,9 @@ static int hackspace;
 void
 init_jobs(char **argv, char **envp)
 {
+#ifndef HAVE_SETPROCTITLE
     char *p, *q;
+#endif
     size_t init_bytes = MAXJOBS_ALLOC*sizeof(struct job);
 
     /*

[-- Attachment #2: Type: application/pgp-signature, Size: 163 bytes --]

Re: [PATCH] Implement printtime hook function

[Peter Stephenson]

On Wed, 26 Jun 2013 02:19:49 -0400
Phil Pennock <zsh-workers+phil.pennock@spodhuis.org> wrote:
> I'm tempted to make this $REPORTTIME only, by amending printtime() to
> take a new bool parameter and renaming the hook to reporttime().

I think that would be preferable, since there are other reasons for
printing time information which are triggered in other ways.

> Another issue is that reporttime() (shell-level function) becomes a new
> magic function additionally called, and we don't have namespaces for
> functions, but I think our normal approach is to accept that it _might_
> conflict with scripts, right?

I'd be tempted to call it zsh_reporttime.  I've been putting zsh_
prefixes in front of new hooks and similar for a while.

> What do people think?  Is this generally useful enough, and should I
> amend printtime?  How best to pass the various values or make them
> available?

It seems like it might be useful.  It's probably limited by the current
implementation to jobs at the command line, though.

If it's restricted to triggers by REPORTTIME I don't think inefficiency
about passing arguments is too much of a worry.  The current method is
probably OK: the set of numbers in the time information has been stable
for a long time.

-- 
Peter Stephenson <p.stephenson@samsung.com>       Principal Software
Engineer Tel: +44 (0)1223 434724              Samsung Cambridge
Solution Centre St John's House, St John's Innovation Park,
Cowley Road, Cambridge, CB4 0DS, UK