[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Patch reviewing: style vs. content [was: [PATCH] Issue 1628]

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: 2005-07-26 13:06:21 CEST

(I've fixed the subject line, again. Grr. :-)

Branko Čibej wrote:
>
> Now about this style thing: When I contribute to a project, I always put
> in that extra 1% effort to get the coding style consistent. Project
> policies range from "no consitency" (e.g., Samba) through "reasonable
> consitency" (e.g., APR or HTTPD or Subversion -- for the record,
> Subversion's style is _not_ my natural style, so I have to put in that
> bit of effort when I code for SVN, too) to "Use Emacs with the GNU style
> or else", such as GCC. And yet the effort to keep the style of the
> contribution consistent is never significant, *if* one gets it through
> their head that projects /do/ have a preferred style and that they have
> a reason for that. That reason is usually to make the code more
> maintainable (I think most people will agree that having all the code in
> a consistent style _does_ make it more maintainable).

Note that the discussion of our attitude to style is really about first-time
contributors. You're pretty much right here, except for the estimate of the
amount of effort needed by a first-time contributor to get the style right. I
submit that "1%" and "never significant" is a gross understatement unless
either the contributor happens to be familiar with a style very like ours
already, or the patch design was very difficult (which is uncommon for a first
patch).

When a patch has a few style nits or many gross styling problems, how are we to
know how much effort the contributor has already put in to getting it right?
Maybe they have already spent hours studying the code and the Hacking guide,
putting in more effort than they consider to be worth their while. Or maybe
they were just lazy. Can we tell?

> In this context, the coding style is (almost) as important as giving a
> good explanation of what your patch does and why, providing a good log
> message (again, project requirements for those differ), and of course
> making the actual implementation right.

You're blurring the distinction between the coding style that is submitted by a
contributor and the coding style that is committed to the repository. The
submitted style is much less important _if_ the reviewer/committer is happy to
take on the task of fixing it.

A volunteer reviewer/committer does not have to offer to fix up the style. In
this thread so far we have said that we ought to offer to do so, but if I don't
have the time or desire to do so I could say, "I am now happy with the content
of your patch; I just need somebody to volunteer to fix up the style before I
can commit it. If you are willing to do this, and need guidance, please ask."

> Doing only part of the work and then expecting others to do the rest, as
> I've seen suggested elswhere in this thread, strikes me as being
> slightly rude.

Well, yes, it can be, but whether it is rude depends entirely on the attitude
of the contributor. If he says, "I don't like your style; fix it if you want,"
then it's rude. If she sends exactly the same patch saying, "I'm new, don't
know your style and don't have your editor automation tools, so apologies if
the style is bad; please fix it as I don't have time to learn it and don't
expect to be sending more patches," then it's fair.

> So are remarks about how one doesn't _have_ to write
> patches, implying that every patch is a royal gift that we should grovel

Those remarks should refer only to an unusual class of patches: those that were
relatively difficult to design and write, or required skills that the frequent
contributors do not have. Such a patch often comes from a developer of other
software who has the desire and ability to add something substantial to
Subversion without getting more involved in the project. It is usually obvious
to us when a contribution like that is valuable.

Most newbie patches are very simple and could have been written by anybody.
It's still helpful that they were written by somebody else, but not worth our
while to spend much effort on them unless the contributor is interested in
becoming a Subversion developer. Of course usually we can't know whether they
are interested, so we have to guess and hope and encourage.

> over. Both tend to ignore the fact that the vast majority of the
> contributors to this project are doing it in their free time; I really
> don't see why they should support other people's lazyness.

Again, it depends how much it helps us. It's only "supporting other people's
laziness" if the same contributor expects us to fix their style repeatedly; for
a one-off submission, it's just a way for us to get the fix or enhancement as
efficiently as we can.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jul 26 13:07:05 2005

This is an archived mail posted to the Subversion Dev mailing list.