[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 12:23:34PM +0200, Christophe de Dinechin wrote:
> > > and there are plenty of public places you can push your work.
> > 
> > So plenty of places, but by no means a shared one? The point is that
> > we need a shared one, to be able to view pending reviews at a glance.
> 
> Note that "we need a way to easily view pending reviews" does not imply
> "this has to be done through review/ branches in a shared repository"
> 

Technically PRs solve the pending reviews without a shared repository,
basically the changes are stored in a web service on top of the git
repository. You can think this as an external shared repository
(repository as storage for stuff, not as a git terminology)
attached to the normal git repository.

> 
> > > If necessary, a cover-letter should say on which commit the series
> > > applies. There are tools like patchew that may also help.
> > 
> > My point is that 1) this is not the case today, and 2) a git link is
> > the best way to summarize the history (faster and more reliable than
> > a cover letter)
> 
> A git branch won't tell you the changes between different versions of
> the same series. You can use -v1, -v2 suffixes, but then you are keeping
> obsolete stuff around. And then, a diff between 2 such branches is not
> the best way of summarizing what changed (we would not need commit logs
> if a diff was expressive enough). And a cover letter usually contains more
> than just history summary, so still useful ;)
> 

I think Christophe D was suggesting the git branch/link as an
addition, not a replacement of the cover letter.

> 
> > >>> 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.

> > > Eventually, the contributor can send a WIP series for some input without
> > > detailed review.
> > 
> > I think you meant “eventually” to mean the same thing as
> > “eventuellement” in French (if necessary or possibly), not in the
> > english sense of “at the end”?
> > 
> > I did that with my early recorder WIP. You apparently did not like
> > that way of proceeding.
> 
> Iirc you sent a link to a git branch without the accompanying patch
> series, and this was what Marc-André asked you not to do.
> 

Yes, this is an issue, you would have to search for the e-mail
containing the link in some way and ping/update that.

> > 
> > > 
> > >>> 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.

Frediano