[PATCH] graph.c: make many functions static

[PATCH] graph.c: make many functions static

[しらいしななこ]

These function are not used anywhere.  Also removes graph_release()
that is never called.

Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
---
 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 */
+
+/*
+ * Output the next line for a graph.
+ * This formats the next graph line into the specified strbuf.  It is not
+ * terminated with a newline.
+ *
+ * Returns 1 if the line includes the current commit, and 0 otherwise.
+ * graph_next_line() will return 1 exactly once for each time
+ * graph_update() is called.
+ */
+static int graph_next_line(struct git_graph *graph, struct strbuf *sb);
+
+/*
+ * Output a padding line in the graph.
+ * This is similar to graph_next_line().  However, it is guaranteed to
+ * never print the current commit line.  Instead, if the commit line is
+ * next, it will simply output a line of vertical padding, extending the
+ * branch lines downwards, but leaving them otherwise unchanged.
+ */
+static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);
+
+/*
+ * Print a strbuf to stdout.  If the graph is non-NULL, all lines but the
+ * first will be prefixed with the graph output.
+ *
+ * If the strbuf ends with a newline, the output will end after this
+ * newline.  A new graph line will not be printed after the final newline.
+ * If the strbuf is empty, no output will be printed.
+ *
+ * Since the first line will not include the graph ouput, the caller is
+ * responsible for printing this line's graph (perhaps via
+ * graph_show_commit() or graph_show_oneline()) before calling
+ * graph_show_strbuf().
+ */
+static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);
+
 /*
  * TODO:
  * - Add colors to the graph.
@@ -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);
-}
-
 static void graph_update_state(struct git_graph *graph, enum graph_state s)
 {
 	graph->prev_state = graph->state;
@@ -685,7 +714,7 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
 	strbuf_addch(sb, '*');
 }
 
-void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
+static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 {
 	int seen_this = 0;
 	int i, j;
@@ -760,7 +789,7 @@ void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 		graph_update_state(graph, GRAPH_COLLAPSING);
 }
 
-void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
+static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
 {
 	int seen_this = 0;
 	int i, j;
@@ -801,7 +830,7 @@ void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
 		graph_update_state(graph, GRAPH_COLLAPSING);
 }
 
-void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)
+static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)
 {
 	int i;
 	int *tmp_mapping;
@@ -906,7 +935,7 @@ void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)
 		graph_update_state(graph, GRAPH_PADDING);
 }
 
-int graph_next_line(struct git_graph *graph, struct strbuf *sb)
+static int graph_next_line(struct git_graph *graph, struct strbuf *sb)
 {
 	switch (graph->state) {
 	case GRAPH_PADDING:
@@ -933,7 +962,7 @@ int graph_next_line(struct git_graph *graph, struct strbuf *sb)
 	return 0;
 }
 
-void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
+static void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
 {
 	int i, j;
 
@@ -1055,7 +1084,7 @@ int graph_show_remainder(struct git_graph *graph)
 }
 
 
-void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
+static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
 {
 	char *p;
 
diff --git a/graph.h b/graph.h
index eab4e3d..bc30d68 100644
--- a/graph.h
+++ b/graph.h
@@ -11,11 +11,6 @@ struct git_graph;
 struct git_graph *graph_init(struct rev_info *opt);
 
 /*
- * Destroy a struct git_graph and free associated memory.
- */
-void graph_release(struct git_graph *graph);
-
-/*
  * Update a git_graph with a new commit.
  * This will cause the graph to begin outputting lines for the new commit
  * the next time graph_next_line() is called.
@@ -27,26 +22,6 @@ void graph_release(struct git_graph *graph);
 void graph_update(struct git_graph *graph, struct commit *commit);
 
 /*
- * Output the next line for a graph.
- * This formats the next graph line into the specified strbuf.  It is not
- * terminated with a newline.
- *
- * Returns 1 if the line includes the current commit, and 0 otherwise.
- * graph_next_line() will return 1 exactly once for each time
- * graph_update() is called.
- */
-int graph_next_line(struct git_graph *graph, struct strbuf *sb);
-
-/*
- * Output a padding line in the graph.
- * This is similar to graph_next_line().  However, it is guaranteed to
- * never print the current commit line.  Instead, if the commit line is
- * next, it will simply output a line of vertical padding, extending the
- * branch lines downwards, but leaving them otherwise unchanged.
- */
-void graph_padding_line(struct git_graph *graph, struct strbuf *sb);
-
-/*
  * Determine if a graph has finished outputting lines for the current
  * commit.
  *
@@ -90,21 +65,6 @@ void graph_show_padding(struct git_graph *graph);
 int graph_show_remainder(struct git_graph *graph);
 
 /*
- * Print a strbuf to stdout.  If the graph is non-NULL, all lines but the
- * first will be prefixed with the graph output.
- *
- * If the strbuf ends with a newline, the output will end after this
- * newline.  A new graph line will not be printed after the final newline.
- * If the strbuf is empty, no output will be printed.
- *
- * Since the first line will not include the graph ouput, the caller is
- * responsible for printing this line's graph (perhaps via
- * graph_show_commit() or graph_show_oneline()) before calling
- * graph_show_strbuf().
- */
-void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);
-
-/*
  * Print a commit message strbuf and the remainder of the graph to stdout.
  *
  * This is similar to graph_show_strbuf(), but it always prints the
-- 
1.5.5.4

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

[Junio C Hamano]

しらいしななこ  <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.

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

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

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

[Adam Simpkins]

On Thu, Jun 19, 2008 at 12:16:11PM -0700, Junio C Hamano wrote:
> しらいしななこ  <nanako3@lavabit.com> writes:
> 
> > ---
> >  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.

Documentation/technical/api-history-graph.txt should also be updated
to remove the discussion of these functions if they are no longer
publicly exposed.

-- 
Adam Simpkins
adam@adamsimpkins.net

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

[Junio C Hamano]

Adam Simpkins <adam@adamsimpkins.net> writes:

> On Thu, Jun 19, 2008 at 12:16:11PM -0700, Junio C Hamano wrote:
>> しらいしななこ  <nanako3@lavabit.com> writes:
> ...
>> > +/* 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.
>
> Documentation/technical/api-history-graph.txt should also be updated
> to remove the discussion of these functions if they are no longer
> publicly exposed.

Actually, I was expecting (not necessarily _hoping_) you to defend these
public API by providing examples that illustrates when calling these from
outside graph API implementation could be useful.

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

[Adam Simpkins]

On Fri, Jun 20, 2008 at 12:37:00AM -0700, Junio C Hamano wrote:
> Adam Simpkins <adam@adamsimpkins.net> writes:
> 
> > On Thu, Jun 19, 2008 at 12:16:11PM -0700, Junio C Hamano wrote:
> >> しらいしななこ  <nanako3@lavabit.com> writes:
> > ...
> >> > +/* 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.
> >
> > Documentation/technical/api-history-graph.txt should also be updated
> > to remove the discussion of these functions if they are no longer
> > publicly exposed.
> 
> Actually, I was expecting (not necessarily _hoping_) you to defend these
> public API by providing examples that illustrates when calling these from
> outside graph API implementation could be useful.

Ah.  I see :-)

I do think that graph_next_line() and graph_padding_line() are
potentially useful as public APIs, since they are more generic than
the other API functions that wrap them.  These functions both output
to a strbuf.  graph_show_oneline() and graph_show_padding() are simple
wrappers around these functions that print the resulting strbuf to
stdout.  graph_next_line() and graph_padding_line() will be needed by
anyone who wants to write the graph output somewhere other than
stdout.  They could also be useful if someone wants to further
manipulate the output before printing it.

graph_show_strbuf() is a somewhat similar situation.
graph_show_commit_msg() is a wrapper around graph_show_strbuf()--it
calls graph_show_strbuf() and then also prints the remainder of the
graph.  A caller would need to use graph_show_strbuf() directly if
they have multiple strbufs that need to be displayed alongside the
graph.  In this case, they'll want to call graph_show_strbuf()
multiple times before printing the remainder of the graph.  (The
caller could also work around this by concatenating the strbufs first,
then passing the entire thing to graph_show_commit_msg().  However,
the downside of this approach is that it requires copying the
strbufs.)

I didn't defend these earlier since I wasn't sure if it was simply
git's style to make functions static until there is a proven need for
them to be public.  At the moment, the existing log-tree.c code is
only using the wrappers around these functions.  I don't really see an
immediate need for the core git code to use any of these functions
directly, but they might of interest to people working on libifying
git.

-- 
Adam Simpkins
adam@adamsimpkins.net

[PATCH] graph.c: make many functions static

[Nanako Shiraishi]

These function are not used anywhere.  Also removes graph_release()
that is never called.

Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
---
 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 */
+
+/*
+ * Output the next line for a graph.
+ * This formats the next graph line into the specified strbuf.  It is not
+ * terminated with a newline.
+ *
+ * Returns 1 if the line includes the current commit, and 0 otherwise.
+ * graph_next_line() will return 1 exactly once for each time
+ * graph_update() is called.
+ */
+static int graph_next_line(struct git_graph *graph, struct strbuf *sb);
+
+/*
+ * Output a padding line in the graph.
+ * This is similar to graph_next_line().  However, it is guaranteed to
+ * never print the current commit line.  Instead, if the commit line is
+ * next, it will simply output a line of vertical padding, extending the
+ * branch lines downwards, but leaving them otherwise unchanged.
+ */
+static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);
+
+/*
+ * Print a strbuf to stdout.  If the graph is non-NULL, all lines but the
+ * first will be prefixed with the graph output.
+ *
+ * If the strbuf ends with a newline, the output will end after this
+ * newline.  A new graph line will not be printed after the final newline.
+ * If the strbuf is empty, no output will be printed.
+ *
+ * Since the first line will not include the graph ouput, the caller is
+ * responsible for printing this line's graph (perhaps via
+ * graph_show_commit() or graph_show_oneline()) before calling
+ * graph_show_strbuf().
+ */
+static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);
+
 /*
  * TODO:
  * - Add colors to the graph.
@@ -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);
-}
-
 static void graph_update_state(struct git_graph *graph, enum graph_state s)
 {
 	graph->prev_state = graph->state;
@@ -685,7 +714,7 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
 	strbuf_addch(sb, '*');
 }
 
-void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
+static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 {
 	int seen_this = 0;
 	int i, j;
@@ -760,7 +789,7 @@ void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 		graph_update_state(graph, GRAPH_COLLAPSING);
 }
 
-void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
+static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
 {
 	int seen_this = 0;
 	int i, j;
@@ -801,7 +830,7 @@ void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
 		graph_update_state(graph, GRAPH_COLLAPSING);
 }
 
-void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)
+static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)
 {
 	int i;
 	int *tmp_mapping;
@@ -906,7 +935,7 @@ void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)
 		graph_update_state(graph, GRAPH_PADDING);
 }
 
-int graph_next_line(struct git_graph *graph, struct strbuf *sb)
+static int graph_next_line(struct git_graph *graph, struct strbuf *sb)
 {
 	switch (graph->state) {
 	case GRAPH_PADDING:
@@ -933,7 +962,7 @@ int graph_next_line(struct git_graph *graph, struct strbuf *sb)
 	return 0;
 }
 
-void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
+static void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
 {
 	int i, j;
 
@@ -1055,7 +1084,7 @@ int graph_show_remainder(struct git_graph *graph)
 }
 
 
-void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
+static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
 {
 	char *p;
 
diff --git a/graph.h b/graph.h
index eab4e3d..bc30d68 100644
--- a/graph.h
+++ b/graph.h
@@ -11,11 +11,6 @@ struct git_graph;
 struct git_graph *graph_init(struct rev_info *opt);
 
 /*
- * Destroy a struct git_graph and free associated memory.
- */
-void graph_release(struct git_graph *graph);
-
-/*
  * Update a git_graph with a new commit.
  * This will cause the graph to begin outputting lines for the new commit
  * the next time graph_next_line() is called.
@@ -27,26 +22,6 @@ void graph_release(struct git_graph *graph);
 void graph_update(struct git_graph *graph, struct commit *commit);
 
 /*
- * Output the next line for a graph.
- * This formats the next graph line into the specified strbuf.  It is not
- * terminated with a newline.
- *
- * Returns 1 if the line includes the current commit, and 0 otherwise.
- * graph_next_line() will return 1 exactly once for each time
- * graph_update() is called.
- */
-int graph_next_line(struct git_graph *graph, struct strbuf *sb);
-
-/*
- * Output a padding line in the graph.
- * This is similar to graph_next_line().  However, it is guaranteed to
- * never print the current commit line.  Instead, if the commit line is
- * next, it will simply output a line of vertical padding, extending the
- * branch lines downwards, but leaving them otherwise unchanged.
- */
-void graph_padding_line(struct git_graph *graph, struct strbuf *sb);
-
-/*
  * Determine if a graph has finished outputting lines for the current
  * commit.
  *
@@ -90,21 +65,6 @@ void graph_show_padding(struct git_graph *graph);
 int graph_show_remainder(struct git_graph *graph);
 
 /*
- * Print a strbuf to stdout.  If the graph is non-NULL, all lines but the
- * first will be prefixed with the graph output.
- *
- * If the strbuf ends with a newline, the output will end after this
- * newline.  A new graph line will not be printed after the final newline.
- * If the strbuf is empty, no output will be printed.
- *
- * Since the first line will not include the graph ouput, the caller is
- * responsible for printing this line's graph (perhaps via
- * graph_show_commit() or graph_show_oneline()) before calling
- * graph_show_strbuf().
- */
-void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);
-
-/*
  * Print a commit message strbuf and the remainder of the graph to stdout.
  *
  * This is similar to graph_show_strbuf(), but it always prints the
-- 
1.6.0.2

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/