Re: [RFC/WIP PATCH 1/3] teach config parsing to read from strbuf

Heiko Voigt [off-list ref] writes:

The idea to allow more kinds of sources specified for "config_file"
structure is not bad per-se, but whenever you design an enhancement
to something that currently supports only on thing to allow taking
another kind, please consider what needs to be done by the next
person who adds the third kind.  That would help catch design
mistakes early.  For example, will the "string-list" (I am not
saying use of string-list makes sense as the third kind; just as an
example off the top of my head) source patch add

	int is_string_list;
        struct string_list *string_list_contents;

fields to this structure?  Sounds insane for at least two reasons:

 * if both is_strbuf and is_string_list are true, what should
   happen?

 * is there a good reason to waste storage for the three fields your
   patch adds when sring_list strage (or FILE * storage for that
   matter) is used?

The helper functions like config_fgetc() and config_ftell() sounds
like you are going in the right direction but may want to do the
OO-in-C in a similar way transport.c does, keeping a pointer to a
structure of methods, but I didn't read the remainder of this patch
very carefully enough to comment further.

Hi Junio,

On Sun, Feb 24, 2013 at 09:54:43PM -0800, Junio C Hamano wrote:

Heiko Voigt [off-list ref] writes:

quoted

diff --git a/config.c b/config.c
index aefd80b..f995e98 100644
--- a/config.c
+++ b/config.c

@@ -13,6 +13,9 @@
 typedef struct config_file {
 	struct config_file *prev;
 	FILE *f;
+	int is_strbuf;
+	struct strbuf *strbuf_contents;
+	int strbuf_pos;
 	const char *name;
 	int linenr;
 	int eof;

The idea to allow more kinds of sources specified for "config_file"
structure is not bad per-se, but whenever you design an enhancement
to something that currently supports only on thing to allow taking
another kind, please consider what needs to be done by the next
person who adds the third kind.  That would help catch design
mistakes early.  For example, will the "string-list" (I am not
saying use of string-list makes sense as the third kind; just as an
example off the top of my head) source patch add

	int is_string_list;
        struct string_list *string_list_contents;

fields to this structure?  Sounds insane for at least two reasons:

 * if both is_strbuf and is_string_list are true, what should
   happen?

 * is there a good reason to waste storage for the three fields your
   patch adds when sring_list strage (or FILE * storage for that
   matter) is used?

The helper functions like config_fgetc() and config_ftell() sounds
like you are going in the right direction but may want to do the
OO-in-C in a similar way transport.c does, keeping a pointer to a
structure of methods, but I didn't read the remainder of this patch
very carefully enough to comment further.

Thanks for taking a look. You suggestion sounds reasonable, I will
modify my patch accordingly.

Cheers Heiko

Hi,

On Sun, Feb 24, 2013 at 09:54:43PM -0800, Junio C Hamano wrote:

The idea to allow more kinds of sources specified for "config_file"
structure is not bad per-se, but whenever you design an enhancement
to something that currently supports only on thing to allow taking
another kind, please consider what needs to be done by the next
person who adds the third kind.  That would help catch design
mistakes early.  For example, will the "string-list" (I am not
saying use of string-list makes sense as the third kind; just as an
example off the top of my head) source patch add

	int is_string_list;
        struct string_list *string_list_contents;

fields to this structure?  Sounds insane for at least two reasons:

 * if both is_strbuf and is_string_list are true, what should
   happen?

 * is there a good reason to waste storage for the three fields your
   patch adds when sring_list strage (or FILE * storage for that
   matter) is used?

The helper functions like config_fgetc() and config_ftell() sounds
like you are going in the right direction but may want to do the
OO-in-C in a similar way transport.c does, keeping a pointer to a
structure of methods, but I didn't read the remainder of this patch
very carefully enough to comment further.

I split up my config-from-strings patch from the "fetch moved submodules
on-demand" series[1] for nicer reviewability. Since Peff almost needed it
and the recursive submodule checkout will definitely[2] need it: How
about making it a seperate topic and implement this infrastructure
first?

Junio I incorporated your comments this seems like a result ready for
inclusion.

[1] http://article.gmane.org/gmane.comp.version-control.git/217018
[2] http://article.gmane.org/gmane.comp.version-control.git/217155

Heiko Voigt (4):
  config: factor out config file stack management
  config: drop file pointer validity check in get_next_char()
  config: make parsing stack struct independent from actual data source
  teach config parsing to read from strbuf

 .gitignore             |   1 +
 Makefile               |   1 +
 cache.h                |   1 +
 config.c               | 140 ++++++++++++++++++++++++++++++++++++++-----------
 t/t1300-repo-config.sh |   4 ++
 test-config.c          |  41 +++++++++++++++
 6 files changed, 158 insertions(+), 30 deletions(-)
 create mode 100644 test-config.c

-- 
1.8.2.rc0.26.gf7384c5

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 <redacted>
---

Peff, I hope you do not mind that I totally copied your commit message
here. The patch takes a different approach though. If you like we can
add a

	Commit-Message-by: Jeff King [off-list ref]

here :-)

Cheers Heiko

 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 = ⊤
-
-		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 simplify adding other sources we extract all functions needed for
parsing into a list of callbacks. We implement those callbacks for the
current file parsing. A new source can implement its own set of callbacks.

Instead of storing the concrete FILE pointer for parsing we store a void
pointer. A new source can use this to store its custom data.

Signed-off-by: Heiko Voigt <redacted>
---
 config.c | 57 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/config.c b/config.c
index f55c43d..19aa205 100644
--- a/config.c
+++ b/config.c

@@ -10,20 +10,42 @@
 #include "strbuf.h"
 #include "quote.h"
 
-typedef struct config_file {
-	struct config_file *prev;
-	FILE *f;
+struct config {
+	struct config *prev;
+	void *data;
 	const char *name;
 	int linenr;
 	int eof;
 	struct strbuf value;
 	struct strbuf var;
-} config_file;
 
-static config_file *cf;
+	int (*fgetc)(struct config *c);
+	int (*ungetc)(int c, struct config *conf);
+	long (*ftell)(struct config *c);
+};
+
+static struct config *cf;
 
 static int zlib_compression_seen;
 
+static int config_file_fgetc(struct config *conf)
+{
+	FILE *f = conf->data;
+	return fgetc(f);
+}
+
+static int config_file_ungetc(int c, struct config *conf)
+{
+	FILE *f = conf->data;
+	return ungetc(c, f);
+}
+
+static long config_file_ftell(struct config *conf)
+{
+	FILE *f = conf->data;
+	return ftell(f);
+}
+
 #define MAX_INCLUDE_DEPTH 10
 static const char include_depth_advice[] =
 "exceeded maximum include depth (%d) while including\n"

@@ -172,13 +194,12 @@ static int get_next_char(void)
 
 	c = '\n';
 	if (cf) {
-		FILE *f = cf->f;
-		c = fgetc(f);
+		c = cf->fgetc(cf);
 		if (c == '\r') {
 			/* DOS like systems */
-			c = fgetc(f);
+			c = cf->fgetc(cf);
 			if (c != '\n') {
-				ungetc(c, f);
+				cf->ungetc(c, cf);
 				c = '\r';
 			}
 		}

@@ -896,7 +917,7 @@ 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)
+static int do_config_from(struct config *top, config_fn_t fn, void *data)
 {
 	int ret;
 

@@ -925,10 +946,13 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 
 	ret = -1;
 	if (f) {
-		config_file top;
+		struct config top;
 
-		top.f = f;
+		top.data = f;
 		top.name = filename;
+		top.fgetc = config_file_fgetc;
+		top.ungetc = config_file_ungetc;
+		top.ftell = config_file_ftell;
 
 		ret = do_config_from(&top, fn, data);
 

@@ -1063,7 +1087,6 @@ static int store_aux(const char *key, const char *value, void *cb)
 {
 	const char *ep;
 	size_t section_len;
-	FILE *f = cf->f;
 
 	switch (store.state) {
 	case KEY_SEEN:

@@ -1075,7 +1098,7 @@ static int store_aux(const char *key, const char *value, void *cb)
 				return 1;
 			}
 
-			store.offset[store.seen] = ftell(f);
+			store.offset[store.seen] = cf->ftell(cf);
 			store.seen++;
 		}
 		break;

@@ -1102,19 +1125,19 @@ static int store_aux(const char *key, const char *value, void *cb)
 		 * Do not increment matches: this is no match, but we
 		 * just made sure we are in the desired section.
 		 */
-		store.offset[store.seen] = ftell(f);
+		store.offset[store.seen] = cf->ftell(cf);
 		/* fallthru */
 	case SECTION_END_SEEN:
 	case START:
 		if (matches(key, value)) {
-			store.offset[store.seen] = ftell(f);
+			store.offset[store.seen] = cf->ftell(cf);
 			store.state = KEY_SEEN;
 			store.seen++;
 		} else {
 			if (strrchr(key, '.') - key == store.baselen &&
 			      !strncmp(key, store.key, store.baselen)) {
 					store.state = SECTION_SEEN;
-					store.offset[store.seen] = ftell(f);
+					store.offset[store.seen] = cf->ftell(cf);
 			}
 		}
 	}

-- 
1.8.2.rc0.26.gf7384c5

This can be used to read configuration values directly from gits
database.

Signed-off-by: Heiko Voigt <redacted>
---
 .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 0000000..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

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 <redacted>
---
 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

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 <redacted>
---

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 [off-list ref]

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

On Tue, Feb 26, 2013 at 08:40:23PM +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.

Makes sense, although...

-	if (cf && ((f = cf->f) != NULL)) {
+	if (cf) {
+		FILE *f = cf->f;

Couldn't we say the same thing about "cf" here (i.e., that it would
never be NULL)? Can we just get rid of this conditional entirely?

-Peff

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:

quoted

+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

On Tue, Feb 26, 2013 at 09:09:41PM +0100, Heiko Voigt wrote:

quoted

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

On Tue, Feb 26, 2013 at 03:05:56PM -0500, Jeff King wrote:

On Tue, Feb 26, 2013 at 08:40:23PM +0100, Heiko Voigt wrote:

quoted

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.

Makes sense, although...

quoted

-	if (cf && ((f = cf->f) != NULL)) {
+	if (cf) {
+		FILE *f = cf->f;

Couldn't we say the same thing about "cf" here (i.e., that it would
never be NULL)? Can we just get rid of this conditional entirely?

That might be true. I will look into it. Just wanted to get rid of an
extra callback in my series.

Cheers Heiko

On Wed, Feb 27, 2013 at 08:52:57AM +0100, Heiko Voigt wrote:

On Tue, Feb 26, 2013 at 03:05:56PM -0500, Jeff King wrote:

quoted

On Tue, Feb 26, 2013 at 08:40:23PM +0100, Heiko Voigt wrote:

quoted

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.

Makes sense, although...

quoted

-	if (cf && ((f = cf->f) != NULL)) {
+	if (cf) {
+		FILE *f = cf->f;

Couldn't we say the same thing about "cf" here (i.e., that it would
never be NULL)? Can we just get rid of this conditional entirely?

That might be true. I will look into it. Just wanted to get rid of an
extra callback in my series.

I had a look and it might be true that cf will never be NULL in a code
path. Nevertheless its much harder to verify by looking at the code
since its a global variable. get_next_char() is called from all over the
place and I would have to look at all the code paths. As far as I know
static global variables are always initialized to zero so its safe to
check even if has not yet been explicitly initialized.

The statement if cf is not NULL all members will be initialized is much
simpler to verify since its just one place now and two places after this
series.

Cheers Heiko

On Thu, Feb 28, 2013 at 01:41:47AM +0100, Heiko Voigt wrote:

On Wed, Feb 27, 2013 at 08:52:57AM +0100, Heiko Voigt wrote:

quoted

On Tue, Feb 26, 2013 at 03:05:56PM -0500, Jeff King wrote:

quoted

On Tue, Feb 26, 2013 at 08:40:23PM +0100, Heiko Voigt wrote:

quoted

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.

Makes sense, although...

quoted

-	if (cf && ((f = cf->f) != NULL)) {
+	if (cf) {
+		FILE *f = cf->f;

Couldn't we say the same thing about "cf" here (i.e., that it would
never be NULL)? Can we just get rid of this conditional entirely?

That might be true. I will look into it. Just wanted to get rid of an
extra callback in my series.

I had a look and it might be true that cf will never be NULL in a code
path. Nevertheless its much harder to verify by looking at the code
since its a global variable. get_next_char() is called from all over the
place and I would have to look at all the code paths. As far as I know
static global variables are always initialized to zero so its safe to
check even if has not yet been explicitly initialized.

The statement if cf is not NULL all members will be initialized is much
simpler to verify since its just one place now and two places after this
series.

To add some empirical information: I just ran the testsuite without the
conditional and it still passes. To me it only make sense to start the
parsing with cf initialized. But I am not familiar enough with the code
to judge whether it is safe to assume this.

Cheers Heiko

Heiko Voigt wrote:

This can be used to read configuration values directly from gits
database.

Signed-off-by: Heiko Voigt <redacted>
---
 .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

[...]

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.

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

Hi,

On Thu, Mar 07, 2013 at 06:42:43PM +0000, Ramsay Jones wrote:

Heiko Voigt wrote:

quoted

+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