Re: [PATCH 4/4] teach config parsing to read from strbuf
Hi,
On Thu, Mar 07, 2013 at 06:42:43PM +, Ramsay Jones wrote:
> Heiko Voigt wrote:
> > +int git_config_from_strbuf(config_fn_t fn, struct strbuf *strbuf, void
> > *data)
> > +{
> > + struct config top;
> > + struct config_strbuf str;
> > +
> > + str.strbuf = strbuf;
> > + str.pos = 0;
> > +
> > + top.data = &str;
>
> You will definitely want to initialise 'top.name' here, rather
> than let it take whatever value happens to be at that position
> on the stack. In your editor, search for 'cf->name' and contemplate
> each such occurrence.
Good catch, thanks. The initialization seems to got lost during
refactoring. In the codepaths we call with the new strbuf function it is
only used for error reporting so I think we need to get the name from
the user of this function so the error messages are useful.
I have extended the test to demonstrate how I imagine this name could
look like.
> Does the 'include' facility work from a strbuf? Should it?
No the 'include' facility does not work here. A special handling would
need to be implemented by the caller. For us 'include' values have no
special meaning and are parsed like normal values.
AFAICS when a config file is given to git config we do not currently
respect includes. So we can just do the same behavior here for the
moment. There is no roadblock against adding it later.
> Are you happy with the error handling/reporting?
Good point, while adding the name for strbuf I noticed that we currently
die on some parsing errors. We should probably make these errors
handleable for strbufs. Currently the main usecase is to read submodule
configurations from the database and in case someone commits a broken
configuration we should be able to continue with that. Otherwise the
repository might render into an unuseable state. I will look into that.
> Do the above additions to the test-suite give you confidence
> that the code works as you intend?
Well, I am reusing most of the existing infrastructure which I assume is
tested using config files. So what I want to test here is the
integration of this new function. As you mentioned the name variable I
have now added a test for the parsing error case as well. I have
refactored the test binary to read configs from stdin so its easiert to
implement further tests from the bash part of the testsuite.
I will send out another iteration shortly.
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 4/4] teach config parsing to read from strbuf
Heiko Voigt wrote:
> This can be used to read configuration values directly from gits
> database.
>
> Signed-off-by: Heiko Voigt
> ---
> .gitignore | 1 +
> Makefile | 1 +
> cache.h| 1 +
> config.c | 47 +++
> t/t1300-repo-config.sh | 4
> test-config.c | 41 +
> 6 files changed, 95 insertions(+)
> create mode 100644 test-config.c
>
[...]
> diff --git a/config.c b/config.c
> index 19aa205..492873a 100644
> --- a/config.c
> +++ b/config.c
> @@ -46,6 +46,37 @@ static long config_file_ftell(struct config *conf)
> return ftell(f);
> }
>
> +struct config_strbuf {
> + struct strbuf *strbuf;
> + int pos;
> +};
> +
> +static int config_strbuf_fgetc(struct config *conf)
> +{
> + struct config_strbuf *str = conf->data;
> +
> + if (str->pos < str->strbuf->len)
> + return str->strbuf->buf[str->pos++];
> +
> + return EOF;
> +}
> +
> +static int config_strbuf_ungetc(int c, struct config *conf)
> +{
> + struct config_strbuf *str = conf->data;
> +
> + if (str->pos > 0)
> + return str->strbuf->buf[--str->pos];
> +
> + return EOF;
> +}
> +
> +static long config_strbuf_ftell(struct config *conf)
> +{
> + struct config_strbuf *str = conf->data;
> + return str->pos;
> +}
> +
> #define MAX_INCLUDE_DEPTH 10
> static const char include_depth_advice[] =
> "exceeded maximum include depth (%d) while including\n"
> @@ -961,6 +992,22 @@ int git_config_from_file(config_fn_t fn, const char
> *filename, void *data)
> return ret;
> }
>
> +int git_config_from_strbuf(config_fn_t fn, struct strbuf *strbuf, void *data)
> +{
> + struct config top;
> + struct config_strbuf str;
> +
> + str.strbuf = strbuf;
> + str.pos = 0;
> +
> + top.data = &str;
You will definitely want to initialise 'top.name' here, rather
than let it take whatever value happens to be at that position
on the stack. In your editor, search for 'cf->name' and contemplate
each such occurrence.
> + top.fgetc = config_strbuf_fgetc;
> + top.ungetc = config_strbuf_ungetc;
> + top.ftell = config_strbuf_ftell;
> +
> + return do_config_from(&top, fn, data);
> +}
> +
> const char *git_etc_gitconfig(void)
> {
> static const char *system_wide;
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 3c96fda..3304bcd 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -1087,4 +1087,8 @@ test_expect_success 'barf on incomplete string' '
> grep " line 3 " error
> '
>
> +test_expect_success 'reading config from strbuf' '
> + test-config strbuf
> +'
> +
> test_done
> diff --git a/test-config.c b/test-config.c
> new file mode 100644
> index 000..7a4103c
> --- /dev/null
> +++ b/test-config.c
> @@ -0,0 +1,41 @@
> +#include "cache.h"
> +
> +static const char *config_string = "[some]\n"
> + " value = content\n";
> +
> +static int config_strbuf(const char *var, const char *value, void *data)
> +{
> + int *success = data;
> + if (!strcmp(var, "some.value") && !strcmp(value, "content"))
> + *success = 0;
> +
> + printf("var: %s, value: %s\n", var, value);
> +
> + return 1;
> +}
> +
> +static void die_usage(int argc, char **argv)
> +{
> + fprintf(stderr, "Usage: %s strbuf\n", argv[0]);
> + exit(1);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + if (argc < 2)
> + die_usage(argc, argv);
> +
> + if (!strcmp(argv[1], "strbuf")) {
> + int success = 1;
> + struct strbuf buf = STRBUF_INIT;
> +
> + strbuf_addstr(&buf, config_string);
> + git_config_from_strbuf(config_strbuf, &buf, &success);
> +
> + return success;
> + }
> +
> + die_usage(argc, argv);
> +
> + return 1;
> +}
>
Does the 'include' facility work from a strbuf? Should it?
Are you happy with the error handling/reporting?
Do the above additions to the test-suite give you confidence
that the code works as you intend?
ATB,
Ramsay Jones
--
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 4/4] teach config parsing to read from strbuf
This can be used to read configuration values directly from gits
database.
Signed-off-by: Heiko Voigt
---
.gitignore | 1 +
Makefile | 1 +
cache.h| 1 +
config.c | 47 +++
t/t1300-repo-config.sh | 4
test-config.c | 41 +
6 files changed, 95 insertions(+)
create mode 100644 test-config.c
diff --git a/.gitignore b/.gitignore
index 6669bf0..386b7f2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -178,6 +178,7 @@
/gitweb/static/gitweb.min.*
/test-chmtime
/test-ctype
+/test-config
/test-date
/test-delta
/test-dump-cache-tree
diff --git a/Makefile b/Makefile
index ba8e243..98da708 100644
--- a/Makefile
+++ b/Makefile
@@ -543,6 +543,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
TEST_PROGRAMS_NEED_X += test-chmtime
TEST_PROGRAMS_NEED_X += test-ctype
+TEST_PROGRAMS_NEED_X += test-config
TEST_PROGRAMS_NEED_X += test-date
TEST_PROGRAMS_NEED_X += test-delta
TEST_PROGRAMS_NEED_X += test-dump-cache-tree
diff --git a/cache.h b/cache.h
index e493563..ada2362 100644
--- a/cache.h
+++ b/cache.h
@@ -1128,6 +1128,7 @@ extern int update_server_info(int);
typedef int (*config_fn_t)(const char *, const char *, void *);
extern int git_default_config(const char *, const char *, void *);
extern int git_config_from_file(config_fn_t fn, const char *, void *);
+extern int git_config_from_strbuf(config_fn_t fn, struct strbuf *strbuf, void
*data);
extern void git_config_push_parameter(const char *text);
extern int git_config_from_parameters(config_fn_t fn, void *data);
extern int git_config(config_fn_t fn, void *);
diff --git a/config.c b/config.c
index 19aa205..492873a 100644
--- a/config.c
+++ b/config.c
@@ -46,6 +46,37 @@ static long config_file_ftell(struct config *conf)
return ftell(f);
}
+struct config_strbuf {
+ struct strbuf *strbuf;
+ int pos;
+};
+
+static int config_strbuf_fgetc(struct config *conf)
+{
+ struct config_strbuf *str = conf->data;
+
+ if (str->pos < str->strbuf->len)
+ return str->strbuf->buf[str->pos++];
+
+ return EOF;
+}
+
+static int config_strbuf_ungetc(int c, struct config *conf)
+{
+ struct config_strbuf *str = conf->data;
+
+ if (str->pos > 0)
+ return str->strbuf->buf[--str->pos];
+
+ return EOF;
+}
+
+static long config_strbuf_ftell(struct config *conf)
+{
+ struct config_strbuf *str = conf->data;
+ return str->pos;
+}
+
#define MAX_INCLUDE_DEPTH 10
static const char include_depth_advice[] =
"exceeded maximum include depth (%d) while including\n"
@@ -961,6 +992,22 @@ int git_config_from_file(config_fn_t fn, const char
*filename, void *data)
return ret;
}
+int git_config_from_strbuf(config_fn_t fn, struct strbuf *strbuf, void *data)
+{
+ struct config top;
+ struct config_strbuf str;
+
+ str.strbuf = strbuf;
+ str.pos = 0;
+
+ top.data = &str;
+ top.fgetc = config_strbuf_fgetc;
+ top.ungetc = config_strbuf_ungetc;
+ top.ftell = config_strbuf_ftell;
+
+ return do_config_from(&top, fn, data);
+}
+
const char *git_etc_gitconfig(void)
{
static const char *system_wide;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 3c96fda..3304bcd 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1087,4 +1087,8 @@ test_expect_success 'barf on incomplete string' '
grep " line 3 " error
'
+test_expect_success 'reading config from strbuf' '
+ test-config strbuf
+'
+
test_done
diff --git a/test-config.c b/test-config.c
new file mode 100644
index 000..7a4103c
--- /dev/null
+++ b/test-config.c
@@ -0,0 +1,41 @@
+#include "cache.h"
+
+static const char *config_string = "[some]\n"
+ " value = content\n";
+
+static int config_strbuf(const char *var, const char *value, void *data)
+{
+ int *success = data;
+ if (!strcmp(var, "some.value") && !strcmp(value, "content"))
+ *success = 0;
+
+ printf("var: %s, value: %s\n", var, value);
+
+ return 1;
+}
+
+static void die_usage(int argc, char **argv)
+{
+ fprintf(stderr, "Usage: %s strbuf\n", argv[0]);
+ exit(1);
+}
+
+int main(int argc, char **argv)
+{
+ if (argc < 2)
+ die_usage(argc, argv);
+
+ if (!strcmp(argv[1], "strbuf")) {
+ int success = 1;
+ struct strbuf buf = STRBUF_INIT;
+
+ strbuf_addstr(&buf, config_string);
+ git_config_from_strbuf(config_strbuf, &buf, &success);
+
+ return success;
+ }
+
+ die_usage(argc, argv);
+
+ return 1;
+}
--
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
