[MPlayer-dev-eng] Indentation changes in patches

Ivan Kalvachev ivan at cacad.com
Mon Jul 12 13:40:18 CEST 2004


Alexander Strasser said:
> 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.
>
> PS: i think many old commits didn't get handled so strictly!
>
>   Alex (beastd)
>

I think we could define cosmetic change this way:
"change that doesn't modify the way code is executed"

In your case there is a change, so it should not be considered cosmetic.
The way I sow it in the CVS is completly wrong, because it applies patch and
then it applies only cosmetics.

In documentation, as humans execute the code, are allowed more liberal changes:)

Best Regards
   Ivan Kalvachev
  iive




More information about the MPlayer-dev-eng mailing list