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

From: John Keeping <john@keeping.me.uk>
Date: Mar 3, 2013, 23:24 UTC
Message-ID: <20130303232413.GN7738@serenity.lan>
In-Reply-To: <7vppzg9k0n.fsf@alter.siamese.dyndns.org>

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