Re: df --output

[Bernhard Voelker]

On 09/21/2012 11:03 AM, Pádraig Brady wrote:
> On 09/21/2012 03:51 AM, Bernhard Voelker wrote:

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

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?

Good point.
Actually I already had thought about whether to put the "-" into
main or get_dev - with this aspect it seems that the other choice
would have been better.

> 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.

ok.

> 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.

Good point. I borrowed the parsing function from cp.c, therefore
the abbreviation feature came in for free ;-)

> 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?

Squashing is unfair when it comes to 'shortlog' statistics.
No, just kidding ;-) It's your choice.

Thanks for the first review.

Have a nice day,
Berny