[FFmpeg-devel] [PATCH 02/19] mmvideo: count preamble size in return value.

[Alexander Strasser]

Nicolas George wrote:
> Le quartidi 14 thermidor, an CCXX, Alexander Strasser a écrit :
> >   Why is that interesting information not in the body of the
> > commit message?
> > 
> >   To make it clear this is no critique of your patch but rather
> > a general concern. I know it is a (big) burden for the programmer
> > to write good commit messages, but the return is hopefully larger
> > than the hassle. As always it is yet another trade-off to be made...
> > 
> >   IMHO in a commit message only 2 things are really required:
> >   1) What you change
> >   2) Why you change it
> > 
> >   Point 1 should be answered directly in the first line (though it
> > may be repeated and extended in the commit message body).
> > 
> >   Point 2 is most of the times in the body as you usually cannot
> > meaningfully encode the answer in one line with about 50 characters
> > while also obeying English grammar.
> > 
> >   Following those guidelines is helpful to both the committer as
> > well as the commit log readers.
> 
> My rule of thumb is to put in the comment information that will be of
> interest only for the reviewer. The reviewer is in-between people who do not
> care about the change and people who care about it.
> 
> People who do not care about the change read "obscure codec: minor fix; will
> not change my life" and do not need extra information. People who care will
> probably already have the file opened just there and only need information
> that is not obvious.
> 
> Reviewers have only the three lines of context that git diff gives. If the
> reason for the change is obvious but four lines above the change, they can
> not see it.
> 
> I admit that in this particular case, the degree of obviousness is near the
> limit.

  I mostly agree. Just that you need to think about people not reviewing
the patch but watching -cvs-log or hunting regressions or developing. They
all need to examine history at some point. It is better they can get the
history from the VCS instead of searching email archives.

> >   To come back to this commit specifically and assuming I understood
> > your patch correctly I would have written something like this:
> > 
> >   mmvideo: decode: Include preamble size in returned count
> > 
> >   MM_PREAMBLE_SIZE is subtracted from buf_size almost immediately. Save
> >   a copy of the original size and return it later.
> 
> I do not mind adding that before pushing.
> 
> >   BTW while writing down that commit message a thought came to my mind.
> > Wouldn't it be better to:
> > 
> >   1) initialize the bytestream right after the (buf_size < MM_PREAMBLE_SIZE) check
> >   2) use the bytestream to peek at the type
> >   3) use the bytestream to skip the preamble
> > 
> >   Peter, any comments from you?
> 
> That would work too, but is beyond the scope of my change.

  Thanks for fixing the issue and improving the commit message! I like
the version without the extra variable better than your original patch.

  Alexander