Re: [boost] [pimpl] No documentation for pointer semant

Subject: Re: [boost] [pimpl] No documentation for pointer semant
From: Dave Gomboc (dave_gomboc_at_[hidden])
Date: 2014-06-06 13:03:51

[Paul A Bristow]:
> I unhappy with a review rigid policy that nothing can be changed during a
> review.  It isn't always helpful.
>
> We never accept anything without requiring some changes?
>
> Can we instead now use git's easy branches to specify a fixed 'master' branch,
> but allow the author to update a 'develop' branch as the review develops?

[Edward Diener]:
> I believe that changes can be made to a reviewed library during a review
> as long as the 'master' branch does not change. After all we want all
> reviewers to be looking at the same 'master' branch as the code to be
> reviewed. But if the developer wants to make changes to other branches
> during the review I think this should be allowed.

May I suggest that either the library author or the review manager
ought to, near the commencement of the review, identify a specific
version to be reviewed?  This is quite distinct from identifying a
branch name for a conventional branch such as master that is expected
to change frequently even over a two-week period.  A specific checkout
is typically identified by a SHA: a branch name such as 'master' is
primarily just an alias to the SHA to which it currently resolves.
Then again, Boost is relatively new to using git, so perhaps a new
branch name can be introduced to refer specifically to the review
version.

The review manager could, at the commencement of the review, specify
the checkout instructions using a specific SHA.  For example, for
Boost.Pimpl, the manager would provide

Source: https://github.com/yet-another-user/boost.pimpl, SHA 59a5c0d

or, in command-line-ese,

git clone https://github.com/yet-another-user/boost.pimpl
git reset --hard 59a5c0d

As I mentioned, not everybody may be comfortable working directly with
SHAs immediately, so the library author and/or the review manager may
wish to introduce a specific label for the purpose of easing
identifying the review version.  If one of them (or somebody else with
write access to the repository) continues from the above two commands
with the further

git checkout -b boost_review
git push origin boost_review

then it still becomes possible for the contents of the master branch
to change (which, generally speaking, is highly desirable) without
disrupting the review process.  In this case, the instructions
provided by the manager could be:

Source: https://github.com/yet-another-user/boost.pimpl, branch boost_review

or, in command-line-ese,

git clone https://github.com/yet-another-user/boost.pimpl
git checkout boost_review

which should be more palatable to the large group of people migrating
from Subversion.  Naturally, in this case it is imperative that
boost_review not itself be pushed to further during the review.
However, there is no reason the library author from continuing with
these commands:

git checkout -b boost_review_fixes
git push -u origin boost_review_fixes

at which point, fixes to items that come up in the review can be
pushed onto that branch, and those people who specifically want to
take a look at the fixes can easily do so.

The above said, the long-term simplicity of specifying a SHA directly
instead of a branch at all is not only that one doesn't have to go and
make a bunch of new branch names in order to continue development, but
even more importantly, one can't push to a SHA to change its contents
--because  modifying a commit after the fact modifies its SHA also --
so everyone is sure to get the same version of the content being
reviewed.

Dave