[Ffmpeg-devel] Re: [Ffmpeg-cvslog] r8465 - trunk/libavformat/utils.c

[Uoti Urpala]

On Wed, 2007-03-21 at 14:12 +0100, Baptiste Coudurier wrote:
> Reimar D?ffinger wrote:
> > So in short: there are cases where adding such a thread reference
> > makes sense, but in most cases IMO it is not necessary (no real
> > discussion about the patch) or misplaced (documentation should be in the
> > code, not in an email thread) or the essence of the discussion can be
> > captured in a two-line log message.
> > 
> 
> Well, two lines is often longer than thread title (not mentioning email
> and commit log which are in common).

Longer maybe but a LOT more useful if it contains most of the same
information. Going through mail threads which almost invariably contain
a lot of irrelevant things and using a separate interface for reading
the mails wastes a lot of time compared to having the information in the
log message. I also think length isn't that important even if it takes
more than two lines.

> Many patches are being discussed longer than one reply, thread is very
> useful when you need to understand/remember why it was done that way.
> 
> By experience, I thought many times "thanks Guillaume for mentioning
> thread reference".

But maybe in some of those cases a better log message would have given
most of the information too without needing to go through mail threads?

I think the average commit messages in both FFmpeg and MPlayer aren't
very good and could be improved significantly. Some examples what I
think could be done better from the most recent FFmpeg commits:

8465:
=====
av_estimate_timings_from_pts() flushes the packet queue but doesn't
reset the streams' cur_dts values.  This can lead to a fatal "error,
non monotone timestamps ..." message later, because the out-of-date
cur_dts values are used to compute some packet's dts.

Fix this by calling av_read_frame_flush() and eliminate code
duplication in the process.

The additional hunk gives more detailed error messages.

patch by Wolfram Gloger, wmglo dent.med.uni-muenchen de
====

This seems like a reasonable description of what the commit does, but
there could be a short summary first. Now it's hard to say whether this
might make changes relevant to given other code without reading the
whole text.


8461:
=====
treat frame_size > 1 as compressed audio
=====

This requires at least checking which file(s) the commit changes to make
much sense, and still doesn't tell what kind of practical effect this
change might have. I think it would be better to mention "mov/mp4 muxer"
in the commit message itself and possibly mention which practical case
this improves/fixes.


8452:
=====
i think this is more correct
=====

This basically requires reading the diff and is useless when skimming
through a list of commit messages. At least mention "packet timestamps"
to give some idea what the change does without reading the diff.


Some commit message guidelines which I think make sense:
- Try to describe the change, what practical effects it has, and
possible nontrivial justification for doing things this way (within sane
limits, it's not worth using a lot of extra time to do every commit in a
"perfect" manner).
- For multiline messages include a summary on the first line which
contains enough information to allow people looking for other things to
tell in most cases that this commit does NOT contain anything relevant.
- Don't write the message only to explain your changes to someone who is
already looking at the diff. The message should make sense without
context other than being a log message of this project. People should be
able to check the message and _then_ decide whether they want to see the
diff. Preferably the message should make sense without looking at the
list of files the commit changes either.