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

[Frediano Ziglio]

> > On 28 Jul 2017, at 10:23, Frediano Ziglio < fziglio at redhat.com > wrote:
> 

> > > > On 27 Jul 2017, at 17:08, Frediano Ziglio < fziglio at redhat.com > wrote:
> > > 
> > 
> 

> > > > Try to sum up the initial problem was patches/series tracking
> > > 
> > 
> 

> > > > So far there are 3 proposal
> > > 
> > 
> 
> > > > 1) PR/MR (GitLab/GitHub style)
> > > 
> > 
> 
> > > > 2) patchew
> > > 
> > 
> 
> > > > 3a) shared git repository
> > > 
> > 
> 
> > > > 3b) links to external git repositories
> > > 
> > 
> 

> > > > 1) PR surely can trace the status of series and is ready to
> > > 
> > 
> 
> > > > use with small initial setup. Not clear if we would like
> > > 
> > 
> 
> > > > to do reviews with it;
> > > 
> > 
> 

> > > Not many answers to my survey yet, but so far a consensus that
> > 
> 
> > > patches should be sent to the mailing list, which I interpret as
> > 
> 
> > > an indication that people are comfortable with mail-based reviews.
> > 
> 

> > No, there is IMO a big distinction. If you open a PR and
> 
> > you get a mail with the patch this does not mean you can
> 
> > review using the ML. Reviewing using the ML require you
> 
> > the ability to reply to these emails.
> 

> Oh, I was assuming that if we send PRs as patches to the ML,
> then we also send comments. So commenting on the PR
> would count as a mail. But granted, not the other way round
> (MR reviews would not be visible in the PR, which is OK for me
> but maybe not for everybody).

> > Not saying this is a possible flow to go just that saying
> 
> > that "patches sent to ML" does not mean "ML review”.
> 

> OK. I only said I interpreted this choice as an indication about
> responders being comfortable with ML reviews.

> > > > 2) similar to patchwork with additional feature but missing
> > > 
> > 
> 
> > > > the state tracking part. Maybe would be not hard to add;
> > > 
> > 
> 

> > > To me, addresses a different issue, so I would propose both 1 and 2.
> > 
> 
> > > Specifically, 1 addresses the server side (managing CI items, list of
> > 
> 
> > > branches
> > 
> 
> > > under review, build status, etc), whereas 2 addresses the mail side
> > 
> 
> > > (turning patches into “CI items”).
> > 
> 

> > CI items? CI is usually bound to compilation/testing, not strictly
> 
> > patches/series.
> 

> What I meant is that it looks like patchew will turn a patch into something
> that is individually tested by the CI. Which I called CI item by lack of a
> better term. It could be a PR, it could be a branch, …

Here with PR we intend something really specific related to some
web application (gitlab, github).

> > > > 3a) Many disagree as not really git ideal and about
> > > 
> > 
> 
> > > > external contributions;
> > > 
> > 
> 
> > > > 3b) Does this improve knowing the state of series? Maybe
> > > 
> > 
> 
> > > > for internal developers only.
> > > 
> > 
> 

> > > I think that 1 implies 3, doesn’t it?
> > 
> 

> > 1 implies "external git repositories", not "links to external git
> > repositories”.
> 

> I see.

> > > > 3a/3b seems quite manual job to do and not much solving
> > > 
> > 
> 
> > > > the state tracking (although solve other issues), maybe
> > > 
> > 
> 
> > > > some ideas could improve the current procedure.
> > > 
> > 
> 

> > > > Maybe would be worth speaking with patchew author if
> > > 
> > 
> 
> > > > is easy doable and agree with the change.
> > > 
> > 
> 

> > > > IMHO the "closest" (more suitable and easy to implement)
> > > 
> > 
> 
> > > > is 1.
> > > 
> > 
> 

> > > Agreed. But I think 2 would be a valuable addition, notably as
> > 
> 
> > > an efficient way to deal with smaller patches.
> > 
> 

> > Explain. Why small patches should not have a PR?
> 

> For a small patch that applies trivially on master, that will be acked within
> minutes of being sent to the mailing list, forcing people to create a PR may
> actually slow things down.

> The problem really exists for things that are not on master, are not to be
> merged on master yet (e.g. stream work), or impact multiple repos
> simultaneously.
> For those, we need to find a more robust way to exchange information, and
> PRs are a good way to do it.

> That being said, I would never object to someone systematically creating a
> PR.
> I would just not mind if small, easy patches were only sent to the ML. Even
> less
> so if I know that patchew will make sure we have CI even for those.

> That is only my personal preference, it’s clearly very open for debate ;-)

> Thanks
> Christophe

You are assuming that small patches are reviewed quickly on the
ML. As an example the patch to add seemless mode in Windows
did not had that attention. Even small patches get lost for different
reasons. If one big reason of PR is to avoid loosing patches then the
PR should be the preferred way even for small ones.

If you are worried about more effort for PRs considering the solution
2 could be an option. If patchew is able to create an "item" (actually
I think they call them just "series") and you are able to see the
merge status and change it if needed you hardly will forget a series.

Frediano