Package: coreutils;
Reported by: A Burgie <dajoker <at> gmail.com>
Date: Fri, 2 Jul 2010 20:55:01 UTC
Severity: normal
Done: Pádraig Brady <P <at> draigBrady.com>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: A Burgie <dajoker <at> gmail.com> To: Jim Meyering <jim <at> meyering.net> Cc: 6555 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com> Subject: bug#6555: stat enhancement Date: Thu, 15 Jul 2010 20:08:28 -0600
[Message part 1 (text/plain, inline)]
Attached is the finished DIFF which I believe meets all of the
requirements. For notes the problem with compilation had to do with
the definition of the function in the find-mount-point.h and
find-mount-point.c files having the 'static' as part of the
definition. Removing that allowed compilation to continue.
Apparently in C something defined as static cannot be used outside
that C file, even if included.
Let me know if anything else must be done. I started the licensing
process but have not heard back at all since I did so.
e0abccac5fbbe1af452cc04f7980f8e7 DIFF0
140caa8e1f0e04c8daa42ced5caa0962537ae2a8 DIFF0
Thanks,
Aaron
On Wed, Jul 7, 2010 at 19:55, A Burgie <dajoker <at> gmail.com> wrote:
> On Wed, Jul 7, 2010 at 00:19, Jim Meyering <jim <at> meyering.net> 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
>>> @item %G - Group name of owner
>>> @item %h - Number of hard links
>>> @item %i - Inode number
>>> +@item %m - Mount point
>>> @item %n - File name
>>> @item %N - Quoted file name with dereference if symbolic link
>>> @item %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 <dajoker <at> gmail.com>
>>> 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 (application/octet-stream, attachment)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.