This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: Get rid of superfluous assignments
On Wed, 7 Mar 2012, Thomas Schwinge wrote:
> Hi!
>
> On Wed, 07 Mar 2012 11:40:02 +0100, I wrote:
> > On Wed, 7 Mar 2012 04:12:12 -0500, Ulrich Drepper <drepper@gmail.com> wrote:
> > > Don't use MIME encoding, it makes it hard to apply patches.
> >
> > I'm set up to push commits myself; so if MIME is hard for your mailer to
> > handle, it's enough to just acknowledge such patches.
>
> Ulrich privately asked me to really ``fix'' my mailer -- there is nothing
> wrong with my mailer; sending MIME-encoded data is a normal thing when
> PGP-signing emails. Any moderatly recent MUA is able to handle this just
> fine; no idea what Ulrich is using.
Indeed, and while it's convenient for patch review to have patches
directly in the body of an email, a lot of mailers nowadays seem to have
problems with copying a text file into the body of an email in a way that
preserves whitespace, so unless someone knows their mailer is OK in that
regard it ends up being better to have the patch in an attachment in order
to apply it cleanly.
> He also asks me (and generally ``people'') to not commit patches
> ourselves, because he has to ``fix all kinds of problems'' if people do
> so. I do not think this is appropriate. For example, on contrary, I do
> remember occasions where he committed something else instead of using
> just the patch I provided, and *I* had to (request to) fix up his commit.
Indeed, it's manifestly beneficial and speeds up development to have a
wide range of people contributing and committing directly without needing
to wait for one or two people to act as gatekeepers. And the patch author
is generally going to be best placed to resolve review comments or fix up
merge problems.
We have checklists on the wiki for contributions and commits. If people
forget to follow points on those, we can - *politely* - point this out so
they can do better in future. We have principles that were agreed (see
<http://sourceware.org/ml/libc-alpha/2012-02/msg00638.html> and followups)
regarding keeping clean build and test status. If a patch is committed
that appears to have been untested, this can be politely pointed out (of
course the problem might not have shown up on a particular platform, with
a particular compiler version). We can also make changes that reduce the
risk of mistakes - such as making all files use ranges for copyright years
and updating the lists at the start of the year so "forgotten copyright
update" mistakes simply go away completely.
If someone *persistently* fails to follow the agreed practices despite
having problems pointed out several times, it may of course be that they
are too careless for commit access. But I expect such cases to be very
rare, and committers generally to be careful enough about their commits.
It may also be the case that someone reviewing a patch isn't sure who has
write access. I've just expanded the lists on
<http://sourceware.org/glibc/wiki/MAINTAINERS> so everyone with commit
access is listed under "Blanket commit" or "Write after approval" (in
addition possibly to other maintainership positions). (Quite possibly
some of the people who haven't committed for several years should no
longer be in the glibc group.)
People committing directly isn't what causes problems. Nor is it even
people committing directly without prior review. The real problems I see
are from *people committing directly without explaining what they are
doing*. There have been various cases lately of build breakage caused by
such patches; more caused by such patches than by ones that had been
properly written up and posted to libc-alpha.
Writing up an explanation of the patch you plan to commit, before you
commit it, as if for review, is a good discipline even if they patch
doesn't need someone else to review it; it forces you to reread the patch
and think again about the choices you made and why you made them and
exactly how you tested the patch and can often lead you to realise that
further changes should be made before committing the patch or that a
different implementation approach would be better or that the patch needs
to be split up into smaller pieces. It is also helpful to other
contributors who may need to build on the patch, fix problems it caused or
update ports following the patch.
This may not have mattered so much when only one or two people made many
contributions. But now there are a wide range of contributors making many
contributions, I think we should move to *everyone* following that
practice - *everyone* writing up patches with a proper explanation
(including some indication of how the patch was tested). Some patches may
be committed without prior review - but they should still have the
explanation *written* before the commit (and posted at the same time as
the commit if not before).
--
Joseph S. Myers
joseph@codesourcery.com