-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Wed, 2 Dec 2009, Martin Sivak wrote:
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.
The idea of task based is so that others can see the major concepts that were
implemented as you worked on these patches. Chances are you worked on pieces
at a time, so those are your tasks (in the context we speak of for patch
review). And git makes it easy for you to group together multiple commits.
Say you committed something and then fixed a typo or renamed something.
Whatever, those can all be smashed down in to one patch because they are still
related to the task worked on. 'git rebase -i' is almost mandatory when
working on anaconda.
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 don't think the driver disk patch set you have is on the order of complexity
like the storage code, but I do agree that it is large. Which is all the more
reason we need to see an organized set of patches to review. Throwing a link
to a tarball to the list is not at all helpful because how do you reply to
that? Replies will have no context. Emailing the patch to the list at least
lets reviewers comment on each other's comments and you retain context about
what the hell they are talking about.
You're bringing back some old code, but I don't see how that's any different
than other features we work on. Create a local branch of master and do your
work there. Fetch and update origin and do 'git merge master' on your local
branch to make sure you are up to date as you work on the DD stuff. Done.
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)
Your commit comments should help guide the reviewer. Also, 'git rebase -i'
lets you reorder patches too so if you do end up with a set where a lower
numbered patch uses something in a higher numbered patch, you can reverse the
patch order.
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.
This is just against the development process we have set up for anaconda now.
We do code review, period. In order for that to be of any value, we all need
to do the following on our patches:
1) Make sure the git comments explain what's going on.
2) Group your changes in to easy to manage patches (the task based idea)
because otherwise the patch is just too much to handle because too many things
are going on. Also, during review the reviewers may say "I like patches 2, 4,
and 8, but the rest need work." Commit those now and work on the others
later.
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...
This is true, but if those mistakes are the responsibility of another
component, we don't have to pull out our hair. While cpio is a simple format,
there's no reason we should get in to the business of owning cpio code. Code
like that will rot and we'll be in the same position in a few years. We
should make use of existing components whenever possible. The days of the
statically linked NIH loader are over.
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).
Personally, I've been using stacked git (stgit) to manage patches. What I
like about it is I can manage revisions to patches during review more easily.
I have my own local workflow for noting when patches are reviewed or not (I
move them to another local branch). Whatever is left in the original branch
is still out for review.
For me, the emails on the list contain useful information but are throw-away
once I process them.
Martin
----- "Jeremy Katz" <katzj@xxxxxxxxxxxxxxxxx> wrote:
On Wed, Dec 2, 2009 at 8:38 AM, Martin Sivak <msivak@xxxxxxxxxx>
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
- --
David Cantrell <dcantrell@xxxxxxxxxx>
Red Hat / Honolulu, HI
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
iEYEARECAAYFAksWiLoACgkQ5hsjjIy1VknNgwCfeFpcLb9Q2qVswHsCStaWawih
fyEAoNvOC/X/9Bk7DRT0h2Pxj6BhsfXf
=AtYw
-----END PGP SIGNATURE-----
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
