Re: [PATCH 1/4] config: factor out config file stack management
On Tue, Feb 26, 2013 at 02:12:23PM -0800, Junio C Hamano wrote: > Heiko Voigt writes: > > > The "do_config_from" means "parse from whatever is in 'top'". Later in > > the series its type changes from config_file to struct config. > > Yuck. It would be nice to have it as struct config_src or > something. "struct config" sounds as if it represents the entire > configuration state and you can ask it to add new ones or enumerate > all known configuration variables, etc. Will change it to struct config_source. I choose that name in lack of a better one. Since it can be considered the base for both sources I just removed the _file postfix. 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 1/4] config: factor out config file stack management
On Tue, Feb 26, 2013 at 02:10:03PM -0800, Junio C Hamano wrote:
> Jeff King writes:
>
> > I wonder if it would be more obvious with the more usual OO-struct
> > functions, like:
> >
> > struct config_source {
> > ...
> > };
> > void config_source_init_file(struct config_source *, const char *fn);
> > void config_source_init_strbuf(struct config_source *,
> > const struct strbuf *buf);
> > void config_source_clear(struct config_source *);
> >
> > int config_source_parse(struct config_source *);
> >
> > and then the use would be something like:
> >
> > struct config_source top;
> > int ret;
> >
> > config_source_init_file(&top, "foo");
> > ret = config_source_parse(&top);
> > config_source_clear(&top);
> >
> > return ret;
> >
> > I.e., "init" constructors, a "clear" destructor, and any methods like
> > "parse" that you need.
>
> Yup, that cocincides with my first impression I sent out for the
> previous RFC/PATCH round.
I agree that your suggestions would make the design more OO and provide
us with more flexibility. However at the following costs:
Currently the push and pop are combined into one function. This design
makes it impossible to forget the cleanup if someone else uses this
function. The same applies to the private data handling around
do_config_from().
All memory is currently handled on the stack. If we have an init/clear
design at least the private data for strbuf needs to be put on the heap.
We could pass in the config_strbuf but IMO that would violate the
encapsulation.
Thus we also require a specialized clear which frees that private data
(we need that to close the file anyway). Because of that I would probably
call it destroy or free.
That means there will be six additional functions instead of one: We
need init and clear for both, a common init and a common clear for the
push/pop part. IMO that will make the code harder to read for the first
time.
Then we would have a hybrid stack/heap solution still involving side
effects (setting the global cf).
Do not get me wrong. I am not against changing it but I would like to
spell out the consequences first before doing so.
Do you still think that is nicer approach?
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 1/4] config: factor out config file stack management
Heiko Voigt writes: > The "do_config_from" means "parse from whatever is in 'top'". Later in > the series its type changes from config_file to struct config. Yuck. It would be nice to have it as struct config_src or something. "struct config" sounds as if it represents the entire configuration state and you can ask it to add new ones or enumerate all known configuration variables, etc. -- 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 1/4] config: factor out config file stack management
Jeff King writes:
> I wonder if it would be more obvious with the more usual OO-struct
> functions, like:
>
> struct config_source {
> ...
> };
> void config_source_init_file(struct config_source *, const char *fn);
> void config_source_init_strbuf(struct config_source *,
> const struct strbuf *buf);
> void config_source_clear(struct config_source *);
>
> int config_source_parse(struct config_source *);
>
> and then the use would be something like:
>
> struct config_source top;
> int ret;
>
> config_source_init_file(&top, "foo");
> ret = config_source_parse(&top);
> config_source_clear(&top);
>
> return ret;
>
> I.e., "init" constructors, a "clear" destructor, and any methods like
> "parse" that you need.
Yup, that cocincides with my first impression I sent out for the
previous RFC/PATCH round.
--
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 1/4] config: factor out config file stack management
On Tue, Feb 26, 2013 at 09:09:41PM +0100, Heiko Voigt wrote:
> > This function name is a bit weird. I would have thought the "from" here
> > was going to be a file, or a string, or whatever. But the filename setup
> > happens outside this function (and yet this function depends on it being
> > set up, as it calls git_parse_file). But maybe it will get less
> > confusing with the other patches on top...
>
> The "do_config_from" means "parse from whatever is in 'top'". Later in
> the series its type changes from config_file to struct config.
Ah, I see. The "from" is the "struct config".
I wonder if it would be more obvious with the more usual OO-struct
functions, like:
struct config_source {
...
};
void config_source_init_file(struct config_source *, const char *fn);
void config_source_init_strbuf(struct config_source *,
const struct strbuf *buf);
void config_source_clear(struct config_source *);
int config_source_parse(struct config_source *);
and then the use would be something like:
struct config_source top;
int ret;
config_source_init_file(&top, "foo");
ret = config_source_parse(&top);
config_source_clear(&top);
return ret;
I.e., "init" constructors, a "clear" destructor, and any methods like
"parse" that you need. I haven't though too hard about it, though, so
maybe there is some reason it does not fit that model (it is a little
uncommon that the "init" would push itself onto a stack, but I think
that's OK).
-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 1/4] config: factor out config file stack management
On Tue, Feb 26, 2013 at 02:54:49PM -0500, Jeff King wrote:
> On Tue, Feb 26, 2013 at 08:38:50PM +0100, Heiko Voigt wrote:
> > +static int do_config_from(struct config_file *top, config_fn_t fn, void
> > *data)
> > +{
> > + int ret;
> > +
> > + /* push config-file parsing state stack */
> > + top->prev = cf;
> > + top->linenr = 1;
> > + top->eof = 0;
> > + strbuf_init(&top->value, 1024);
> > + strbuf_init(&top->var, 1024);
> > + cf = top;
> > +
> > + ret = git_parse_file(fn, data);
> > +
> > + /* pop config-file parsing state stack */
> > + strbuf_release(&top->value);
> > + strbuf_release(&top->var);
> > + cf = top->prev;
> > +
> > + return ret;
> > +}
>
> This function name is a bit weird. I would have thought the "from" here
> was going to be a file, or a string, or whatever. But the filename setup
> happens outside this function (and yet this function depends on it being
> set up, as it calls git_parse_file). But maybe it will get less
> confusing with the other patches on top...
The "do_config_from" means "parse from whatever is in 'top'". Later in
the series its type changes from config_file to struct config.
The name 'git_parse_file' becomes definitely wrong after this series.
Maybe I should rename it?
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 1/4] config: factor out config file stack management
On Tue, Feb 26, 2013 at 08:38:50PM +0100, Heiko Voigt wrote:
> Because a config callback may start parsing a new file, the
> global context regarding the current config file is stored
> as a stack. Currently we only need to manage that stack from
> git_config_from_file. Let's factor it out to allow new
> sources of config data.
>
> Signed-off-by: Heiko Voigt
> ---
>
> Peff, I hope you do not mind that I totally copied your commit message
> here.
I don't mind at all.
> The patch takes a different approach though. If you like we can add a
>
> Commit-Message-by: Jeff King
>
> here :-)
I think my name is plastered all over "git log" enough as it is.
> +static int do_config_from(struct config_file *top, config_fn_t fn, void
> *data)
> +{
> + int ret;
> +
> + /* push config-file parsing state stack */
> + top->prev = cf;
> + top->linenr = 1;
> + top->eof = 0;
> + strbuf_init(&top->value, 1024);
> + strbuf_init(&top->var, 1024);
> + cf = top;
> +
> + ret = git_parse_file(fn, data);
> +
> + /* pop config-file parsing state stack */
> + strbuf_release(&top->value);
> + strbuf_release(&top->var);
> + cf = top->prev;
> +
> + return ret;
> +}
This function name is a bit weird. I would have thought the "from" here
was going to be a file, or a string, or whatever. But the filename setup
happens outside this function (and yet this function depends on it being
set up, as it calls git_parse_file). But maybe it will get less
confusing with the other patches on top...
-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
