[PATCH] trace.c: mark file-local function static

[PATCH] trace.c: mark file-local function static

[Vasyl']

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

Re: [PATCH] trace.c: mark file-local function static

[Thiago Farina]

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>

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

Re: [PATCH] trace.c: mark file-local function static

[Drew Northup]

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?

-- 
-Drew Northup N1XIM
   AKA RvnPhnx on OPN
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [PATCH] trace.c: mark file-local function static

[Thiago Farina]

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?

Re: [PATCH] trace.c: mark file-local function static

[Drew Northup]

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?
Alas the best way to avoid such a situation is to explain why a change
was made to begin with.

-- 
-Drew Northup N1XIM
   AKA RvnPhnx on OPN
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [PATCH] trace.c: mark file-local function static

[Thiago Farina]

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.


Thanks.

Re: [PATCH] trace.c: mark file-local function static

[Drew Northup]

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

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

Re: [PATCH] trace.c: mark file-local function static

[Nguyen Thai Ngoc Duy]

On Mon, Dec 20, 2010 at 11:53 PM, Drew Northup <drew.northup@maine.edu> wrote:
>> Why such trivial thing needs further explanation?
>
> Because even trivial fixes may break non-trivial things.

The only thing it could break is compilation.

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

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

Re: [PATCH] trace.c: mark file-local function static

[Drew Northup]

On Tue, 2010-12-21 at 21:52 +0700, Nguyen Thai Ngoc Duy wrote:
> On Mon, Dec 20, 2010 at 11:53 PM, Drew Northup <drew.northup@maine.edu> wrote:

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

No, but 1 sentence would be dandy.

-- 
-Drew Northup N1XIM
   AKA RvnPhnx on OPN
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [PATCH] trace.c: mark file-local function static

[Thiago Farina]

On Tue, Dec 21, 2010 at 10:29 AM, Drew Northup <drew.northup@maine.edu> wrote:
> 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.
>

Why you are complain about a one word change at all? Are you the
maintainer? Are you going to make a change that uses do_nothing in
future? If for some reason someone needs to use this function in other
place, the problem is of this person not you, he you will have to
argue and defend his change for whatever reasons he has, and will be a
problem for the maintainer not for you.


> 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.
>
We all know that, and Junio is doing a good job enforcing GOOD commit
messages. But in this case this is STUPID in my pov (others can and
probably will disagree with me, but I don't care).

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

Well, what the hell we are talking about so? If you are arguing for a
commit message, I would expect that you would have a suggestion, at
least to show why you are so interested in this particular patch.

And many contributors here, besides suggesting BETTER commit messages,
they help others with how the patch could be written, and you are not
helping in either way.

Re: [PATCH] trace.c: mark file-local function static

[Junio C Hamano]

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;
 	try_to_free_routine = routine;
 	return old;
 }

Re: [PATCH] trace.c: mark file-local function static

[Thiago Farina]

On Tue, Dec 21, 2010 at 3:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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;

Maybe I'm missing something, or I'm confused (or I do not understand
what I'm reading), but how you are assign routine to do_nothing if you
have removed do_nothing above?

Re: [PATCH] trace.c: mark file-local function static

[Junio C Hamano]

Thiago Farina <tfransosi@gmail.com> writes:

> Maybe I'm missing something, or I'm confused (or I do not understand
> what I'm reading), but how you are assign routine to do_nothing if you
> have removed do_nothing above?

Apply the patch and read the first twenty lines of resulting wrapper.c

Re: [PATCH] trace.c: mark file-local function static

[Johannes Sixt]

On Dienstag, 21. Dezember 2010, Junio C Hamano wrote:
> 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.

Exactly. A clean implementation is not worth the effort for this debugging 
facility.

BTW, these days it is not just Windows that is affected because we use threads 
in start_async everywhere if possible.

-- Hannes