On Mon, 2010-12-20 at 22:28 -0200, Thiago Farina wrote:
> On Mon, Dec 20, 2010 at 2:53 PM, Drew Northup <drew.northup@xxxxxxxxx> wrote:
> >
> > On Mon, 2010-12-20 at 13:17 -0200, Thiago Farina wrote:
> >> On Mon, Dec 20, 2010 at 10:00 AM, Drew Northup <drew.northup@xxxxxxxxx> wrote:
> >> >
> >> > On Thu, 2010-12-16 at 21:43 -0200, Thiago Farina wrote:
> >> >> On Thu, Dec 16, 2010 at 8:38 PM, Vasyl' <vvavrychuk@xxxxxxxxx> wrote:
> >> >> > Signed-off-by: Vasyl' Vavrychuk <vvavrychuk@xxxxxxxxx>
> >> >> > ---
> >> >> > trace.c | 2 +-
> >> >> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >> >> >
> >> >> > diff --git a/trace.c b/trace.c
> >> >> > index 1e560cb..62586fa 100644
> >> >> > --- a/trace.c
> >> >> > +++ b/trace.c
> >> >> > @@ -25,7 +25,7 @@
> >> >> > #include "cache.h"
> >> >> > #include "quote.h"
> >> >> >
> >> >> > -void do_nothing(size_t unused)
> >> >> > +static void do_nothing(size_t unused)
> >> >> > {
> >> >> > }
> >> >> >
> >> >> If it means something, this looks sane to me.
> >> >>
> >> >> Acked-by: Thiago Farina <tfransosi@xxxxxxxxx>
> >> >
> >> > It may be sane, but why should we trust that it is without a commit
> >> > message?
> >>
> >> Why such trivial thing needs further explanation?
> >
> > Because even trivial fixes may break non-trivial things.
> > In addition, without justification we'd just as soon have somebody come
> > back with another patch six months down the road that changes it back to
> > the original code. Now that wouldn't make a whole lot of sense, now
> > would it?
>
> I don't think so, it's making the function private, because the
> function is used only in that file and as such if you see a function
> marked as static you know that and doesn't need further explanation in
> my pov (but it seems you don't think like that).
All the patch above does is tell the compiler to enforce compilation of
that function as static. That is most definitely not the same thing as
making it private. (I think we can agree that it is being used as if it
were being enforced as private at some meta-level, but the compiler
isn't going to enforce it for us...) While I think the code change is
fairly clear, as I said earlier: without a commit message we don't have
a good reason for not making it non-static again later on, flip-flopping
ad-infinitum.
Commit messages for isolated changes such as this build up a story, if
you will, providing future contributors with insight as to why the group
made a change when it did--even when the change is minor (in fact often
most importantly when the change is minor)--by putting it in context.
> > Alas the best way to avoid such a situation is to explain why a change
> > was made to begin with.
> >
>
> So, you are welcome to contribute and suggest such description for
> this trivial (that may break non-trivial things) patch. So we can
> please you and others in the future.
As I am complaining that I don't know what the submitter was thinking
that sounds particularly odd to me. How I am supposed to describe for
the group what the commit's author was thinking in a commit message that
I would like to see added to a patch when in fact the whole problem is
that I don't know specifically what he was thinking? "Changed function
to static for no known reason whatsoever" isn't much of a commit
message.
--
-Drew Northup N1XIM
AKA RvnPhnx on OPN
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html