Proposal for post-commit review on certain kinds of safe changes.

[Karl Fogel]

I'd like to propose that we do post-commit review for certain kinds of minor, "safe" changes. Post-commit means the change could be pushed directly to 'master', if the committer wants to and is able to. People can still review it there, and any problems can be fixed in followup commits.

Well, instead of describing the change I propose to make to CONTRIBUTING.md, how about I just show it to you? Here it is:


--- CONTRIBUTING.md
+++ CONTRIBUTING.md
@@ -52,6 +52,44 @@ testing plan will come in the form of a test. For now, use a
Given-When-Then description like
[this one](https://github.com/OpenTechStrategies/psm/blob/master/psm-app/cms-web/src/main/test/resources/features/enrollment/create_enrollment.feature).

+### Certain minor changes may be committed directly to 'master'
+
+Certain kinds of "safe" changes may be reviewed post-commit instead of
+pre-commit, meaning that they can be committed directly to `master`
+without going through a PR, when the committer has push access to do
+so.
+
+The purpose of this is to save time for busy developers. It avoids
+the low-but-still-noticeable overhead of the PR review process, for
+changes where the that process would not provide much additional
+protection beyond what post-commit review provides anyway. In
+practice, that means the following kinds of changes:
+
+* Obvious typo fixes.
+
+ If there's an obvious typo that doesn't affect code flow (e.g., a
+ typo in a code comment, or even in a user-visible string), you can
+ just fix it. However, if the typo affects code behavior, other than
+ in how user-visible text is displayed, then it should go through the
+ normal PR review process.
+
+* Whitespace and formatting cleanups.
+
+ Commits that are formatting-only and make the code more compliant
+ with our coding guidelines can just be committed directly. There is
+ no need to take a reviewer's time with them.
+
+* Developer documentation changes.
+
+ If the substance of a development documentation change is agreed on,
+ and it's just a matter of wording, then the change can just be
+ committed directly, since after all, it's easy to improve it later.
+ (For example, the commit that added this section to this document
+ would qualify.)
+
+ Substantive changes to user-facing documentation should, of course,
+ still go through the normal PR process.
+
### Branching and Branch Names

We do all development on lightweight branches, with each branch

[Karl Fogel]

On Monday, August 7, 2017 at 11:16:23 AM UTC-5, Karl Fogel wrote:

I'd like to propose that we do post-commit review for certain kinds of minor, "safe" changes.  Post-commit means the change could be pushed directly to 'master', if the committer wants to and is able to.  People can still review it there, and any problems can be fixed in followup commits.

Well, instead of describing the change I propose to make to CONTRIBUTING.md, how about I just show it to you?  Here it is: [...]

Just FYI, here's some IRC conversation we had about this proposal, before reaching consensus on it:

  <kfogel>     bengal_: Just made a post to PSM-dev that I think you
               might be interested in: "Proposal for post-commit
               review on certain kinds of safe changes."
 
  <jasonaowen> kfogel: I'm not a huge fan of that proposal :/
 
  <jasonaowen> kfogel: I've seen too many "simple fixes" break things
 
  <jasonaowen> kfogel: I guess I'll go say this stuff on the mailing
               list
 
  <kfogel>     jasonaowen: Have you really seen simple fixes of the
               type I describe in that proposal break things?
 
  <kfogel>     jasonaowen: remember, I excepted virtually all real
               code changes, except for typo fixes in strings
 
  <kfogel>     jasonaowen: also, the question is not so much "will it
               ever break things" (although I think in practice it
               almost never will), but how hard are those breakages to
               detect and recover from?
 
  <jasonaowen> kfogel: reformatting carries some risk
 
  <kfogel>     jasonaowen: One doesn't introduce subtle bugs by these
               methods.  One makes the whole app stop working, if one
               really botches a whitespace change, for example.
 
  <jasonaowen> kfogel: "how hard to detect and recover from" -> how do
               would-be reviewers find out about these changes?
 
  <kfogel>     jasonaowen: if there's breakage, that's easy to detect:
               it's broken
 
  <kfogel>     jasonaowen: if there's not breakage, then these changes
               are reviewed when people encounter the new
               documentation (which is really the only part that needs
               post-commit review; I just don't think it's that
               important for reviewers to spend time on looking at
               someone's whitespace change).
 
  <jasonaowen> kfogel: my experience has been fixes of the kind you
               describe get reviewed and merged really quickly
 
  <kfogel>     jasonaowen: note, for example, that my glorious
               comment-style botch of a couple of weeks ago wouldn't
               qualify here.
 
  <jasonaowen> :)
 
  <kfogel>     jasonaowen: there is inevitable overhead to the PR
               process.  I experience it directly; so do we all.  It's
               not huge, but it does slow things down and discourage
               certain kinds of easy changes.
 
  <kfogel>     jasonaowen: I mean, for a typo fix or a whitespace
               change, it will literally multiply the person-time by a
               factor of a few, and the wall-clock time by an
               arbitrary amount.
 
  <jasonaowen> kfogel: sumanah has been doing a great job of fixing
               little things using the PR process
 
  <kfogel>     jasonaowen: Arguing with someone's lived experience is
               tough :-).  sumanah has only had the PR process; the
               ease of direct commit hasn't been available to compare
               to, before now.  Also, how many of those little things
               match the description I gave?  They may be little, but
               most of them probably would still use the PR process.
 
  <kfogel>     jasonaowen: if you mean true typo fixes and formatting
               fixes, my answer is: why on earth do we want to spend
               reviewer's time on those, and impose the task-switching
               overhead on reviewers?
 
  <sumanah>    I could actually merge directly to master if I chose
               to, without going through the PR process
 
  <kfogel>     sumanah: I know; it's the policy change we're talking
               about, though.
 
  <kfogel>     sumanah: That is, I assume the reason you don't do that
               is just policy.
 
  <sumanah>    I prefer to always have someone else review my fixes.
 
  <kfogel>     jasonaowen: (when I said "arguing with someone's lived
               experience is tough", I meant mine, btw)
 
  <kfogel>     sumanah, jasonaowen: No one is ever required to use
               direct commit.
 
  <jasonaowen> kfogel: (sure, that's fair)
 
  <jasonaowen> kfogel: in general, PRs give the opportunity to step
               back - ie, maybe `fix "the the"` is a no-brainer, but
               what does that paragraph even add? can we just delete
               it?
 
  <kfogel>     jasonaowen: Sure, but then we're letting random typo
               locations decide our time prioritization.  It's as bad
               as letting one's inbox decide one's priorities.
 
  <kfogel>     jasonaowen: Like, great, you got a chance to review
               that paragraph and maybe improve it even more.  But
               meanwhile, you task-switched from something else to do
               the review in the first place, and spent time on a
               paragraph that was fine anyway, when other things are
               more important.
 
  <kfogel>     jasonaowen: I.e., I guess I'm saying that if we can't
               be constantly reviewing every line of the code base --
               which obviously we can't -- then the scaled-down
               version of that is my answer to your point above.
 
  <jasonaowen> kfogel: the prioritization angle is an interesting
               point I'll have to think more about
 
  <kfogel>     jasonaowen: FWIW I didn't just make this up for this
               project.  The "obvious fix" rule is a thing in many
               open source projects, and one I've seen work well.
 
  <kfogel>     jasonaowen: In practice, I do not see breakage
               happening from it, and if there is breakage it should
               be the sort that is very quick to recover from (not all
               breakage is created equal, a point which is often lost
               in a hard-line "never ever break master" position).
 
  <jasonaowen> kfogel: but people aren't actively seeking out typos;
               they encounter them in code/docs they're reviewing for
               other reasons. it seems like either others on the team
               are also working in that area, so the task-switching
               overhead is slightly lower, or it's a useful team sync
               opportunity - "why are you looking over there?"
 
  <kfogel>     jasonaowen: that's my point, though.  If I'm working in
               an area, and I see typos or formatting issues, I want
               to fix those *quickly* and resume the real work.  So
               the best way would be 'git stash', fix the trivial
               stuff and push the commit, and then 'git stash pop' (or
               whatever) and continue my real PR-directed work.
 
  <slifty>     jasonaowen: at some point we may want to clean up the
               private key file, but for now I'm OK with just trusting
               travis to delete things (the key is stored in a
               variable anyway)
 
  <jasonaowen> kfogel: yeah, I was definitely raised in a "never ever
               break master" env
 
  <kfogel>     jasonaowen: I was raised in that env too.  And with an
               obvious fix rule.  I truly don't recall master ever
               breaking from one of the obvious fixes.
 
  <kfogel>     jasonaowen: I mean, I could be wrong, since this is
               just from memory over many years, but in practice,
               obvious fixes are safe.  They sometimes get
               followed up to, but not because they broke master, just
               because someone saw the commit and decided to poke
               around and, e.g., clean up similar whitespace problems
               elsewhere, or rewrite the doc, or whatever.
 
  <jasonaowen> kfogel: I recognize that at least some of my response
               is emotional, so I'll be fine with disagreeing and
               committing (ha)
 
  <jasonaowen> kfogel: I haven't worked on a project with that policy
               before; maybe it'll be good
 
  <kfogel>     jasonaowen: Well, I think it'll help people separate
               formatting changes from other changes, b/c the overhead
               of the former will be gone.
 
  <kfogel>     jasonaowen: mind if I follow up to my post with a
               transcript of this conversation?
 
  <jasonaowen> kfogel: I don't think I mentioned any of the other
               places I've worked at by name, so sure
 
  <jasonaowen> kfogel: has it ever been a problem that different
               people disagree on what an obvious fix is?
 
  <kfogel>     jasonaowen: I don't recall.
 
  <kfogel>     jasonaowen: I mean, the rule implies that one must use
               some judgment: if the committer thinks there could be
               disagreement, then she should make a PR.
 
  <kfogel>     jasonaowen: I'll add that to the text, btw.
 
  <jasonaowen> kfogel: +1
 
  <kfogel>     slifty, cdonnelly, sumanah, vasile: if you're all okay
               with the conclusion jasonaowen and I came to above
               (basically, "obvious fixes" as described in my diff can
               be pushed directly to master, but we make it clear that
               one can always use a PR if one wants to or judges that
               there might be some controversy), then I'll commit it.
 
  * slifty reads up
 
  <sumanah>    kfogel: Yes I'm fine with that. We can always revisit
               if it turns out our risk balance went too far in the
               other direction
 
  <kfogel>     sumanah: yup
 
  <cdonnelly>  kfogel: Sure
 
  <cdonnelly>  kfogel: I mean, I *do* like the fact that PRs draw my
               attention to what everyone is working on
 
  <cdonnelly>  kfogel: but I think you and jasonaowen covered that
               above
 
  <slifty>     kfogel: the main thing to keep in mind at this point is
               that once this PR is finished, any push to master
               triggers a build and deployment to a live server that
               the clients will have access to
 
  <sumanah>    Alongside this policy change, I'd like to make it so
               that we get automatic GitHub notifications in this
               channel of all merges to master
 
  <slifty>     kfogel: I suppose that is probably also true in the
               other projects that use the rule you're proposing!
 
  <sumanah>    as a lightweight "here's a thing that just changed"
               heads-up
 
  <cdonnelly>  kfogel: ^^ see sumanah's comments
 
  <kfogel>     slifty: *nod*
 
  <slifty>     kfogel: at the end of the day this policy will increase
               risk for the benefit of faster process; the big
               question on my mind is whether we have been seeing a
               problem with the speed of process
 
  <kfogel>     slifty: we have, imho.
 
  <slifty>     kfogel: TL;DR -- is this proposed change a solution to
               a problem we're facing, or is it a solution to a
               problem we don't have
 
  <kfogel>     slifty: that's why I was motivated to propose this
 
  <slifty>     kfogel: ok cool; in that case I support it!
 
  <kfogel>     slifty: otherwise I wouldn't have bothered
 
  <kfogel>     slifty: thanks
 
  <slifty>     kfogel: :+1:
 
  * slifty rushes to make a whole bunch of commits to master
 
  <cdonnelly>  :)
 
  <cdonnelly>  sumanah: I like the idea of getting notifications of
               merges to master
 
  <cdonnelly>  kfogel: ^^
 
  <kfogel>     sumanah: I'd like that.  Is that easy to set up?  In
               such a way that the merge-to-master event is fairly
               concise?  E.g., even if it includes a bunch of commits,
               they don't all get spewed into channel?
 
  <kfogel>     cdonnelly: ah, was just responding ^^
 
  <kfogel>     sumanah: basically, we want just the merge commit to be
               posted in-channel, and when there's no merge commit,
               then the directly pushed commit(s).
 
  <kfogel>     sumanah: While this is, strictly speaking, well
               defined, it might not be easy to implement.

[Cecilia Donnelly]

One special case of this would be uploading notes files directly to our team-notes directory.  Such notes aren't really the kind of change that the PR process was designed for, and adding them wouldn't affect other parts of the application (so would be a safe change).  Changes could still be made to them after they were initially uploaded, but there's no need to hold them for the length of an entire PR.

We've had some discussion around the fact that even though we want notes to be publicly available, the repository isn't really the ideal location for binary files like .docx notes.  They're relatively large, and it's difficult for git to compress them.  Other places we could store notes are in a linked GitHub wiki or in an issue.

Thoughts welcome,

Cecilia

--
You received this message because you are subscribed to the Google Groups "psm-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to psm-dev+unsubscribe@googlegroups.com.
To post to this group, send email to psm...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/psm-dev/6601768e-23f6-4572-ac60-8a7675439ae2%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Cecilia Donnelly
cdon...@opentechstrategies.com

[James Vasile]

If an obvious fix breaks master, let's reassess. Until then, I'm +1
on an obvious fix rule.

>> email to psm-dev+u...@googlegroups.com.

>> To post to this group, send email to psm...@googlegroups.com.
>> To view this discussion on the web visit
>> https://groups.google.com/d/msgid/psm-dev/6601768e-23f6-4572-ac60-8a7675439ae2%40googlegroups.com.
>>
>> For more options, visit https://groups.google.com/d/optout.
>
>
>
>
> --
> Cecilia Donnelly
> cdon...@opentechstrategies.com
>

> --
> You received this message because you are subscribed to the Google Groups
> "psm-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an

> email to psm-dev+u...@googlegroups.com.

> To post to this group, send email to psm...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/psm-dev/CACfdBSCVSEAQd_eeWy-oQ%3DVr4FwGU-jU2vZ4o7d%2Bq9C__NT43g%40mail.gmail.com.

>
> For more options, visit https://groups.google.com/d/optout.



--

James Vasile
Open Tech Strategies
+1-212-444-2023