[Spice-devel] Proposal: review branches (was Re: [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code)

[Frediano Ziglio]

> 
> On Tue, Jul 25, 2017 at 07:09:22AM -0400, Frediano Ziglio wrote:
> > > 
> > > > >>> I see several benefits to doing this:
> > > > >>> 
> > > > >>> 1. We always know exactly which component and branch is being
> > > > >>> patched
> > > > >>> 
> > > > > 
> > > > > As long as contributor keep pinging or resending his series, this is
> > > > > already the case.
> > > > 
> > > > As Frediano said at the beginning of the series, “I’m tired of hearing
> > > > this
> > > > reply”.
> > > 
> > > And this is not an actionable answer... My perception is that there
> > > rarely are 'ping' on old series. Does this mean we are doing a good job
> > > at reviews? (I doubt it or we would not have this conversation) Does
> > > this mean patch senders do not want to do that? Why? Does this mean it's
> > > done a lot, but to no avail? All I'm reading is "I'm not happy with how
> > > things work", with nothing specific.
> > > 
> > 
> > Patch series are getting old (even years) repeatedly pinged (5/6 times)
> > but they continue to not getting any feedback/ack/comment.
> > If you can't remember any... this just confirms the problem.
> 
> So I went through my mails (searched for mails containing 'ping'), in
> the last months I found 24 series which needed a ping, among these, 3
> needed several pings, and it stopped at 2 pings. Maybe I missed some.
> 

As I said there are series that last years, not months and you
should consider the multiple sending of them too.

> > 
> > > > 
> > > > > 
> > > > >>> 3. If you want to test, a git checkout is enough to test (assuming
> > > > >>> you
> > > > >>> did
> > > > >>> the git fetch above). Simpler IMO than git am (even if I assume
> > > > >>> most of
> > > > >>> us
> > > > >>> have scripts to process incoming mail)
> > > > > 
> > > > > qemu uses patchew, I think it would be worth to consider it for
> > > > > spice as well. It applies the patches on a mailing list, run some
> > > > > tests, gives you a stable URL, tracks the review (and the various
> > > > > iteration recently iirc)…
> > > > 
> > > > Good tool, but different problem.
> > > 
> > > I personally don't know which exact problem we are tackling ;) I see
> > > some people having issues keeping track of pending series, others saying
> > > that CI would be nice, ... Are we trying to solve one single very
> > > specific
> > > problem? If yes, what is it? Patch reviews not being done in a timely
> > > manner? Patch series being forgotten? Patch series status hard to know
> > > by email? Something else? (note that you said "problem", not "problems"
> > > :)
> > > 
> > > Christophe
> > > 
> > 
> > If the review is done months after been sent to the ML the author
> > has to review again the patches, not counting the time he has to
> > spend to update the series and test it again (as the master in the
> > meantime has moved).
> > 
> > Just for instance the png patches were sent on November and
> > are being seriously reviewed this month (so 8 months after).
> > I agree in this case where not pinged that much there were
> > also different discussion on them about.
> > There are a lot of stuff that get lost if the review is done
> > so late. Think about the searches done on the web, the experiments
> > before coming to that version, the commands used which could be
> > helpful again, talks with other people.
> 
> Ok, I take from that email that the main issue is untimely reviews, with
> which I agree. However, what we are currently discussing is how to
> better keep track of pending patch series. If the reason for the
> untimely reviews is that people are not aware that series A and B need
> reviewing, then I agree this is going to help. If the reason for the
> delayed reviews is because we don't have enough reviewers, no tools are
> going to help us here.
> 
> Christophe
> 

I think technically the problem is tracking the series, yes.
patchwork and patchew seems good at spotting them and offer different
features but still you end up with a long list of series with no
status where is easy to loose them.
I don't fully agree such a tool is useless even if the real problem is
few people are able to review patches fast enough. Having the status
of pending work helps choosing the pace of new development and having
a short list of pending series helps people reuse pending patches
if needed.
Spending more time on later reviews is also not helping much too.

Frediano