Re: df --output

[Pádraig Brady]

On 09/21/2012 03:51 AM, Bernhard Voelker wrote:
> Finally, I made it the whole long way to df's new --output option.
> Example use:
>
>    $ src/df --o=target,ipcent,pcent .
>    Mounted on  IUse% Use%
>    /media/sdb5    5%  75%
>
> The discussion about it started with http://bugs.gnu.org/10915.
>
> The biggest changes are in PATCH 03, 11, 17 and of course
> the new files for documentation [19] and tests [20].
>
> [PATCH 01/20] df: move the call of get_header from get_dev to main
> [PATCH 02/20] df: rename some displayable fields
> [PATCH 03/20] df: rework internal processing of output columns
> [PATCH 04/20] df: apply ambsalign to the last field with
> [PATCH 05/20] df: fix bracket opening style for structs
> [PATCH 06/20] df: use enum value for long option --total
> [PATCH 07/20] df: fix typo in comment
> [PATCH 08/20] df: remove now-obsolescent condition
> [PATCH 09/20] df: only sum up grand total if required
> [PATCH 10/20] df: print '-' into the target column of the total line
> [PATCH 11/20] df: Add basic support for mixed block and inode fields
> [PATCH 12/20] df: remove remainder of pre-mixed fields support
> [PATCH 13/20] df: minor cleanup to improve readability
> [PATCH 14/20] df: cleanup header_mode handling regarding --inodes
> [PATCH 15/20] df: give posix_format variable a better scope
> [PATCH 16/20] df: remove old comment
> [PATCH 17/20] df: add new --output option
> [PATCH 18/20] df: plug two minor memory holes detected by valgrind
> [PATCH 19/20] df: document the new --output option
> [PATCH 20/20] df: add a test for the --output option
>
> For once, I have attached the patch set in a BZ2 file.
>
> Thanks to Padraig and Jim who have given me some valuable guidance.
>
> Have a nice day,
> Berny

This is really well done.
Being syntax-checked and valgrinded.
Thanks!

I'd adjust the following slightly:

diff --git a/NEWS b/NEWS
+  df --total now prints '-' into the target column (mount point) of the
+  summary line, accommodating to the upcoming --output option where the
+  target field can be on another than the last column.

to:

+  df --total now prints '-' into the target column (mount point) of the
+  summary line, accommodating to the --output option where the target
+  field can be in any column.

In patch 10 you print '-' into the target column of the total line.
I wonder could you use _("total") or "-" depending on whether
the source field is present or not?

I noticed in patch 11 that you used "char*" rather than "char *".
I added the original inconsistency, but you might as well fix it
up while moving the code.

I'd slightly prefer the comment removed in patch 16 to stay.
It was describing history rather than being stale.

The docs in patch 19 state:

+The @var{field_list} items can be abbreviated - as long as they are
+unambiguous among the others. The definition of the @var{field_list} can
+even be splitted among several @option{--output} uses.

I'd be inclined to not support abbreviations.
It could lead to needless divergences in scripts and
possible issues later if ever adding new fields.
The values are very short in any case.

The split out patches makes it very easy to review.
I'm thinking we might squash before commit though
to summarise the history in git and the auto generated release message?

thanks again!
Pádraig.