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

[Dominik 'Rathann' Mierzejewski]

On Monday, 12 November 2007 at 14:36, Uoti Urpala wrote:
> 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.

Agreed.

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

IMHO it is a matter of courtesy to at least ask the maintainer to review
your changes. You're welcome to threaten to apply within a few days if
there's no response, as is standard practice these days, but you should
ask anyway. For all you know, he may have been working on the same part
of the code and your commit may break his pending changes.

[...]
> > 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".

So what is the reason?

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

But not to the point when cosmetic changes obscure the code changes, as
often happens in your commits.

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

The point (which I already made on IRC a while ago) is that you don't have
to read through cosmetic-only commits. So the read overhead is actually
a lot smaller.

> Having twice as many commits in the revision history is a significant
> drawback too.

How so? Besides, not every code change involves reindentation or other
cosmetic changes. In fact, only a small minority of them do.
Don't exaggerate it.

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

Let's not delve into such detail. IMHO any cosmetic changes over one line
should be separated and no exceptions.

Regards,
R.

-- 
MPlayer developer and RPMs maintainer: http://mplayerhq.hu http://rpm.livna.org
There should be a science of discontent. People need hard times and
oppression to develop psychic muscles.
	-- from "Collected Sayings of Muad'Dib" by the Princess Irulan