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

[Christophe de Dinechin]

> On 24 Jul 2017, at 16:06, Marc-André Lureau <marcandre.lureau at redhat.com> wrote:
> 
> 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’.

No, in some repository shared by the team (at large), so that we can, at a glance, look at what has not been reviewed yet.

> This is not how 'distributed' is supposed to work imho. If every contributor should start pushing is branch, and leave it outdated for ages..

The problem of outdated patches is what we have today, see Frediano’s message that started the thread.

> What are the rules about cleaning up those old personal branches? Who should be allowed to push, when etc? no, thanks, use git

How is using git URLs not “using git”?

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

> Then you can use the mailing list to announce it, or to send your patches for review.

Last time I shared a private link as an RFC, you told me this was not the right way to do things. Which is why I am now suggesting we also keep the mail patch.


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

It gets obsolete even faster if you remove the branch I push for illustration seconds after I push it ;-)

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

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

A broken link is useful information. It means “this work has been merged”. As for the burden, it’s totally negligible relative to figuring out today if a specific patch has been merged.

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

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

Some of us disagree.


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

If you are not interested, don’t look at the ‘review’ branches then. Where’s 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.

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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170725/c69f7466/attachment-0001.html>