GNU bug report logs - #6555
stat enhancement

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 6555 in the body.
You can then email your comments to 6555 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: A Burgie <dajoker <at> gmail.com>
To: bug-coreutils <at> gnu.org
Cc: meyering <at> redhat.com
Subject: stat enhancement
Date: Fri, 2 Jul 2010 14:49:10 -0600

[Message part 1 (text/plain, inline)]

To Whom it May Concern:

I have made a small change to the stat command adding in the ability
to print the mountpoint of the specified file/directory.  This is
something I have heard several others (besides myself) trying to do
via various methods involving the df command or some recursive
directory traversals but which would make sense in the stat command.
Michael Meskes suggested CC-ing meyering <at> redhat.com as well which I am
doing.

Reading through the submission process for the package I am not sure
if the actual e-mail body should be the output of the 'git
format-patch --stdout -1' command or not so I am attaching it.  If
preferred I will send it inline next time and in the future.

Thank-you for your time,
Aaron Burgemeister

[DIFF (application/octet-stream, attachment)]

Message #8 received at 6555 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: A Burgie <dajoker <at> gmail.com>
Cc: meyering <at> redhat.com, 6555 <at> debbugs.gnu.org
Subject: Re: bug#6555: stat enhancement
Date: Mon, 05 Jul 2010 13:03:43 +0100

On 02/07/10 21:49, A Burgie wrote:
> To Whom it May Concern:
> 
> I have made a small change to the stat command adding in the ability
> to print the mountpoint of the specified file/directory.  This is
> something I have heard several others (besides myself) trying to do
> via various methods involving the df command or some recursive
> directory traversals but which would make sense in the stat command.
> Michael Meskes suggested CC-ing meyering <at> redhat.com as well which I am
> doing.
> 
> Reading through the submission process for the package I am not sure
> if the actual e-mail body should be the output of the 'git
> format-patch --stdout -1' command or not so I am attaching it.  If
> preferred I will send it inline next time and in the future.

The idea seems sensible.
Using df on a file _is_ a little unintuitive,
and also parsing the output from df is awkward.

You copied (while re-indenting with non GNU options)
the find_mount_point() function from df.c to stat.c
We'll have to refactor that out for both to use
(if indeed it's the best method to use which I've not
looked into at this stage).

cheers,
Pádraig.

Message #11 received at 6555 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: A Burgie <dajoker <at> gmail.com>
Cc: 6555 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigBrady.com>
Subject: Re: bug#6555: stat enhancement
Date: Mon, 05 Jul 2010 14:48:43 +0200

Pádraig Brady wrote:
> On 02/07/10 21:49, A Burgie wrote:
>> To Whom it May Concern:
>>
>> I have made a small change to the stat command adding in the ability
>> to print the mountpoint of the specified file/directory.  This is
>> something I have heard several others (besides myself) trying to do
>> via various methods involving the df command or some recursive
>> directory traversals but which would make sense in the stat command.
>> Michael Meskes suggested CC-ing meyering <at> redhat.com as well which I am
>> doing.
>>
>> Reading through the submission process for the package I am not sure
>> if the actual e-mail body should be the output of the 'git
>> format-patch --stdout -1' command or not so I am attaching it.  If
>> preferred I will send it inline next time and in the future.

Inline is slightly preferred.
No big deal.

> The idea seems sensible.

I think so, too.
However, the patch that adds the option would do well to add
a test that exercises it and to mention it in the NEWS file.
Your change will qualify as "significant".
Can you file a copyright assignment?  Here are guidelines:

    http://git.savannah.gnu.org/cgit/coreutils.git/tree/HACKING#n327

> Using df on a file _is_ a little unintuitive,
> and also parsing the output from df is awkward.
>
> You copied (while re-indenting with non GNU options)
> the find_mount_point() function from df.c to stat.c
> We'll have to refactor that out for both to use
> (if indeed it's the best method to use which I've not
> looked into at this stage).

Yes, it has to be factored out.
I haven't looked either, yet ;-)

Thanks for contributing!

Message #14 received at 6555 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#6555: stat enhancement
Date: Mon, 5 Jul 2010 07:33:03 -0600

On Mon, Jul 5, 2010 at 06:48, Jim Meyering <jim <at> meyering.net> wrote:
>> The idea seems sensible.
>
> I think so, too.
> However, the patch that adds the option would do well to add
> a test that exercises it and to mention it in the NEWS file.
> Your change will qualify as "significant".
> Can you file a copyright assignment?  Here are guidelines:
>
>    http://git.savannah.gnu.org/cgit/coreutils.git/tree/HACKING#n327

I was wondering if that would apply, but I did not think it would as
it was not really my own code (I copied from previously-GPL'd code and
only, perhaps, added three or four lines of my own).  Please confirm
if I am mistaken on this.

>> Using df on a file _is_ a little unintuitive,
>> and also parsing the output from df is awkward.
>>
>> You copied (while re-indenting with non GNU options)
>> the find_mount_point() function from df.c to stat.c
>> We'll have to refactor that out for both to use
>> (if indeed it's the best method to use which I've not
>> looked into at this stage).
>
> Yes, it has to be factored out.
> I haven't looked either, yet ;-)

I should have noticed the spacing issue.  I'll send a new DIFF file
here shortly with the fix and also with the NEWS stuff if I can figure
that out.  Test cases I know almost nothing about, but I'll check
there too and hopefully have a worthwhile addition.

Thank-you for the feedback,
AB

Message #17 received at 6555 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: A Burgie <dajoker <at> gmail.com>
Cc: 6555 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#6555: stat enhancement
Date: Mon, 05 Jul 2010 15:50:13 +0200

A Burgie wrote:
> On Mon, Jul 5, 2010 at 06:48, Jim Meyering <jim <at> meyering.net> wrote:
>>> The idea seems sensible.
>>
>> I think so, too.
>> However, the patch that adds the option would do well to add
>> a test that exercises it and to mention it in the NEWS file.
>> Your change will qualify as "significant".
>> Can you file a copyright assignment?  Here are guidelines:
>>
>>    http://git.savannah.gnu.org/cgit/coreutils.git/tree/HACKING#n327
>
> I was wondering if that would apply, but I did not think it would as
> it was not really my own code (I copied from previously-GPL'd code and
> only, perhaps, added three or four lines of my own).  Please confirm
> if I am mistaken on this.

I figured that between moving that function into a file on its
own, declaring the function in its own header file,
adjusting df.c and stat.c to include the new .h file,
adjusting src/Makefile.am to list the new file, adding a test and
adding to NEWS you would end up adding more than 10 or 15 lines.

Oh.  And documentation.  You'll want to add a line or two to
the section on stat in doc/coreutils.texi.

If it's too much work on the portability front (wouldn't surprise me),
it may be ok simply to put the statically-declared function body
in a new .c file and include the .c file from each of stat.c and df.c.

BTW, please adjust this part of your patch not to dereference NULL
when find_mount_point fails:

  +    case 'm':
  +      out_string (pformat, prefix_len, find_mount_point (filename, statbuf));

Also, I'm a little reluctant to change the default format to
add an entire new line just for "Mountpoint: %s\n".
There is value in not changing the number of lines in the default output:
compatibility.

Message #20 received at 6555 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#6555: stat enhancement
Date: Mon, 5 Jul 2010 08:01:00 -0600

On Mon, Jul 5, 2010 at 07:50, Jim Meyering <jim <at> meyering.net> wrote:
> A Burgie wrote:
>> On Mon, Jul 5, 2010 at 06:48, Jim Meyering <jim <at> meyering.net> wrote:
>>>> The idea seems sensible.
>>>
>>> I think so, too.
>>> However, the patch that adds the option would do well to add
>>> a test that exercises it and to mention it in the NEWS file.
>>> Your change will qualify as "significant".
>>> Can you file a copyright assignment?  Here are guidelines:
>>>
>>>    http://git.savannah.gnu.org/cgit/coreutils.git/tree/HACKING#n327
>>
>> I was wondering if that would apply, but I did not think it would as
>> it was not really my own code (I copied from previously-GPL'd code and
>> only, perhaps, added three or four lines of my own).  Please confirm
>> if I am mistaken on this.
>
> I figured that between moving that function into a file on its
> own, declaring the function in its own header file,
> adjusting df.c and stat.c to include the new .h file,
> adjusting src/Makefile.am to list the new file, adding a test and
> adding to NEWS you would end up adding more than 10 or 15 lines.

Ah.... I see.  Very well; I'll get to work on the legal side of things.

> Oh.  And documentation.  You'll want to add a line or two to
> the section on stat in doc/coreutils.texi.

Consider it done.

> If it's too much work on the portability front (wouldn't surprise me),
> it may be ok simply to put the statically-declared function body
> in a new .c file and include the .c file from each of stat.c and df.c.

I'll work to do it the right way and let you know if I fail there.

> BTW, please adjust this part of your patch not to dereference NULL
> when find_mount_point fails:
>
>  +    case 'm':
>  +      out_string (pformat, prefix_len, find_mount_point (filename, statbuf));

Any pointers (pun intended) on the best way to do that?  I'll check
the df.c file to see what it does but if it's not there and you have a
better option that'd be appreciated.  I'm not sure it's been pointed
out yet, but I'm not a C expert yet.

> Also, I'm a little reluctant to change the default format to
> add an entire new line just for "Mountpoint: %s\n".
> There is value in not changing the number of lines in the default output:
> compatibility.

Agreed.  My reasoning in doing so was perhaps a bit shortsighted.
Most of the stat output seems to fit nicely within eighty columns and
I did not want to disturb that too much.  Some lines could easily go
outside this (File, Permissions with user/group names, etc.) but File
was on the first line and I was concerned about current users who may
be running stat and then piping output through 'head' or 'tail' to get
just a portion of it.  I could easily enough add Mountpoint after the
filename or anywhere else you feel is better, or it could be removed
from the default output entirely which was what I did originally
before submitting the patch.

Thank-you for your patience,
AB

Message #23 received at 6555 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: A Burgie <dajoker <at> gmail.com>
Cc: 6555 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#6555: stat enhancement
Date: Mon, 05 Jul 2010 17:26:43 +0200

A Burgie wrote:
> On Mon, Jul 5, 2010 at 07:50, Jim Meyering <jim <at> meyering.net> wrote:
>> A Burgie wrote:
>>> On Mon, Jul 5, 2010 at 06:48, Jim Meyering <jim <at> meyering.net> wrote:
>>>>> The idea seems sensible.
>>>>
>>>> I think so, too.
>>>> However, the patch that adds the option would do well to add
>>>> a test that exercises it and to mention it in the NEWS file.
>>>> Your change will qualify as "significant".
>>>> Can you file a copyright assignment?  Here are guidelines:
>>>>
>>>>    http://git.savannah.gnu.org/cgit/coreutils.git/tree/HACKING#n327
>>>
>>> I was wondering if that would apply, but I did not think it would as
>>> it was not really my own code (I copied from previously-GPL'd code and
>>> only, perhaps, added three or four lines of my own).  Please confirm
>>> if I am mistaken on this.
>>
>> I figured that between moving that function into a file on its
>> own, declaring the function in its own header file,
>> adjusting df.c and stat.c to include the new .h file,
>> adjusting src/Makefile.am to list the new file, adding a test and
>> adding to NEWS you would end up adding more than 10 or 15 lines.
>
> Ah.... I see.  Very well; I'll get to work on the legal side of things.
>
>> Oh.  And documentation.  You'll want to add a line or two to
>> the section on stat in doc/coreutils.texi.
>
> Consider it done.
>
>> If it's too much work on the portability front (wouldn't surprise me),
>> it may be ok simply to put the statically-declared function body
>> in a new .c file and include the .c file from each of stat.c and df.c.
>
> I'll work to do it the right way and let you know if I fail there.
>
>> BTW, please adjust this part of your patch not to dereference NULL
>> when find_mount_point fails:
>>
>>  +    case 'm':
>>  +      out_string (pformat, prefix_len, find_mount_point (filename, statbuf));
>
> Any pointers (pun intended) on the best way to do that?  I'll check
> the df.c file to see what it does but if it's not there and you have a
> better option that'd be appreciated.  I'm not sure it's been pointed
> out yet, but I'm not a C expert yet.

Hmm...  Handling failure is actually looking complicated.
If someone used the %m format and find_mount_point fails,
then stat should exit nonzero, but the use of the new find_mount_point
would be down in print_stat, which is a void function, and which is passed
as a callback to print_it, which is also a void function.
Doing this cleanly would mean modifying signatures of at least
print_it, print_stat and print_statfs to return a success/fail
indicator to do_statfs.

So I've just adjusted those interfaces and added some comments.
Patch below.  That should make it easier for you.

Note the out_file_context function.
It currently prints "?" (and a diagnostic to stderr) when
it fails to determine a context string.  Such a failure
did not make stat itself exit nonzero, but with this patch,
now it does.

Your change should be sure to do the same.

>> Also, I'm a little reluctant to change the default format to
>> add an entire new line just for "Mountpoint: %s\n".
>> There is value in not changing the number of lines in the default output:
>> compatibility.
>
> Agreed.  My reasoning in doing so was perhaps a bit shortsighted.
> Most of the stat output seems to fit nicely within eighty columns and
> I did not want to disturb that too much.  Some lines could easily go
> outside this (File, Permissions with user/group names, etc.) but File
> was on the first line and I was concerned about current users who may
> be running stat and then piping output through 'head' or 'tail' to get
> just a portion of it.  I could easily enough add Mountpoint after the
> filename or anywhere else you feel is better, or it could be removed
> from the default output entirely which was what I did originally
> before submitting the patch.

Unless someone clamors for the added information,
let's just omit it from the default format strings.

I'll push these two change-sets shortly.

From d6d47eedb938743a56431988e73a5a3205687ba9 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> redhat.com>
Date: Mon, 5 Jul 2010 17:16:23 +0200
Subject: [PATCH 1/2] system.h: define ATTRIBUTE_WARN_UNUSED_RESULT

* src/system.h (ATTRIBUTE_WARN_UNUSED_RESULT): Define.
---
 src/system.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/src/system.h b/src/system.h
index 859b663..d3fd901 100644
--- a/src/system.h
+++ b/src/system.h
@@ -483,6 +483,14 @@ enum
 # define ATTRIBUTE_UNUSED __attribute__ ((__unused__))
 #endif

+/* The warn_unused_result attribute appeared first in gcc-3.4.0 */
+#undef ATTRIBUTE_WARN_UNUSED_RESULT
+#if __GNUC__ < 3 || (__GNUC__ == 3 && __GNUC_MINOR__ < 4)
+# define ATTRIBUTE_WARN_UNUSED_RESULT __attribute__ ((__warn_unused_result__))
+#else
+# define ATTRIBUTE_WARN_UNUSED_RESULT /* empty */
+#endif
+
 #if defined strdupa
 # define ASSIGN_STRDUPA(DEST, S)		\
   do { DEST = strdupa (S); } while (0)
--
1.7.2.rc1.192.g262ff


From e78f266f2ec5489824a28ef710c82bd6ca5ccf01 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> redhat.com>
Date: Mon, 5 Jul 2010 17:18:29 +0200
Subject: [PATCH 2/2] stat: getfilecon failure now evokes nonzero exit status

Add comments and adjust interfaces to allow low-level failure
to propagate out to callers.
* src/stat.c (out_file_context): Return bool, not void,
so we can tell callers about failure.
(print_statfs, print_stat, print_it): Propagate failure to caller.
(do_statfs): Propagate print_it failure to caller.
(do_stat): Likewise.
I nearly forgot to update do_stat to propagate print_it failure,
and it compiled just fine in spite of that.  To prevent possibility
of a repeat, I've marked each function that returns non-void with
ATTRIBUTE_WARN_UNUSED_RESULT.
---
 src/stat.c |   60 ++++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/src/stat.c b/src/stat.c
index f1b5ef1..c3730f0 100644
--- a/src/stat.c
+++ b/src/stat.c
@@ -88,7 +88,7 @@
 /* BeOS has a statvfs function, but it does not return sensible values
    for f_files, f_ffree and f_favail, and lacks f_type, f_basetype and
    f_fstypename.  Use 'struct fs_info' instead.  */
-static int
+static int ATTRIBUTE_WARN_UNUSED_RESULT
 statfs (char const *filename, struct fs_info *buf)
 {
   dev_t device = dev_for_path (filename);
@@ -185,7 +185,7 @@ static char const *trailing_delim = "";
    Others have statfs.f_fstypename[MFSNAMELEN] (NetBSD 1.5.2).
    Still others have neither and have to get by with f_type (GNU/Linux).
    But f_type may only exist in statfs (Cygwin).  */
-static char const *
+static char const * ATTRIBUTE_WARN_UNUSED_RESULT
 human_fstype (STRUCT_STATVFS const *statfsbuf)
 {
 #ifdef STATXFS_FILE_SYSTEM_TYPE_MEMBER_NAME
@@ -436,7 +436,7 @@ human_fstype (STRUCT_STATVFS const *statfsbuf)
 #endif
 }

-static char *
+static char * ATTRIBUTE_WARN_UNUSED_RESULT
 human_access (struct stat const *statbuf)
 {
   static char modebuf[12];
@@ -445,7 +445,7 @@ human_access (struct stat const *statbuf)
   return modebuf;
 }

-static char *
+static char * ATTRIBUTE_WARN_UNUSED_RESULT
 human_time (struct timespec t)
 {
   static char str[MAX (INT_BUFSIZE_BOUND (intmax_t),
@@ -491,11 +491,14 @@ out_uint_x (char *pformat, size_t prefix_len, uintmax_t arg)
 }

 /* Very specialized function (modifies FORMAT), just so as to avoid
-   duplicating this code between both print_statfs and print_stat.  */
-static void
+   duplicating this code between both print_statfs and print_stat.
+   Return zero upon success, nonzero upon failure.  */
+static bool ATTRIBUTE_WARN_UNUSED_RESULT
 out_file_context (char const *filename, char *pformat, size_t prefix_len)
 {
   char *scontext;
+  bool fail = false;
+
   if ((follow_links
        ? getfilecon (filename, &scontext)
        : lgetfilecon (filename, &scontext)) < 0)
@@ -503,19 +506,22 @@ out_file_context (char const *filename, char *pformat, size_t prefix_len)
       error (0, errno, _("failed to get security context of %s"),
              quote (filename));
       scontext = NULL;
+      fail = true;
     }
   strcpy (pformat + prefix_len, "s");
   printf (pformat, (scontext ? scontext : "?"));
   if (scontext)
     freecon (scontext);
+  return fail;
 }

-/* print statfs info */
-static void
+/* Print statfs info.  Return zero upon success, nonzero upon failure.  */
+static bool ATTRIBUTE_WARN_UNUSED_RESULT
 print_statfs (char *pformat, size_t prefix_len, char m, char const *filename,
               void const *data)
 {
   STRUCT_STATVFS const *statfsbuf = data;
+  bool fail = false;

   switch (m)
     {
@@ -589,22 +595,24 @@ print_statfs (char *pformat, size_t prefix_len, char m, char const *filename,
       out_int (pformat, prefix_len, statfsbuf->f_ffree);
       break;
     case 'C':
-      out_file_context (filename, pformat, prefix_len);
+      fail |= out_file_context (filename, pformat, prefix_len);
       break;
     default:
       fputc ('?', stdout);
       break;
     }
+  return fail;
 }

-/* print stat info */
-static void
+/* Print stat info.  Return zero upon success, nonzero upon failure.  */
+static bool
 print_stat (char *pformat, size_t prefix_len, char m,
             char const *filename, void const *data)
 {
   struct stat *statbuf = (struct stat *) data;
   struct passwd *pw_ent;
   struct group *gw_ent;
+  bool fail = false;

   switch (m)
     {
@@ -620,7 +628,7 @@ print_stat (char *pformat, size_t prefix_len, char m,
             {
               error (0, errno, _("cannot read symbolic link %s"),
                      quote (filename));
-              return;
+              return true;
             }
           printf (" -> ");
           out_string (pformat, prefix_len, quote (linkname));
@@ -714,12 +722,13 @@ print_stat (char *pformat, size_t prefix_len, char m,
         out_uint (pformat, prefix_len, statbuf->st_ctime);
       break;
     case 'C':
-      out_file_context (filename, pformat, prefix_len);
+      fail |= out_file_context (filename, pformat, prefix_len);
       break;
     default:
       fputc ('?', stdout);
       break;
     }
+  return fail;
 }

 /* Output a single-character \ escape.  */
@@ -763,11 +772,16 @@ print_esc_char (char c)
   putchar (c);
 }

-static void
+/* Print the information specified by the format string, FORMAT,
+   calling PRINT_FUNC for each %-directive encountered.
+   Return zero upon success, nonzero upon failure.  */
+static bool ATTRIBUTE_WARN_UNUSED_RESULT
 print_it (char const *format, char const *filename,
-          void (*print_func) (char *, size_t, char, char const *, void const *),
+          bool (*print_func) (char *, size_t, char, char const *, void const *),
           void const *data)
 {
+  bool fail = false;
+
   /* Add 2 to accommodate our conversion of the stat `%s' format string
      to the longer printf `%llu' one.  */
   enum
@@ -807,7 +821,7 @@ print_it (char const *format, char const *filename,
                 putchar ('%');
                 break;
               default:
-                print_func (dest, len + 1, *fmt_char, filename, data);
+                fail |= print_func (dest, len + 1, *fmt_char, filename, data);
                 break;
               }
             break;
@@ -866,10 +880,12 @@ print_it (char const *format, char const *filename,
   free (dest);

   fputs (trailing_delim, stdout);
+
+  return fail;
 }

 /* Stat the file system and print what we find.  */
-static bool
+static bool ATTRIBUTE_WARN_UNUSED_RESULT
 do_statfs (char const *filename, bool terse, char const *format)
 {
   STRUCT_STATVFS statfsbuf;
@@ -899,12 +915,12 @@ do_statfs (char const *filename, bool terse, char const *format)
                 "Inodes: Total: %-10c Free: %d\n");
     }

-  print_it (format, filename, print_statfs, &statfsbuf);
-  return true;
+  bool fail = print_it (format, filename, print_statfs, &statfsbuf);
+  return ! fail;
 }

 /* stat the file and print what we find */
-static bool
+static bool ATTRIBUTE_WARN_UNUSED_RESULT
 do_stat (char const *filename, bool terse, char const *format)
 {
   struct stat statbuf;
@@ -959,8 +975,8 @@ do_stat (char const *filename, bool terse, char const *format)
             }
         }
     }
-  print_it (format, filename, print_stat, &statbuf);
-  return true;
+  bool fail = print_it (format, filename, print_stat, &statbuf);
+  return ! fail;
 }

 void
--
1.7.2.rc1.192.g262ff

Message #26 received at 6555 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: A Burgie <dajoker <at> gmail.com>
Cc: 6555 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#6555: stat enhancement
Date: Mon, 05 Jul 2010 17:41:43 +0200

Jim Meyering wrote:
> I'll push these two change-sets shortly.
>
> Subject: [PATCH 1/2] system.h: define ATTRIBUTE_WARN_UNUSED_RESULT
...
> +/* The warn_unused_result attribute appeared first in gcc-3.4.0 */
> +#undef ATTRIBUTE_WARN_UNUSED_RESULT
> +#if __GNUC__ < 3 || (__GNUC__ == 3 && __GNUC_MINOR__ < 4)
> +# define ATTRIBUTE_WARN_UNUSED_RESULT __attribute__ ((__warn_unused_result__))
> +#else
> +# define ATTRIBUTE_WARN_UNUSED_RESULT /* empty */
> +#endif

Just noticed I reversed the if/else branches above.
This works a lot better:

commit 61aae73f5427c987b20604fbec5772e02edc0f74
Author: Jim Meyering <meyering <at> redhat.com>
Date:   Mon Jul 5 17:16:23 2010 +0200

    system.h: define ATTRIBUTE_WARN_UNUSED_RESULT

    * src/system.h (ATTRIBUTE_WARN_UNUSED_RESULT): Define.

diff --git a/src/system.h b/src/system.h
index 859b663..9e14681 100644
--- a/src/system.h
+++ b/src/system.h
@@ -483,6 +483,14 @@ enum
 # define ATTRIBUTE_UNUSED __attribute__ ((__unused__))
 #endif

+/* The warn_unused_result attribute appeared first in gcc-3.4.0 */
+#undef ATTRIBUTE_WARN_UNUSED_RESULT
+#if __GNUC__ < 3 || (__GNUC__ == 3 && __GNUC_MINOR__ < 4)
+# define ATTRIBUTE_WARN_UNUSED_RESULT /* empty */
+#else
+# define ATTRIBUTE_WARN_UNUSED_RESULT __attribute__ ((__warn_unused_result__))
+#endif
+
 #if defined strdupa
 # define ASSIGN_STRDUPA(DEST, S)		\
   do { DEST = strdupa (S); } while (0)

Message #29 received at 6555 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#6555: stat enhancement
Date: Mon, 5 Jul 2010 09:52:08 -0600

On Mon, Jul 5, 2010 at 09:41, Jim Meyering <jim <at> meyering.net> wrote:
> Jim Meyering wrote:
>> I'll push these two change-sets shortly.
>>
>> Subject: [PATCH 1/2] system.h: define ATTRIBUTE_WARN_UNUSED_RESULT
> ...
>> +/* The warn_unused_result attribute appeared first in gcc-3.4.0 */
>> +#undef ATTRIBUTE_WARN_UNUSED_RESULT
>> +#if __GNUC__ < 3 || (__GNUC__ == 3 && __GNUC_MINOR__ < 4)
>> +# define ATTRIBUTE_WARN_UNUSED_RESULT __attribute__ ((__warn_unused_result__))
>> +#else
>> +# define ATTRIBUTE_WARN_UNUSED_RESULT /* empty */
>> +#endif
>
> Just noticed I reversed the if/else branches above.
> This works a lot better:
>
> commit 61aae73f5427c987b20604fbec5772e02edc0f74
> Author: Jim Meyering <meyering <at> redhat.com>
> Date:   Mon Jul 5 17:16:23 2010 +0200
>
>    system.h: define ATTRIBUTE_WARN_UNUSED_RESULT
>
>    * src/system.h (ATTRIBUTE_WARN_UNUSED_RESULT): Define.
>
> diff --git a/src/system.h b/src/system.h
> index 859b663..9e14681 100644
> --- a/src/system.h
> +++ b/src/system.h
> @@ -483,6 +483,14 @@ enum
>  # define ATTRIBUTE_UNUSED __attribute__ ((__unused__))
>  #endif
>
> +/* The warn_unused_result attribute appeared first in gcc-3.4.0 */
> +#undef ATTRIBUTE_WARN_UNUSED_RESULT
> +#if __GNUC__ < 3 || (__GNUC__ == 3 && __GNUC_MINOR__ < 4)
> +# define ATTRIBUTE_WARN_UNUSED_RESULT /* empty */
> +#else
> +# define ATTRIBUTE_WARN_UNUSED_RESULT __attribute__ ((__warn_unused_result__))
> +#endif
> +
>  #if defined strdupa
>  # define ASSIGN_STRDUPA(DEST, S)               \
>   do { DEST = strdupa (S); } while (0)
>

Just to make sure I understand why something else was invalid, I
wrapped the print statement with an if that basically had the same
logic as df.c (mp=find_mount_point....; if(mp){print....})

That, to me, seemed valid, though it would not print anything at all
if find_mount_point returned a null.  I suppose it would be preferred
for the question-mark result which is perhaps what your version is
doing.  Anyway, just thought I'd throw that out there.

Sent in the e-mail for the legal side of things; waiting to hear back from them.

Message #32 received at 6555 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#6555: stat enhancement
Date: Mon, 5 Jul 2010 09:53:49 -0600

Nevermind.... I understand your reasoning now to allow the failure to
propagate out.

On Mon, Jul 5, 2010 at 09:52, A Burgie <dajoker <at> gmail.com> wrote:
> On Mon, Jul 5, 2010 at 09:41, Jim Meyering <jim <at> meyering.net> wrote:
>> Jim Meyering wrote:
>>> I'll push these two change-sets shortly.
>>>
>>> Subject: [PATCH 1/2] system.h: define ATTRIBUTE_WARN_UNUSED_RESULT
>> ...
>>> +/* The warn_unused_result attribute appeared first in gcc-3.4.0 */
>>> +#undef ATTRIBUTE_WARN_UNUSED_RESULT
>>> +#if __GNUC__ < 3 || (__GNUC__ == 3 && __GNUC_MINOR__ < 4)
>>> +# define ATTRIBUTE_WARN_UNUSED_RESULT __attribute__ ((__warn_unused_result__))
>>> +#else
>>> +# define ATTRIBUTE_WARN_UNUSED_RESULT /* empty */
>>> +#endif
>>
>> Just noticed I reversed the if/else branches above.
>> This works a lot better:
>>
>> commit 61aae73f5427c987b20604fbec5772e02edc0f74
>> Author: Jim Meyering <meyering <at> redhat.com>
>> Date:   Mon Jul 5 17:16:23 2010 +0200
>>
>>    system.h: define ATTRIBUTE_WARN_UNUSED_RESULT
>>
>>    * src/system.h (ATTRIBUTE_WARN_UNUSED_RESULT): Define.
>>
>> diff --git a/src/system.h b/src/system.h
>> index 859b663..9e14681 100644
>> --- a/src/system.h
>> +++ b/src/system.h
>> @@ -483,6 +483,14 @@ enum
>>  # define ATTRIBUTE_UNUSED __attribute__ ((__unused__))
>>  #endif
>>
>> +/* The warn_unused_result attribute appeared first in gcc-3.4.0 */
>> +#undef ATTRIBUTE_WARN_UNUSED_RESULT
>> +#if __GNUC__ < 3 || (__GNUC__ == 3 && __GNUC_MINOR__ < 4)
>> +# define ATTRIBUTE_WARN_UNUSED_RESULT /* empty */
>> +#else
>> +# define ATTRIBUTE_WARN_UNUSED_RESULT __attribute__ ((__warn_unused_result__))
>> +#endif
>> +
>>  #if defined strdupa
>>  # define ASSIGN_STRDUPA(DEST, S)               \
>>   do { DEST = strdupa (S); } while (0)
>>
>
> Just to make sure I understand why something else was invalid, I
> wrapped the print statement with an if that basically had the same
> logic as df.c (mp=find_mount_point....; if(mp){print....})
>
> That, to me, seemed valid, though it would not print anything at all
> if find_mount_point returned a null.  I suppose it would be preferred
> for the question-mark result which is perhaps what your version is
> doing.  Anyway, just thought I'd throw that out there.
>
> Sent in the e-mail for the legal side of things; waiting to hear back from them.
>

Message #35 received at 6555 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#6555: stat enhancement
Date: Tue, 6 Jul 2010 12:18:16 -0600

Is there any documentation on doing this the right (meaning, GNU) way?
 I've separated out find_mount_point() into a separate .c file,
created a .h file, put both in ./gnulib/lib and linked-to them from
lib, included the .h file in stat.c and df.c, but cannot compile
(w/make).  I believe I still need to modify a makefile to include the
findmountpoint.c file somewhere but have never worked w/make except as
an end user (./configure && make && make install).  Also I do not know
that I am using the correct directories (./gnulib/lib) for my new .c
and .h files.  My current error makes me think I'm missing something
else that's probably obvious:

make[4]: Entering directory `/home/aburgemeister/code/coreutils/lib'
  GEN    configmake.h
make[4]: Leaving directory `/home/aburgemeister/code/coreutils/lib'
make[3]: Leaving directory `/home/aburgemeister/code/coreutils/lib'
make[2]: Leaving directory `/home/aburgemeister/code/coreutils/lib'
Making all in src
make[2]: Entering directory `/home/aburgemeister/code/coreutils/src'
  GEN    fs.h
make  all-am
make[3]: Entering directory `/home/aburgemeister/code/coreutils/src'
  CC     stat.o
stat.c: In function 'print_stat':
stat.c:686:7: error: a label can only be part of a statement and a
declaration is not a statement
stat.c: At top level:
../lib/findmountpoint.h:28:1: warning: 'find_mount_point' used but never defined
make[3]: *** [stat.o] 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


Thanks,
AB

On Mon, Jul 5, 2010 at 09:53, A Burgie <dajoker <at> gmail.com> wrote:
> Nevermind.... I understand your reasoning now to allow the failure to
> propagate out.
>
> On Mon, Jul 5, 2010 at 09:52, A Burgie <dajoker <at> gmail.com> wrote:
>> On Mon, Jul 5, 2010 at 09:41, Jim Meyering <jim <at> meyering.net> wrote:
>>> Jim Meyering wrote:
>>>> I'll push these two change-sets shortly.
>>>>
>>>> Subject: [PATCH 1/2] system.h: define ATTRIBUTE_WARN_UNUSED_RESULT
>>> ...
>>>> +/* The warn_unused_result attribute appeared first in gcc-3.4.0 */
>>>> +#undef ATTRIBUTE_WARN_UNUSED_RESULT
>>>> +#if __GNUC__ < 3 || (__GNUC__ == 3 && __GNUC_MINOR__ < 4)
>>>> +# define ATTRIBUTE_WARN_UNUSED_RESULT __attribute__ ((__warn_unused_result__))
>>>> +#else
>>>> +# define ATTRIBUTE_WARN_UNUSED_RESULT /* empty */
>>>> +#endif
>>>
>>> Just noticed I reversed the if/else branches above.
>>> This works a lot better:
>>>
>>> commit 61aae73f5427c987b20604fbec5772e02edc0f74
>>> Author: Jim Meyering <meyering <at> redhat.com>
>>> Date:   Mon Jul 5 17:16:23 2010 +0200
>>>
>>>    system.h: define ATTRIBUTE_WARN_UNUSED_RESULT
>>>
>>>    * src/system.h (ATTRIBUTE_WARN_UNUSED_RESULT): Define.
>>>
>>> diff --git a/src/system.h b/src/system.h
>>> index 859b663..9e14681 100644
>>> --- a/src/system.h
>>> +++ b/src/system.h
>>> @@ -483,6 +483,14 @@ enum
>>>  # define ATTRIBUTE_UNUSED __attribute__ ((__unused__))
>>>  #endif
>>>
>>> +/* The warn_unused_result attribute appeared first in gcc-3.4.0 */
>>> +#undef ATTRIBUTE_WARN_UNUSED_RESULT
>>> +#if __GNUC__ < 3 || (__GNUC__ == 3 && __GNUC_MINOR__ < 4)
>>> +# define ATTRIBUTE_WARN_UNUSED_RESULT /* empty */
>>> +#else
>>> +# define ATTRIBUTE_WARN_UNUSED_RESULT __attribute__ ((__warn_unused_result__))
>>> +#endif
>>> +
>>>  #if defined strdupa
>>>  # define ASSIGN_STRDUPA(DEST, S)               \
>>>   do { DEST = strdupa (S); } while (0)
>>>
>>
>> Just to make sure I understand why something else was invalid, I
>> wrapped the print statement with an if that basically had the same
>> logic as df.c (mp=find_mount_point....; if(mp){print....})
>>
>> That, to me, seemed valid, though it would not print anything at all
>> if find_mount_point returned a null.  I suppose it would be preferred
>> for the question-mark result which is perhaps what your version is
>> doing.  Anyway, just thought I'd throw that out there.
>>
>> Sent in the e-mail for the legal side of things; waiting to hear back from them.
>>
>

Message #38 received at 6555 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: A Burgie <dajoker <at> gmail.com>
Cc: 6555 <at> debbugs.gnu.org
Subject: Re: bug#6555: stat enhancement
Date: Tue, 06 Jul 2010 22:53:40 +0100

On 06/07/10 19:18, A Burgie wrote:
> Is there any documentation on doing this the right (meaning, GNU) way?
>  I've separated out find_mount_point() into a separate .c file,
> created a .h file, put both in ./gnulib/lib and linked-to them from
> lib, included the .h file in stat.c and df.c, but cannot compile
> (w/make).  I believe I still need to modify a makefile to include the
> findmountpoint.c file somewhere but have never worked w/make except as
> an end user (./configure && make && make install).  Also I do not know
> that I am using the correct directories (./gnulib/lib) for my new .c
> and .h files.

lib/ is for gnulib stuff so best avoided.
Have a look at how a function was extracted to a separate file previously.
For example, search for "operand2sig" in `git show 265c4b83`

> My current error makes me think I'm missing something
> else that's probably obvious:
> 
>   CC     stat.o
> stat.c: In function 'print_stat':
> stat.c:686:7: error: a label can only be part of a statement and a
> declaration is not a statement
> stat.c: At top level:

is your case 'm': typoed?

> ../lib/findmountpoint.h:28:1: warning: 'find_mount_point' used but never defined

that'll probably be sorted if you follow what was done for operand2sig

cheers,
Pádraig.

Message #41 received at 6555 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: A Burgie <dajoker <at> gmail.com>
Cc: 6555 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#6555: stat enhancement
Date: Wed, 07 Jul 2010 00:15:25 +0200

A Burgie wrote:

> Is there any documentation on doing this the right (meaning, GNU) way?
>  I've separated out find_mount_point() into a separate .c file,
> created a .h file, put both in ./gnulib/lib and linked-to them from

For now, you can just put them in src/.  Simpler that way.
If needed, we can move them later.

Message #44 received at 6555 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#6555: stat enhancement
Date: Tue, 6 Jul 2010 18:04:42 -0600

[Message part 1 (text/plain, inline)]

Well I am closer, but am still technically nowhere since it still will
not compile.  The problem with the odd declaration error was because I
was trying to declare a c-pointer in the case section of the switch
statement.  I guess that's not allowed in C (back to the easy
languages for me).  Despite this I am still stuck with the compilation
complaining about find_mount_point's lack of existence though I'd bet
a nickel I have everything setup properly.  Attached are a couple DIFF
files for those who may want to try to replay things and laugh at my
mediocrity on the condition that an explanation is provided where I'm
failing.  I'd have just one DIFF file but I can't figure out how to
make Git show what I want so I think the combination of these two show
it all (first use of Git as well).

Sorry to be slow at this.  I'd like to learn and have yet to find
documentation on GNU's site (or anywhere else) other than the diff for
operand2sig which helped me a little but not quite enough for what
seems like a very simple operation (extract function, put in file,
create .h file, include .h file in previous .c files, modify
Makefile.am, compile).

Thanks,
AB

On Tue, Jul 6, 2010 at 16:15, Jim Meyering <jim <at> meyering.net> wrote:
> A Burgie wrote:
>
>> Is there any documentation on doing this the right (meaning, GNU) way?
>>  I've separated out find_mount_point() into a separate .c file,
>> created a .h file, put both in ./gnulib/lib and linked-to them from
>
> For now, you can just put them in src/.  Simpler that way.
> If needed, we can move them later.
>

[DIFF0 (application/octet-stream, attachment)]

[DIFF1 (application/octet-stream, attachment)]

Message #47 received at 6555 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: A Burgie <dajoker <at> gmail.com>
Cc: 6555 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#6555: stat enhancement
Date: Wed, 07 Jul 2010 08:19:04 +0200

A Burgie wrote:
> Well I am closer, but am still technically nowhere since it still will
> not compile.  The problem with the odd declaration error was because I
> was trying to declare a c-pointer in the case section of the switch
> statement.  I guess that's not allowed in C (back to the easy
> languages for me).  Despite this I am still stuck with the compilation
> complaining about find_mount_point's lack of existence though I'd bet
> a nickel I have everything setup properly.  Attached are a couple DIFF
> files for those who may want to try to replay things and laugh at my
> mediocrity on the condition that an explanation is provided where I'm
> failing.  I'd have just one DIFF file but I can't figure out how to
> make Git show what I want so I think the combination of these two show
> it all (first use of Git as well).
>
> Sorry to be slow at this.  I'd like to learn and have yet to find
> documentation on GNU's site (or anywhere else) other than the diff for
> operand2sig which helped me a little but not quite enough for what
> seems like a very simple operation (extract function, put in file,
> create .h file, include .h file in previous .c files, modify
> Makefile.am, compile).

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

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

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

> +#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-"*".

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

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

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

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

> +    error (0, errno, _("cannot get current directory"));
> +    return NULL;
> +  }
> +
> +  if (S_ISDIR (file_stat->st_mode))
> +    /* FILE is a directory, so just chdir there directly.  */
> +  {
> +    last_stat = *file_stat;
> +    if (chdir (file) < 0)
> +    {
> +      error (0, errno, _("cannot change to directory %s"), quote (file));
> +      return NULL;
> +    }
> +  }
> +  else
> +    /* FILE is some other kind of file; use its directory.  */
> +  {
> +    char *xdir = dir_name (file);
> +    char *dir;
> +    ASSIGN_STRDUPA (dir, xdir);
> +    free (xdir);
> +
> +    if (chdir (dir) < 0)
> +    {
> +      error (0, errno, _("cannot change to directory %s"), quote (dir));
> +      return NULL;
> +    }
> +
> +    if (stat (".", &last_stat) < 0)
> +    {
> +      error (0, errno, _("cannot stat current directory (now %s)"),
> +          quote (dir));
> +      goto done;
> +    }
> +  }
> +
> +  /* Now walk up FILE's parents until we find another file system or /,
> +   * chdiring as we go.  LAST_STAT holds stat information for the last place
> +   * we visited.  */

Same here.

> +  while (true)
> +  {
> +    struct stat st;
> +    if (stat ("..", &st) < 0)
> +    {
> +      error (0, errno, _("cannot stat %s"), quote (".."));
> +      goto done;
> +    }
> +    if (st.st_dev != last_stat.st_dev || st.st_ino == last_stat.st_ino)
> +      /* cwd is the mount point.  */
> +      break;
> +    if (chdir ("..") < 0)
> +    {
> +      error (0, errno, _("cannot change to directory %s"), quote (".."));
> +      goto done;
> +    }
> +    last_stat = st;
> +  }
> +
> +  /* Finally reached a mount point, see what it's called.  */
> +  mp = xgetcwd ();
> +
> +done:
> +  /* Restore the original cwd.  */
> +  {
> +    int save_errno = errno;
> +    if (restore_cwd (&cwd) != 0)
> +      error (EXIT_FAILURE, errno,
> +          _("failed to return to initial working directory"));
> +    free_cwd (&cwd);
> +    errno = save_errno;
> +  }
> +
> +  return mp;
> +}
> +
> +
> diff --git a/src/findmountpoint.h b/src/findmountpoint.h
> new file mode 100644
> index 0000000..3943b1e
> --- /dev/null
> +++ b/src/findmountpoint.h
> @@ -0,0 +1,27 @@
> +/* stat-related time functions.
> + *
> + *    Copyright (C) 2005, 2007, 2009-2010 Free Software Foundation, Inc.
> + *
> + *    This program is free software: you can redistribute it and/or modify
> + *    it under the terms of the GNU General Public License as published by
> + *    the Free Software Foundation; either version 3 of the License, or
> + *    (at your option) any later version.
> + *
> + *    This program is distributed in the hope that it will be useful,
> + *    but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *    GNU General Public License for more details.
> + *
> + *    You should have received a copy of the GNU General Public License
> + *    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */

remove the "*"s on continued lines.

> +#ifndef FINDMOUNTPOINT_H
> +#define FINDMOUNTPOINT_H 1
> +#endif

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

> +
> +/* 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.

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

Once you've addressed all that,
(don't worry about a test.  I'll add that)
you'll want to write a ChangeLog entry.
They're discussed in HACKING.
Run "git log|less" to see many examples.

Message #50 received at 6555 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: A Burgie <dajoker <at> gmail.com>
Cc: 6555 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#6555: stat enhancement
Date: Wed, 07 Jul 2010 08:29:55 +0200

A Burgie wrote:
> Well I am closer, but am still technically nowhere since it still will
> not compile.  The problem with the odd declaration error was because I
> was trying to declare a c-pointer in the case section of the switch
> statement.  I guess that's not allowed in C (back to the easy
> languages for me).  Despite this I am still stuck with the compilation
> complaining about find_mount_point's lack of existence though I'd bet

You have to add the following to src/Makefile.am,
since with your change, df and stat are no longer derived
solely from df.c and stat.c respectively:

df_SOURCES = stat.c find-mount-point.c
stat_SOURCES = df.c find-mount-point.c

> a nickel I have everything setup properly.  Attached are a couple DIFF
> files for those who may want to try to replay things and laugh at my
> mediocrity on the condition that an explanation is provided where I'm
> failing.  I'd have just one DIFF file but I can't figure out how to
> make Git show what I want so I think the combination of these two show
> it all (first use of Git as well).
>
> Sorry to be slow at this.  I'd like to learn and have yet to find
> documentation on GNU's site (or anywhere else) other than the diff for
> operand2sig which helped me a little but not quite enough for what
> seems like a very simple operation (extract function, put in file,
> create .h file, include .h file in previous .c files, modify
> Makefile.am, compile).

Note that I didn't even review (more than superficially)
or test your patch.  If you have to make any change at all
when moving the function from df.c to the new file, please
tell us about it.  Otherwise, the function body in the new file
should be identical.

Message #53 received at 6555 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#6555: stat enhancement
Date: Wed, 7 Jul 2010 19:55:46 -0600

[Message part 1 (text/plain, inline)]

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)]

Message #56 received at 6555 <at> debbugs.gnu.org (full text, mbox):

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: Re: 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)]

Message #59 received at 6555 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#6555: stat enhancement
Date: Thu, 12 Aug 2010 21:01:40 -0600

I have received confirmation of the completion of my exciting
paperwork.  I can send the PDF for proof but at this point all of the
legal stuff should be handled.

Thanks,
AB

On Thu, Jul 15, 2010 at 20:08, A Burgie <dajoker <at> gmail.com> wrote:
> 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
>>
>

Message #64 received at 6555-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: A Burgie <dajoker <at> gmail.com>
Cc: 6555-done <at> debbugs.gnu.org, Jim Meyering <jim <at> meyering.net>
Subject: Re: bug#6555: stat enhancement
Date: Wed, 18 Aug 2010 15:13:22 +0100

[Message part 1 (text/plain, inline)]

On 13/08/10 04:01, A Burgie wrote:
> I have received confirmation of the completion of my exciting
> paperwork.  I can send the PDF for proof but at this point all of the
> legal stuff should be handled.

The attached passes `make syntax-check`
I'll clean it up further and apply this evening.

cheers,
Pádraig.

[stat-mnt.diff (text/x-patch, attachment)]

Message #67 received at 6555 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: A Burgie <dajoker <at> gmail.com>
Cc: 6555 <at> debbugs.gnu.org, Jim Meyering <jim <at> meyering.net>
Subject: Re: bug#6555: stat enhancement
Date: Thu, 19 Aug 2010 00:45:37 +0100

[Message part 1 (text/plain, inline)]

On 13/08/10 04:01, A Burgie wrote:
> I have received confirmation of the completion of my exciting
> paperwork.  I can send the PDF for proof but at this point all of the
> legal stuff should be handled.

I updated the patch further, adding a test,
and fixing a small mem leak in print_stat().
However I'm wary about pushing as I'm worried
about bypassing most of the logic from df::show_point().
I.E. do we lose anything by not using read_file_system_list()?

cheers,
Pádraig.

[stat-mnt.diff (text/x-patch, attachment)]

Message #70 received at 6555 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 6555 <at> debbugs.gnu.org, A Burgie <dajoker <at> gmail.com>
Subject: Re: bug#6555: stat enhancement
Date: Thu, 19 Aug 2010 17:24:49 +0200

Pádraig Brady wrote:
> On 13/08/10 04:01, A Burgie wrote:
>> I have received confirmation of the completion of my exciting
>> paperwork.  I can send the PDF for proof but at this point all of the
>> legal stuff should be handled.
>
> I updated the patch further, adding a test,
> and fixing a small mem leak in print_stat().
> However I'm wary about pushing as I'm worried
> about bypassing most of the logic from df::show_point().
> I.E. do we lose anything by not using read_file_system_list()?

Hi Pádraig and Aaron,
That patch looks fine.  Thanks to both of you!

I noticed that there is a semantic difference with df.  Running
"df symlink-to-dir" works like "df dir", while "stat --format=%m
symlink-to-dir" operates on the symlink, not on the directory.  That does
seem to be the proper default, given stat's --dereference (-L) option
(which I confirmed does work fine).

It might be worth adding a note in the texinfo doc that people looking
for df-like semantics from %m should use --dereference (-L).

Message #73 received at 6555 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 6555 <at> debbugs.gnu.org, A Burgie <dajoker <at> gmail.com>
Subject: Re: bug#6555: stat enhancement
Date: Thu, 19 Aug 2010 16:51:48 +0100

On 19/08/10 16:24, Jim Meyering wrote:
> Pádraig Brady wrote:
>> On 13/08/10 04:01, A Burgie wrote:
>>> I have received confirmation of the completion of my exciting
>>> paperwork.  I can send the PDF for proof but at this point all of the
>>> legal stuff should be handled.
>>
>> I updated the patch further, adding a test,
>> and fixing a small mem leak in print_stat().
>> However I'm wary about pushing as I'm worried
>> about bypassing most of the logic from df::show_point().
>> I.E. do we lose anything by not using read_file_system_list()?
> 
> Hi Pádraig and Aaron,
> That patch looks fine.  Thanks to both of you!
> 
> I noticed that there is a semantic difference with df.  Running
> "df symlink-to-dir" works like "df dir", while "stat --format=%m
> symlink-to-dir" operates on the symlink, not on the directory.  That does
> seem to be the proper default, given stat's --dereference (-L) option
> (which I confirmed does work fine).
> 
> It might be worth adding a note in the texinfo doc that people looking
> for df-like semantics from %m should use --dereference (-L).

I also noticed differences with /dev/nodes.
That's probably ok though, but would need to be documented:

$ stat -c%m /dev/sda3
/dev
$ df -P /dev/sda3 | sed -n '1!s/.* \([^ ]*$\)/\1/p'
/boot

I also noticed differences with bind mounts though,
which I need to look into further.
There are also comments in df::show_point() to
indicate find_mount_points() is only a fall back and
may hang. Perhaps we need to move more of show_point()
to `stat`?

$ mkdir tdir
$ touch tfile
$ sudo mount --bind /dev/shm/ tdir
$ touch tdir/tfile
$ sudo mount --bind /dev/shm/tfile tfile

$ df -P tfile tdir/ tdir/tfile | sed -n '1!s/.* \([^ ]*$\)/\1/p'
/dev/shm
/dev/shm
/dev/shm
$ stat -c%m tfile tdir/ tdir/tfile
/old_home
/old_home/padraig/git/tdir
/old_home/padraig/git/tdir

BTW, I updated the patch to output '?' when find_mount_point() == NULL

cheers,
Pádraig.

Message #76 received at 6555 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: A Burgie <dajoker <at> gmail.com>
Cc: 6555 <at> debbugs.gnu.org
Subject: Re: bug#6555: stat enhancement
Date: Mon, 23 Aug 2010 20:18:49 +0100

On 19/08/10 16:51, Pádraig Brady wrote:
> I also noticed differences with bind mounts though,
> which I need to look into further.
> There are also comments in df::show_point() to
> indicate find_mount_points() is only a fall back and
> may hang. Perhaps we need to move more of show_point()
> to `stat`?

Looking more at correlating `statm -c%m file` and `df -P file`,
I noticed an inconsistency with how df deals with bind mounts.
Take for example:

  $ mount | column -t
  /dev/shm/tdir                on  /old_home/padraig/git/tdir
  /old_home/padraig/git/tfile  on  /dev/shm/tdir/tfile

I.E. /old_home/padraig/git/tdir is replaced by /dev/shm/tdir
and /dev/shm/tdir/tfile is replaced by /old_home/padraig/git/tfile

Now if we pass an already canonicalized absolute path
to the mount target to df, we get inconsistent results:

~/git$ df -Ph tfile /dev/shm/tdir//tfile /dev/shm/tdir/tfile
Filesystem            Size  Used Avail Use% Mounted on
/dev/sda8              37G   34G  3.0G  92% /old_home
/dev/sda8              37G   34G  3.0G  92% /old_home
/dev/sda8              37G   34G  3.0G  92% /old_home
/old_home/padraig/git/tfile   37G   34G  3.0G  92% /dev/shm/tdir/tfile
$ df -Ph tdir /old_home/padraig/git/tdir/ /old_home/padraig/git/tdir
Filesystem            Size  Used Avail Use% Mounted on
tmpfs                1004M  272K 1003M   1% /dev/shm
tmpfs                1004M  272K 1003M   1% /dev/shm
/dev/shm/tdir        1004M  272K 1003M   1% /old_home/padraig/git/tdir

I think that df should resolve to disk devices if possible.
I.E. the last line output in each of the above 2 commands above
is inconsistent and a bit confusing IMHO. This patch
removes the optimization which triggers this.

diff --git a/src/df.c b/src/df.c
index 76622fb..24877ab 100644
--- a/src/df.c
+++ b/src/df.c
@@ -643,54 +643,35 @@ show_point (const char *point, const struct stat *statp)
   struct mount_entry *me;
   struct mount_entry const *best_match = NULL;

-  /* If POINT is an absolute file name, see if we can find the
-     mount point without performing any extra stat calls at all.  */
-  if (*point == '/')
-    {
-      /* Find the best match: prefer non-dummies, and then prefer the
-         last match if there are ties.  */
-
-      for (me = mount_list; me; me = me->me_next)
-        if (STREQ (me->me_mountdir, point) && !STREQ (me->me_type, "lofs")
-            && (!best_match || best_match->me_dummy || !me->me_dummy))
-          best_match = me;
-    }


Note now that we have `stat -c%m`, that seems a more appropriate
place to output an aliased target if present, like:

~/git$ stat -c%m tdir /old_home/padraig/git/tdir/ /dev/shm/tdir
/dev/shm/tdir
/dev/shm/tdir
/dev/shm

cheers,
Pádraig.

p.s. I'll also update NEWS with the changed behavior
if I apply the above

Message #79 received at 6555 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: A Burgie <dajoker <at> gmail.com>
Cc: 6555 <at> debbugs.gnu.org
Subject: Re: bug#6555: stat enhancement
Date: Wed, 25 Aug 2010 22:54:34 +0100

[Message part 1 (text/plain, inline)]

As a follow up to df not outputting bind mounts,
here is the amended stat -c%m patch than _does_
resolve bind mounts. I was mulling over an option
to output the mount point of the base device
for a file, but one can just use df -P for that now,
or run stat in a loop like:

mp=; lp=/your/file/name
while [ "$lp" != "$mp" ]; do
  mp=$lp; lp=$(./src/stat -c%m "$mp")
done
echo $lp

Some example output in the presence of bind mounts:

$ mount | column -t | grep git
/dev/shm/tdir                      on  /home/padraig/git/tdir
/home/padraig/git/tfile            on  /dev/shm/tdir/tfile
/home/padraig/git/t/coreutils/t/1  on  /home/padraig/git/t/coreutils/t/2
/home/padraig/git/t/coreutils/t/1  on  /home/padraig/git/t/coreutils/t/3
/home/padraig/git/t/coreutils/t/f  on  /home/padraig/git/t/coreutils/t/bf

~/git/t/coreutils$ ./src/stat -c%m /home/padraig/git/tdir/tfile /dev/shm/tdir/tfile t/2 t
/dev/shm/tdir
/home/padraig/git/tfile
/home/padraig/git/t/coreutils/t/1
/home

cheers,
Pádraig.

[stat-mnt.diff (text/x-patch, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.