[distcc] Patch?

[Jean Delvare]

Hi Eddie,

> Here's a patch for what I've worked on. Just to reiterate this patch
> does the following to distccmon-text:
> 
>     1) Add a timestamp to every line from the distccmon-text output
>  
>     2) Only print the newline after a series of distcc calls have been
> displayed
> 
> And here's the patch. Any chance this could make it into the official
> tree? (I tried submitting using tla, but apparently I don't have
> access. :) )

Both improvements sound very good to me, even if I am not a regular user
of distccmon-text.

I however have several objections to your implementation, and some more
general comments about patch submission.

> --- orig/src/mon-text.c
> +++ mod/src/mon-text.c
> @@ -36,7 +36,7 @@
>  #include "exitcode.h"
>  #include "snprintf.h"
>  #include "mon.h"
> -
> +#include "time.h"

I can't find any file named time.h in the distcc project. Don't you
really mean <time.h>?

> @@ -85,25 +85,38 @@
>       * other program, so make sure we're always line buffered. */
>      setvbuf (stdout, NULL, _IOLBF, BUFSIZ);
>  
> +    char currentTimeAsString[9];

Older C compilers will not like variables being declared in the middle
of a block. Also, please change the name of that variable. The distcc
project names variables this_way, not thatWay. Same applies to all
variables declared in your patch.

> -#if 1
> +
> +            havePrinted = 1;
> +
>              if (i->curr_phase == DCC_PHASE_DONE)
>                  continue;
> -#endif

I wouldn't drop #if/#endif, even if I don't know why it is there. I
guess Martin has good reasons.

> +            strftime(currentTimeAsString, 9, "%H:%M:%S",
> localtime(&currentTime));

Note that your mailer breaks long lines. I had to edit your patch
manually before I could apply it. Either configure your client
differently, or submit your patches as attachements instead of inline.

>              /* Assume 80 cols = */
> -            printf("%6ld  %-10.10s  %-30.30s %24.24s[%d]\n",
> +            printf("[%8s] %6ld  %-10.10s  %-30.30s %24.24s[%d]\n",
> +                    currentTimeAsString,
>                     (long) i->cpid,
>                     dcc_get_phase_name(i->curr_phase),
>                     i->file, i->host, i->slot);

This doesn't fit in 80 columns anymore, while this was very convenient.
Also notice that you are computing the time string for each file/host
line, while it will always be (almost) the same time. I would suggest
that you compute the time string *outside* of the for loop, and print it
only *once*, before the loop. This would result in something like:

[12:59:26]
  7181  Compile     mon-text.c                                      arrakis[0]
  7177  Compile     mon.c                                         localhost[0]
  7157  Compile     popt.c                                         mahadeva[0]

instead of:

[12:59:26]   7181  Compile     mon-text.c                                      arrakis[0]
[12:59:26]   7177  Compile     mon.c                                         localhost[0]
[12:59:26]   7157  Compile     popt.c                                         mahadeva[0]

This has the following advantages:
1* Still fits on 80 columns.
2* Less computation.

If you insist to have a the time on each line, please at least change
the widths so that the lines will fit on 80 columns again.

> -        printf("\n");
> +        if(havePrinted)
> +            printf("\n");

Same can be done without the additional variable, see attached patch.

And a more general comment: I'd suggest that you do not include several
improvements in a single patch. One change per patch makes reviewing and
merging way easier.

Care to redo a patch implementing (1) only (since mine implements (2)
more efficiently, or so I think), and addressing the points I listed
above?

Martin, like my patch?

Thanks,
-- 
Jean Delvare
-------------- next part --------------
--- distcc-2.18.3/src/mon-text.c.orig	2004-10-24 07:05:49.000000000 +0200
+++ distcc-2.18.3/src/mon-text.c	2005-02-20 13:04:19.000000000 +0100
@@ -103,7 +103,9 @@
                    i->file, i->host, i->slot);
         }
 
-        printf("\n");
+        /* Scroll only if something is actually happening */
+        if (i != list)
+            printf("\n");
 
         /* XXX: usleep() is probably not very portable */
         usleep(delay * 1000000);