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

[Nicolas George]

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.

>   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.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120731/7069304d/attachment.asc>