[MPlayer-dev-eng] Indentation changes in patches

[Attila Kinali]

On Mon, Jul 12, 2004 at 02:28:07PM +0200, Alexander Strasser wrote:
> On Mon, Jul 12, 2004 at 01:54:05PM +0200, Sascha Sommer wrote:
> > My personal opinion on this is that the reindention you described is ok
> > as long as it does not reindent big parts of the code. 10-15 lines or so might
> > be ok. In cvs howto this limit seems to be 5 lines 
> > 
> > <Quote>
> >  NOTE: If you had to put if(){ .. } over a large (> 5 lines) chunk of code,
> >  do NOT change the indentation of the inner part (move it right)!
> > </Quote>
> > 
> The 5 lines limit seems a bit too low to me, too.
> If these are really lot's of lines, say over 15 maybe then it shouldn't
> be indented. But i think it isn't the best solution.
> If the indented code in the is the same as before it could be
> easily recognized by the reviewers...

No, the limit is actualy already quite high. An indentation change
always means that you have to compare each line character by character
which is not that easy as normaly the changed lines are grouped by diff
thus you have to read over severaly lines to see what is changed.

> And otherwise if you make it in two changes both have to be reverted
> anyway. So it is easier to revert only the one patch that contained
> both but therefore a little more work while reviewing the patch.
> I think it isn't good at least for us humans to make it separate.

Normaly you dont need many indentation changes. to quote your example:
	
OLD code:
	dothis()
	this = that;
	goaway( TRUE );

NEW code:
	if ( we_are_not_completely_insane() )
	{
	  dothis()
	  this = that;
	  goaway( TRUE );
	 }

should be rather:
	
OLD code:
	dothis()
	this = that;
	goaway( TRUE );

NEW code:
      if ( we_are_not_completely_insane() )
      {
	dothis()
	this = that;
	goaway( TRUE );
      }

Ie move the if() { } left instead of moving the code to the right.
I know this creates very low indenting levels after a while, but then
you can apply one big reindention at once instead of applying one for
every patch (as has happend with x11_common.c). 

The intention of this rule is to have as few indentation changes as
possible to keep cvs diffs readable w/o problems and having the code
still somewhat readable. Yes, i use here "somewhat" as it is not always
the case and after a while the code gets really obfuscated, but again,
then it's the time to reindent it.

If you are still unsure about this rule, please ask.


			Attila Kinali