Date: Fri, 25 Nov 2016 20:30:48 -0500 From: Ed Maste <emaste@freebsd.org> To: =?UTF-8?Q?Dag=2DErling_Sm=C3=B8rgrav?= <des@des.no> Cc: John Baldwin <jhb@freebsd.org>, Marcelo Araujo <araujobsdport@gmail.com>, Marcelo Araujo <araujo@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org> Subject: Re: svn commit: r309109 - head/lib/libutil Message-ID: <CAPyFy2BNiWHPy4v%2BNqEAHjNeZnmmhjydNFpUP7LajVkf7x3H%2BA@mail.gmail.com> In-Reply-To: <86vavc3bwm.fsf@desk.des.no> References: <201611241450.uAOEoLA5079215@repo.freebsd.org> <CAOfEmZgexP65RKz1=WxcFBWTYGYbnSHWMzknkGAeFN=DwjBL8A@mail.gmail.com> <861sy0n8re.fsf@desk.des.no> <2094160.1ufjjsmd6m@ralph.baldwin.cx> <86vavc3bwm.fsf@desk.des.no>
next in thread | previous in thread | raw e-mail | index | archive | help
On 24 November 2016 at 14:39, Dag-Erling Sm=C3=B8rgrav <des@des.no> wrote: > > Precisely. If memory serves, I wrote that comment after receiving a > patch from someone who made the same mistake that I had already made and > reverted *twice*. It's the logical, sane thing to do: replace a BSD > primitive with the equivalent POSIX primitive, except the latter has > subtly different semantics and works in some of flopen(3)'s typical use > cases, but not all, and crucially, not in the pidfile(3) case. In other words, nobody else has changed this code, and in one case where someone proposed a broken patch they did it by contacting you directly. This seems like exactly the desired behaviour, without needing any warning in the code. The comment added in r309109 hardly seems appropriate for this case, especially given that the revision history doesn't offer much insight. Please rephrase the comment to explain instead why the "obvious" improvements are not appropriate. > I just remembered that I wrote a unit test for flopen(3). So maybe the > comment is redundant... if you assume that people build and run the > tests, and I'm willing to bet that they don't, because our test > framework is not very developer-friendly. I share your frustration with the lack of developer friendliness in our tests. But running the test suite must be a part of the release checklist and if the test detects a regression here our process must prevent it from making it into a release.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPyFy2BNiWHPy4v%2BNqEAHjNeZnmmhjydNFpUP7LajVkf7x3H%2BA>