[FFmpeg-devel] DVCPRO HD: request for review

[Roman V. Shaposhnik]

On Sat, 2008-08-30 at 00:13 +0200, Michael Niedermayer wrote:
> > > it seems you have missed that this contains a cosmetic change (reindent) that
> > > should be seperate from functional changes
> > 
> > And I missed it deliberately. I can see quite clearly why submitting
> > patches generated with the moral equivalent of "diff -w" is a very
> > desirable thing to do. However, as a matter of established *commit*
> > practice I don't see any value in doing a commit that butchers 
> > established indentation rules be followed by the commit that restores
> > them. Please explain.
> 
> As you say you see the sense in it for a patch, why would that sense not
> apply to svn?

Because -w is for human consumption and commits are machine
transactions. And as far as transactions go, when I add an else
clause to the existing if statement the transaction has both
indentation of the old code and addition of the new one. Now, 
if you're worried about "software archeology" you can always
use diff -w quite easily and have the benefit of both worlds.
Well, at least with the reasonable SCMs you can.

> a odd bug caused by a 170k change is a lot harder to debug with a binary
> search in the history than one caused by a 5k change, simply because there
> is less code that can have caused the bug.

We are not talking about 170k of a diff here. The above statement is
for a specific case of adding an else clause.

> If you agree that vertically aligning this would be better, why dont
> you do it?

Honestly? I don't care one way or the other -- so it is your call. I can
alight it quite easily. That is not a problem.

> yes, 1% overall speed absolutely is more important than 4 or 5 extra
> lines of C code.

I see. Well, that is definitely not how I would approach it, but I'm
here only for as long as we need to integrate the DVCPRO HD patch so
I'll try to do my best. Now, since I'm simply not wired to think
that way -- please bear with me in cases where I might prove to
be quite dense.

> > > > Now I honestly really like you both, and I hope we all can settle these
> > > > problems for the sake of FFmpeg project.
> > > 
> > > /me invites roman to a virtual glass of vodka to discuss what we shall
> > > do now with the 170k in svn.
> > 
> > Accepted. Here's what I can propose:
> >   1. You enumerate all the issues you want me to deal with.
> >   2. The issues have to be reasonable and not the kind of nitpicks
> >      that anybody can find in any code (even yours). For example
> >      the ((s->buf[1]>>2)&0x3) vs. (s->buf[1]&0xC) is a silly 
> >      nitpick. dv100_dct_mode vs. mb->dct_mode was a very legitimate
> >      issue. for(j = 0; j < 2; j++, y_ptr += y_stride) is a tougher
> >      case but I'm willing to compromise for your benefit here.
> >   3. If the number of issues identified (and better yet agreed upon
> >      by at least one extra developer except you e.g. Baptiste) is
> >      small enough (say 1-5) we leave the code be and I quickly
> >      deal with them.
> >   4. If the number is 10+ the code clearly has to be reverted.
> >   5. If the number is 6-9 -- I don't know what to do.
> 
> Iam fine with this, except point 2, that is i want ALL issues we agree
> on fixed no matter how minor you think they are.
> If you disagree about something that is different, but minorness is not
> a excuse to not fix it.

Agreed. Can you, please, enumerate all the issues then? And also, since
you've agreed to leave the code in SVN be, I presume that you no longer
insist on committing individual chunks (the ones you've OKed) and 
you are fine with me simply cleaning the code up in SVN. If I
misunderstood, please reply ASAP so that I can rollback the changes in
SVN. 

Thanks,
Roman.