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

[Uoti Urpala]

On Mon, 2007-11-12 at 00:07 +0100, Michael Niedermayer wrote:
> 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:
> > > - 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

People committing code without understanding what they are doing is bad.
Avoiding that does not require strict code ownership.

> 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 spirit of the text in svn-howto is wrong too. "You didn't understand
what you were doing" is a valid complaint no matter what file you
changed. "Your commit was correct but you should have asked the
maintainer anyway" is not.

> 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

Not "anything". Things you understand and can be reasonably certain are
correct. If someone displays bad judgment about this then they can be
told to limit their commits to the part of code they're familiar with.
When someone does post a patch because he doesn't feel certain enough to
commit directly it doesn't necessarily have to be the maintainer who
verifies it. Usually a project should have multiple people who, even if
not familiar enough with some parts of code, at least _understand_ when
they're not familiar enough and so can be trusted to approve changes
anywhere in the code.

This does not lead to a mess and has been shown to work in practice.
That you consider strict code ownership to be the obviously best or even
only working system probably reflects your limited knowledge about
development outside MPlayer/FFmpeg. You may have learned to code but
your understanding of development issues in a wider sense is lacking.

> 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 reason I have a problem with that is not "because it's my code".
Certainly you don't have problems with me changing it back because I am
the maintainer of that mplayer.c code and the change was committed
without my permission?

Talking about hypocrisy, what's your rationalization for not adding a
workaround for Cygwin libc but wanting a lot more invasive workarounds
for gcc-2.95?
 
> > > - The parts that are strongly worded against any indentation changes

> 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

I agree in the case of "all else is equal", but the current text in
svn-howto is worded a lot more strongly than that. The quality of the
resulting code should be the primary deciding factor and minimizing
indentation changes etc only secondary.

> 2. cosmetic changes like reindent belong to seperate commits, consequently
> intermediate revisions will be badly indented

Depends on the size of the changes. Sometimes the logic changes can be
easier to see without reindentation but not always - at other times it's
easier to read them if the resulting code is correctly indented. Making
two commits is more work and higher overhead to read on -cvslog even in
the case where you find the isolated parts slightly easier to read.
Having twice as many commits in the revision history is a significant
drawback too.

> 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 ...

Current text in svn-howto says NOT to do that if the reindent required
is over 5 lines (at least in the case where you add an if() over some
code).