[hibernate-dev] Retrospective on "Pull Requests": a waste of time?

Tue Jul 9 15:00:19 EDT 2013

Some random thoughts

There has been a tendency to let PR sit a bit longer than it should as
we all try to get our stuff done before diving into other's PRs. 
I have been particularly guilty and Hibernate OGM is a particularly bad
example. I did not see too much lagging PRs on other projects but I have
to admit that when I'm done with something, I am eagerly refreshing the
PR page to see if someone has picked it up and gave me feedback.
While a bit obsessive compulsive, it also shows a good pattern where
feedback comes shortly (ideally starting to trickle within the hour).

Should we have a team member on watch whose priority for a week is
addressing pull requests?

The average work can be done in small PRs, but long work like the OGM and Search
"metamodel" are more experiments and carving stages. It's hard to split
them into small independent PRs.

I also think opening issues to things that are not fully on the topic is
a good strategy to keep the cycle on a given PR short. That does not
mean one won't work directly on these after the PR is done but at least
it gives equal chance compared to other issues we want to tackle by
being back on the common pool. And I am not discouraging refactoring
things to open the door to more maintainability. Frankly most part of
Hibernate Search's original code base are gone multiple times in
particular the backend but the bootstrap as well as the query part have
been rewritten and evolved as needs appeared.

The scanner issue you mention is actually more common for big PRs where
getting a grasp of such a beast as OGM's options modeing isreally hard.
So you start to bite at it from one angle and slowly learn it. So
smaller PRs should help.

About the preview, I have to disagree with Sanne and Hardy, I do like
them and find them to be the least worse tool to show a preview and get
feedback. I'm sorry but I have done it many times on JIRA and via emails
and the feedback is not the same by far.

When something not directly related to the Pr hitches the original PR
writer, he tends to create isolated commits and apply them before the
core of the PR work is done. Which is a fine approach in my opinion.
When the hitch comes from a reviewer then we have two choices:

1. have the original writer fix it
2. have the reviewer propose a fixing commit atop

So far we have done 1 but I would be fine with experimenting with 2 as
well.

Like many rules, they are meant to be broken and good judgement is highly
preferable, so whatever agreement comes out of this thread that will be
a general direction.

Emmanuel

On Tue 2013-07-09 16:56, Sanne Grinovero wrote:
> I think we all agree that PRs have many merits: it's almost pointless
> to try highlighting them as we're all experienced developers, and I
> guess we all have some horror stories from back in the dark ages when
> we couldn't use them.
> 
> When we moved to git very few of us had some experience with it, but
> those who had did propose a solid workflow; we're using this process
> now since approximately 2 years, and while I think it's been quite
> good, it's time to apply some Retrospective.
> 
> I won't annoy you with the long list of benefits, far more interesting
> to discuss what I'm considering - in my experience - a strong
> drawback:
> 
> ### Delays ###
> 
> Pull requests introduce some delay; sometimes it's short and
> reasonable, sometimes it's huge and well needed, but sometimes it gets
> very long without providing a significant value to the project.
> Actually, since these delays can cost us significant time, there is a
> large opportunity cost and not all of us are always aware of what you
> might be blocking.
> 
> # Context Switch #
> Now even if it introduces a strong delay, often it's not a big problem
> as we can "context switch" to other subjects or even other projects,
> but in practice I'm struggling a lot by this context switch and -
> assuming I'm not the only one hit by this - I think we should avoid it
> as much as possible: context switch is not just energy draining, it's
> also demotivating and has a direct impact on the code quality. That's
> huge: code quality is the core of what we're trying to improve with
> the pull requests process, but in practice it's achieving the
> opposite!
> 
> # Real tangible delays #
> In some cases, a failed review doesn't just force a context switch but
> brings to failed deliveries, or unability to work on the next task. Ok
> this is not common, but it happens.
> 
> # Conflicts #
> This is not a soft-problem nor too hard to handle, but of course the
> more we keep parallel branches the more we have conflicts to resolve.
> Some of you pointed out it's no big deal but I've often volunteered
> for some hard ones... I guess I should change that strategy so you can
> better feel the pain ;-)
> 
> 
> ### Counter-measures ###
> I don't think it's worth cancelling the PRs process as there are great
> benefits, but it's worth exploring how to minimize the undesired
> side-effects.
> I also don't think we can nail down precise "rules" .. I will just ask
> to use common sense and try hard to merge pulls quickly.
> 
> To add some specific directions: I noticed frequently some good points
> are made on a PR which are not strictly a blocker to the pull itself.
> Of course if the tests fail or if it introduces other blockers which
> prevent other developers to continue on their work, it should not be
> merged.. but in most other cases we should try to ask for "follow up"
> improvements rather than "fix now".
> 
> If we can create follow-up improvements, we can easily prioritize
> them. Decoupling the priority of further improvements from the main
> goal of the pull request is important to make sure that we're spending
> time on the highest value instead of entertaining ourselves with
> design puzzlers.. I know the feeling, we all are scientists and enjoy
> some nitty gritty polishing but often there's no real gain from it,
> not even long term readability as I'd rather hope our code to evolve
> faster.. we're not polishing a statue of immutable stone.
> 
> Also, a follow-up improvement can easily get reassigned to someone
> else; for example often the contributor pointing out a possible
> improvement has a better idea of the change he would like to see
> applied, so he could "just code it" and propose it as a change.
> 
> Finally, we could try to *reject* PRs more often. If you think there
> is a blocking problem with the pull, today we would usually ask for
> improvements and wait for the puller to fix, we could as well make it
> very clear when we think that it's a blocker problem by rejecting it..
> I suspect that could have some good effects:
>  - no push-force on the same pull hiding previous dicussions
>  - potentially a different reviewer on the next attempt (we had cases
> on pulls gettting stuck because the single reviewer was away!)
>  - no long lists of dormant pull requests
>   I think this would also encourage people to be quick in merging if
> there is *any* open pull request.
> The problem here is to not take it as an offence, but I think if we
> were all doing it more regularly, - if it becomes a normal process
> rather than an exception - then there would be no problem with it.
> Possibly we would need to explain that nicely to newcomer
> contributors!
> 
> # Avoid pull requests for previews
> This is becoming more frequent but I came to dislike pulls which stay
> open for long (days). I'd prefer to keep the queue empty and try some
> alternatives, like sending pull requests to specific developer's
> rather than to the main project. This way you could "invite" specific
> devs, and they could close the pulls to signal they consider the
> review task complete.. or explore something like that.
> 
> # Avoid showing off scanner capabilities :-)
> I've been very guilty myself on this one; I remember occasionally to
> have attacked a PR with a sense of "let's see how many nitty gritty
> details I can find", then effectively pointing out many typos and
> small inaccuracies but possibly failing to evaluate the grand plan. I
> hope I've learned to "let go" a bit.. for these cases I think it's of
> course great to comment on github but use good judgment to see if it's
> worth delaying the merge. Often I find myself applying minor fixed
> myself, some other times, I expect follow up polishing pulls, or we
> open new issues.
> 
> # When to delay?
> Consider that a sequence of PRs will show up in history as a sequence
> of commits, and it's going to be impossible to figure out from the
> master history if the changes are caused by a single PR or by multiple
> PRs.
> Consider also that when asking a PR improvement, we often stack
> additional commits...
> I really prefer if we all could "fixup" the wrong commits, but in case
> there is no need for it, you could as well merge and ask for further
> improvements.. could be a good rule of thumb?
> 
> Let's fight our love for perfection and bring home some *useful* stuff
> ASAP. (Hint: renaming internal classes is not in this category)
> 
> -- Sanne
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev