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

[Dominik 'Rathann' Mierzejewski]

On Tuesday, 13 November 2007 at 16:29, Uoti Urpala wrote:
> On Mon, 2007-11-12 at 15:48 +0100, Dominik 'Rathann' Mierzejewski wrote:
> > On Monday, 12 November 2007 at 14:36, Uoti Urpala wrote:
> > > On Mon, 2007-11-12 at 00:07 +0100, Michael Niedermayer wrote:
> > > > 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.
> 
> As I said above leaving the reindentation out of the first commit does
> not necessarily make it easier to read even in isolation.

And you seem to be the only one with that opinion. While I respect your
right to have a different opinion, I'm quite convinced that you should
respect the opinion of other developers and do things the way we ask you
to do them.

> Also
> completely skipping everything marked "cosmetic" on -cvslog means really
> silly errors slip through easier.

Example? As I said on IRC, purely cosmetic commits can be machine-verified
in most cases.

> > > 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.
> 
> If you say that only a small minority of commits are affected it is an
> argument for splitting/not splitting not mattering much either way. It
> does not affect the relative merits of the alternatives; any benefit
> from splitting would only affect the same "small minority" too.

You're saying you will ignore the rule, because it happens only in a few
cases. I'm saying it shouldn't be a big burden on you to follow the rule
exactly because it concerns only a few cases. Now which one of us is being
unreasonable? You're thinking about your own convenience. I'm thinking
about the convenience of anyone who looks at the commits. Are you saying
other people should adapt to your ways? Shouldn't it be the other way
around?

> > > > 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.
> 
> You misunderstood what the subject was. It was not about splitting
> changes. svn-howto.txt contains bad instructions telling not to fix the
> indentation _at all_.

OK, that point should be replaced with what Michael wrote above.

> IMO one line is too small even as a guideline, and such formatting rules
> generally should not be strict "no exceptions" rules.

IMNSHO, no. Precisely because you'll be arguing that your mixed
functional/cosmetic change is "small" and doesn't have to be split into two
commits.

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