Date: Tue, 9 Jun 2015 16:28:11 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bryan Drewery <bdrewery@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r284163 - head/bin/cp Message-ID: <20150609152946.Y935@besplex.bde.org> In-Reply-To: <201506081924.t58JOJQw095752@svn.freebsd.org> References: <201506081924.t58JOJQw095752@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
"Clean up" is 2 words.
> Modified: head/bin/cp/cp.c
> ==============================================================================
> --- 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; \
This loses the indentation of the statement in the while loop.
> @@ -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
> 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. It is necessary to make such changes if you
use indent(1) to generate and check the changes -- otherwise, indent keeps
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
of 2 spaces in 28 lines, and only uses the 1-space misformatting in this and
in 2 other lines. Standard copyrights use normal 2-space formatting, so
switching to 1-space almost always gives inconsistent formatting even if
it is done consistently outside of the copyrights.
> @@ -119,24 +123,27 @@ copy_file(const FTSENT *entp, int dne)
> goto done;
> }
> }
> -
> +
> if (fflag) {
> - /* remove existing destination file name,
> - * create a new file */
> - (void)unlink(to.p_path);
> - if (!lflag && !sflag) {
> - to_fd = open(to.p_path, O_WRONLY | O_TRUNC | O_CREAT,
> - fs->st_mode & ~(S_ISUID | S_ISGID));
> - }
> + /*
> + * Remove existing destination file name create a new
> + * file.
> + */
This fixes most of the grammar errors, but completely removing the comma
splice gives a larger error.
> ...
> @@ -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.
> @@ -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
outdentation doesn't quite work, since the double quotes and comma make
the one of the lines too long, although the length of this line in the
output is only 78. Actually, 78 is also too long, and indicates further
bugs. The message should be formatted the same as in the synopsis in
the man page. But the man page formatting has a 5-space left margin,
so the maximum length of a matching string is 75 or 74... Oops, the
usage string has to be longer to include "usage: " That is 7 longer,
so matching the man page is impossible in general unless the man page
formatting has a large right margin (2-3 spaces), but I think it has
at most 1 space. Comparison shows that the above is broken for the
second line but not the first:
man page, FreeBSD-11:
cp [-R [-H | -L | -P]] [-f | -i | -n] [-alpvx] source_file target_file
cp [-R [-H | -L | -P]] [-f | -i | -n] [-alpvx]
source_file ... target_directory
man page, FreeBSD-10:
cp [-R [-H | -L | -P]] [-f | -i | -n] [-alpvx] source_file target_file
cp [-R [-H | -L | -P]] [-f | -i | -n] [-alpvx] source_file ...
target_directory
usage:
usage: cp [-R [-H | -L | -P]] [-f | -i | -n] [-alpvx] source_file target_file
cp [-R [-H | -L | -P]] [-f | -i | -n] [-alpvx] source_file ... target_directory
The following bugs are evident:
- the man page is misformatted in FreeBSD-11 -- the second line is split at
a bad place.
- the usage message is misformatted -- the second line is not split. This
bug was already implemented using the string concatenation obfuscation,
but in the old version it was a little easier to see that the string was
too long -- the outdented line obviously has nearly maximal length, so
any concatenation to it makes the string too long.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150609152946.Y935>