threads / patch / 33055

patchRevert "graph.c: mark private file-scope symbols as static"

Subject: [PATCH] Revert "graph.c: mark private file-scope symbols as static"

## tl;dr

14 messages between Mar 2, 2013 and Mar 4, 2013. Diffs are folded; open one to read it.

replies: 13people: 5as markdown or json

John Keeping· Mar 2, 2013, 12:46 UTC · lore
This reverts commit ba35480439d05b8f6cca50527072194fe3278bbb.

CGit uses these symbols to output the correct HTML around graph elements. Making these symbols private means that CGit cannot be updated to use Git 1.8.0 or newer, so let's not do that.

Signed-off-by: John Keeping <john@keeping.me.uk>
---

I realise that Git isn't a library so making the API useful for outside projects isn't a priority, but making these two methods public makes life a lot easier for CGit.

Additionally, it seems that Johan added graph_set_column_colors specifically so that CGit should use it - there's no value to having that as a method just for its use in graph.c and he was the author of CGit commit 268b34a (ui-log: Colorize commit graph, 2010-11-15).

 graph.c | 32 ++------------------------------
 graph.h | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 30 deletions(-)
Show changes to 2 files +29 −30

graph.c, graph.h

diff --git a/graph.c b/graph.c
index 2a3fc5c..b24d04c 100644
--- a/graph.c
+++ b/graph.c
@@ -8,34 +8,6 @@
 /* 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);
-
-/*
- * Set up a custom scheme for column colors.
- *
- * The default column color scheme inserts ANSI color escapes to colorize
- * the graph. The various color escapes are stored in an array of strings
- * where each entry corresponds to a color, except for the last entry,
- * which denotes the escape for resetting the color back to the default.
- * When generating the graph, strings from this array are inserted before
- * and after the various column characters.
- *
- * This function allows you to enable a custom array of color escapes.
- * The 'colors_max' argument is the index of the last "reset" entry.
- *
- * This functions must be called BEFORE graph_init() is called.
- */
-static void graph_set_column_colors(const char **colors, unsigned short colors_max);
-
-/*
  * 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
@@ -90,7 +62,7 @@ enum graph_state {
 static const char **column_colors;
 static unsigned short column_colors_max;
 
-static void graph_set_column_colors(const char **colors, unsigned short colors_max)
+void graph_set_column_colors(const char **colors, unsigned short colors_max)
 {
 	column_colors = colors;
 	column_colors_max = colors_max;
@@ -1144,7 +1116,7 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
 		graph_update_state(graph, GRAPH_PADDING);
 }
 
-static int graph_next_line(struct git_graph *graph, struct strbuf *sb)
+int graph_next_line(struct git_graph *graph, struct strbuf *sb)
 {
 	switch (graph->state) {
 	case GRAPH_PADDING:
diff --git a/graph.h b/graph.h
index 19b0f66..aff960c 100644
--- a/graph.h
+++ b/graph.h
@@ -4,6 +4,22 @@
 /* A graph is a pointer to this opaque structure */
 struct git_graph;
 
+/*
+ * Set up a custom scheme for column colors.
+ *
+ * The default column color scheme inserts ANSI color escapes to colorize
+ * the graph. The various color escapes are stored in an array of strings
+ * where each entry corresponds to a color, except for the last entry,
+ * which denotes the escape for resetting the color back to the default.
+ * When generating the graph, strings from this array are inserted before
+ * and after the various column characters.
+ *
+ * This function allows you to enable a custom array of color escapes.
+ * The 'colors_max' argument is the index of the last "reset" entry.
+ *
+ * This functions must be called BEFORE graph_init() is called.
+ */
+void graph_set_column_colors(const char **colors, unsigned short colors_max);
 
 /*
  * Create a new struct git_graph.
@@ -33,6 +49,17 @@ void graph_update(struct git_graph *graph, struct commit *commit);
  */
 int graph_is_commit_finished(struct git_graph const *graph);
 
+/*
+ * 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);
+
 
 /*
  * graph_show_*: helper functions for printing to stdout
-- 
1.8.2.rc1.339.g93ec2c9
Johan Herland· Mar 2, 2013, 14:54 UTC · re: John Keeping · lore

Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"

On Sat, Mar 2, 2013 at 1:46 PM, John Keeping <john@keeping.me.uk> wrote:
Show 17 quoted lines
> This reverts commit ba35480439d05b8f6cca50527072194fe3278bbb.
>
> CGit uses these symbols to output the correct HTML around graph
> elements.  Making these symbols private means that CGit cannot be
> updated to use Git 1.8.0 or newer, so let's not do that.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>
> I realise that Git isn't a library so making the API useful for outside
> projects isn't a priority, but making these two methods public makes
> life a lot easier for CGit.
>
> Additionally, it seems that Johan added graph_set_column_colors
> specifically so that CGit should use it - there's no value to having
> that as a method just for its use in graph.c and he was the author of
> CGit commit 268b34a (ui-log: Colorize commit graph, 2010-11-15).
Correct,
Acked-by: Johan Herland <johan@herland.net>
Have fun! :)
...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net
Thomas Rast· Mar 2, 2013, 19:16 UTC · re: John Keeping · lore

Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"

John Keeping <john@keeping.me.uk> writes:
Show 17 quoted lines
> This reverts commit ba35480439d05b8f6cca50527072194fe3278bbb.
>
> CGit uses these symbols to output the correct HTML around graph
> elements.  Making these symbols private means that CGit cannot be
> updated to use Git 1.8.0 or newer, so let's not do that.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>
> I realise that Git isn't a library so making the API useful for outside
> projects isn't a priority, but making these two methods public makes
> life a lot easier for CGit.
>
> Additionally, it seems that Johan added graph_set_column_colors
> specifically so that CGit should use it - there's no value to having
> that as a method just for its use in graph.c and he was the author of
> CGit commit 268b34a (ui-log: Colorize commit graph, 2010-11-15).

Perhaps you could add a comment in the source to prevent this from happening again?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
John Keeping· Mar 3, 2013, 10:29 UTC · re: Thomas Rast · lore

Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"

On Sat, Mar 02, 2013 at 08:16:13PM +0100, Thomas Rast wrote:
Show 22 quoted lines
> John Keeping <john@keeping.me.uk> writes:
> 
> > This reverts commit ba35480439d05b8f6cca50527072194fe3278bbb.
> >
> > CGit uses these symbols to output the correct HTML around graph
> > elements.  Making these symbols private means that CGit cannot be
> > updated to use Git 1.8.0 or newer, so let's not do that.
> >
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
> >
> > I realise that Git isn't a library so making the API useful for outside
> > projects isn't a priority, but making these two methods public makes
> > life a lot easier for CGit.
> >
> > Additionally, it seems that Johan added graph_set_column_colors
> > specifically so that CGit should use it - there's no value to having
> > that as a method just for its use in graph.c and he was the author of
> > CGit commit 268b34a (ui-log: Colorize commit graph, 2010-11-15).
> 
> Perhaps you could add a comment in the source to prevent this from
> happening again?

That feels wrong to me; would we really want a list of "$OUTSIDE_PROJECT uses this" against all methods? CGit is using Git's internal API and so should be prepared for breakage and to do what is necessary to work around it - it's just this one case where adding a 2 line function to Git makes CGit's life a lot easier.

I would hope that having this message in the history should be enough to prevent this changing in the future; and it means that the comment is associated with a date so that someone can decide to check whether CGit is still using this function in the distant future.

John
Junio C Hamano· Mar 3, 2013, 21:08 UTC · re: John Keeping · lore

Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"

John Keeping <john@keeping.me.uk> writes:
Show 26 quoted lines
> On Sat, Mar 02, 2013 at 08:16:13PM +0100, Thomas Rast wrote:
>> John Keeping <john@keeping.me.uk> writes:
>> 
>> > This reverts commit ba35480439d05b8f6cca50527072194fe3278bbb.
>> >
>> > CGit uses these symbols to output the correct HTML around graph
>> > elements.  Making these symbols private means that CGit cannot be
>> > updated to use Git 1.8.0 or newer, so let's not do that.
>> >
>> > Signed-off-by: John Keeping <john@keeping.me.uk>
>> > ---
>> >
>> > I realise that Git isn't a library so making the API useful for outside
>> > projects isn't a priority, but making these two methods public makes
>> > life a lot easier for CGit.
>> >
>> > Additionally, it seems that Johan added graph_set_column_colors
>> > specifically so that CGit should use it - there's no value to having
>> > that as a method just for its use in graph.c and he was the author of
>> > CGit commit 268b34a (ui-log: Colorize commit graph, 2010-11-15).
>> 
>> Perhaps you could add a comment in the source to prevent this from
>> happening again?
> ...
> I would hope that having this message in the history should be enough to
> prevent this changing in the future....

Given how it happened in the first place, I do not think anything short of in-code comment would have helped. There wouldn't be any hint to look into the history without one.

John Keeping· Mar 3, 2013, 21:42 UTC · re: Junio C Hamano · lore

Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"

On Sun, Mar 03, 2013 at 01:08:50PM -0800, Junio C Hamano wrote:
Show 32 quoted lines
> John Keeping <john@keeping.me.uk> writes:
> 
> > On Sat, Mar 02, 2013 at 08:16:13PM +0100, Thomas Rast wrote:
> >> John Keeping <john@keeping.me.uk> writes:
> >> 
> >> > This reverts commit ba35480439d05b8f6cca50527072194fe3278bbb.
> >> >
> >> > CGit uses these symbols to output the correct HTML around graph
> >> > elements.  Making these symbols private means that CGit cannot be
> >> > updated to use Git 1.8.0 or newer, so let's not do that.
> >> >
> >> > Signed-off-by: John Keeping <john@keeping.me.uk>
> >> > ---
> >> >
> >> > I realise that Git isn't a library so making the API useful for outside
> >> > projects isn't a priority, but making these two methods public makes
> >> > life a lot easier for CGit.
> >> >
> >> > Additionally, it seems that Johan added graph_set_column_colors
> >> > specifically so that CGit should use it - there's no value to having
> >> > that as a method just for its use in graph.c and he was the author of
> >> > CGit commit 268b34a (ui-log: Colorize commit graph, 2010-11-15).
> >> 
> >> Perhaps you could add a comment in the source to prevent this from
> >> happening again?
> > ...
> > I would hope that having this message in the history should be enough to
> > prevent this changing in the future....
> 
> Given how it happened in the first place, I do not think anything
> short of in-code comment would have helped.  There wouldn't be any
> hint to look into the history without one.
So you'd accept a patch doing that?  Something like this perhaps:
    NOTE: Although these functions aren't used in Git outside graph.c,
    they are used by CGit.
Junio C Hamano· Mar 3, 2013, 22:49 UTC · re: John Keeping · lore

Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"

John Keeping <john@keeping.me.uk> writes:
Show 17 quoted lines
> On Sun, Mar 03, 2013 at 01:08:50PM -0800, Junio C Hamano wrote:
>> >> > Additionally, it seems that Johan added graph_set_column_colors
>> >> > specifically so that CGit should use it - there's no value to having
>> >> > that as a method just for its use in graph.c and he was the author of
>> >> > CGit commit 268b34a (ui-log: Colorize commit graph, 2010-11-15).
>> >> 
>> >> Perhaps you could add a comment in the source to prevent this from
>> >> happening again?
>> > ...
>> > I would hope that having this message in the history should be enough to
>> > prevent this changing in the future....
>> 
>> Given how it happened in the first place, I do not think anything
>> short of in-code comment would have helped.  There wouldn't be any
>> hint to look into the history without one.
>
> So you'd accept a patch doing that?

The answer obviously depends on the specifics of "that" ;-) I was merely agreeing with what Thomas said. A straight-revert would be insufficient to prevent this from recurring again.

> Something like this perhaps:
>
>     NOTE: Although these functions aren't used in Git outside graph.c,
>     they are used by CGit.

It would be a good place to start, although I prefer to see it completed with s/used by CGit/& in order to do such and such/ by somebody working on CGit.

Also it probably is worth adding contact information for folks who work on CGit (http://hjemli.net/git/cgit/ might be sufficient), as changing these functions (e.g. changing the function signature) will affect them; making them "static" is not the only way to hurt them.

Thanks.
John Keeping· Mar 3, 2013, 23:24 UTC · re: Junio C Hamano · lore

Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"

On Sun, Mar 03, 2013 at 02:49:12PM -0800, Junio C Hamano wrote:
Show 32 quoted lines
> John Keeping <john@keeping.me.uk> writes:
> 
> > On Sun, Mar 03, 2013 at 01:08:50PM -0800, Junio C Hamano wrote:
> >> >> > Additionally, it seems that Johan added graph_set_column_colors
> >> >> > specifically so that CGit should use it - there's no value to having
> >> >> > that as a method just for its use in graph.c and he was the author of
> >> >> > CGit commit 268b34a (ui-log: Colorize commit graph, 2010-11-15).
> >> >> 
> >> >> Perhaps you could add a comment in the source to prevent this from
> >> >> happening again?
> >> > ...
> >> > I would hope that having this message in the history should be enough to
> >> > prevent this changing in the future....
> >> 
> >> Given how it happened in the first place, I do not think anything
> >> short of in-code comment would have helped.  There wouldn't be any
> >> hint to look into the history without one.
> >
> > So you'd accept a patch doing that?
> 
> The answer obviously depends on the specifics of "that" ;-) I was
> merely agreeing with what Thomas said.  A straight-revert would be
> insufficient to prevent this from recurring again.
> 
> > Something like this perhaps:
> >
> >     NOTE: Although these functions aren't used in Git outside graph.c,
> >     they are used by CGit.
> 
> It would be a good place to start, although I prefer to see it
> completed with s/used by CGit/& in order to do such and such/ by
> somebody working on CGit.
CGit uses graph_set_column_colors() to set the column colors to:
    "<span class='column1'>",
    ...
    "<span class='column6'>",
    "</span>"

(the last value is RESET), thus avoiding the need to filter the output to convert ANSI colours to HTML.

Similarly, it accesses graph_next_line() directly so that it can output the necessary HTML around each line.

> Also it probably is worth adding contact information for folks who
> work on CGit (http://hjemli.net/git/cgit/ might be sufficient),
The current CGit homepage is http://git.zx2c4.com/cgit/
>                                                                 as
> changing these functions (e.g. changing the function signature) will
> affect them; making them "static" is not the only way to hurt them.

But since CGit uses a specific version of Git (as a submodule), in general it doesn't need to worry about keeping consistency across different versions. CGit has been using Git 1.7.6 for quite a while - I posted a series of patches to take it to 1.7.12 yesterday [1] but hit this issue when I got to 1.8.x.

I think CGit expects to have to respond to changes in Git, so I don't think it's worth restricting changes in Git for that reason - it's just a case of exposing useful functionality somehow.

[1] http://hjemli.net/pipermail/cgit/2013-March/000933.html
Junio C Hamano· Mar 3, 2013, 23:32 UTC · re: John Keeping · lore

Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"

John Keeping <john@keeping.me.uk> writes:
>> Also it probably is worth adding contact information for folks who
>> work on CGit (http://hjemli.net/git/cgit/ might be sufficient),
>
> The current CGit homepage is http://git.zx2c4.com/cgit/

As the hjemli.net address is what I got as the first hit by asking [CGit] to websearch, it makes it clearer that we do need such contact information there.

> I think CGit expects to have to respond to changes in Git, so I don't
> think it's worth restricting changes in Git for that reason - it's just
> a case of exposing useful functionality somehow.
I think you misread me.

It is not about restricting. It is to use their expertise to come up with generally more useful API than responding only to the immediate need we see in in-tree users. We want a discussion/patch to update the graph.c infrastructure to be Cc'ed to them.

For example, with the current codeflow, the callers of these functions we have in-tree may be limited to those in graph.c but if there are legitimate reason CGit wants to call them from sideways, perhaps there may be use cases in our codebase in the future to call them from outside the normal graph.c codeflow.

John Keeping· Mar 4, 2013, 00:03 UTC · re: Junio C Hamano · lore

[PATCH v2] Revert "graph.c: mark private file-scope symbols as static"

This reverts commit ba35480439d05b8f6cca50527072194fe3278bbb.

CGit uses these symbols to output the correct HTML around graph elements. Making these symbols private means that CGit cannot be updated to use Git 1.8.0 or newer, so let's not do that.

On top of the revert, also add comments so that we avoid reintroducing this problem in the future and suggest to those modifying this API that they might want to discuss it with the CGit developers.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Sun, Mar 03, 2013 at 03:32:08PM -0800, Junio C Hamano wrote:
Show 6 quoted lines
> John Keeping <john@keeping.me.uk> writes:
> > I think CGit expects to have to respond to changes in Git, so I don't
> > think it's worth restricting changes in Git for that reason - it's just
> > a case of exposing useful functionality somehow.
> 
> I think you misread me.

You're right - this makes more sense that what I originally thought you meant :-)

Show 10 quoted lines
> It is not about restricting. It is to use their expertise to come up
> with generally more useful API than responding only to the immediate
> need we see in in-tree users. We want a discussion/patch to update
> the graph.c infrastructure to be Cc'ed to them.
> 
> For example, with the current codeflow, the callers of these
> functions we have in-tree may be limited to those in graph.c but if
> there are legitimate reason CGit wants to call them from sideways,
> perhaps there may be use cases in our codebase in the future to call
> them from outside the normal graph.c codeflow.
 graph.c | 32 ++------------------------------
 graph.h | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 30 deletions(-)
Show changes to 2 files +35 −30

graph.c, graph.h

diff --git a/graph.c b/graph.c
index 2a3fc5c..b24d04c 100644
--- a/graph.c
+++ b/graph.c
@@ -8,34 +8,6 @@
 /* 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);
-
-/*
- * Set up a custom scheme for column colors.
- *
- * The default column color scheme inserts ANSI color escapes to colorize
- * the graph. The various color escapes are stored in an array of strings
- * where each entry corresponds to a color, except for the last entry,
- * which denotes the escape for resetting the color back to the default.
- * When generating the graph, strings from this array are inserted before
- * and after the various column characters.
- *
- * This function allows you to enable a custom array of color escapes.
- * The 'colors_max' argument is the index of the last "reset" entry.
- *
- * This functions must be called BEFORE graph_init() is called.
- */
-static void graph_set_column_colors(const char **colors, unsigned short colors_max);
-
-/*
  * 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
@@ -90,7 +62,7 @@ enum graph_state {
 static const char **column_colors;
 static unsigned short column_colors_max;
 
-static void graph_set_column_colors(const char **colors, unsigned short colors_max)
+void graph_set_column_colors(const char **colors, unsigned short colors_max)
 {
 	column_colors = colors;
 	column_colors_max = colors_max;
@@ -1144,7 +1116,7 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
 		graph_update_state(graph, GRAPH_PADDING);
 }
 
-static int graph_next_line(struct git_graph *graph, struct strbuf *sb)
+int graph_next_line(struct git_graph *graph, struct strbuf *sb)
 {
 	switch (graph->state) {
 	case GRAPH_PADDING:
diff --git a/graph.h b/graph.h
index 19b0f66..0be62bd 100644
--- a/graph.h
+++ b/graph.h
@@ -4,6 +4,25 @@
 /* A graph is a pointer to this opaque structure */
 struct git_graph;
 
+/*
+ * Set up a custom scheme for column colors.
+ *
+ * The default column color scheme inserts ANSI color escapes to colorize
+ * the graph. The various color escapes are stored in an array of strings
+ * where each entry corresponds to a color, except for the last entry,
+ * which denotes the escape for resetting the color back to the default.
+ * When generating the graph, strings from this array are inserted before
+ * and after the various column characters.
+ *
+ * This function allows you to enable a custom array of color escapes.
+ * The 'colors_max' argument is the index of the last "reset" entry.
+ *
+ * This functions must be called BEFORE graph_init() is called.
+ *
+ * NOTE: This function isn't used in Git outside graph.c but it is used
+ * by CGit (http://git.zx2c4.com/cgit/) to use HTML for colors.
+ */
+void graph_set_column_colors(const char **colors, unsigned short colors_max);
 
 /*
  * Create a new struct git_graph.
@@ -33,6 +52,20 @@ void graph_update(struct git_graph *graph, struct commit *commit);
  */
 int graph_is_commit_finished(struct git_graph const *graph);
 
+/*
+ * 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.
+ *
+ * NOTE: This function isn't used in Git outside graph.c but it is used
+ * by CGit (http://git.zx2c4.com/cgit/) to wrap HTML around graph lines.
+ */
+int graph_next_line(struct git_graph *graph, struct strbuf *sb);
+
 
 /*
  * graph_show_*: helper functions for printing to stdout
-- 
1.8.2.rc1.339.g93ec2c9
Jason A. Donenfeld· Mar 4, 2013, 00:12 UTC · re: John Keeping · lore

Re: [PATCH v2] Revert "graph.c: mark private file-scope symbols as static"

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
On Sun, Mar 3, 2013 at 7:03 PM, John Keeping <john@keeping.me.uk> wrote:
Show 149 quoted lines
> This reverts commit ba35480439d05b8f6cca50527072194fe3278bbb.
>
> CGit uses these symbols to output the correct HTML around graph
> elements.  Making these symbols private means that CGit cannot be
> updated to use Git 1.8.0 or newer, so let's not do that.
>
> On top of the revert, also add comments so that we avoid reintroducing
> this problem in the future and suggest to those modifying this API
> that they might want to discuss it with the CGit developers.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
> On Sun, Mar 03, 2013 at 03:32:08PM -0800, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>> > I think CGit expects to have to respond to changes in Git, so I don't
>> > think it's worth restricting changes in Git for that reason - it's just
>> > a case of exposing useful functionality somehow.
>>
>> I think you misread me.
>
> You're right - this makes more sense that what I originally thought you
> meant :-)
>
>> It is not about restricting. It is to use their expertise to come up
>> with generally more useful API than responding only to the immediate
>> need we see in in-tree users. We want a discussion/patch to update
>> the graph.c infrastructure to be Cc'ed to them.
>>
>> For example, with the current codeflow, the callers of these
>> functions we have in-tree may be limited to those in graph.c but if
>> there are legitimate reason CGit wants to call them from sideways,
>> perhaps there may be use cases in our codebase in the future to call
>> them from outside the normal graph.c codeflow.
>
>  graph.c | 32 ++------------------------------
>  graph.h | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 30 deletions(-)
>
> diff --git a/graph.c b/graph.c
> index 2a3fc5c..b24d04c 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -8,34 +8,6 @@
>  /* 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);
> -
> -/*
> - * Set up a custom scheme for column colors.
> - *
> - * The default column color scheme inserts ANSI color escapes to colorize
> - * the graph. The various color escapes are stored in an array of strings
> - * where each entry corresponds to a color, except for the last entry,
> - * which denotes the escape for resetting the color back to the default.
> - * When generating the graph, strings from this array are inserted before
> - * and after the various column characters.
> - *
> - * This function allows you to enable a custom array of color escapes.
> - * The 'colors_max' argument is the index of the last "reset" entry.
> - *
> - * This functions must be called BEFORE graph_init() is called.
> - */
> -static void graph_set_column_colors(const char **colors, unsigned short colors_max);
> -
> -/*
>   * 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
> @@ -90,7 +62,7 @@ enum graph_state {
>  static const char **column_colors;
>  static unsigned short column_colors_max;
>
> -static void graph_set_column_colors(const char **colors, unsigned short colors_max)
> +void graph_set_column_colors(const char **colors, unsigned short colors_max)
>  {
>         column_colors = colors;
>         column_colors_max = colors_max;
> @@ -1144,7 +1116,7 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
>                 graph_update_state(graph, GRAPH_PADDING);
>  }
>
> -static int graph_next_line(struct git_graph *graph, struct strbuf *sb)
> +int graph_next_line(struct git_graph *graph, struct strbuf *sb)
>  {
>         switch (graph->state) {
>         case GRAPH_PADDING:
> diff --git a/graph.h b/graph.h
> index 19b0f66..0be62bd 100644
> --- a/graph.h
> +++ b/graph.h
> @@ -4,6 +4,25 @@
>  /* A graph is a pointer to this opaque structure */
>  struct git_graph;
>
> +/*
> + * Set up a custom scheme for column colors.
> + *
> + * The default column color scheme inserts ANSI color escapes to colorize
> + * the graph. The various color escapes are stored in an array of strings
> + * where each entry corresponds to a color, except for the last entry,
> + * which denotes the escape for resetting the color back to the default.
> + * When generating the graph, strings from this array are inserted before
> + * and after the various column characters.
> + *
> + * This function allows you to enable a custom array of color escapes.
> + * The 'colors_max' argument is the index of the last "reset" entry.
> + *
> + * This functions must be called BEFORE graph_init() is called.
> + *
> + * NOTE: This function isn't used in Git outside graph.c but it is used
> + * by CGit (http://git.zx2c4.com/cgit/) to use HTML for colors.
> + */
> +void graph_set_column_colors(const char **colors, unsigned short colors_max);
>
>  /*
>   * Create a new struct git_graph.
> @@ -33,6 +52,20 @@ void graph_update(struct git_graph *graph, struct commit *commit);
>   */
>  int graph_is_commit_finished(struct git_graph const *graph);
>
> +/*
> + * 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.
> + *
> + * NOTE: This function isn't used in Git outside graph.c but it is used
> + * by CGit (http://git.zx2c4.com/cgit/) to wrap HTML around graph lines.
> + */
> +int graph_next_line(struct git_graph *graph, struct strbuf *sb);
> +
>
>  /*
>   * graph_show_*: helper functions for printing to stdout
> --
> 1.8.2.rc1.339.g93ec2c9
>
Junio C Hamano· Mar 4, 2013, 03:42 UTC · re: Jason A. Donenfeld · lore

Re: [PATCH v2] Revert "graph.c: mark private file-scope symbols as static"

"Jason A. Donenfeld" <Jason@zx2c4.com> writes:
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

I suspect that many people outside CGit circle may not know who this Jason (who never appeared in git@vger.kernel.org) is, and are wondering in what capacity he is sending a S-o-b for somebody else's patch without explanation. An inroduction may help others (as can be seen on Cc: line, I added Lars there because I didn't even know he was still the man behind CGit).

Later last year, the project was taken over by an active contributor with the other active contributor (Ferry Huberts)'s support and then later with Lars's blessing:

  http://hjemli.net/pipermail/cgit/2013-January/000884.html

That active contributor is Jason. The repository has also been moved to Jason's http://git.zx2c4.com/cgit/ even though mailing list and its archive seem to be still suported by Lars's hjemli.net. And the latest CGit 0.9.1 was release mid November 2012 by him.

John Keeping has been actively working on making sure that CGit works with newer codebase for the past couple of days, until he found out that after our v1.7.12.4 some symbols were unavailable to him from us X-< which triggered this topic. Jason's S-o-b as the CGit maintainer is certainly appropriate for this patch.

So, thanks, Jason and John for your efforts.
Jason A. Donenfeld· Mar 4, 2013, 04:25 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] Revert "graph.c: mark private file-scope symbols as static"

On Sun, Mar 3, 2013 at 10:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> I suspect that many people outside CGit circle may not know who this
> Jason
> That active contributor is Jason. The repository has also been moved
> to Jason's http://git.zx2c4.com/cgit/
> So, thanks, Jason and John for your efforts.
Hey folks,

Sorry for not providing the explanation myself, but Junio -- thanks for introducing me!

Jason
Johan Herland· Mar 4, 2013, 00:52 UTC · re: John Keeping · lore

Re: [PATCH v2] Revert "graph.c: mark private file-scope symbols as static"

On Mon, Mar 4, 2013 at 1:03 AM, John Keeping <john@keeping.me.uk> wrote:
Show 11 quoted lines
> This reverts commit ba35480439d05b8f6cca50527072194fe3278bbb.
>
> CGit uses these symbols to output the correct HTML around graph
> elements.  Making these symbols private means that CGit cannot be
> updated to use Git 1.8.0 or newer, so let's not do that.
>
> On top of the revert, also add comments so that we avoid reintroducing
> this problem in the future and suggest to those modifying this API
> that they might want to discuss it with the CGit developers.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
Still-Acked-by: Johan Herland <johan@herland.net>
...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net

← back to recent threads