Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()
On Tue, Mar 12, 2013 at 05:16:20PM +0100, Heiko Voigt wrote: > > Ok if you say so I will do that :-). I was thinking about adding a patch > > that would remove cf as a global variable and explicitely pass it down > > to get_next_char. That makes it more obvious that it actually is != NULL. > > Looking at your callgraph I do not think its that much work. What do you > > think? > > I just had a look and unfortunately there are other functions that use > this variable (namely handle_path_include) for which its not that easy > to pass this in. So I will just drop the check here. Ah, I should have read all your responses before answering the last email. I think handle_path_include is surmountable, and I started to do a proof-of-concept patch, but there's a harder one: die_bad_config, which is called from git_config_int and friends. Getting the "cf" pointer there is hard, because you would have to update every config callback that uses git_config_int. Not worth it, I think. Thanks for looking, though. -Peff -- 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 v2 2/4] config: drop file pointer validity check in get_next_char()
On Tue, Mar 12, 2013 at 05:00:56PM +0100, Heiko Voigt wrote: > > That is, every path to get_next_char happens while we are in > > git_config_from_file, and that function guarantees that cf = &top, and > > that top.f != NULL. We do not have to even do any analysis of the > > conditions for each call, because we never change "cf" nor "top.f" > > except when we set them in git_config_from_file. > > Ok if you say so I will do that :-). I was thinking about adding a patch > that would remove cf as a global variable and explicitely pass it down > to get_next_char. That makes it more obvious that it actually is != NULL. > Looking at your callgraph I do not think its that much work. What do you > think? Yeah, I think that makes it more obvious what is going on, but you will run into trouble if you ever want that information to cross the "void *" boundary of a config callback, as adding a new parameter there is hard. The only place we do that now is for git_config_include, and I think you could get by with stuffing it into the config_include_data struct (which is already there to deal with this problem). It would also make something like this patch hard or impossible: http://article.gmane.org/gmane.comp.version-control.git/190267 as it wants to access "cf" from arbitrary callbacks. That is not a patch that is actively under consideration, but I had considered polishing it up at some point. > BTW, how did you generate this callgraph? Do you have a nice tool for that? I just did it manually in vi, working backwards from every instance of get_next_char, as it is not that big a graph. I suspect something like cscope could make it easier, but I don't know of any tool that will build the graph automatically (it would not be that hard from the data cscope has, though, so I wouldn't be surprised if such a tool exists). -Peff -- 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 v2 2/4] config: drop file pointer validity check in get_next_char()
On Tue, Mar 12, 2013 at 05:00:56PM +0100, Heiko Voigt wrote:
> On Tue, Mar 12, 2013 at 07:00:03AM -0400, Jeff King wrote:
> > On Sun, Mar 10, 2013 at 05:58:57PM +0100, Heiko Voigt wrote:
> >
> > > The only location where cf is set in this file is in do_config_from().
> > > This function has only one callsite which is config_from_file(). In
> > > config_from_file() its ensured that the f member is set to non-zero.
> > > [...]
> > > - if (cf && ((f = cf->f) != NULL)) {
> > > + if (cf) {
> >
> > I still think we can drop this conditional entirely. The complete call
> > graph looks like:
> >
> > git_config_from_file
> > -> git_parse_file
> > -> get_next_char
> > -> get_value
> > -> get_next_char
> > -> parse_value
> > -> get_next_char
> > -> get_base_var
> > -> get_next_char
> > -> get_extended_base_var
> > -> get_next_char
> >
> > That is, every path to get_next_char happens while we are in
> > git_config_from_file, and that function guarantees that cf = &top, and
> > that top.f != NULL. We do not have to even do any analysis of the
> > conditions for each call, because we never change "cf" nor "top.f"
> > except when we set them in git_config_from_file.
>
> Ok if you say so I will do that :-). I was thinking about adding a patch
> that would remove cf as a global variable and explicitely pass it down
> to get_next_char. That makes it more obvious that it actually is != NULL.
> Looking at your callgraph I do not think its that much work. What do you
> think?
I just had a look and unfortunately there are other functions that use
this variable (namely handle_path_include) for which its not that easy
to pass this in. So I will just drop the check here.
Cheers Heiko
--
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 v2 2/4] config: drop file pointer validity check in get_next_char()
On Tue, Mar 12, 2013 at 07:00:03AM -0400, Jeff King wrote:
> On Sun, Mar 10, 2013 at 05:58:57PM +0100, Heiko Voigt wrote:
>
> > The only location where cf is set in this file is in do_config_from().
> > This function has only one callsite which is config_from_file(). In
> > config_from_file() its ensured that the f member is set to non-zero.
> > [...]
> > - if (cf && ((f = cf->f) != NULL)) {
> > + if (cf) {
>
> I still think we can drop this conditional entirely. The complete call
> graph looks like:
>
> git_config_from_file
> -> git_parse_file
> -> get_next_char
> -> get_value
> -> get_next_char
> -> parse_value
> -> get_next_char
> -> get_base_var
> -> get_next_char
> -> get_extended_base_var
> -> get_next_char
>
> That is, every path to get_next_char happens while we are in
> git_config_from_file, and that function guarantees that cf = &top, and
> that top.f != NULL. We do not have to even do any analysis of the
> conditions for each call, because we never change "cf" nor "top.f"
> except when we set them in git_config_from_file.
Ok if you say so I will do that :-). I was thinking about adding a patch
that would remove cf as a global variable and explicitely pass it down
to get_next_char. That makes it more obvious that it actually is != NULL.
Looking at your callgraph I do not think its that much work. What do you
think?
BTW, how did you generate this callgraph? Do you have a nice tool for that?
Cheers Heiko
--
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 v2 2/4] config: drop file pointer validity check in get_next_char()
On Sun, Mar 10, 2013 at 05:58:57PM +0100, Heiko Voigt wrote:
> The only location where cf is set in this file is in do_config_from().
> This function has only one callsite which is config_from_file(). In
> config_from_file() its ensured that the f member is set to non-zero.
> [...]
> - if (cf && ((f = cf->f) != NULL)) {
> + if (cf) {
I still think we can drop this conditional entirely. The complete call
graph looks like:
git_config_from_file
-> git_parse_file
-> get_next_char
-> get_value
-> get_next_char
-> parse_value
-> get_next_char
-> get_base_var
-> get_next_char
-> get_extended_base_var
-> get_next_char
That is, every path to get_next_char happens while we are in
git_config_from_file, and that function guarantees that cf = &top, and
that top.f != NULL. We do not have to even do any analysis of the
conditions for each call, because we never change "cf" nor "top.f"
except when we set them in git_config_from_file.
-Peff
--
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
[PATCH v2 2/4] config: drop file pointer validity check in get_next_char()
The only location where cf is set in this file is in do_config_from().
This function has only one callsite which is config_from_file(). In
config_from_file() its ensured that the f member is set to non-zero.
Signed-off-by: Heiko Voigt
---
config.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index 2c299dc..f55c43d 100644
--- a/config.c
+++ b/config.c
@@ -169,10 +169,10 @@ int git_config_from_parameters(config_fn_t fn, void *data)
static int get_next_char(void)
{
int c;
- FILE *f;
c = '\n';
- if (cf && ((f = cf->f) != NULL)) {
+ if (cf) {
+ FILE *f = cf->f;
c = fgetc(f);
if (c == '\r') {
/* DOS like systems */
--
1.8.2.rc0.26.gf7384c5
--
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
