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

[Christophe de Dinechin]

> 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, …

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

> 
>> 
>> Thanks,
>> Christophe
>>> 
> 
> --
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170728/f2965c03/attachment-0001.html>