> On 24 Jul 2017, at 16:06, Marc-André Lureau <marcandre.lur...@redhat.com> > wrote: > > Hi > > ----- Original Message ----- >>> >>>> On 21 Jul 2017, at 12:41, Frediano Ziglio <fzig...@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.
_______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
