threads / patch / 14021

patchgraph.c: make many functions static

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

## tl;dr

5 messages between Jun 18, 2008 and Jun 20, 2008. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

しらいしななこ· Jun 18, 2008, 23:21 UTC · lore

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(-)
Show changes to 2 files +43 −54

graph.c, graph.h

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
Junio C Hamano· Jun 19, 2008, 19:16 UTC · re: しらいしななこ · lore

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

しらいしななこ  <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.
Adam Simpkins· Jun 20, 2008, 06:00 UTC · re: Junio C Hamano · lore

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

On Thu, Jun 19, 2008 at 12:16:11PM -0700, Junio C Hamano wrote:
Show 25 quoted lines
> しらいしななこ  <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
Junio C Hamano· Jun 20, 2008, 07:37 UTC · re: Adam Simpkins · lore

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

Adam Simpkins <adam@adamsimpkins.net> writes:
Show 17 quoted lines
> 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.

Adam Simpkins· Jun 20, 2008, 17:19 UTC · re: Junio C Hamano · lore

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

On Fri, Jun 20, 2008 at 12:37:00AM -0700, Junio C Hamano wrote:
Show 23 quoted lines
> 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

← back to recent threads