Re: [PATCH] add new sort option --xargs (-x)

[Jim Meyering]

"Bo Borgerson" <address@hidden> wrote:
> I had a capitalized error message in this patch.
> I also didn't use a correct commit message format.

Thanks for noticing and correcting.

> Subject: [PATCH] Add new sort option --files0-from=F
>
> * src/sort.c: support new option
> * tests/misc/sort-files0-from: test new option
> * tests/misc/Makefile.am: indicate new test
> * docs/coreutils.texti: explain new option

s/texti/texi/

> * NEWS: advertise new option

Please use capitals and periods in ChangeLogs. ;-)
Follow existing style -- there are plenty of examples.

> diff --git a/NEWS b/NEWS
> index e208b30..492c4e9 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -55,6 +55,11 @@ GNU coreutils NEWS                                    -*- 
> outline -*-
>    options --general-numeric-sort/-g, --month-sort/-M, --numeric-sort/-n
>    and --random-sort/-R, resp.
>
> +  sort accepts a new option, --files0-from=F, that specifies a file
> +  containing a null-separated list of files to sort.  This list is used

s/null/NUL/

> +  instead of filenames passed on the command-line to avoid problems with
> +  maximum command-line (argv) length.
> +
>  ** Improvements
>
>    id and groups work around an AFS-related bug whereby those programs
> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index ee7dbb2..5415394 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -3667,6 +3667,22 @@ Terminate with an error if @var{prog} exits with 
> nonzero status.
>  Whitespace and the backslash character should not appear in
>  @var{prog}; they are reserved for future use.
>
> address@hidden address@hidden
> address@hidden address@hidden
> address@hidden including files from @command{du}

s/du/sort/

If this text is verbatim or nearly identical to that for
wc and/or du, please see if you can use a macro to factor it out.
Hmm... I went to check and spotted the same error (use of `du' in
wc's section) so went ahead and factored out the duplication.

Now, your doc change will be to add this line:

@files0fromOption{sort,}

...
>        --compress-program=PROG  compress temporaries with PROG;\n\
>                                decompress them with PROG -d\n\
> +      --files0-from=F       read input from the files specified by\n\
> +                            NUL-terminated names in file F\n\

Split the string.  Otherwise, your addition pushes its length beyond
a portability limit whose exact number I forget but it's around 500.

>    -k, --key=POS1[,POS2]     start a key at POS1, end it at POS2 (origin 1)\n\
>    -m, --merge               merge already sorted files; do not sort\n\
>  "), stdout);
> @@ -395,7 +399,8 @@ enum
>    CHECK_OPTION = CHAR_MAX + 1,
>    COMPRESS_PROGRAM_OPTION,
>    RANDOM_SOURCE_OPTION,
> -  SORT_OPTION
> +  SORT_OPTION,
> +  FILES0_FROM_OPTION

No big deal, but it's good practice to alphabetize.

...
> diff --git a/tests/misc/sort-files0-from b/tests/misc/sort-files0-from
> new file mode 100755
> index 0000000..a96ab1a
> --- /dev/null
> +++ b/tests/misc/sort-files0-from
> @@ -0,0 +1,105 @@
> +#!/bin/sh
> +# Test "sort --files0-from=F".
> +
> +# Copyright (C) 2002, 2003, 2005-2008 Free Software Foundation, Inc.

This should have only 1 year number: 2008.

If the file is based on some other, please indicate that.
That will help me as reviewer, and future maintainers.

E.g., I put this comment in the wc test of --files0-from:

  # This file bears a striking resemblance to tests/du/files0-from.