[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

The review process for new "complicated" stuff (Re: DriverDisc v3 integration)

Sorry Jeremy, but I do not agree with you in this situation.

How do you make sense of 20 or more patches in row implementing a "new" feature? Task patches work well if you have only a few of them formated as task patches. Storage rewrite went the tarball way too at the beginning as I recall (at least as a summary to work with). This also can't be commited or reviewed partially, because it needs the rest to work.

This situation is pretty special, most of the code was removed or abandoned in F11 due to storage rewrite and the underlying codebase changed a lot during the development. Thats also the reason we usually do not have separate devel branches for new stuff, it makes reviewing and merging to master hard to coordinate. Also have in mind, that some of the patches are already merged together because they were written for rhel5 ages ago and only backported. They cannot be considered task patches either.

I could post this as (about) 20 emails where the first one would have parts which aren't present in the final code, because we changed the architecture and algorithms. How do you review that without the big picture and dozens of emails about the concepts? Especially when there are more people reviewing it. Posting the final patch at least helps the people test it. (And note that I described the algorithm and goals in the email)

I'm not saying task patches are worthless, I just think reviewing a new longterm work this way can waste a lot of time, because you will be watching code which was only temporary or wrong and was fixed during the development. If you want, I'll gladly give you a link to git repo with the devel branch for this, I just do not think that it is wise to spam devel list with patches which aren't always task structured, are not standalone and will go into master only together with the rest.

Deadline is not excuse and I asked for consideration only with the cpio work, the rest were valid comments. On the other hand, using something complicated which doesn't fit the needs and designing a wrapper so it does vs. doing it directly in a simple way also doesn't seem much different (cpio is pretty simple format). Mistakes can appear in both... 

And on another note, how do you keep track of unreviewed/unresolved patches? It happened many times to me, that an email laid in the queue for couple of weeks before I managed to get the review. Nobody likes to touch some areas.. and with lots of patches mixed together, the devel list is a mess sometimes (zimbra doesn't help much).


----- "Jeremy Katz" <katzj fedoraproject org> wrote:

> On Wed, Dec 2, 2009 at 8:38 AM, Martin Sivak <msivak redhat com>
> wrote:
> > Sorry no task based patches this time.
> >
> > I needed to post it this way as partners want to apply it and test
> it. I can give you access to the git repo, so you can see the patch
> flow, but managing twenty (or so) email threads + all replies is not
> really comfortable... Especially when there is a lot of backporting,
> prototyping and stub filling going on in the source.
> Task based patches are the *only* way to be able to sanely review and
> integrate patches.  If someone randomly came and dropped a patch like
> this on the list, we'd tell them to please reformat their patches and
> send them correctly.  Why do you for some reason get a pass on that?
> Deadlines are not a compelling reason; if anything, a deadline makes
> it even more important to get them sorted out and posted in an easy
> to
> review format as there's less room for errors.
> - Jeremy
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list redhat com
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]