Pádraig Brady <[EMAIL PROTECTED]> wrote:
> Subject: [PATCH] Add new program: truncate
Nice!
Thanks for doing all that.
Here's a first pass:
- fix a typo s/tound/round/ : > [EMAIL PROTECTED] => 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.
_______________________________________________
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
