[MPlayer-cvslog] r32931 - in trunk/gui/skin: skin.c skin.h

[Diego Biurrun]

On Sun, Feb 27, 2011 at 10:44:07AM +0100, Ingo Brückl wrote:
> Diego Biurrun wrote on Sun, 27 Feb 2011 10:00:03 +0100:
> 
> > On Sat, Feb 19, 2011 at 02:23:16PM +0100, ib wrote:
> >>
> >> Log:
> >> Remove unused function.
> 
> > Which unused function?
> 
> Hmm... would it be more descriptive to say "Remove unused function
> trimleft()"?

Yes.

> And what should be a verbose description? "The function has been removed
> since it isn't used anywhere"?

Just the above sentence is enough.

> I really mean this genuinely and don't intend to offence.

No offence taken.

> Oughtn't a log message be read together with the patch?

No, that is the point - it should be possible to read the log message
without the patch.  If the log message cannot be understood without
seeing the diff along with it, then it is bad and should be fixed.

> But if the patch is trivial has there to be a very descriptive message?

Yes.  A descriptive message need not be long.  The example above is
instructive, while

  Remove unused function.

is much too generic - there are many functions in that file and several
may get removed at some point, but

  Remove unused function trimleft().

is short but descriptive enough for such a trivial case.  Just the fact
that a function is unused suffices as reason to remove it, so no further
explanation is required.  It's just that it can be an interesting fact
to see which of the many functions was removed.

You already removed several functions from the GUI if memory serves me
right.  Now imagine that a year from now you want to find the commit
that removed foo() and there are a half dozen log messages that read
"Remove unused function.", maybe you don't even remember exactly which
file the function was in, you will be forced to look through all the
diffs...

Diego