{"thread":{"id":"14021","subject":"[PATCH] graph.c: make many functions static","startedAt":"2008-06-18T23:21:10Z","lastAt":"2008-06-20T17:19:11Z","messageCount":5,"participants":["しらいしななこ","Junio C Hamano","Adam Simpkins"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"80266","messageId":"20080619082110.6117@nanako3.lavabit.com","threadId":"14021","inReplyTo":null,"subject":"[PATCH] graph.c: make many functions static","fromName":"しらいしななこ","fromEmail":"nanako3@lavabit.com","sentAt":"2008-06-18T23:21:10Z","receivedAt":"2008-06-18T23:21:10Z","isPatch":true,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"These function are not used anywhere.  Also removes graph_release()\nthat is never called.\n\nSigned-off-by: Nanako Shiraishi <nanako3@lavabit.com>\n---\n graph.c |   57 +++++++++++++++++++++++++++++++++++++++++++--------------\n graph.h |   40 ----------------------------------------\n 2 files changed, 43 insertions(+), 54 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex e2633f8..5f82170 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -4,6 +4,43 @@\n #include \"diff.h\"\n #include \"revision.h\"\n \n+/* Internal API */\n+\n+/*\n+ * Output the next line for a graph.\n+ * This formats the next graph line into the specified strbuf.  It is not\n+ * terminated with a newline.\n+ *\n+ * Returns 1 if the line includes the current commit, and 0 otherwise.\n+ * graph_next_line() will return 1 exactly once for each time\n+ * graph_update() is called.\n+ */\n+static int graph_next_line(struct git_graph *graph, struct strbuf *sb);\n+\n+/*\n+ * Output a padding line in the graph.\n+ * This is similar to graph_next_line().  However, it is guaranteed to\n+ * never print the current commit line.  Instead, if the commit line is\n+ * next, it will simply output a line of vertical padding, extending the\n+ * branch lines downwards, but leaving them otherwise unchanged.\n+ */\n+static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);\n+\n+/*\n+ * Print a strbuf to stdout.  If the graph is non-NULL, all lines but the\n+ * first will be prefixed with the graph output.\n+ *\n+ * If the strbuf ends with a newline, the output will end after this\n+ * newline.  A new graph line will not be printed after the final newline.\n+ * If the strbuf is empty, no output will be printed.\n+ *\n+ * Since the first line will not include the graph ouput, the caller is\n+ * responsible for printing this line's graph (perhaps via\n+ * graph_show_commit() or graph_show_oneline()) before calling\n+ * graph_show_strbuf().\n+ */\n+static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);\n+\n /*\n  * TODO:\n  * - Add colors to the graph.\n@@ -180,14 +217,6 @@ struct git_graph *graph_init(struct rev_info *opt)\n \treturn graph;\n }\n \n-void graph_release(struct git_graph *graph)\n-{\n-\tfree(graph->columns);\n-\tfree(graph->new_columns);\n-\tfree(graph->mapping);\n-\tfree(graph);\n-}\n-\n static void graph_update_state(struct git_graph *graph, enum graph_state s)\n {\n \tgraph->prev_state = graph->state;\n@@ -685,7 +714,7 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)\n \tstrbuf_addch(sb, '*');\n }\n \n-void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)\n+static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)\n {\n \tint seen_this = 0;\n \tint i, j;\n@@ -760,7 +789,7 @@ void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)\n \t\tgraph_update_state(graph, GRAPH_COLLAPSING);\n }\n \n-void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)\n+static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)\n {\n \tint seen_this = 0;\n \tint i, j;\n@@ -801,7 +830,7 @@ void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)\n \t\tgraph_update_state(graph, GRAPH_COLLAPSING);\n }\n \n-void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)\n+static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)\n {\n \tint i;\n \tint *tmp_mapping;\n@@ -906,7 +935,7 @@ void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)\n \t\tgraph_update_state(graph, GRAPH_PADDING);\n }\n \n-int graph_next_line(struct git_graph *graph, struct strbuf *sb)\n+static int graph_next_line(struct git_graph *graph, struct strbuf *sb)\n {\n \tswitch (graph->state) {\n \tcase GRAPH_PADDING:\n@@ -933,7 +962,7 @@ int graph_next_line(struct git_graph *graph, struct strbuf *sb)\n \treturn 0;\n }\n \n-void graph_padding_line(struct git_graph *graph, struct strbuf *sb)\n+static void graph_padding_line(struct git_graph *graph, struct strbuf *sb)\n {\n \tint i, j;\n \n@@ -1055,7 +1084,7 @@ int graph_show_remainder(struct git_graph *graph)\n }\n \n \n-void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)\n+static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)\n {\n \tchar *p;\n \ndiff --git a/graph.h b/graph.h\nindex eab4e3d..bc30d68 100644\n--- a/graph.h\n+++ b/graph.h\n@@ -11,11 +11,6 @@ struct git_graph;\n struct git_graph *graph_init(struct rev_info *opt);\n \n /*\n- * Destroy a struct git_graph and free associated memory.\n- */\n-void graph_release(struct git_graph *graph);\n-\n-/*\n  * Update a git_graph with a new commit.\n  * This will cause the graph to begin outputting lines for the new commit\n  * the next time graph_next_line() is called.\n@@ -27,26 +22,6 @@ void graph_release(struct git_graph *graph);\n void graph_update(struct git_graph *graph, struct commit *commit);\n \n /*\n- * Output the next line for a graph.\n- * This formats the next graph line into the specified strbuf.  It is not\n- * terminated with a newline.\n- *\n- * Returns 1 if the line includes the current commit, and 0 otherwise.\n- * graph_next_line() will return 1 exactly once for each time\n- * graph_update() is called.\n- */\n-int graph_next_line(struct git_graph *graph, struct strbuf *sb);\n-\n-/*\n- * Output a padding line in the graph.\n- * This is similar to graph_next_line().  However, it is guaranteed to\n- * never print the current commit line.  Instead, if the commit line is\n- * next, it will simply output a line of vertical padding, extending the\n- * branch lines downwards, but leaving them otherwise unchanged.\n- */\n-void graph_padding_line(struct git_graph *graph, struct strbuf *sb);\n-\n-/*\n  * Determine if a graph has finished outputting lines for the current\n  * commit.\n  *\n@@ -90,21 +65,6 @@ void graph_show_padding(struct git_graph *graph);\n int graph_show_remainder(struct git_graph *graph);\n \n /*\n- * Print a strbuf to stdout.  If the graph is non-NULL, all lines but the\n- * first will be prefixed with the graph output.\n- *\n- * If the strbuf ends with a newline, the output will end after this\n- * newline.  A new graph line will not be printed after the final newline.\n- * If the strbuf is empty, no output will be printed.\n- *\n- * Since the first line will not include the graph ouput, the caller is\n- * responsible for printing this line's graph (perhaps via\n- * graph_show_commit() or graph_show_oneline()) before calling\n- * graph_show_strbuf().\n- */\n-void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);\n-\n-/*\n  * Print a commit message strbuf and the remainder of the graph to stdout.\n  *\n  * This is similar to graph_show_strbuf(), but it always prints the\n-- \n1.5.5.4\n"},{"id":"80356","messageId":"7vhcbptev8.fsf@gitster.siamese.dyndns.org","threadId":"14021","inReplyTo":"20080619082110.6117@nanako3.lavabit.com","subject":"Re: [PATCH] graph.c: make many functions static","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-19T19:16:11Z","receivedAt":"2008-06-19T19:16:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"しらいしななこ  <nanako3@lavabit.com> writes:\n\n> These function are not used anywhere.  Also removes graph_release()\n> that is never called.\n>\n> Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>\n\nI CCed Adam, who is the primary author in this area.\n\n> ---\n>  graph.c |   57 +++++++++++++++++++++++++++++++++++++++++++--------------\n>  graph.h |   40 ----------------------------------------\n>  2 files changed, 43 insertions(+), 54 deletions(-)\n>\n> diff --git a/graph.c b/graph.c\n> index e2633f8..5f82170 100644\n> --- a/graph.c\n> +++ b/graph.c\n> @@ -4,6 +4,43 @@\n>  #include \"diff.h\"\n>  #include \"revision.h\"\n>  \n> +/* Internal API */\n> + ...\n> +static int graph_next_line(struct git_graph *graph, struct strbuf *sb);\n> +static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);\n> +static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);\n\nI think these are probably fine, not in the sense that nobody calls these\nfunctions _right now_ but in the sense that I do not foresee a calling\nsequence outside the graph.c internal that needs to call these directly,\ninstead of calling graph_show_*() functions that use these.\n\n> @@ -180,14 +217,6 @@ struct git_graph *graph_init(struct rev_info *opt)\n>  \treturn graph;\n>  }\n>  \n> -void graph_release(struct git_graph *graph)\n> -{\n> -\tfree(graph->columns);\n> -\tfree(graph->new_columns);\n> -\tfree(graph->mapping);\n> -\tfree(graph);\n> -}\n\n\nBut I do not think this is right.  The current lack of caller of this\nclean-up function simply means the current users are leaking.  I think\nthey are all of \"set up rev_info, do a lengthy operation and exit\" pattern\nand clean-up immediately before exit is often omitted as unnecessary, but\nif we had a clean-up function for the revision API that function would\ncall this one.  I'd rather leave this in place, and let libification\nminded people figure out the cleanest places and ways to make this\ncalled.\n\nOther three clean-ups looked Ok to me.  Thanks.\n"},{"id":"80437","messageId":"20080620060035.GA22345@adamsimpkins.net","threadId":"14021","inReplyTo":"7vhcbptev8.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] graph.c: make many functions static","fromName":"Adam Simpkins","fromEmail":"adam@adamsimpkins.net","sentAt":"2008-06-20T06:00:37Z","receivedAt":"2008-06-20T06:00:37Z","isPatch":true,"sender":{"key":"adam@adamsimpkins.net","avatar":"https://gravatar.com/avatar/d3fd2c0b3e2d2136b56e95726ee03227bee4eb562f627dcd3ebca0623fa05054?d=mp&s=160"},"body":"On Thu, Jun 19, 2008 at 12:16:11PM -0700, Junio C Hamano wrote:\n> しらいしななこ  <nanako3@lavabit.com> writes:\n> \n> > ---\n> >  graph.c |   57 +++++++++++++++++++++++++++++++++++++++++++--------------\n> >  graph.h |   40 ----------------------------------------\n> >  2 files changed, 43 insertions(+), 54 deletions(-)\n> >\n> > diff --git a/graph.c b/graph.c\n> > index e2633f8..5f82170 100644\n> > --- a/graph.c\n> > +++ b/graph.c\n> > @@ -4,6 +4,43 @@\n> >  #include \"diff.h\"\n> >  #include \"revision.h\"\n> >  \n> > +/* Internal API */\n> > + ...\n> > +static int graph_next_line(struct git_graph *graph, struct strbuf *sb);\n> > +static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);\n> > +static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);\n> \n> I think these are probably fine, not in the sense that nobody calls these\n> functions _right now_ but in the sense that I do not foresee a calling\n> sequence outside the graph.c internal that needs to call these directly,\n> instead of calling graph_show_*() functions that use these.\n\nDocumentation/technical/api-history-graph.txt should also be updated\nto remove the discussion of these functions if they are no longer\npublicly exposed.\n\n-- \nAdam Simpkins\nadam@adamsimpkins.net\n"},{"id":"80440","messageId":"7vmylgo8v7.fsf@gitster.siamese.dyndns.org","threadId":"14021","inReplyTo":"20080620060035.GA22345@adamsimpkins.net","subject":"Re: [PATCH] graph.c: make many functions static","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-20T07:37:00Z","receivedAt":"2008-06-20T07:37:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Simpkins <adam@adamsimpkins.net> writes:\n\n> On Thu, Jun 19, 2008 at 12:16:11PM -0700, Junio C Hamano wrote:\n>> しらいしななこ  <nanako3@lavabit.com> writes:\n> ...\n>> > +/* Internal API */\n>> > + ...\n>> > +static int graph_next_line(struct git_graph *graph, struct strbuf *sb);\n>> > +static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);\n>> > +static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);\n>> \n>> I think these are probably fine, not in the sense that nobody calls these\n>> functions _right now_ but in the sense that I do not foresee a calling\n>> sequence outside the graph.c internal that needs to call these directly,\n>> instead of calling graph_show_*() functions that use these.\n>\n> Documentation/technical/api-history-graph.txt should also be updated\n> to remove the discussion of these functions if they are no longer\n> publicly exposed.\n\nActually, I was expecting (not necessarily _hoping_) you to defend these\npublic API by providing examples that illustrates when calling these from\noutside graph API implementation could be useful.\n"},{"id":"80507","messageId":"20080620171910.GA31552@adamsimpkins.net","threadId":"14021","inReplyTo":"7vmylgo8v7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] graph.c: make many functions static","fromName":"Adam Simpkins","fromEmail":"adam@adamsimpkins.net","sentAt":"2008-06-20T17:19:11Z","receivedAt":"2008-06-20T17:19:11Z","isPatch":true,"sender":{"key":"adam@adamsimpkins.net","avatar":"https://gravatar.com/avatar/d3fd2c0b3e2d2136b56e95726ee03227bee4eb562f627dcd3ebca0623fa05054?d=mp&s=160"},"body":"On Fri, Jun 20, 2008 at 12:37:00AM -0700, Junio C Hamano wrote:\n> Adam Simpkins <adam@adamsimpkins.net> writes:\n> \n> > On Thu, Jun 19, 2008 at 12:16:11PM -0700, Junio C Hamano wrote:\n> >> しらいしななこ  <nanako3@lavabit.com> writes:\n> > ...\n> >> > +/* Internal API */\n> >> > + ...\n> >> > +static int graph_next_line(struct git_graph *graph, struct strbuf *sb);\n> >> > +static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);\n> >> > +static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);\n> >> \n> >> I think these are probably fine, not in the sense that nobody calls these\n> >> functions _right now_ but in the sense that I do not foresee a calling\n> >> sequence outside the graph.c internal that needs to call these directly,\n> >> instead of calling graph_show_*() functions that use these.\n> >\n> > Documentation/technical/api-history-graph.txt should also be updated\n> > to remove the discussion of these functions if they are no longer\n> > publicly exposed.\n> \n> Actually, I was expecting (not necessarily _hoping_) you to defend these\n> public API by providing examples that illustrates when calling these from\n> outside graph API implementation could be useful.\n\nAh.  I see :-)\n\nI do think that graph_next_line() and graph_padding_line() are\npotentially useful as public APIs, since they are more generic than\nthe other API functions that wrap them.  These functions both output\nto a strbuf.  graph_show_oneline() and graph_show_padding() are simple\nwrappers around these functions that print the resulting strbuf to\nstdout.  graph_next_line() and graph_padding_line() will be needed by\nanyone who wants to write the graph output somewhere other than\nstdout.  They could also be useful if someone wants to further\nmanipulate the output before printing it.\n\ngraph_show_strbuf() is a somewhat similar situation.\ngraph_show_commit_msg() is a wrapper around graph_show_strbuf()--it\ncalls graph_show_strbuf() and then also prints the remainder of the\ngraph.  A caller would need to use graph_show_strbuf() directly if\nthey have multiple strbufs that need to be displayed alongside the\ngraph.  In this case, they'll want to call graph_show_strbuf()\nmultiple times before printing the remainder of the graph.  (The\ncaller could also work around this by concatenating the strbufs first,\nthen passing the entire thing to graph_show_commit_msg().  However,\nthe downside of this approach is that it requires copying the\nstrbufs.)\n\nI didn't defend these earlier since I wasn't sure if it was simply\ngit's style to make functions static until there is a proven need for\nthem to be public.  At the moment, the existing log-tree.c code is\nonly using the wrappers around these functions.  I don't really see an\nimmediate need for the core git code to use any of these functions\ndirectly, but they might of interest to people working on libifying\ngit.\n\n-- \nAdam Simpkins\nadam@adamsimpkins.net\n"}]}