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

[Frediano Ziglio]

> 
> > On 25 Jul 2017, at 19:37, Christophe Fergeau <cfergeau at redhat.com> wrote:
> > 
> > On Tue, Jul 25, 2017 at 02:26:36PM +0200, Christophe de Dinechin wrote:
> >>>>> 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 think that we (but not I) are doing an OK job at reviews, but we
> >> apparently drop
> >> some reviews, e.g. because they were too complex, or did not represent the
> >> priority of the time.
> >> 
> >> That being said, I observe that there are better ways to track WIP than
> >> pure mail.
> >> Redmine, JIRA, pull requests, whatever. All well known solutions to the
> >> problems
> >> we complain about.
> >> 
> >> As an aside, these tools typically solve many other problems too, like
> >> being able to
> >> record things to be done *before* there is a patch for them, or CI, or
> >> priorities, etc.
> >> Frankly, Bugzilla + Mail brings me back a good 15 years ago or something.
> >> 
> >> I don’t care much about which tool we use. I do mind that we have none.
> > 
> > The patch submitter's mind who sends ping when the series gets too old
> > can be seen as such a tool, with the added benefit that they know if the
> > series is still relevant, they can solve complex conflicts when
> > rebasing, ... :)
> > 
> >>> (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.
> >> 
> >> It’s funny, because
> >> a) I never said I was unhappy, and
> >> b) I gave a very simple, very specific suggestion for action, which was to
> >> add a
> >> URL to a branch with the name review/<author>/<topic> on freedesktop.org
> >> to the
> >> cover letter or patch description.
> >> 
> >> So how you turn that into “I’m not happy with how things work with nothing
> >> specific”
> >> is a bit beyond my understanding.
> > 
> > The quote (from you) on top of this part of my answer was
> > « As Frediano said at the beginning of the series, “I’m tired of hearing
> > this reply”. »
> > I was specifically referring to that, which is non-specific, and which I
> > interpret as unhappiness. I'll blame written media for any
> > misinterpretation here :)
> 
> The good thing about written media is that we can go back for the full
> context, namely:
> 
> > 
> >>>> 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”.
> 
> I was simply quoting Frediano to indicate that he had already responded.
> 
> Ping is not a solution, it is a symptom of the problem. The problem being
> forgotten reviews.
> 
> Also, ping is a response for forgotten reviews, how does it help figuring out
> the
> origin repository? Read again what I wrote, that’s clearly what I am talking
> about.
> 
> So I’m sorry, but it looks like you are giving a canned response, “ping”,
> that not
> only ignores Frediano’s frustration with having to ping, but also ignores the
> problem
> I’m bringing up as being also solved by git URLs, namely the difficulty to
> find
> which repository a patch applies to. Hence my reusing the “I’m tired of”
> formulation :-)
> 
> 
> >>> 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"
> >>> :)
> >> 
> >> Starting with the first in your list, but with the full knowledge of the
> >> state of the art
> >> for tools solving this problem solves other problems that we discussed
> >> earlier at the
> >> same time, and listing these problems as well.
> > 
> > Yes, but it's good to know what is the main issue people are having with
> > the current workflow, so that we make sure this really is fixed by any
> > change
> > in tools and processes. The other features would then just be additional
> > niceties.
> 
> I agree.
> 
> Now, any objection to
> 
> 1. Recommending that we use git URLs in patches?

No, I used in the past. Simply "You can find the series at...".
One of the advantage of this is that if people update the branch
(for instance just to keep in sync with master) people can
find the updated patches.

> 2. Having a shared location for branches under review?
> 

Not all people have the right to update these branches, to be
fair with these people you'll have to make it for them which
is expensive and should be the work of a tool. PRs solve this
issue using multiple repositories. Maybe tools like patchwork
or patchew could be extended but this could take a bit of effort.

> 
> Thanks,
> Christophe
> 
> > 
> > Christophe

Frediano