[MPlayer-dev-eng] Indentation changes in patches

[Alexander Strasser]

On Sun, Aug 01, 2004 at 11:57:09AM +0900, Attila Kinali wrote:
> On Mon, Jul 12, 2004 at 02:28:07PM +0200, Alexander Strasser wrote:
> > 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.
>
This can be done by diff or any similar tool ( you know they can compare
each line character by character :). Means a little more work for the
reviewer as he has to extract the text he wants to compare or run diff
with options to ignore those changes. But these are no pure cosmetical
changes so he isn't misdirected by the patch to code that hasn't changed.
The code changed as it is now executed only when it meets the condition
for example. So you have to take that into account anyway. But I also
understand your viewpoint here.

> 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).
This makes it not much better. Running indent over the code might be
a solution, but i fear nice human indenting will be removed by
automatic indentation.

Maybe it is best to submit two patches one that makes the functional
change and the other that fixes the _broken_ indentation again.
But it's suboptimal imho.
And it should at least be allowed therefore, as you said in your
cvs-howto.txt review.

  Alex (beastd)