patchtrace.c: mark file-local function static

> Signed-off-by: Vasyl' Vavrychuk <vvavrychuk@gmail.com>
> ---
>  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)
>  {
>  }
>

> --
> 1.7.1
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

> On Thu, Dec 16, 2010 at 8:38 PM, Vasyl' <vvavrychuk@gmail.com> wrote:
> > Signed-off-by: Vasyl' Vavrychuk <vvavrychuk@gmail.com>
> > ---
> >  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@gmail.com>

>
> On Thu, 2010-12-16 at 21:43 -0200, Thiago Farina wrote:
>> On Thu, Dec 16, 2010 at 8:38 PM, Vasyl' <vvavrychuk@gmail.com> wrote:
>> > Signed-off-by: Vasyl' Vavrychuk <vvavrychuk@gmail.com>
>> > ---
>> >  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@gmail.com>
>
> It may be sane, but why should we trust that it is without a commit
> message?

> On Mon, Dec 20, 2010 at 10:00 AM, Drew Northup <drew.northup@maine.edu> wrote:
> >
> > On Thu, 2010-12-16 at 21:43 -0200, Thiago Farina wrote:
> >> On Thu, Dec 16, 2010 at 8:38 PM, Vasyl' <vvavrychuk@gmail.com> wrote:
> >> > Signed-off-by: Vasyl' Vavrychuk <vvavrychuk@gmail.com>
> >> > ---
> >> >  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@gmail.com>
> >
> > It may be sane, but why should we trust that it is without a commit
> > message?
> 
> Why such trivial thing needs further explanation?

>
> 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@maine.edu> wrote:
>> >
>> > On Thu, 2010-12-16 at 21:43 -0200, Thiago Farina wrote:
>> >> On Thu, Dec 16, 2010 at 8:38 PM, Vasyl' <vvavrychuk@gmail.com> wrote:
>> >> > Signed-off-by: Vasyl' Vavrychuk <vvavrychuk@gmail.com>
>> >> > ---
>> >> >  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@gmail.com>
>> >
>> > 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?

> On Mon, Dec 20, 2010 at 2:53 PM, Drew Northup <drew.northup@maine.edu> 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@maine.edu> wrote:
> >> >
> >> > On Thu, 2010-12-16 at 21:43 -0200, Thiago Farina wrote:
> >> >> On Thu, Dec 16, 2010 at 8:38 PM, Vasyl' <vvavrychuk@gmail.com> wrote:
> >> >> > Signed-off-by: Vasyl' Vavrychuk <vvavrychuk@gmail.com>
> >> >> > ---
> >> >> >  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@gmail.com>
> >> >
> >> > 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).

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

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

> Yeah I would expect somebody reverting this patch _if_ this function
> is useful outside trace.c again. On the other hand, this patch saves
> do_nothing from global namespace so somewhere, some time, somebody can
> use it.
> 
> This is pretty much a clean-up patch from my perspective. Do we really
> need two paragraph explanation for marking a function static?

> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> This is pretty much a clean-up patch from my perspective. Do we really
>> need two paragraph explanation for marking a function static?
>
> I've already applied it, but I think it is much better to do this instead
> (on top of Vasyl' Vavrychuk's patch).
>
> A more interesting topic is why the try-to-free-pack-memory logic needs to
> be disabled in the first place.  3a09425 (Do not call release_pack_memory
> in malloc wrappers when GIT_TRACE is used, 2010-05-08) explains that it is
> to avoid a race on Windows, and it looks like a workaround not a solution
> ("can be called without locking"---"why aren't we locking then?").
>
> Not that it matters in the context of "trace", which is a debugging
> facility, that this is a workaround.
>
> -- >8 --
> Subject: set_try_to_free_routine(NULL) means "do nothing special"
>
> This way, the next caller that wants to disable our memory reclamation
> machinery does not have to define its own do_nothing() stub.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  trace.c   |    8 ++------
>  wrapper.c |    2 ++
>  2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/trace.c b/trace.c
> index 62586fa..0fb2a2c 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -25,10 +25,6 @@
>  #include "cache.h"
>  #include "quote.h"
>
> -static void do_nothing(size_t unused)
> -{
> -}
> -
>  /* Get a trace file descriptor from GIT_TRACE env variable. */
>  static int get_trace_fd(int *need_close)
>  {
> @@ -76,7 +72,7 @@ void trace_printf(const char *fmt, ...)
>        if (!fd)
>                return;
>
> -       set_try_to_free_routine(do_nothing);    /* is never reset */
> +       set_try_to_free_routine(NULL);  /* is never reset */
>        strbuf_init(&buf, 64);
>        va_start(ap, fmt);
>        len = vsnprintf(buf.buf, strbuf_avail(&buf), fmt, ap);
> @@ -108,7 +104,7 @@ void trace_argv_printf(const char **argv, const char *fmt, ...)
>        if (!fd)
>                return;
>
> -       set_try_to_free_routine(do_nothing);    /* is never reset */
> +       set_try_to_free_routine(NULL);  /* is never reset */
>        strbuf_init(&buf, 64);
>        va_start(ap, fmt);
>        len = vsnprintf(buf.buf, strbuf_avail(&buf), fmt, ap);
> diff --git a/wrapper.c b/wrapper.c
> index 4c1639f..8d7dd31 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -12,6 +12,8 @@ static void (*try_to_free_routine)(size_t size) = do_nothing;
>  try_to_free_t set_try_to_free_routine(try_to_free_t routine)
>  {
>        try_to_free_t old = try_to_free_routine;
> +       if (!routine)
> +               routine = do_nothing;

> A more interesting topic is why the try-to-free-pack-memory logic needs to
> be disabled in the first place.  3a09425 (Do not call release_pack_memory
> in malloc wrappers when GIT_TRACE is used, 2010-05-08) explains that it is
> to avoid a race on Windows, and it looks like a workaround not a solution
> ("can be called without locking"---"why aren't we locking then?").
>
> Not that it matters in the context of "trace", which is a debugging
> facility, that this is a workaround.