Re: New utility pb (patch included)

[Alfred M. Szmidt]

   I have created a new utility pb (progress bar) for gathering progress
   information from different utilities. Currently cp and mv are supported
   with a patch included. In future I could add support for other utilities
   such dd too.

Wonderful!  Always wanted something like this, but was always to darn
lazy to do it.

   Most likely there are lot of bugs at the moment but I could fix them and
   improve the utility in the future.

Have you found any bugs yet?  I haven't had a chance to use pb just
yet, so I wouldn't know.  But that is darn high on my list of things
to play with (will be so nice when copying files over ftp in
GNU/Hurd!)

Some comments from a little old me as a first look (it was a very
quick one):

You sent the Makefile.in file that gets generated, no need for that.
Such files are as you know, autogenerated.

     -v, --verbose            Don't clean progress info with a newline

I would expect a different behaviour from --verbose, like showing even
more information.  Not "cleaning the progressbar info".  That IMHO
should be in a seperate option.


   diff -Nur coreutils-5.1.3_orig/src/copy.c coreutils-5.1.3_new/src/copy.c
   --- coreutils-5.1.3_orig/src/copy.c  2004-02-07 17:42:04.000000000 +0200
   +++ coreutils-5.1.3_new/src/copy.c   2004-02-13 11:28:12.000000000 +0200
   @@ -21,6 +21,7 @@
    #include <stdio.h>
    #include <assert.h>
    #include <sys/types.h>
   +#include <signal.h>

    #if HAVE_HURD_H
    # include <hurd.h>
   @@ -73,6 +74,16 @@
      dev_t st_dev;
    };

   +/* Tell something about current file being processed. */
   +struct status_info {
   +  char program_name[10];
   +  const char *name;
   +  off_t size;
   +  off_t index;
   +};

Please follow the GCS, you have a bunch of such small "errors" in your
patch, and the new program.

   diff -Nur coreutils-5.1.3_orig/src/pb.c coreutils-5.1.3_new/src/pb.c
   --- coreutils-5.1.3_orig/src/pb.c    1970-01-01 02:00:00.000000000 +0200
   +++ coreutils-5.1.3_new/src/pb.c     2004-02-13 16:20:47.000000000 +0200
[...snip...]
   +/* Convert an integer to SI-units and return a pointer to the string. */
   +const char *si_units(off_t size)

Can't you use one of the library functions for this?

[...snip...]
   +  /* Initializing signal handlers */
   +  if (signal(SIGCHLD, signal_handler) == SIG_ERR)
   +    {
   +      error (0, errno, _("failed to initialize a signal handler"));
   +      exit (EXIT_FAILURE);
   +    }

WHat is wrong with just: error (EXIT_FAILURE, errno, ...);?

[...snip...]
   +      /* Update the progress bar every 100 ms. */
   +      if (++counter == 10)

Personally, I dislike that kind of thing.  Hard to read IMHO (yes,
small nitpick but oh well).  Something like:

,----
| counter++;
| if (counter == 10)
|  ...
`----

Is far nicer.  Also, making the default update interval a macro, would
be nice.


If you'd like some real, useful, comments, I'll happily supply them.
It would be wonderful to see this in coreutils, or something like
this.  The nice thing about this is that it is quite general, and easy
to smack into other programs.


Cheerio, and happy hacking!