[MPlayer-dev-eng] Indentation changes in patches

[Shachar Raindel]

On Sun, 1 Aug 2004 15:29:19 +0200, Alexander Strasser <eclipse7 at gmx.net> wrote:
> 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.
> 

diff -ub will generate a patch which doesn't contains any external
whitespace changes. Could be useful for this stuff.

> > 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.
> 
How about asking the contributing developers to perform the following
procedure if they need indentation changes:

A. Modify the file as you like, including indentation if needed.
B. Generate a patch without the indentation stuff, using diff -ub
C. Apply this patch to a clean CVS co, making sure it will work apply
correctly. Preferably, test the patched tree to see that nothing
functional was left out.
D. Generate an indentation patch, by running diff -u between the two
CVS trees. The developer should also check the diff -ub between the
trees gives empty results.

IMHO it will have the following positive effects:
I. Much less patches that do not apply to the current CVS will be sent
to the list, as the contributing developer will test it first on his
clean CVS check-out.
II. The mplayer source will remain readable, while still separating
cosmetics from functional changes.
III. All of the parties involved will have to manually edit only the
minimal amount of code, as diff features are used to remove
indentation changes, allowing the developers to focus on coding,
rather than hunting the cosmetics that gets their patch rejected.

    Cheers,
    Shachar