bug#6555: stat enhancement

[A Burgie]

On Wed, Jul 7, 2010 at 00:19, Jim Meyering <address@hidden> wrote:
> A Burgie wrote:
> Start by reading README* and HACKING.
> The GNU Coding Standards has plenty of useful background info.
> Run "info standards" or see http://www.gnu.org/prep/standards/.
>
> ...
>>  ** New features
>>
>> +  stat now shows the mountpoint of a specified file or directory
>> +  in its default output.  It also will show this when a format is
>> +  explicitly specified through the use of the %m specifier.
>
> As discussed, I'd rather not change the default output.
>
>  stat can now print the mount point of a file via its new %m format directive

Sorry... old version of NEWS.  It wasn't really there at the time and
will not be in the future.

>> -  du now uses less than half as much memory when operating on trees
>> -  with many hard-linked files.  With --count-links (-l), or when
>> -  operating on trees with no hard-linked files, there is no change.
>
> Oops.  Your patch would revert the two recent additions to NEWS,
> above and below.

rebasing is my friend.... rebasing is my friend.  It'll be right by
shipping time.

>> -** Bug fixes
>> -
>> -  du no longer multiply counts a file that is a directory or whose
>> -  link count is 1, even if the file is reached multiple times by
>> -  following symlinks or via multiple arguments.
>>
>>  * Noteworthy changes in release 8.5 (2010-04-23) [stable]
>>
>> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
>> index 21cf36d..ea3f142 100644
>> --- a/doc/coreutils.texi
>> +++ b/doc/coreutils.texi
>> @@ -10666,6 +10666,7 @@ The valid @var{format} directives for files with 
>> @option{--format} and
>> address@hidden %G - Group name of owner
>> address@hidden %h - Number of hard links
>> address@hidden %i - Inode number
>> address@hidden %m - Mount point
>> address@hidden %n - File name
>> address@hidden %N - Quoted file name with dereference if symbolic link
>> address@hidden %o - I/O block size
>> diff --git a/gnulib b/gnulib
>> --- a/gnulib
>> +++ b/gnulib
>> @@ -1 +1 @@
>> -Subproject commit 7773f84fe1aa3bb17defad704ee87f2615894ae4
>> +Subproject commit 7773f84fe1aa3bb17defad704ee87f2615894ae4-dirty
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 0630a06..f090087 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -145,6 +145,7 @@ noinst_HEADERS =  \
>>    copy.h             \
>>    cp-hash.h          \
>>    dircolors.h                \
>> +  findmountpoint.h   \
>>    fs.h                       \
>>    group-list.h               \
>>    ls.h                       \
>> diff --git a/src/stat.c b/src/stat.c
>> index c3730f0..f283437 100644
>> --- a/src/stat.c
>> +++ b/src/stat.c
>> @@ -68,6 +68,9 @@
>>  #include "quotearg.h"
>>  #include "stat-time.h"
>>  #include "strftime.h"
>> +#include "findmountpoint.h"
>
> nameslikethis are hard to read.
> I prefer find-mount-point.h.

Done thoughout.

>> +#include "save-cwd.h"
>> +#include "xgetcwd.h"
>>
>>  #if USE_STATVFS
>>  # define STRUCT_STATVFS struct statvfs
>> @@ -612,6 +615,7 @@ print_stat (char *pformat, size_t prefix_len, char m,
>>    struct stat *statbuf = (struct stat *) data;
>>    struct passwd *pw_ent;
>>    struct group *gw_ent;
>> +  char * mp;
>
> Remove the space-after-"*".

Done.

>>    bool fail = false;
>>
>>    switch (m)
>> @@ -679,6 +683,14 @@ print_stat (char *pformat, size_t prefix_len, char m,
>>      case 't':
>>        out_uint_x (pformat, prefix_len, major (statbuf->st_rdev));
>>        break;
>> +    case 'm':
>> +      mp = find_mount_point (filename, statbuf);
>> +      if (mp) {
>> +        out_string (pformat, prefix_len, mp);
>> +      } else {
>> +        fail = true;
>> +      }
>
> Your brace-using style is inconsistent with the rest of the code.
> Drop them in this case, since those are one-line "if" and "else" bodies.

Wondered about that.  I see in HACKING it talks about this.  Fixed.

>> +      break;
>>      case 'T':
>>        out_uint_x (pformat, prefix_len, minor (statbuf->st_rdev));
>>        break;
>> @@ -1025,6 +1037,7 @@ The valid format sequences for files (without 
>> --file-system):\n\
>>        fputs (_("\
>>    %h   Number of hard links\n\
>>    %i   Inode number\n\
>> +  %m   Mount point\n\
>>    %n   File name\n\
>>    %N   Quoted file name with dereference if symbolic link\n\
>>    %o   I/O block size\n\
>>
>> commit 70d1f1c97f322a164ee872c0399d9fbccc862b18
>> Author: Aaron Burgemeister <address@hidden>
>> Date:   Tue Jul 6 18:01:53 2010 -0600
>>
>>     broken-20100707000000Z
>>
>> diff --git a/src/findmountpoint.c b/src/findmountpoint.c
>> new file mode 100644
>> index 0000000..665e2fc
>> --- /dev/null
>> +++ b/src/findmountpoint.c
>> @@ -0,0 +1,93 @@
>
> Every .c file must first include <config.h>.

Ah ha.  I added a bit more to actually get things where they are now
but that is good to know.

>> +#include "save-cwd.h"
>> +#include "xgetcwd.h"
>> +
>> +
>> +/* Return the root mountpoint of the file system on which FILE exists, in
>> + * malloced storage.  FILE_STAT should be the result of stating FILE.
>> + * Give a diagnostic and return NULL if unable to determine the mount point.
>> + * Exit if unable to restore current working directory.  */
>
> We don't use this style of comment.
> Remove the "*" on continued lines.

Sorry... I copied from an example I found, I think in shred.c, though
the second example is wrong while the first is correct.  Noted for the
future and fixed.

>> +static char *
>> +find_mount_point (const char *file, const struct stat *file_stat)
>> +{
>> +  struct saved_cwd cwd;
>> +  struct stat last_stat;
>> +  char *mp = NULL;    /* The malloced mount point.  */
>> +
>> +  if (save_cwd (&cwd) != 0)
>> +  {
>
> You have reindented this function (changing
> the brace positioning style to be contrary to the rest of coreutils).

Finally figured out about the ':set [no]paste' stuff in vi so I can
get around this.  Hopefully fixed once and for all.

>
> The #ifndef...#endif is supposed to span the contents of the file.

Fixed, and suddenly the point of compile-time conditionals comes back to me.

>> +
>> +/* Return the root mountpoint of the file system on which FILE exists, in
>> + * malloced storage.  FILE_STAT should be the result of stating FILE.
>> + * Give a diagnostic and return NULL if unable to determine the mount point.
>> + * Exit if unable to restore current working directory.  */
>
> Please remove this comment.
> It duplicates the one in the .c file.

Done.

>> +static char * find_mount_point (const char *, const struct stat *);

I think I can figure out a changelog.  The one thing left, though, is
a bit less cosmetic.  Something about my makefile (which has the two
new _SOURCES sections you suggested) is still not letting make compile
everything properly.

stat.o: In function `print_stat':
/home/aburgemeister/code/coreutils/src/stat.c:687: undefined reference
to `find_mount_point'
collect2: ld returned 1 exit status
make[3]: *** [df] Error 1
make[3]: Leaving directory `/home/aburgemeister/code/coreutils/src'
make[2]: *** [all] Error 2
make[2]: Leaving directory `/home/aburgemeister/code/coreutils/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/aburgemeister/code/coreutils'
make: *** [all] Error 2

DIFF0
Description: Binary data