Re: [PATCH] graph.c: make many functions static

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 19, 2008, 19:16 UTC
Message-ID: <7vhcbptev8.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080619082110.6117@nanako3.lavabit.com>

しらいしななこ  <nanako3@lavabit.com> writes:

> These function are not used anywhere.  Also removes graph_release()
> that is never called.
>
> Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>

I CCed Adam, who is the primary author in this area.

Show 18 quoted lines

> ---
>  graph.c |   57 +++++++++++++++++++++++++++++++++++++++++++--------------
>  graph.h |   40 ----------------------------------------
>  2 files changed, 43 insertions(+), 54 deletions(-)
>
> diff --git a/graph.c b/graph.c
> index e2633f8..5f82170 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -4,6 +4,43 @@
>  #include "diff.h"
>  #include "revision.h"
>  
> +/* Internal API */
> + ...
> +static int graph_next_line(struct git_graph *graph, struct strbuf *sb);
> +static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);
> +static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);

I think these are probably fine, not in the sense that nobody calls these functions _right now_ but in the sense that I do not foresee a calling sequence outside the graph.c internal that needs to call these directly, instead of calling graph_show_*() functions that use these.

Show 11 quoted lines

> @@ -180,14 +217,6 @@ struct git_graph *graph_init(struct rev_info *opt)
>  	return graph;
>  }
>  
> -void graph_release(struct git_graph *graph)
> -{
> -	free(graph->columns);
> -	free(graph->new_columns);
> -	free(graph->mapping);
> -	free(graph);
> -}

But I do not think this is right. The current lack of caller of this clean-up function simply means the current users are leaking. I think they are all of "set up rev_info, do a lengthy operation and exit" pattern and clean-up immediately before exit is often omitted as unnecessary, but if we had a clean-up function for the revision API that function would call this one. I'd rather leave this in place, and let libification minded people figure out the cleanest places and ways to make this called.

Other three clean-ups looked Ok to me.  Thanks.