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

[Marc-André Lureau]

Hi

----- Original Message -----
> > 
> > > On 21 Jul 2017, at 12:41, Frediano Ziglio <fziglio at redhat.com> wrote:
> > > 
> > >> 
> > >> On Fri, Jul 21, 2017 at 06:18:49AM -0400, Frediano Ziglio wrote:
> > >>> Beside that I wonder why I had to wait 8 months for these reviews,
> > >>> not counting the time to decide to rewrite this part of code
> > >>> (with all the wasted time trying to not do it) and the time we
> > >>> waited to fix a known bug which is also a regression (image copy
> > >>> paste are not working) and potentially a security risk (the library
> > >>> versions used contain security bugs but actually the code is disabled).
> > >>> Looks like that if a Red Hat client is not pushing for a fix
> > >>> solutions are not that important.
> > >> 
> > >> People can be busy with other things, and patches can fall through the
> > >> cracks, it's ok to send a 'ping' message once in a while to bring a
> > >> patch series  back to people's attention.
> > >> 
> > >> Christophe
> > >> 
> > > 
> > > I'm really getting tired of this reply..
> > 
> > Hmmm, maybe we can do a few things to reduce that problem.
> > It would be nice if we had a way to get back to unreviewed patches
> > without having to browse through the mailing list.
> > 
> > Also, as a newbie, I must say I often spend quite some time finding
> > the base for a patch, which is a shame, because that’s a problem
> > that git URLs solve quite nicely. The problem is amplified by the
> > fact that we have multiple components, and patches sometimes
> > don’t really specify which component they apply to.
> > 
> > So maybe we can fix the two problems with one stone. What about
> > 
> > 1. Pushing branches for review under a consistent name, e.g. something
> > like ‘review/c3d/recorder’
> > 

You forgot to say that you mean 'pushing in upstream official repo'. This is not how 'distributed' is supposed to work imho. If every contributor should start pushing is branch, and leave it outdated for ages.. What are the rules about cleaning up those old personal branches? Who should be allowed to push, when etc? no, thanks, use git and there are plenty of public places you can push your work. Then you can use the mailing list to announce it, or to send your patches for review.


> > 2. Adding the git URL in the patch, e.g.
> > https://cgit.freedesktop.org/spice/spice-gtk/log/?h=review/c3d/recorder.
> > 

That you can already do in a mail, although such url tends to become obsolete pretty quickly. patches series are better archived. If necessary, a cover-letter should say on which commit the series applies. There are tools like patchew that may also help. 

> > 3. Once the patch has been committed, simply do a ‘git push freedesktop
> > :review/c3d/recorder’
> > to delete the branch.

This is extra burden, and will leave broken links

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

A series that is no longer being proposed is basically considered abandoned. I think that rule of thumb works fine in practice.

> > 2. When you work on branch, a daily “git fetch” will fetch all the new
> > “review”
> > branches, so a simple git branch -a will show you what needs to be
> > reviewed.

As a developer or as a maintainer, you are not often interested by the various iterations of a work. It's enough to be on CC of a series to review. Eventually, the contributor can send a WIP series for some input without detailed review.

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