That makes sense, if the summary of the change was "Style cleanups within ..." rather than "Replaced CHECK with CHECK_READY" then doing an overall style sweep of the file together sounds ok to me. Here the reviewer will be more likely to look through the file for other style changes that are needed (e.g. s/.get()./->/, using statements, etc).
When I read the CHECK_READY summary I tend to expect an isolated change addressing CHECK_READY, ideally a complete (rather than one file) sweep to use the CHECK_READY pattern in the code base. Thanks for cleaning this stuff up Neil! On Tue, May 10, 2016 at 6:25 AM, Neil Conway <[email protected]> wrote: > Hi Ben, > > Thanks for raising this! > > My thinking for grouping the changes together in a single review is > basically what AlexR said: I agree with doing "one thing per patch", > but I felt like a header cleanup was sufficiently close to CHECK > cleanup that they could be grouped together. If there's consensus that > folks would rather see such changes separated, I'm happy to do that in > the future. > > Re: the <stout/check.hpp> change, my mistake -- although I confess I > didn't realize we have a strict "no dependence on transitive includes" > policy. Is that documented anywhere? > > Anyway, RRs here: > > https://reviews.apache.org/r/47112 avoids depending on transitively > included headers > https://reviews.apache.org/r/47113 replaces "using namespace process;" > with more fine-grained "using" statements > https://reviews.apache.org/r/47114 fixes angle bracket style. > > Thanks, > Neil > > On Sun, May 8, 2016 at 10:28 AM, Alex R <[email protected]> wrote: > > I agree that "atomic patches" (those that do one thing per patch) are a > good > > thing because they simplify navigating history, do blame and bisect. But > how > > to define that "one thing"? Some people would say, that a new feature is > one > > thing, and if introducing a feature requires some refactoring, it should > be > > done in the same patch, so that motivation for the refactoring is clear. > On > > the other hand there will be folks who would say that putting all > > loosely-coupled changes like refactoring, tests, protobuf update, > > implementation into the same patch makes it hardly reviewable and > > complicates finding problems via blame / bisect. > > > > Let's take a step back and think, why we need to read commit history in > the > > first place. Based on my experience, I see several cases: > > * Searching which patch introduced a bug, a regression, or some > peculiar > > behaviour. > > * Learning how a feature was implemented and why. > > > > That's why it is important to separate functional changes from cleanups. > > > > However, do we still want to separate different sorts of cleanups as > well or > > squash them together to avoid the churn? If I search for a bug and see a > > style fix patch, I simply skip. I'd rather prefer to have one single > patch > > for all style fixes than a tiny patch for each type of cleanup. > > > > Regarding r/46827/, you are right that doing audit of include and using > > sections was not the primary intention, but I considered it fine to > include > > those changes since they were not making the code "worse" (modulo > removing > > <stout/check.hpp> which was a mistake). > > > > On 8 May 2016 at 02:38, Benjamin Mahler <[email protected]> wrote: > >> > >> Hm.. any reason that unrelated headers were touched and the using > >> statement was removed in this patch? > >> > >> My concern with mixing unrelated changes within a single patch is that > >> patches become less precise. If one reads the patch there is additional > >> overhead in understanding what is related to the goal of the change and > what > >> is not. I know it's a small example here but I see value in being > >> disciplined about this regardless of patch size. > >> > >> The other concern is that the reviewer of this patch has to review these > >> two additional changes: > >> 1. How does the header audit look? Anything else need added or > removed? > >> 2. How does the 'using' audit look? Anything else need added or > removed? > >> > >> (1) and (2) could be done together in a single patch. As in turns out, > the > >> header audit looks like it has a few issues, but I'm guessing the > reviewer > >> glossed over it because the point of this change was CHECK_READY :) > >> -<stout/check.hpp> was removed but CHECK_NONE and CHECK_SOME are used > >> -<glog/logging.h> is not present but LOG is used > >> -<stout/nothing.hpp> is absent but Nothing is used > >> -<process/process.hpp> is absent but process::Process / process::wait > / > >> process::terminate are used. > >> > >> Then for the 'using' audit, we now avoid pulling in all of the process:: > >> namespace in favor of finer-grained using statements. > >> > >> On Mon, May 2, 2016 at 4:08 AM, <[email protected]> wrote: > >>> > >>> Repository: mesos > >>> Updated Branches: > >>> refs/heads/master 78f6101cc -> 4f9040db6 > >>> > >>> > >>> Replaced CHECK with CHECK_READY. > >>> > >>> Also removes some unused header includes. > >>> > >>> Review: https://reviews.apache.org/r/46827/ > >>> > >>> > >>> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo > >>> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/4f9040db > >>> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/4f9040db > >>> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/4f9040db > >>> > >>> Branch: refs/heads/master > >>> Commit: 4f9040db62294f370f94aaba675f05b1ccf7a310 > >>> Parents: 78f6101 > >>> Author: Neil Conway <[email protected]> > >>> Authored: Mon May 2 13:05:56 2016 +0200 > >>> Committer: Alexander Rukletsov <[email protected]> > >>> Committed: Mon May 2 13:05:56 2016 +0200 > >>> > >>> ---------------------------------------------------------------------- > >>> src/zookeeper/contender.cpp | 7 ++----- > >>> 1 file changed, 2 insertions(+), 5 deletions(-) > >>> ---------------------------------------------------------------------- > >>> > >>> > >>> > >>> > http://git-wip-us.apache.org/repos/asf/mesos/blob/4f9040db/src/zookeeper/contender.cpp > >>> ---------------------------------------------------------------------- > >>> diff --git a/src/zookeeper/contender.cpp b/src/zookeeper/contender.cpp > >>> index 4b1cc65..5eb4b58 100644 > >>> --- a/src/zookeeper/contender.cpp > >>> +++ b/src/zookeeper/contender.cpp > >>> @@ -14,26 +14,23 @@ > >>> // See the License for the specific language governing permissions and > >>> // limitations under the License > >>> > >>> -#include <set> > >>> #include <string> > >>> > >>> #include <mesos/zookeeper/contender.hpp> > >>> #include <mesos/zookeeper/detector.hpp> > >>> #include <mesos/zookeeper/group.hpp> > >>> > >>> +#include <process/check.hpp> > >>> #include <process/defer.hpp> > >>> #include <process/dispatch.hpp> > >>> #include <process/future.hpp> > >>> #include <process/id.hpp> > >>> > >>> -#include <stout/check.hpp> > >>> #include <stout/lambda.hpp> > >>> #include <stout/option.hpp> > >>> -#include <stout/some.hpp> > >>> > >>> using namespace process; > >>> > >>> -using std::set; > >>> using std::string; > >>> > >>> namespace zookeeper { > >>> @@ -208,7 +205,7 @@ void LeaderContenderProcess::cancel() > >>> > >>> void LeaderContenderProcess::cancelled(const Future<bool>& result) > >>> { > >>> - CHECK(candidacy.isReady()); > >>> + CHECK_READY(candidacy); > >>> LOG(INFO) << "Membership cancelled: " << candidacy.get().id(); > >>> > >>> // Can be called as a result of either withdraw() or server side > >>> > >> > > >
