[RFC/WIP PATCH 0/3] fetch moved submodules on-demand

[RFC/WIP PATCH 0/3] fetch moved submodules on-demand

[Heiko Voigt]

Hi,

this is a series I have been working on and off. My plan is that it
might be part of a slightly bigger series also implementing on-demand
clone of submodules into the .git/modules folder if a submodule is
configured like that.

This is needed as preparation for the final goal of automatic
checkout/deletion of submodules when they appear/disappear. My plan
is to introduce a new .gitmodules variable

	submodule.<name>.init

to specify whether a submodule should be initialized (and thus cloned)
by default or not. If not configured it will default to off to stay
closed to the current behavior. If it proves useful we can maybe change
that default later to on.

That way we would get much closer to "clone/fetch and get everything you
need to work on a project".

I send this series mainly to inform people what I have been working and
to maybe get some early feedback about the approach. Let me know what
you think.

Cheers Heiko


Heiko Voigt (3):
  teach config parsing to read from strbuf
  implement fetching of moved submodules
  submodule: simplify decision tree whether to or not to fetch

 .gitignore                  |   1 +
 Makefile                    |   2 +
 cache.h                     |   1 +
 config.c                    | 119 +++++++++++++++++-----
 submodule-config-cache.c    |  96 ++++++++++++++++++
 submodule-config-cache.h    |  34 +++++++
 submodule.c                 | 242 ++++++++++++++++++++++++++++++++++++--------
 t/t1300-repo-config.sh      |   4 +
 t/t5526-fetch-submodules.sh |  31 ++++++
 test-config.c               |  41 ++++++++
 10 files changed, 502 insertions(+), 69 deletions(-)
 create mode 100644 submodule-config-cache.c
 create mode 100644 submodule-config-cache.h
 create mode 100644 test-config.c

-- 
1.8.2.rc0.25.g5062c01

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

[Heiko Voigt]

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

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 .gitignore             |   1 +
 Makefile               |   1 +
 cache.h                |   1 +
 config.c               | 119 ++++++++++++++++++++++++++++++++++++++-----------
 t/t1300-repo-config.sh |   4 ++
 test-config.c          |  41 +++++++++++++++++
 6 files changed, 142 insertions(+), 25 deletions(-)
 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 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;
@@ -24,6 +27,46 @@ static config_file *cf;
 
 static int zlib_compression_seen;
 
+static int config_file_valid(struct config_file *file)
+{
+	if (file->f)
+		return 1;
+	if (file->is_strbuf)
+		return 1;
+
+	return 0;
+}
+
+static int config_fgetc(struct config_file *file)
+{
+	if (!file->is_strbuf)
+		return fgetc(file->f);
+
+	if (file->strbuf_pos < file->strbuf_contents->len)
+		return file->strbuf_contents->buf[file->strbuf_pos++];
+
+	return EOF;
+}
+
+static int config_ungetc(int c, struct config_file *file)
+{
+	if (!file->is_strbuf)
+		return ungetc(c, file->f);
+
+	if (file->strbuf_pos > 0)
+		return file->strbuf_contents->buf[--file->strbuf_pos];
+
+	return EOF;
+}
+
+static long config_ftell(struct config_file *file)
+{
+	if (!file->is_strbuf)
+		return ftell(file->f);
+
+	return file->strbuf_pos;
+}
+
 #define MAX_INCLUDE_DEPTH 10
 static const char include_depth_advice[] =
 "exceeded maximum include depth (%d) while including\n"
@@ -169,16 +212,15 @@ 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)) {
-		c = fgetc(f);
+	if (cf && config_file_valid(cf)) {
+		c = config_fgetc(cf);
 		if (c == '\r') {
 			/* DOS like systems */
-			c = fgetc(f);
+			c = config_fgetc(cf);
 			if (c != '\n') {
-				ungetc(c, f);
+				config_ungetc(c, cf);
 				c = '\r';
 			}
 		}
@@ -896,6 +938,38 @@ 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;
+}
+
+static void config_file_init(struct config_file *top, const char *filename,
+			     FILE *f, struct strbuf *string)
+{
+	top->f = f;
+	top->name = filename;
+	top->is_strbuf = f ? 0 : 1;
+	top->strbuf_contents = string;
+	top->strbuf_pos = 0;
+}
+
 int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 {
 	int ret;
@@ -905,28 +979,24 @@ 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);
+		config_file_init(&top, filename, f, NULL);
 
-		/* 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);
 	}
 	return ret;
 }
 
+int git_config_from_strbuf(config_fn_t fn, struct strbuf *strbuf, void *data)
+{
+	config_file top;
+
+	config_file_init(&top, NULL, NULL, strbuf);
+
+	return do_config_from(&top, fn, data);
+}
+
 const char *git_etc_gitconfig(void)
 {
 	static const char *system_wide;
@@ -1053,7 +1123,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:
@@ -1065,7 +1134,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] = config_ftell(cf);
 			store.seen++;
 		}
 		break;
@@ -1092,19 +1161,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] = config_ftell(cf);
 		/* fallthru */
 	case SECTION_END_SEEN:
 	case START:
 		if (matches(key, value)) {
-			store.offset[store.seen] = ftell(f);
+			store.offset[store.seen] = config_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] = config_ftell(cf);
 			}
 		}
 	}
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.25.g5062c01

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

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

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

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

[Heiko Voigt]

Hi Junio,

On Sun, Feb 24, 2013 at 09:54:43PM -0800, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> > 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

[PATCH 0/4] allow more sources for config values

[Heiko Voigt]

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

[PATCH 1/4] config: factor out config file stack management

[Heiko Voigt]

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 <hvoigt@hvoigt.net>
---

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 <peff@peff.net>

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 = &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

Re: [PATCH 1/4] config: factor out config file stack management

[Jeff King]

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 <hvoigt@hvoigt.net>
> ---
> 
> 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 <peff@peff.net>
> 
> 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

Re: [PATCH 1/4] config: factor out config file stack management

[Heiko Voigt]

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

Re: [PATCH 1/4] config: factor out config file stack management

[Jeff King]

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

Re: [PATCH 1/4] config: factor out config file stack management

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

Re: [PATCH 1/4] config: factor out config file stack management

[Heiko Voigt]

On Tue, Feb 26, 2013 at 02:10:03PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> 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

Re: [PATCH 1/4] config: factor out config file stack management

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> 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.

Re: [PATCH 1/4] config: factor out config file stack management

[Heiko Voigt]

On Tue, Feb 26, 2013 at 02:12:23PM -0800, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> 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

[PATCH 2/4] config: drop file pointer validity check in get_next_char()

[Heiko Voigt]

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 <hvoigt@hvoigt.net>
---
 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

Re: [PATCH 2/4] config: drop file pointer validity check in get_next_char()

[Jeff King]

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

Re: [PATCH 2/4] config: drop file pointer validity check in get_next_char()

[Heiko Voigt]

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:
> 
> > 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?

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

Cheers Heiko

Re: [PATCH 2/4] config: drop file pointer validity check in get_next_char()

[Heiko Voigt]

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:
> > 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?
> 
> 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

Re: [PATCH 2/4] config: drop file pointer validity check in get_next_char()

[Heiko Voigt]

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:
> > 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:
> > > 
> > > > 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?
> > 
> > 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

[PATCH 3/4] config: make parsing stack struct independent from actual data source

[Heiko Voigt]

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 <hvoigt@hvoigt.net>
---
 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

[PATCH 4/4] teach config parsing to read from strbuf

[Heiko Voigt]

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

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 .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

Re: [PATCH 4/4] teach config parsing to read from strbuf

[Ramsay Jones]

Heiko Voigt wrote:
> This can be used to read configuration values directly from gits
> database.
> 
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
>  .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 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;
> +}
> 

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

Re: [PATCH 4/4] teach config parsing to read from strbuf

[Heiko Voigt]

Hi,

On Thu, Mar 07, 2013 at 06:42:43PM +0000, 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

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

[Jeff King]

On Mon, Feb 25, 2013 at 02:04:18AM +0100, Heiko Voigt wrote:

> This can be used to read configuration values directly from gits
> database.
> 
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>

FWIW, I implemented something quite similar as a 2-patch series here:

  http://article.gmane.org/gmane.comp.version-control.git/189142

  http://article.gmane.org/gmane.comp.version-control.git/189143

but they were never merged as we ended up throwing out the feature built
on top (including config from blobs). I didn't look too closely at your
implementation, but it looks like you touched the same spots in the same
way. Which is probably a good thing, and may give another data point for
Junio's "what would it look like to read another source" comment.

-Peff

[RFC/WIP PATCH 2/3] implement fetching of moved submodules

[Heiko Voigt]

Change calculation of changed submodule paths to read revisions config.

We now read the submodule name for every changed submodule path in a
commit. We then use the submodules names for fetching instead of the
submodule paths.

We lazily read all configurations of changed submodule path into a cache
which can then be used to lookup the configuration for a certain revision
and path or name.

It is expected that .gitmodules files do not change often between
commits. Thats why we lookup the .gitmodules sha1 and use a submodule
configuration cache to lazily lookup the parsed result for a submodule
stored by its name.

This cache could be reused for other purposes which need knowledge about
submodule configurations. For example automatic clone on fetch of a new
submodule if it is configured to be automatically initialized.

The submodule configuration in the current worktree can be stored using
the null sha1.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 Makefile                    |   1 +
 submodule-config-cache.c    |  96 ++++++++++++++++++++++
 submodule-config-cache.h    |  34 ++++++++
 submodule.c                 | 196 ++++++++++++++++++++++++++++++++++++++------
 t/t5526-fetch-submodules.sh |  31 +++++++
 5 files changed, 335 insertions(+), 23 deletions(-)
 create mode 100644 submodule-config-cache.c
 create mode 100644 submodule-config-cache.h

diff --git a/Makefile b/Makefile
index 98da708..2e1d83b 100644
--- a/Makefile
+++ b/Makefile
@@ -858,6 +858,7 @@ LIB_OBJS += strbuf.o
 LIB_OBJS += streaming.o
 LIB_OBJS += string-list.o
 LIB_OBJS += submodule.o
+LIB_OBJS += submodule-config-cache.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
 LIB_OBJS += trace.o
diff --git a/submodule-config-cache.c b/submodule-config-cache.c
new file mode 100644
index 0000000..8ea5ac4
--- /dev/null
+++ b/submodule-config-cache.c
@@ -0,0 +1,96 @@
+#include "cache.h"
+#include "submodule-config-cache.h"
+#include "strbuf.h"
+#include "hash.h"
+
+void submodule_config_cache_init(struct submodule_config_cache *cache)
+{
+	init_hash(&cache->for_name);
+	init_hash(&cache->for_path);
+}
+
+static int free_one_submodule_config(void *ptr, void *data)
+{
+	struct submodule_config *entry = ptr;
+
+	strbuf_release(&entry->path);
+	strbuf_release(&entry->name);
+	free(entry);
+
+	return 0;
+}
+
+void submodule_config_cache_free(struct submodule_config_cache *cache)
+{
+	/* NOTE: its important to iterate over the name hash here
+	 * since paths might have multiple entries */
+	for_each_hash(&cache->for_name, free_one_submodule_config, NULL);
+	free_hash(&cache->for_path);
+	free_hash(&cache->for_name);
+}
+
+static unsigned int hash_sha1_string(const unsigned char *sha1, const char *string)
+{
+	int c;
+	unsigned int hash, string_hash = 5381;
+	memcpy(&hash, sha1, sizeof(hash));
+
+	/* djb2 hash */
+	while ((c = *string++))
+		string_hash = ((string_hash << 5) + hash) + c; /* hash * 33 + c */
+
+	return hash + string_hash;
+}
+
+void submodule_config_cache_update_path(struct submodule_config_cache *cache,
+		struct submodule_config *config)
+{
+	void **pos;
+	int hash = hash_sha1_string(config->gitmodule_sha1, config->path.buf);
+	pos = insert_hash(hash, config, &cache->for_path);
+	if (pos) {
+		config->next = *pos;
+		*pos = config;
+	}
+}
+
+void submodule_config_cache_insert(struct submodule_config_cache *cache, struct submodule_config *config)
+{
+	unsigned int hash;
+	void **pos;
+
+	hash = hash_sha1_string(config->gitmodule_sha1, config->name.buf);
+	pos = insert_hash(hash, config, &cache->for_name);
+	if (pos) {
+		config->next = *pos;
+		*pos = config;
+	}
+}
+
+struct submodule_config *submodule_config_cache_lookup_path(struct submodule_config_cache *cache,
+	const unsigned char *gitmodule_sha1, const char *path)
+{
+	unsigned int hash = hash_sha1_string(gitmodule_sha1, path);
+	struct submodule_config *config = lookup_hash(hash, &cache->for_path);
+
+	while (config &&
+		hashcmp(config->gitmodule_sha1, gitmodule_sha1) &&
+		strcmp(path, config->path.buf))
+		config = config->next;
+
+	return config;
+}
+
+struct submodule_config *submodule_config_cache_lookup_name(struct submodule_config_cache *cache,
+	const unsigned char *gitmodule_sha1, const char *name)
+{
+	unsigned int hash = hash_sha1_string(gitmodule_sha1, name);
+	struct submodule_config *config = lookup_hash(hash, &cache->for_name);
+
+	while (config &&
+		hashcmp(config->gitmodule_sha1, gitmodule_sha1) &&
+		strcmp(name, config->name.buf))
+		config = config->next;
+
+	return config;
+}
diff --git a/submodule-config-cache.h b/submodule-config-cache.h
new file mode 100644
index 0000000..2b48c27
--- /dev/null
+++ b/submodule-config-cache.h
@@ -0,0 +1,34 @@
+#ifndef SUBMODULE_CONFIG_CACHE_H
+#define SUBMODULE_CONFIG_CACHE_H
+
+#include "hash.h"
+#include "strbuf.h"
+
+struct submodule_config_cache {
+	struct hash_table for_path;
+	struct hash_table for_name;
+};
+
+/* one submodule_config_cache entry */
+struct submodule_config {
+	struct strbuf path;
+	struct strbuf name;
+	unsigned char gitmodule_sha1[20];
+	int fetch_recurse_submodules;
+	struct submodule_config *next;
+};
+
+void submodule_config_cache_init(struct submodule_config_cache *cache);
+void submodule_config_cache_free(struct submodule_config_cache *cache);
+
+void submodule_config_cache_update_path(struct submodule_config_cache *cache,
+		struct submodule_config *config);
+void submodule_config_cache_insert(struct submodule_config_cache *cache,
+		struct submodule_config *config);
+
+struct submodule_config *submodule_config_cache_lookup_path(struct submodule_config_cache *cache,
+	const unsigned char *gitmodule_sha1, const char *path);
+struct submodule_config *submodule_config_cache_lookup_name(struct submodule_config_cache *cache,
+	const unsigned char *gitmodule_sha1, const char *name);
+
+#endif /* SUBMODULE_CONFIG_CACHE_H */
diff --git a/submodule.c b/submodule.c
index 9ba1496..b603000 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "submodule.h"
+#include "submodule-config-cache.h"
 #include "dir.h"
 #include "diff.h"
 #include "commit.h"
@@ -11,11 +12,12 @@
 #include "sha1-array.h"
 #include "argv-array.h"
 
+struct submodule_config_cache submodule_config_cache;
 static struct string_list config_name_for_path;
 static struct string_list config_fetch_recurse_submodules_for_name;
 static struct string_list config_ignore_for_name;
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
-static struct string_list changed_submodule_paths;
+static struct string_list changed_submodule_names;
 static int initialized_fetch_ref_tips;
 static struct sha1_array ref_tips_before_fetch;
 static struct sha1_array ref_tips_after_fetch;
@@ -487,34 +489,177 @@ static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
 	return is_present;
 }
 
+static int read_sha1_strbuf(struct strbuf *s, const unsigned char *sha1,
+			    enum object_type *type)
+{
+	unsigned long size;
+	void *sha1_data;
+
+	sha1_data = read_sha1_file(sha1, type, &size);
+	if (!sha1_data)
+		return 0;
+
+	strbuf_attach(s, sha1_data, size, size);
+
+	return size;
+}
+
+struct parse_submodule_config_parameter {
+	unsigned char *gitmodule_sha1;
+	struct submodule_config_cache *cache;
+};
+
+static int name_and_item_from_var(const char *var, struct strbuf *name, struct strbuf *item)
+{
+	/* find the name and add it */
+	strbuf_addstr(name, var + strlen("submodule."));
+	char *end = strrchr(name->buf, '.');
+	if (!end) {
+		strbuf_release(name);
+		return 0;
+	}
+	*end = '\0';
+	if (((end + 1) - name->buf) < name->len)
+		strbuf_addstr(item, end + 1);
+
+	return 1;
+}
+
+static struct submodule_config *lookup_or_create_by_name(struct submodule_config_cache *cache,
+		unsigned char *gitmodule_sha1, const char *name)
+{
+	struct submodule_config *config;
+	config = submodule_config_cache_lookup_name(cache, gitmodule_sha1, name);
+	if (config)
+		return config;
+
+	config = xmalloc(sizeof(*config));
+
+	strbuf_init(&config->name, 1024);
+	strbuf_addstr(&config->name, name);
+
+	strbuf_init(&config->path, 1024);
+
+	hashcpy(config->gitmodule_sha1, gitmodule_sha1);
+	config->fetch_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+	config->next = NULL;
+
+	submodule_config_cache_insert(cache, config);
+
+	return config;
+}
+
+static void warn_multiple_config(struct submodule_config *config, const char *option)
+{
+	warning("%s:.gitmodules, multiple configurations found for submodule.%s.%s. "
+			"Skipping second one!", sha1_to_hex(config->gitmodule_sha1),
+			option, config->name.buf);
+}
+
+static int parse_submodule_config_into_cache(const char *var, const char *value, void *data)
+{
+	struct parse_submodule_config_parameter *me = data;
+	struct submodule_config *submodule_config;
+
+	/* We only read submodule.<name> entries */
+	if (prefixcmp(var, "submodule."))
+		return 0;
+
+	struct strbuf name = STRBUF_INIT, item = STRBUF_INIT;
+	if (!name_and_item_from_var(var, &name, &item))
+		return 0;
+
+	submodule_config = lookup_or_create_by_name(me->cache, me->gitmodule_sha1, name.buf);
+
+	if (!suffixcmp(var, ".path")) {
+		if (*submodule_config->path.buf != '\0') {
+			warn_multiple_config(submodule_config, "path");
+			return 0;
+		}
+		strbuf_addstr(&submodule_config->path, value);
+		submodule_config_cache_update_path(me->cache, submodule_config);
+	}
+
+	if (!suffixcmp(var, ".fetchrecursesubmodules")) {
+		if (submodule_config->fetch_recurse_submodules != RECURSE_SUBMODULES_DEFAULT) {
+			warn_multiple_config(submodule_config, "fetchrecursesubmodules");
+			return 0;
+		}
+		submodule_config->fetch_recurse_submodules =
+			parse_fetch_recurse_submodules_arg(var, value);
+	}
+
+	strbuf_release(&name);
+	strbuf_release(&item);
+
+	return 0;
+}
+
+static struct submodule_config *get_submodule_config_for_commit_path(struct submodule_config_cache *cache,
+		const unsigned char *commit_sha1, const char *path)
+{
+	struct strbuf rev = STRBUF_INIT;
+	struct strbuf config = STRBUF_INIT;
+	unsigned char sha1[20];
+	enum object_type type;
+	struct submodule_config *submodule_config;
+	struct parse_submodule_config_parameter parameter;
+
+	strbuf_addf(&rev, "%s:.gitmodules", sha1_to_hex(commit_sha1));
+	if (get_sha1(rev.buf, sha1) < 0) {
+		strbuf_release(&rev);
+		return NULL;
+	}
+	strbuf_release(&rev);
+
+	submodule_config = submodule_config_cache_lookup_path(cache, sha1, path);
+	if (submodule_config)
+		return submodule_config;
+
+	if (!read_sha1_strbuf(&config, sha1, &type))
+		return NULL;
+
+	if (type != OBJ_BLOB) {
+		strbuf_release(&config);
+		return NULL;
+	}
+
+	/* fill the submodule config into the cache */
+	parameter.cache = cache;
+	parameter.gitmodule_sha1 = sha1;
+	git_config_from_strbuf(parse_submodule_config_into_cache, &config, &parameter);
+	strbuf_release(&config);
+
+	return submodule_config_cache_lookup_path(cache, sha1, path);
+}
+
 static void submodule_collect_changed_cb(struct diff_queue_struct *q,
 					 struct diff_options *options,
 					 void *data)
 {
+	const unsigned char *commit_sha1 = data;
 	int i;
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
+		struct string_list_item *name_item;
+		struct submodule_config *submodule_config;
+
 		if (!S_ISGITLINK(p->two->mode))
 			continue;
 
-		if (S_ISGITLINK(p->one->mode)) {
-			/* NEEDSWORK: We should honor the name configured in
-			 * the .gitmodules file of the commit we are examining
-			 * here to be able to correctly follow submodules
-			 * being moved around. */
-			struct string_list_item *path;
-			path = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);
-			if (!path && !is_submodule_commit_present(p->two->path, p->two->sha1))
-				string_list_append(&changed_submodule_paths, xstrdup(p->two->path));
-		} else {
-			/* Submodule is new or was moved here */
-			/* NEEDSWORK: When the .git directories of submodules
-			 * live inside the superprojects .git directory some
-			 * day we should fetch new submodules directly into
-			 * that location too when config or options request
-			 * that so they can be checked out from there. */
+		submodule_config = get_submodule_config_for_commit_path(&submodule_config_cache,
+				commit_sha1, p->two->path);
+		if (!submodule_config)
 			continue;
-		}
+
+		name_item = unsorted_string_list_lookup(&changed_submodule_names, submodule_config->name.buf);
+		if (name_item)
+			continue;
+
+		if (is_submodule_commit_present(p->two->path, p->two->sha1))
+			continue;
+
+		string_list_append(&changed_submodule_names, xstrdup(submodule_config->name.buf));
 	}
 }
 
@@ -550,6 +695,8 @@ static void calculate_changed_submodule_paths(void)
 	if (!config_name_for_path.nr)
 		return;
 
+	submodule_config_cache_init(&submodule_config_cache);
+
 	init_revisions(&rev, NULL);
 	argv_array_push(&argv, "--"); /* argv[0] program name */
 	sha1_array_for_each_unique(&ref_tips_after_fetch,
@@ -563,7 +710,7 @@ static void calculate_changed_submodule_paths(void)
 
 	/*
 	 * Collect all submodules (whether checked out or not) for which new
-	 * commits have been recorded upstream in "changed_submodule_paths".
+	 * commits have been recorded upstream in "changed_submodule_names".
 	 */
 	while ((commit = get_revision(&rev))) {
 		struct commit_list *parent = commit->parents;
@@ -573,6 +720,7 @@ static void calculate_changed_submodule_paths(void)
 			DIFF_OPT_SET(&diff_opts, RECURSIVE);
 			diff_opts.output_format |= DIFF_FORMAT_CALLBACK;
 			diff_opts.format_callback = submodule_collect_changed_cb;
+			diff_opts.format_callback_data = commit->object.sha1;
 			diff_setup_done(&diff_opts);
 			diff_tree_sha1(parent->item->object.sha1, commit->object.sha1, "", &diff_opts);
 			diffcore_std(&diff_opts);
@@ -585,6 +733,8 @@ static void calculate_changed_submodule_paths(void)
 	sha1_array_clear(&ref_tips_before_fetch);
 	sha1_array_clear(&ref_tips_after_fetch);
 	initialized_fetch_ref_tips = 0;
+
+	submodule_config_cache_free(&submodule_config_cache);
 }
 
 int fetch_populated_submodules(const struct argv_array *options,
@@ -639,7 +789,7 @@ int fetch_populated_submodules(const struct argv_array *options,
 				if ((intptr_t)fetch_recurse_submodules_option->util == RECURSE_SUBMODULES_OFF)
 					continue;
 				if ((intptr_t)fetch_recurse_submodules_option->util == RECURSE_SUBMODULES_ON_DEMAND) {
-					if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
+					if (!unsorted_string_list_lookup(&changed_submodule_names, name))
 						continue;
 					default_argv = "on-demand";
 				}
@@ -648,13 +798,13 @@ int fetch_populated_submodules(const struct argv_array *options,
 				    gitmodules_is_unmerged)
 					continue;
 				if (config_fetch_recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) {
-					if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
+					if (!unsorted_string_list_lookup(&changed_submodule_names, name))
 						continue;
 					default_argv = "on-demand";
 				}
 			}
 		} else if (command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {
-			if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
+			if (!unsorted_string_list_lookup(&changed_submodule_names, name))
 				continue;
 			default_argv = "on-demand";
 		}
@@ -685,7 +835,7 @@ int fetch_populated_submodules(const struct argv_array *options,
 	}
 	argv_array_clear(&argv);
 out:
-	string_list_clear(&changed_submodule_paths, 1);
+	string_list_clear(&changed_submodule_names, 1);
 	return result;
 }
 
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index ca5b027..7ee4c2b 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -450,4 +450,35 @@ test_expect_success "don't fetch submodule when newly recorded commits are alrea
 	test_i18ncmp expect.err actual.err
 '
 
+test_expect_success "fetch new commits when submodule got renamed" '
+	(
+		cd submodule &&
+		git checkout -b rename_sub &&
+		echo a >a &&
+		git add a &&
+		git commit -ma &&
+		git rev-parse HEAD >../expect
+	) &&
+	git clone . downstream2 &&
+	(
+		cd downstream2 &&
+		git submodule update --init --recursive &&
+		git checkout -b rename &&
+		mv submodule submodule_renamed &&
+		git config -f .gitmodules submodule.submodule.path submodule_renamed &&
+		git add submodule_renamed .gitmodules &&
+		git commit -m "rename submodule -> submodule-renamed" &&
+		git push origin rename
+	) &&
+	(
+		cd downstream &&
+		git fetch --recurse-submodules=on-demand &&
+		(
+			cd submodule &&
+			git rev-parse origin/rename_sub >../../actual
+		)
+	) &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.2.rc0.25.g5062c01

[RFC/WIP PATCH 3/3] submodule: simplify decision tree whether to or not to fetch

[Heiko Voigt]

To make extending this logic later easier.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 submodule.c | 50 +++++++++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/submodule.c b/submodule.c
index b603000..a6fe16e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -737,6 +737,23 @@ static void calculate_changed_submodule_paths(void)
 	submodule_config_cache_free(&submodule_config_cache);
 }
 
+static int get_fetch_recurse_config(const char *name, int command_line_option)
+{
+	if (command_line_option != RECURSE_SUBMODULES_DEFAULT)
+		return command_line_option;
+
+	struct string_list_item *fetch_recurse_submodules_option;
+	fetch_recurse_submodules_option = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, name);
+	if (fetch_recurse_submodules_option)
+		/* local config overrules everything except commandline */
+		return (intptr_t)fetch_recurse_submodules_option->util;
+
+	if (gitmodules_is_unmerged)
+		return RECURSE_SUBMODULES_OFF;
+
+	return config_fetch_recurse_submodules;
+}
+
 int fetch_populated_submodules(const struct argv_array *options,
 			       const char *prefix, int command_line_option,
 			       int quiet)
@@ -781,32 +798,19 @@ int fetch_populated_submodules(const struct argv_array *options,
 		if (name_for_path)
 			name = name_for_path->util;
 
-		default_argv = "yes";
-		if (command_line_option == RECURSE_SUBMODULES_DEFAULT) {
-			struct string_list_item *fetch_recurse_submodules_option;
-			fetch_recurse_submodules_option = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, name);
-			if (fetch_recurse_submodules_option) {
-				if ((intptr_t)fetch_recurse_submodules_option->util == RECURSE_SUBMODULES_OFF)
-					continue;
-				if ((intptr_t)fetch_recurse_submodules_option->util == RECURSE_SUBMODULES_ON_DEMAND) {
-					if (!unsorted_string_list_lookup(&changed_submodule_names, name))
-						continue;
-					default_argv = "on-demand";
-				}
-			} else {
-				if ((config_fetch_recurse_submodules == RECURSE_SUBMODULES_OFF) ||
-				    gitmodules_is_unmerged)
-					continue;
-				if (config_fetch_recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) {
-					if (!unsorted_string_list_lookup(&changed_submodule_names, name))
-						continue;
-					default_argv = "on-demand";
-				}
-			}
-		} else if (command_line_option == RECURSE_SUBMODULES_ON_DEMAND) {
+		switch (get_fetch_recurse_config(name, command_line_option)) {
+		default:
+		case RECURSE_SUBMODULES_DEFAULT:
+		case RECURSE_SUBMODULES_ON_DEMAND:
 			if (!unsorted_string_list_lookup(&changed_submodule_names, name))
 				continue;
 			default_argv = "on-demand";
+			break;
+		case RECURSE_SUBMODULES_ON:
+			default_argv = "yes";
+			break;
+		case RECURSE_SUBMODULES_OFF:
+			continue;
 		}
 
 		strbuf_addf(&submodule_path, "%s/%s", work_tree, ce->name);
-- 
1.8.2.rc0.25.g5062c01