svn commit: r284163 - head/bin/cp

[Bryan Drewery]

On 6/9/2015 1:28 AM, Bruce Evans wrote:
> On Mon, 8 Jun 2015, Bryan Drewery wrote:
> 
>> Log:
>>  Cleanup some style(9) issues.
>>
>>  - Whitespace.
>>  - Comments.
>>  - Wrap long lines.
> 
> cp's style had a remarlable amount of bitrot.
> 
> This change unimproves it in some places.

I have been traveling and packing. I'm replying now but won't have time
to address the issues until next week. I was trying to avoid doing any
of this but touched code which was horrendously misstyled and chained
into reindenting the whole file and doing it wrong :). At this point I
don't want to tweak this much more.

>> --- head/bin/cp/cp.c    Mon Jun  8 19:13:04 2015    (r284162)
>> +++ head/bin/cp/cp.c    Mon Jun  8 19:24:18 2015    (r284163)
>> @@ -75,8 +75,8 @@ __FBSDID("$FreeBSD$");
>> #include "extern.h"
>>
>> #define    STRIP_TRAILING_SLASH(p) {                    \
>> -        while ((p).p_end > (p).p_path + 1 && (p).p_end[-1] == '/')    \
>> -                *--(p).p_end = 0;                    \
>> +    while ((p).p_end > (p).p_path + 1 && (p).p_end[-1] == '/')    \
>> +    *--(p).p_end = 0;                        \
> 

Woops.

> 
>> @@ -245,10 +245,10 @@ main(int argc, char *argv[])
>>             type = FILE_TO_FILE;
>>
>>         if (have_trailing_slash && type == FILE_TO_FILE) {
>> -            if (r == -1)
>> +            if (r == -1) {
> 
> This adds excessive braces.
> 
>>                 errx(1, "directory %s does not exist",
>> -                     to.p_path);
>> -            else
>> +                    to.p_path);
>> +            } else

It is a multi-line statement due to the hard 80-width wrap. I feel it is
fine in this case.

>>                 errx(1, "%s is not a directory", to.p_path);
>>         }
>>     } else
>> ...
>> @@ -379,7 +379,8 @@ copy(char *argv[], enum op type, int fts
>>                 mode = curr->fts_statp->st_mode;
>>                 if ((mode & (S_ISUID | S_ISGID | S_ISTXT)) ||
>>                     ((mode | S_IRWXU) & mask) != (mode & mask))
>> -                    if (chmod(to.p_path, mode & mask) != 0){
>> +                    if (chmod(to.p_path, mode & mask) !=
>> +                        0) {
>>                         warn("chmod: %s", to.p_path);
>>                         rval = 1;
>>                     }
> 
> This changes from a minor misformatting to avoid a long line to even uglier
> formatting with a split line.

I agree 100%. I did it because of our hard 80-width cut-off. What would
the proper style be? My inclination would be to wrap at the first comma
but then it is even more odd. I find our 80-width cut-off to be strange
when editors/tmux/window manager/etc can resize and wrap long lines already.

Actually I don't see a width restriction in style(9) at all but surely
we have this rule documented somewhere. My guess is that it is inherited
by KNF.

> It is necessary to make such changes if you
> use indent(1) to generate and check the changes -- otherwise, indent keeps

Do you have an indent configuration I can use?

> reporting the misformatting -- but since cp rarely went near indent it
> may be better to keep its minor misformattings.
> 
>> Modified: head/bin/cp/utils.c
>> ==============================================================================
>>
>> --- head/bin/cp/utils.c    Mon Jun  8 19:13:04 2015    (r284162)
>> +++ head/bin/cp/utils.c    Mon Jun  8 19:24:18 2015    (r284163)
>> ...
>> -/* Small (default) buffer size in bytes. It's inefficient for this to be
>> - * smaller than MAXPHYS */
>> +/*
>> + * Small (default) buffer size in bytes. It's inefficient for this to be
>> + * smaller than MAXPHYS.
>> + */
> 
> Still has unusual sentence break of 1 space.  cp uses normal sentence
> breaks

I did a minimal effort on comments and didn't clean up grammar or
breaks. I have not adopted 2 space breaks into my style(9) conformation yet.


>> @@ -345,7 +352,7 @@ setfile(struct stat *fs, int fd)
>>     fdval = fd != -1;
>>     islink = !fdval && S_ISLNK(fs->st_mode);
>>     fs->st_mode &= S_ISUID | S_ISGID | S_ISVTX |
>> -               S_IRWXU | S_IRWXG | S_IRWXO;
>> +        S_IRWXU | S_IRWXG | S_IRWXO;
> 
> Here the formatting was reasonable, but it was in gnu style and was hard to
> maintain since it is not supported by indent(1).  It is still hard to
> maintain,
> since it has fancy splitting earlier than necessary to put the S_IS* and
> S_IR* parts of the expressions on separate lines.  indent(1) cannot
> reproduce
> this splitting.  Also, with the normal indentation of the condinuation
> line,
> the fancy splitting is not so readable.

I'm do not see how this was proper before or how it is worse now. The
indentation is tabs and then 4 spaces. I don't see exceptions to this in
style(9) or in other code.

> 
>> @@ -543,8 +550,10 @@ usage(void)
>> {
>>
>>     (void)fprintf(stderr, "%s\n%s\n",
>> -"usage: cp [-R [-H | -L | -P]] [-f | -i | -n] [-alpsvx] source_file
>> target_file",
>> -"       cp [-R [-H | -L | -P]] [-f | -i | -n] [-alpsvx] source_file
>> ... "
>> -"target_directory");
>> +        "usage: cp [-R [-H | -L | -P]] [-f | -i | -n] [-alpsvx] "
>> +        "source_file target_file",
>> +        "       cp [-R [-H | -L | -P]] [-f | -i | -n] [-alpsvx] "
>> +        "source_file ... "
>> +        "target_directory");
>>     exit(EX_USAGE);
>> }
> 
> This breaks the careful outdentation and obfuscates the strings.  The

Again, this broke the 80-width limit. I preferred the old way but was
going on down the 80-width line on my screen fixing violations.

I suggest we update our styles to not require this awful wrapping. It
makes `grep -r` very difficult when strings are split up. Perhaps I am
mistaken on the rule but we have a lot of code that needlessly wraps early.

Regards,
Bryan Drewery

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20150624/63ea4dda/attachment.bin>