Re: [PATCH v2 1/4] config: factor out config file stack management
On Tue, Mar 12, 2013 at 03:04:56PM -0400, Jeff King wrote: On Tue, Mar 12, 2013 at 04:44:35PM +0100, Heiko Voigt wrote: Can we throw in a comment at the top here with the expected usage? In particular, do_config_from is expecting the caller to have filled in certain fields (at this point, top-f and top-name), but there is nothing to make that clear. Of course. Will do that in the next iteration. How about I squash this in: [...] +/* The fields data, name and the source specific callbacks of top need + * to be initialized before calling this function. + */ static int do_config_from_source(struct config_source *top, config_fn_t fn, voi I think that is OK, but it may be even better to list the fields by name. Also, our multi-line comment style is: /* * Multi-line comment. */ Ok will do both. I would add that to the third patch: config: make parsing stack struct independent from actual data source because that contains the final modification to config_file/config_source. It does not matter to the end result, but I find it helps with reviewing when the comment is added along with the function, and then expanded as the function is changed. It helps to understand the effects of later patches if they need to tweak comments. To make the series more clear to others who read it later, I will add the comment from the beginning. 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 1/4] config: factor out config file stack management
On Sun, Mar 10, 2013 at 05:57:53PM +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.
[...]
+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;
+}
Can we throw in a comment at the top here with the expected usage? In
particular, do_config_from is expecting the caller to have filled in
certain fields (at this point, top-f and top-name), but there is
nothing to make that clear.
-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 1/4] config: factor out config file stack management
On Tue, Mar 12, 2013 at 06:52:00AM -0400, Jeff King wrote:
On Sun, Mar 10, 2013 at 05:57:53PM +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.
[...]
+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;
+}
Can we throw in a comment at the top here with the expected usage? In
particular, do_config_from is expecting the caller to have filled in
certain fields (at this point, top-f and top-name), but there is
nothing to make that clear.
Of course. Will do that in the next iteration. How about I squash this in:
diff --git a/config.c b/config.c
index b8c8640..b7632c9 100644
--- a/config.c
+++ b/config.c
@@ -948,6 +954,9 @@ int git_default_config(const char *var, const char *value, v
return 0;
}
+/* The fields data, name and the source specific callbacks of top need
+ * to be initialized before calling this function.
+ */
static int do_config_from_source(struct config_source *top, config_fn_t fn, voi
{
int ret;
I would add that to the third patch:
config: make parsing stack struct independent from actual data source
because that contains the final modification to config_file/config_source.
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 1/4] config: factor out config file stack management
On Tue, Mar 12, 2013 at 04:44:35PM +0100, Heiko Voigt wrote: Can we throw in a comment at the top here with the expected usage? In particular, do_config_from is expecting the caller to have filled in certain fields (at this point, top-f and top-name), but there is nothing to make that clear. Of course. Will do that in the next iteration. How about I squash this in: [...] +/* The fields data, name and the source specific callbacks of top need + * to be initialized before calling this function. + */ static int do_config_from_source(struct config_source *top, config_fn_t fn, voi I think that is OK, but it may be even better to list the fields by name. Also, our multi-line comment style is: /* * Multi-line comment. */ I would add that to the third patch: config: make parsing stack struct independent from actual data source because that contains the final modification to config_file/config_source. It does not matter to the end result, but I find it helps with reviewing when the comment is added along with the function, and then expanded as the function is changed. It helps to understand the effects of later patches if they need to tweak comments. I do not care that much in this instance, since we have already discussed it, and I know what is going on, 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
[PATCH v2 1/4] config: factor out config file stack management
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 hvo...@hvoigt.net
---
config.c | 38 --
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/config.c b/config.c
index aefd80b..2c299dc 100644
--- a/config.c
+++ b/config.c
@@ -896,6 +896,28 @@ int git_default_config(const char *var, const char *value,
void *dummy)
return 0;
}
+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;
+}
+
int git_config_from_file(config_fn_t fn, const char *filename, void *data)
{
int ret;
@@ -905,22 +927,10 @@ int git_config_from_file(config_fn_t fn, const char
*filename, void *data)
if (f) {
config_file top;
- /* push config-file parsing state stack */
- top.prev = cf;
top.f = f;
top.name = filename;
- 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;
+
+ ret = do_config_from(top, fn, data);
fclose(f);
}
--
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
