[MPlayer-cvslog] r24941 - trunk/mplayer.c

[Michael Niedermayer]

Hi

On Sat, Nov 10, 2007 at 07:50:55PM +0100, Diego Biurrun wrote:
> On Fri, Nov 09, 2007 at 12:21:53PM +0200, Uoti Urpala wrote:
> > On Wed, 2007-11-07 at 15:32 -0500, Compn wrote:
> > > can you give us a link or outline your 'good development policy' for us?
> > > maybe there is something mplayer/ffmpeg team can add/remove from its
> > > policy. 
> > 
> > Here are some things that I think could be improved in svn-howto.txt
> > which is probably the closest to policy (though I wouldn't classify all
> > these things as "policy"):
> > - The svn cp hack for reverting doesn't belong in Subversion basics. IMO
> > much of the other text in the "Reverting broken commits" part is
> > questionable too. However in practice this comes up seldom enough that
> > improving the text isn't too important.
> 
> I agree, this does not belong in Subversion basics.

sure, it could be moved somewhere else, but that wont really change the
policy, its spirit, meaning or anything else
was this thread not about the policy being good/bad instead rather than
some paragraphs being in the wrong section?


> 
> > - There are parts implying strict code ownership by maintainers and even
> > an explicit warning against against committing "trivial looking
> > fixes" (wtf?). Those should be replaced by something more flexible.
> 
> This is historic.  I think Arpi added it after I made some silly
> mistake, but I don't remember all the details.

many people made silly mistakes, it all could be described as people
commiting to code they dont maintain and dont fully understand
posting a patch which the maintainer of a specific part of the code
would review and comment on prevents that

so no doubt the policy can be worded better, but the spirit of it is
correct just the way its said is maybe not very clear but this falls more
in the nitpicking category IMHO than the "the policy is bad" one

the commit anything, anywhere system uoti seems to want (if i understood
him correctly) will lead to a mess i think theres no real disagreement here

also uoti qualifies as hypocrit if he on one hand wants to commit to code
he doesnt maintain but OTOH has a problem with people changing "his" (yeah
no strict ownership) code so it compiles with gcc 2.95


> 
> > - The parts that are strongly worded against any indentation changes
> > such as "If you really need to make indentation changes (try to avoid
> > this)" and the requirement to even intentionally leave incorrect
> > indentation in the file if the reindent required would be > 5 lines
> > should be removed. The first priority should be the quality of the
> > resulting code.
> 
> I agree this could or even should be reworded.  However, our policy
> nowadays is to clean up indentation immediately afterwards.  The text
> does not convey this, yes.

well, avoiding unneeded indention changes is certainly desireable, it was
so in the past and still is, as it keeps svn log more readable as well as
keeping svn blame more usefull
of course if an indention change is needed to keep a file properly indented
it should be in a commit seperate from all functional change
that is
1. if you can implement something in several ways choose the one which
minimizes the difference to the previous version if all else is equal
2. cosmetic changes like reindent belong to seperate commits, consequently
intermediate revisions will be badly indented
3. the indention of changed code should be sane after you are finished
with changing it in possibly several commits

i think the policy does say this already ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-cvslog/attachments/20071112/f48a577c/attachment.pgp>