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: John Keeping j...@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 j...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert graph.c: mark private file-scope symbols as static
John Keeping j...@keeping.me.uk writes: On Sat, Mar 02, 2013 at 08:16:13PM +0100, Thomas Rast wrote: John Keeping j...@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 j...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: John Keeping j...@keeping.me.uk writes: On Sat, Mar 02, 2013 at 08:16:13PM +0100, Thomas Rast wrote: John Keeping j...@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 j...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert graph.c: mark private file-scope symbols as static
John Keeping j...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: John Keeping j...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert graph.c: mark private file-scope symbols as static
John Keeping j...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert graph.c: mark private file-scope symbols as static
On Sat, Mar 2, 2013 at 1:46 PM, John Keeping j...@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 j...@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 jo...@herland.net Have fun! :) ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert graph.c: mark private file-scope symbols as static
John Keeping j...@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 j...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
