bug-coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Bug#467378: coreutils: Please include a program to truncate files


From: Jim Meyering
Subject: Re: Bug#467378: coreutils: Please include a program to truncate files
Date: Wed, 26 Mar 2008 18:52:50 +0100

Pádraig Brady <address@hidden> wrote:
> Subject: [PATCH] Add new program: truncate

Nice!
Thanks for doing all that.

Here's a first pass:

  - fix a typo s/tound/round/ : > address@hidden  => tound up to multiple of

  - please indent using only spaces (no TABs) and add this at the end of
    the file:
/*
 * Local variables:
 *  indent-tabs-mode: nil
 * End:
 */

  - don't use a bare "%s" as a format string in a diagnostic;
    instead, always include some English text to say what failed
    or was unexpected.  E.g., this
      > +      if (stat (ref_file, &sb) != 0)
      > +       error (EXIT_FAILURE, errno, "%s", quote (ref_file));
    would become this:
      > +      if (stat (ref_file, &sb) != 0)
      > +        error (EXIT_FAILURE, errno, _("cannot access %s"), quote 
(ref_file));

  - I noticed this spacing oddity in a couple of places (space after "*"):
    > +static int parse_len (const char *str, off_t * size);
    You can fix that by adding this line to ~/.indent.pro:
      -Toff_t
    and rerunning GNU indent:

  - Remove trailing blanks in these format strings:
  (I see that it makes diagnostics a little more symmetric, but all other
  tools emit no space before the ": strerror(error)" string, so it's
  better for consistency.  Another alternative: use quotearg_colon rather
  than just quote, since it's delimited on the right by a ":", and most
  of the time, the quote()-supplied `...' delimiters are not useful)
    > +   if (parse_len (optarg, &size) == -1)
    > +     error (EXIT_FAILURE, errno, "%s ", quote (optarg));
    > +   /* Rounding to multiple of 0 is nonsensical */
    > +   if ((rel_mode == rm_rup || rel_mode == rm_rdn) && size == 0)
    > +     error (EXIT_FAILURE, EINVAL, "%s ", quote (optarg));

  - run "make syntax-check":
   = The "po-check" failure would have told you that you need to add the
       file name, src/truncate.c to po/POTFILES.in.
   = Then, the "sc_prohibit_safe_read_without_use" failure would have
       told you to remove the unnecessary `#include "safe-read.h"' line.

  - Rather than relying on EOVERFLOW (which, as you note, may not apply),
    how about just saying you can't use that file because its
    size appears to be negative?
      > +                 /* overflow is the only reason I can think
      > +                    this would ever go negative for the above types */
      > +                 error (0, EOVERFLOW, "%s", quote (fname));
    E.g.,
      error (0, 0, "%s has unusable, apparently negative size", quote (fname));

  - I've made a point of avoiding prototypes for static functions (defining
    usage before main, etc.), in most src/*.c files, so for consistency,
    it'd be nice, but with just two simple functions here, it's not a big deal.

  - when you amend that commit, please leave an empty line between the first
    line of the log message and all the remaining ones.  Otherwise, some git
    tools end up using a *very* long (entire log concatenated) subject line.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]