Re: [PATCH] df: fail when the mount list cannot be read but is needed [was: new snapshot available: coreutils-8.17.70-1bacb4]

[Jim Meyering]

Bernhard Voelker wrote:
> On 08/14/2012 10:51 AM, Jim Meyering wrote:
>> Here is the patch that I expect to squash in.
>> Most are nits, but there was an exploitable bit
>> of (debugging?) code in the new test.
>>
>> diff --git a/NEWS b/NEWS
>> index 0f63ddf..46d0a41 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -2,8 +2,8 @@ GNU coreutils NEWS                                    -*- 
>> outline -*-
>>
>>  * Noteworthy changes in release ?.? (????-??-??) [?]
>>
>> -  df now fails in cases when the list of mounted file systems (/etc/mtab)
>> -  cannot be read but the file system type information is needed to process
>> +  df now fails when the list of mounted file systems (/etc/mtab) cannot
>> +  be read, yet the file system type information is needed to process
>>    certain options like -a, -l, -t and -x.
>>    [This bug was present in "the beginning".]
>>
>> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
>> index 2fe0dd3..744b41a 100644
>> --- a/doc/coreutils.texi
>> +++ b/doc/coreutils.texi
>> @@ -10754,9 +10754,9 @@ df invocation
>>  @var{dir}} to test whether @var{dir} is on a file system of type
>>  @samp{ext3} or @samp{reiserfs}.
>>
>> -As the list of file systems (@var{mtab}) is needed to determine the
>> +Since the list of file systems (@var{mtab}) is needed to determine the
>>  file system type, failure includes the cases when that list cannot
>> -be read and either of the options @option{-a}, @option{-l}, @option{-t}
>> +be read and one or more of the options @option{-a}, @option{-l}, @option{-t}
>>  or @option{-x} is used together with a file name argument.
>
> Thanks for the rewording both in NEWS and cu.texi.
> Often it's still hard for me to find the right wording in English ...

Don't worry.
The wording changes were more for style than for correctness.
They're the sorts of mistakes that native speakers might well make.

>> diff --git a/src/df.c b/src/df.c
>> index a2e6866..835cc2e 100644
>> --- a/src/df.c
>> +++ b/src/df.c
>> @@ -1104,12 +1104,12 @@ main (int argc, char **argv)
>>           or when either of -a, -l, -t or -x is used with file name
>>           arguments. Otherwise, merely give a warning and proceed.  */
>>        int status = 0;
>> -      if ( !(optind < argc)
>> -          ||(optind < argc
>> -             && (show_all_fs
>> -                 || show_local_fs
>> -                 || fs_select_list != NULL
>> -                 || fs_exclude_list != NULL)))
>> +      if ( ! (optind < argc)
>> +           || (optind < argc

Actually, I realized that we should remove the latter "optind < argc"
conjunct.  Because in the RHS of the outer "||", that expression is already
guaranteed to be true.  I.e.,

diff --git a/src/df.c b/src/df.c
index a2e6866..9d956cd 100644
--- a/src/df.c
+++ b/src/df.c
@@ -1104,12 +1104,11 @@ main (int argc, char **argv)
          or when either of -a, -l, -t or -x is used with file name
          arguments. Otherwise, merely give a warning and proceed.  */
       int status = 0;
-      if ( !(optind < argc)
-          ||(optind < argc
-             && (show_all_fs
-                 || show_local_fs
-                 || fs_select_list != NULL
-                 || fs_exclude_list != NULL)))
+      if ( ! (optind < argc)
+           || (show_all_fs
+               || show_local_fs
+               || fs_select_list != NULL
+               || fs_exclude_list != NULL))
         {
           status = EXIT_FAILURE;
         }

>> +               && (show_all_fs
>> +                   || show_local_fs
>> +                   || fs_select_list != NULL
>> +                   || fs_exclude_list != NULL)))
>>          {
>>            status = EXIT_FAILURE;
>>          }
>
> I wonder if there's something like "make diff-syntax-check" which enforces
> CU's coding styles for changed code ... the normal syntax-check didn't
> complain.

The spaces around "!" are not a big deal.
The main change was to add a space after "||".
If you compare the code as-is with the code filtered
through indent --no-tabs, it will give you an idea.
indent doesn't know about all types, so I have a bunch of lines
like this in ~/.indent.pro:

  -Ttcflag_t
  -Tcc_t
  -TDIR