bug#10253: mention +FORMAT in ls time style reminder help blurb

2011-12-10 Thread jidanni
Well wherever you say 4/5ths of something
that means the other 1/5th does not exist,
so it would be better to say nothing.





bug#10253: mention +FORMAT in ls time style reminder help blurb

2011-12-10 Thread Jim Meyering
Eric Blake wrote:
> On 12/09/2011 12:22 PM, Jim Meyering wrote:
>> jida...@jidanni.org wrote:
>>> $ ls -t1 --time-style=%c -og
>>> ls: invalid argument `%c' for `time style'
>>> Valid arguments are:
>>>   - `full-iso'
>>>   - `long-iso'
>>>   - `iso'
>>>   - `locale' <-you forgot to also mention "+FORMAT"
>>> Try `ls --help' for more information.
>>> $ ls -t1 --time-style=+%c -og
>>
>> Thanks for the report.
>> However, that doesn't lend itself well to a clean fix, since +FORMAT
>> is not a literal string option argument like the others, and currently
>> that diagnostic is generated automatically based on the list of literal 
>> strings.
>
> I agree that listing `+FORMAT' doesn't fit the pattern.  But maybe we
> can make it obvious that there is a non-literal possibility, as well as
> also mentioning the posix- prefix already covered by 'ls --help':
>
> Valid arguments are:
>   - `[posix-]full-iso'
>   - `[posix-]long-iso'
>   - `[posix-]iso'
>   - `[posix-]locale'
>   - `+' followed by formatting directives
> Try `ls --help' for more information.

Sure, that is possible, and adding the [posix-] prefixes would be a nice bonus.
The trouble is that the current code is not only nice and compact, but will
safely and automatically reflect any addition to the list of time styles:

switch (XARGMATCH ("time style", style,
   time_style_args,
   time_style_types))

That handles everything, including printing the offending diagnostic
and exiting.  If you pick it apart in preparation for open-coding, you
then have to find a way to ensure that the new diagnostic stays in sync
with list of possible option arguments.

All feasible, but is it worth it, just for an improved diagnostic that
won't be seen unless you use ls's --time-style=V option with an invalid V ?

I'm leaning away, but if someone comes up with a clean and
maintainable way to do it, no problem.

> Also, I noticed that 'ls --help' uses FORMAT in the text for
> --time-style, but doesn't define FORMAT anywhere else; we probably ought
> to have a one-line sentence at the bottom, after the paragraph on SIZE,
> stating:
>
> See `date --help' for valid directives that may appear in FORMAT.

Good idea.  Patch most welcome.





bug#9157: [PATCH] dd: sparse conv flag

2011-12-10 Thread Jim Meyering
Roman Rybalko wrote:
> ---
>  doc/coreutils.texi |4 +++
>  src/dd.c   |   22 +++-
>  tests/Makefile.am  |1 +
>  tests/dd/sparse|   70 
> 
>  4 files changed, 96 insertions(+), 1 deletions(-)
>  create mode 100755 tests/dd/sparse

Thank you for the patch.  It's nice to see accompanying test suite changes.
I see that copyright paperwork is now on file with the FSF, so...

Did you notice that cp now has the capability (using extents)
to preserve holes efficiently on some file system types?
Recent kernels also have SEEK_HOLE/SEEK_DATA support with which
it is more efficient to detect holes.

Here are some things we'll have to consider before
adding a new hole-punching option to dd:

Your patch may create a hole in the destination for each sequence of
length seek_size or greater of zero bytes in the input.
As you may have seen in the cp-related discussion, one may
want different options:
  - preserve a file's hole/non-hole structure
  - efficiently detect existing holes and fill them with explicit zeros in dest
  - efficiently detect existing holes and seek-in-dest for each sequence of
zeros (longer than some minimum) in non-hole input

> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index 424446c..761c698 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -8127,6 +8127,10 @@ Pad every input block to size of @samp{ibs} with 
> trailing zero bytes.
>  When used with @samp{block} or @samp{unblock}, pad with spaces instead of
>  zero bytes.
>
> +@item sparse
> +@opindex sparse
> +Make sparse output file.

Please say a little more here.
I.e., when might a hole be introduced?
When is this option useful?

>  @end table
>
>  The following ``conversions'' are really file flags
> diff --git a/src/dd.c b/src/dd.c
> index 0824f6c..0393740 100644
> --- a/src/dd.c
> +++ b/src/dd.c
> @@ -126,7 +126,8 @@ enum
>  C_NOCREAT = 01,
>  C_EXCL = 02,
>  C_FDATASYNC = 04,
> -C_FSYNC = 010
> +C_FSYNC = 010,
> +C_SPARSE = 020
>};
>
>  /* Status bit masks.  */
> @@ -268,6 +269,7 @@ static struct symbol_value const conversions[] =
>{"sync", C_SYNC},  /* Pad input records to ibs with NULs. */
>{"fdatasync", C_FDATASYNC},/* Synchronize output data before 
> finishing.  */
>{"fsync", C_FSYNC},/* Also synchronize output metadata.  */
> +  {"sparse", C_SPARSE},  /* Make sparse output file. */
>{"", 0}
>  };
>
> @@ -533,6 +535,9 @@ Each CONV symbol may be:\n\
>fsync likewise, but also write metadata\n\
>  "), stdout);
>fputs (_("\
> +  sparsemake sparse output file\n\
> +"), stdout);
> +  fputs (_("\
>  \n\
>  Each FLAG symbol may be:\n\
>  \n\
> @@ -985,6 +990,21 @@ iwrite (int fd, char const *buf, size_t size)
>  {
>ssize_t nwritten;
>process_signals ();
> +  if (conversions_mask & C_SPARSE)
> +{
> +  off_t seek_size = 0;
> +  while (total_written + seek_size < size && buf[total_written + 
> seek_size] == 0)
> +++seek_size;
> +  if (seek_size)
> +{
> +  off_t cur_off = 0;
> +  cur_off = lseek(fd, seek_size, SEEK_CUR);
> +  if (cur_off < 0)
> +break;

dd must not ignore lseek failure.

> +  total_written += seek_size;
> +  continue;
> +}
> +}
>nwritten = write (fd, buf + total_written, size - total_written);
>if (nwritten < 0)
>  {
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index ebd1b11..0f1376a 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -364,6 +364,7 @@ TESTS =   \
>dd/skip-seek   \
>dd/skip-seek2  \
>dd/skip-seek-past-file \
> +  dd/sparse \
>dd/stderr  \
>dd/unblock \
>dd/unblock-sync\
> diff --git a/tests/dd/sparse b/tests/dd/sparse
> new file mode 100755
> index 000..f0e0806
> --- /dev/null
> +++ b/tests/dd/sparse
> @@ -0,0 +1,70 @@
> +#!/bin/sh
> +# Ensure that dd conv=sparse works.
> +
> +# Copyright (C) 2003, 2005-2011 Free Software Foundation, Inc.

Use only 2011 as the copyright year.

> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public Licen