Date: Wed, 24 Jun 2015 22:24:12 -0500 From: Bryan Drewery <bdrewery@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> 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: <558B745C.8040305@FreeBSD.org> In-Reply-To: <20150609152946.Y935@besplex.bde.org> References: <201506081924.t58JOJQw095752@svn.freebsd.org> <20150609152946.Y935@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--5TnR9hbiIaAbEsS7FB5tPK1o5mXoQammU
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable
On 6/9/2015 1:28 AM, Bruce Evans wrote:
> On Mon, 8 Jun 2015, Bryan Drewery wrote:
>=20
>> Log:
>> Cleanup some style(9) issues.
>>
>> - Whitespace.
>> - Comments.
>> - Wrap long lines.
>=20
> cp's style had a remarlable amount of bitrot.
>=20
> 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] =3D=3D '/'=
) \
>> - *--(p).p_end =3D 0; \
>> + while ((p).p_end > (p).p_path + 1 && (p).p_end[-1] =3D=3D '/') =
\
>> + *--(p).p_end =3D 0; \
>=20
Woops.
>=20
>> @@ -245,10 +245,10 @@ main(int argc, char *argv[])
>> type =3D FILE_TO_FILE;
>>
>> if (have_trailing_slash && type =3D=3D FILE_TO_FILE) {
>> - if (r =3D=3D -1)
>> + if (r =3D=3D -1) {
>=20
> This adds excessive braces.
>=20
>> 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 =3D curr->fts_statp->st_mode;
>> if ((mode & (S_ISUID | S_ISGID | S_ISTXT)) ||
>> ((mode | S_IRWXU) & mask) !=3D (mode & mask))
>> - if (chmod(to.p_path, mode & mask) !=3D 0){
>> + if (chmod(to.p_path, mode & mask) !=3D
>> + 0) {
>> warn("chmod: %s", to.p_path);
>> rval =3D 1;
>> }
>=20
> This changes from a minor misformatting to avoid a long line to even ug=
lier
> 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 alrea=
dy.
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 ke=
eps
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.
>=20
>> Modified: head/bin/cp/utils.c
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
>>
>> --- 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.
>> + */
>=20
> 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 y=
et.
>> @@ -345,7 +352,7 @@ setfile(struct stat *fs, int fd)
>> fdval =3D fd !=3D -1;
>> islink =3D !fdval && S_ISLNK(fs->st_mode);
>> fs->st_mode &=3D S_ISUID | S_ISGID | S_ISVTX |
>> - S_IRWXU | S_IRWXG | S_IRWXO;
>> + S_IRWXU | S_IRWXG | S_IRWXO;
>=20
> Here the formatting was reasonable, but it was in gnu style and was har=
d 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* an=
d
> 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.
>=20
>> @@ -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);
>> }
>=20
> 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 earl=
y.
Regards,
Bryan Drewery
--5TnR9hbiIaAbEsS7FB5tPK1o5mXoQammU
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iQEcBAEBAgAGBQJVi3RcAAoJEDXXcbtuRpfPpcEIAJuqfa0rdLGVUG0UlN5mlm22
Z5nUUisEI0sGA2Y3glks+d7vU8qktNVahzQPkqXh4ESb1MX/k1kKWEJyhCbCVmpB
3zazlkyjFEQxULWM24zaQvf+YCDtoDCFsnn3ka3j5ueLzFk72uVguKU5Ss6niUyU
PCUbF4Z6RXed5K7HI0WUSbFsLtJWPYewZZc/7X65j1HAAKXAOlMCennXqLoFSFzE
Y5LqVh7syrVbhgoc+7eD8J2+8M/QUsTtOBe3FBWaMS3mQSs+Sqe/xXwkcFBUSWTw
V2jfHY9XKttQAJRivRkI+iUiUYMxPocoAireOxu29kXuaPEa8McjIIiyDgqgLeM=
=yF7A
-----END PGP SIGNATURE-----
--5TnR9hbiIaAbEsS7FB5tPK1o5mXoQammU--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?558B745C.8040305>