[MPlayer-dev-eng] Indentation changes in patches

[Sascha Sommer]

On Monday 12 July 2004 12:53, Alexander Strasser wrote:
> My last patch did not get applied as it was, because it ``changed''
> indentation. It got applied in two steps, first functional changes
> and after that the indentation ``changes''. But this is stupid and
> should definitely be changed.
>
> To make it clear here is an example of the indentation ``change'':
>
> OLD code:
> dothis()
> this = that;
> goaway( TRUE );
>
> NEW code:
> if ( we_are_not_completely_insane() )
> {
>   dothis()
>   this = that;
>   goaway( TRUE );
> }
>
> I think this change conforms to the rules in patches.txt and doesn't
> ``change'' indentation. It only corrects it for the modifications.
> Don't get me wrong: i'm against changing indentation out of the range
> of the patch, but her it is needed because else the indentation gets
> broken. And always making to seperate patches is no solution on the
> long run. Also the code is easier to be detected as changed, and it
> has changed in meaning even if the chars are the same.
>
> I the newly indented code should be considered as new patch code and
> the old should be considered as removed cause it anyway gives the code
> a different meaning.
>
> Changes of this type maybe should be rejected as cosmetics if they
> don't indent according to the rest of the file...
> ... but that's another story.
>
> Please comment on what you think about it.
>

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>

Sascha