All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Revert "graph.c: mark private file-scope symbols as static"
@ 2013-03-02 12:46 John Keeping
  2013-03-02 14:54 ` Johan Herland
  2013-03-02 19:16 ` Thomas Rast
  0 siblings, 2 replies; 14+ messages in thread
From: John Keeping @ 2013-03-02 12:46 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, John Keeping, Johan Herland

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(-)

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"
  2013-03-02 12:46 [PATCH] Revert "graph.c: mark private file-scope symbols as static" John Keeping
@ 2013-03-02 14:54 ` Johan Herland
  2013-03-02 19:16 ` Thomas Rast
  1 sibling, 0 replies; 14+ messages in thread
From: Johan Herland @ 2013-03-02 14:54 UTC (permalink / raw)
  To: John Keeping; +Cc: git, Junio C Hamano

On Sat, Mar 2, 2013 at 1:46 PM, John Keeping <john@keeping.me.uk> wrote:
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"
  2013-03-02 12:46 [PATCH] Revert "graph.c: mark private file-scope symbols as static" John Keeping
  2013-03-02 14:54 ` Johan Herland
@ 2013-03-02 19:16 ` Thomas Rast
  2013-03-03 10:29   ` John Keeping
  1 sibling, 1 reply; 14+ messages in thread
From: Thomas Rast @ 2013-03-02 19:16 UTC (permalink / raw)
  To: John Keeping; +Cc: git, Junio C Hamano, Johan Herland

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?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"
  2013-03-02 19:16 ` Thomas Rast
@ 2013-03-03 10:29   ` John Keeping
  2013-03-03 21:08     ` Junio C Hamano
  0 siblings, 1 reply; 14+ messages in thread
From: John Keeping @ 2013-03-03 10:29 UTC (permalink / raw)
  To: Thomas Rast; +Cc: git, Junio C Hamano, Johan Herland

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?

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"
  2013-03-03 10:29   ` John Keeping
@ 2013-03-03 21:08     ` Junio C Hamano
  2013-03-03 21:42       ` John Keeping
  0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2013-03-03 21:08 UTC (permalink / raw)
  To: John Keeping; +Cc: Thomas Rast, git, Johan Herland

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"
  2013-03-03 21:08     ` Junio C Hamano
@ 2013-03-03 21:42       ` John Keeping
  2013-03-03 22:49         ` Junio C Hamano
  0 siblings, 1 reply; 14+ messages in thread
From: John Keeping @ 2013-03-03 21:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Thomas Rast, git, Johan Herland

On Sun, Mar 03, 2013 at 01:08:50PM -0800, Junio C Hamano wrote:
> 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"
  2013-03-03 21:42       ` John Keeping
@ 2013-03-03 22:49         ` Junio C Hamano
  2013-03-03 23:24           ` John Keeping
  0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2013-03-03 22:49 UTC (permalink / raw)
  To: John Keeping; +Cc: Thomas Rast, git, Johan Herland, Lars Hjemli

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.

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"
  2013-03-03 22:49         ` Junio C Hamano
@ 2013-03-03 23:24           ` John Keeping
  2013-03-03 23:32             ` Junio C Hamano
  0 siblings, 1 reply; 14+ messages in thread
From: John Keeping @ 2013-03-03 23:24 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Thomas Rast, git, Johan Herland, Lars Hjemli

On Sun, Mar 03, 2013 at 02:49:12PM -0800, Junio C Hamano wrote:
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Revert "graph.c: mark private file-scope symbols as static"
  2013-03-03 23:24           ` John Keeping
@ 2013-03-03 23:32             ` Junio C Hamano
  2013-03-04  0:03               ` [PATCH v2] " John Keeping
  0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2013-03-03 23:32 UTC (permalink / raw)
  To: John Keeping; +Cc: Thomas Rast, git, Johan Herland, Lars Hjemli

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2] Revert "graph.c: mark private file-scope symbols as static"
  2013-03-03 23:32             ` Junio C Hamano
@ 2013-03-04  0:03               ` John Keeping
  2013-03-04  0:12                 ` Jason A. Donenfeld
  2013-03-04  0:52                 ` Johan Herland
  0 siblings, 2 replies; 14+ messages in thread
From: John Keeping @ 2013-03-04  0:03 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Rast, git, Johan Herland, Lars Hjemli, Jason A. Donenfeld

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Revert "graph.c: mark private file-scope symbols as static"
  2013-03-04  0:03               ` [PATCH v2] " John Keeping
@ 2013-03-04  0:12                 ` Jason A. Donenfeld
  2013-03-04  3:42                   ` Junio C Hamano
  2013-03-04  0:52                 ` Johan Herland
  1 sibling, 1 reply; 14+ messages in thread
From: Jason A. Donenfeld @ 2013-03-04  0:12 UTC (permalink / raw)
  To: John Keeping; +Cc: Junio C Hamano, Thomas Rast, git, Johan Herland, Lars Hjemli

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Revert "graph.c: mark private file-scope symbols as static"
  2013-03-04  0:03               ` [PATCH v2] " John Keeping
  2013-03-04  0:12                 ` Jason A. Donenfeld
@ 2013-03-04  0:52                 ` Johan Herland
  1 sibling, 0 replies; 14+ messages in thread
From: Johan Herland @ 2013-03-04  0:52 UTC (permalink / raw)
  To: John Keeping
  Cc: Junio C Hamano, Thomas Rast, git, Lars Hjemli, Jason A. Donenfeld

On Mon, Mar 4, 2013 at 1:03 AM, John Keeping <john@keeping.me.uk> wrote:
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Revert "graph.c: mark private file-scope symbols as static"
  2013-03-04  0:12                 ` Jason A. Donenfeld
@ 2013-03-04  3:42                   ` Junio C Hamano
  2013-03-04  4:25                     ` Jason A. Donenfeld
  0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2013-03-04  3:42 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: John Keeping, Thomas Rast, git, Johan Herland, Lars Hjemli

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Revert "graph.c: mark private file-scope symbols as static"
  2013-03-04  3:42                   ` Junio C Hamano
@ 2013-03-04  4:25                     ` Jason A. Donenfeld
  0 siblings, 0 replies; 14+ messages in thread
From: Jason A. Donenfeld @ 2013-03-04  4:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: John Keeping, Thomas Rast, git, Johan Herland, Lars Hjemli

On Sun, Mar 3, 2013 at 10:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2013-03-04  4:25 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-02 12:46 [PATCH] Revert "graph.c: mark private file-scope symbols as static" John Keeping
2013-03-02 14:54 ` Johan Herland
2013-03-02 19:16 ` Thomas Rast
2013-03-03 10:29   ` John Keeping
2013-03-03 21:08     ` Junio C Hamano
2013-03-03 21:42       ` John Keeping
2013-03-03 22:49         ` Junio C Hamano
2013-03-03 23:24           ` John Keeping
2013-03-03 23:32             ` Junio C Hamano
2013-03-04  0:03               ` [PATCH v2] " John Keeping
2013-03-04  0:12                 ` Jason A. Donenfeld
2013-03-04  3:42                   ` Junio C Hamano
2013-03-04  4:25                     ` Jason A. Donenfeld
2013-03-04  0:52                 ` Johan Herland

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.