[RFC/WIP 0/3] Shared config

[RFC/WIP 0/3] Shared config

[Nguyễn Thái Ngọc Duy]

Another approach is [1]. I think this one is somewhat simpler and
exposes less risk by putting the shared config under version control.

This is not complete (for example, git-prune should learn about this),
but is there any serious flaw that I missed?

One thing that I haven't thought of is the shared config between
subprojects. Maybe there's a better approach for subprojects.

(The resistance to nd/struct-pathspec seems great. This is the second
time I come to address it and end up with something else)

[1] http://thread.gmane.org/gmane.comp.version-control.git/162309

Nguyễn Thái Ngọc Duy (3):
  config: read full file content before parsing
  config: add git_config_from_sha1() to read from a blob
  config: add core.sharedconfig

 cache.h       |    1 +
 config.c      |  110 +++++++++++++++++++++++++++++++++++++++++++--------------
 environment.c |    1 +
 3 files changed, 85 insertions(+), 27 deletions(-)

-- 
1.7.3.3.476.g893a9

[PATCH 1/3] config: read full file content before parsing

[Nguyễn Thái Ngọc Duy]

config files are usually short enough that we can read in full. This
allows the parser to parse in-memory config, for example from a blob.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 config.c |   62 +++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/config.c b/config.c
index f138c34..c8bf46f 100644
--- a/config.c
+++ b/config.c
@@ -12,7 +12,8 @@
 
 #define MAXNAME (256)
 
-static FILE *config_file;
+static char *config_file;
+static size_t config_file_size, config_file_pos;
 static const char *config_file_name;
 static int config_linenr;
 static int config_file_eof;
@@ -108,28 +109,26 @@ int git_config_parse_environment(void) {
 
 static int get_next_char(void)
 {
-	int c;
-	FILE *f;
+	size_t unread = config_file_size - config_file_pos;
+
+	if (config_file == NULL)
+		return '\n';
 
-	c = '\n';
-	if ((f = config_file) != NULL) {
-		c = fgetc(f);
+	if (unread) {
+		char c = config_file[config_file_pos++];
 		if (c == '\r') {
 			/* DOS like systems */
-			c = fgetc(f);
-			if (c != '\n') {
-				ungetc(c, f);
-				c = '\r';
-			}
+			if (unread > 1 &&
+			    config_file[config_file_pos] == '\n')
+				c = config_file[config_file_pos++];
 		}
 		if (c == '\n')
 			config_linenr++;
-		if (c == EOF) {
-			config_file_eof = 1;
-			c = '\n';
-		}
+		return c;
 	}
-	return c;
+
+	config_file_eof = 1;
+	return '\n';
 }
 
 static char *parse_value(void)
@@ -786,19 +785,27 @@ int git_default_config(const char *var, const char *value, void *dummy)
 
 int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 {
-	int ret;
-	FILE *f = fopen(filename, "r");
+	struct stat st;
+	int ret = -1, fd;
+
+	if (lstat(filename, &st))
+		return -1;
+	fd = open(filename, O_RDONLY);
+	if (fd == -1)
+		return -1;
 
-	ret = -1;
-	if (f) {
-		config_file = f;
+	config_file = xmalloc(st.st_size);
+	if (read_in_full(fd, config_file, st.st_size) == st.st_size) {
+		config_file_pos = 0;
+		config_file_size = st.st_size;
 		config_file_name = filename;
 		config_linenr = 1;
 		config_file_eof = 0;
 		ret = git_parse_file(fn, data);
-		fclose(f);
-		config_file_name = NULL;
 	}
+	close(fd);
+	free(config_file);
+	config_file_name = NULL;
 	return ret;
 }
 
@@ -921,7 +928,7 @@ static int store_aux(const char *key, const char *value, void *cb)
 				return 1;
 			}
 
-			store.offset[store.seen] = ftell(config_file);
+			store.offset[store.seen] = config_file_pos;
 			store.seen++;
 		}
 		break;
@@ -948,19 +955,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(config_file);
+		store.offset[store.seen] = config_file_pos;
 		/* fallthru */
 	case SECTION_END_SEEN:
 	case START:
 		if (matches(key, value)) {
-			store.offset[store.seen] = ftell(config_file);
+			store.offset[store.seen] = config_file_pos;
 			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(config_file);
+					store.offset[store.seen] = config_file_pos;
 			}
 		}
 	}
@@ -1380,6 +1387,7 @@ int git_config_rename_section(const char *old_name, const char *new_name)
 {
 	int ret = 0, remove = 0;
 	char *config_filename;
+	FILE *config_file;
 	struct lock_file *lock = xcalloc(sizeof(struct lock_file), 1);
 	int out_fd;
 	char buf[1024];
-- 
1.7.3.3.476.g893a9

[PATCH 2/3] config: add git_config_from_sha1() to read from a blob

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 config.c |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/config.c b/config.c
index c8bf46f..e7a9ff4 100644
--- a/config.c
+++ b/config.c
@@ -809,6 +809,40 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 	return ret;
 }
 
+static int git_config_from_sha1(config_fn_t fn, const char *sha1_name, void *data)
+{
+	unsigned char sha1[20];
+	enum object_type type;
+	unsigned long size;
+	int ret;
+
+	if (get_sha1(sha1_name, sha1)) {
+		error("bad shared config reference '%s'", sha1_name);
+		return -1;
+	}
+
+	config_file = read_sha1_file(sha1, &type, &size);
+	if (!config_file) {
+		error("bad shared config '%s'", sha1_name);
+		return -1;
+	}
+	if (type == OBJ_BLOB) {
+		config_file_pos = 0;
+		config_file_size = size;
+		config_file_name = sha1_name;
+		config_linenr = 1;
+		config_file_eof = 0;
+		ret = git_parse_file(fn, data);
+	}
+	else {
+		error("shared config '%s' is not a blob", sha1_name);
+		return -1;
+	}
+	free(config_file);
+	config_file_name = NULL;
+	return ret;
+}
+
 const char *git_etc_gitconfig(void)
 {
 	static const char *system_wide;
-- 
1.7.3.3.476.g893a9

Re: [PATCH 2/3] config: add git_config_from_sha1() to read from a blob

[Thiago Farina]

2010/12/9 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  config.c |   34 ++++++++++++++++++++++++++++++++++
>  1 files changed, 34 insertions(+), 0 deletions(-)
>
> diff --git a/config.c b/config.c
> index c8bf46f..e7a9ff4 100644
> --- a/config.c
> +++ b/config.c
> @@ -809,6 +809,40 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
>        return ret;
>  }
>

> +static int git_config_from_sha1(config_fn_t fn, const char *sha1_name, void *data)
> +{

Is worth documenting the return value of this function and what it
does? It returns 0 on success otherwise returns -1.

> +       unsigned char sha1[20];
> +       enum object_type type;
> +       unsigned long size;
> +       int ret;
> +
> +       if (get_sha1(sha1_name, sha1)) {
> +               error("bad shared config reference '%s'", sha1_name);
> +               return -1;
> +       }
> +
> +       config_file = read_sha1_file(sha1, &type, &size);
> +       if (!config_file) {
> +               error("bad shared config '%s'", sha1_name);
> +               return -1;
> +       }
> +       if (type == OBJ_BLOB) {
> +               config_file_pos = 0;
> +               config_file_size = size;
> +               config_file_name = sha1_name;
> +               config_linenr = 1;
> +               config_file_eof = 0;
> +               ret = git_parse_file(fn, data);
> +       }
> +       else {

style nit: Shouldn't this else be on the end of the previous line?

Re: [PATCH 2/3] config: add git_config_from_sha1() to read from a blob

[Jonathan Nieder]

Thiago Farina wrote:
> 2010/12/9 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:

>> +static int git_config_from_sha1(config_fn_t fn, const char *sha1_name, void *data)
>> +{
>
> Is worth documenting the return value of this function and what it
> does? It returns 0 on success otherwise returns -1.

If that is the return value, then no.  It is the usual in git (and
other C programs that follow libc conventions).

[PATCH 3/3] config: add core.sharedconfig

[Nguyễn Thái Ngọc Duy]

core.sharedconfig can take anything that resolves to a blob.
$GIT_DIR/config will override the shared config. Nested shared
config is not allowed.

No protection is provided. It's up to the project to maintain good
config. The config could be in a separate branch that only a few
people are allowed to push, for example. To be safest, just put SHA-1
there.

git-fsck and git-prune should learn about this key and protect it from
being pruned.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Hopefully nobody sets core.sharedconfig = :path/to/config or similar

 And I should not open $GIT_DIR/config twice. Well, it does not hurt
 much.

 cache.h       |    1 +
 config.c      |   14 ++++++++++++++
 environment.c |    1 +
 3 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/cache.h b/cache.h
index e83bc2d..e91ce35 100644
--- a/cache.h
+++ b/cache.h
@@ -559,6 +559,7 @@ extern int read_replace_refs;
 extern int fsync_object_files;
 extern int core_preload_index;
 extern int core_apply_sparse_checkout;
+extern const char *core_shared_config;
 
 enum safe_crlf {
 	SAFE_CRLF_FALSE = 0,
diff --git a/config.c b/config.c
index e7a9ff4..735b3f4 100644
--- a/config.c
+++ b/config.c
@@ -883,6 +883,14 @@ int git_config_from_parameters(config_fn_t fn, void *data)
 	return 0;
 }
 
+static int get_shared_config(const char *var, const char *value, void *dummy)
+{
+	if (!strcmp(var, "core.sharedconfig"))
+		return git_config_string(&core_shared_config, var, value);
+
+	return 0;
+}
+
 int git_config(config_fn_t fn, void *data)
 {
 	int ret = 0, found = 0;
@@ -910,6 +918,12 @@ int git_config(config_fn_t fn, void *data)
 
 	repo_config = git_pathdup("config");
 	if (!access(repo_config, R_OK)) {
+		git_config_from_file(get_shared_config, repo_config, NULL);
+		if (core_shared_config) {
+			ret += git_config_from_sha1(fn, core_shared_config, data);
+			found += 1;
+		}
+
 		ret += git_config_from_file(fn, repo_config, data);
 		found += 1;
 	}
diff --git a/environment.c b/environment.c
index 913b058..8bfb548 100644
--- a/environment.c
+++ b/environment.c
@@ -55,6 +55,7 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
 char *notes_ref_name;
 int grafts_replace_parents = 1;
 int core_apply_sparse_checkout;
+const char *core_shared_config;
 struct startup_info *startup_info;
 
 /* Parallel index stat data preload? */
-- 
1.7.3.3.476.g893a9

Re: [PATCH 3/3] config: add core.sharedconfig

[Thiago Farina]

2010/12/9 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> core.sharedconfig can take anything that resolves to a blob.
> $GIT_DIR/config will override the shared config. Nested shared
> config is not allowed.
>
> No protection is provided. It's up to the project to maintain good
> config. The config could be in a separate branch that only a few
> people are allowed to push, for example. To be safest, just put SHA-1
> there.
>
> git-fsck and git-prune should learn about this key and protect it from
> being pruned.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Hopefully nobody sets core.sharedconfig = :path/to/config or similar
>
>  And I should not open $GIT_DIR/config twice. Well, it does not hurt
>  much.
>
>  cache.h       |    1 +
>  config.c      |   14 ++++++++++++++
>  environment.c |    1 +
>  3 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index e83bc2d..e91ce35 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -559,6 +559,7 @@ extern int read_replace_refs;
>  extern int fsync_object_files;
>  extern int core_preload_index;
>  extern int core_apply_sparse_checkout;
> +extern const char *core_shared_config;
>

Why you need to export this string? Isn't it used only in config.c?

Re: [PATCH 3/3] config: add core.sharedconfig

[Nguyen Thai Ngoc Duy]

2010/12/9 Thiago Farina <tfransosi@gmail.com>:
>> diff --git a/cache.h b/cache.h
>> index e83bc2d..e91ce35 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -559,6 +559,7 @@ extern int read_replace_refs;
>>  extern int fsync_object_files;
>>  extern int core_preload_index;
>>  extern int core_apply_sparse_checkout;
>> +extern const char *core_shared_config;
>>
>
> Why you need to export this string? Isn't it used only in config.c?
>

git-prune should know about this but I haven't got that far.
-- 
Duy

Re: [PATCH 3/3] config: add core.sharedconfig

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> core.sharedconfig can take anything that resolves to a blob.
> $GIT_DIR/config will override the shared config. Nested shared
> config is not allowed.
>
> No protection is provided. It's up to the project to maintain good
> config. The config could be in a separate branch that only a few
> people are allowed to push, for example. To be safest, just put SHA-1
> there.
>
> git-fsck and git-prune should learn about this key and protect it from
> being pruned.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Hopefully nobody sets core.sharedconfig = :path/to/config or similar
>
>  And I should not open $GIT_DIR/config twice. Well, it does not hurt
>  much.

That kind of sloppy thinking adds up, though.

> @@ -910,6 +918,12 @@ int git_config(config_fn_t fn, void *data)
>  
>  	repo_config = git_pathdup("config");
>  	if (!access(repo_config, R_OK)) {
> +		git_config_from_file(get_shared_config, repo_config, NULL);
> +		if (core_shared_config) {
> +			ret += git_config_from_sha1(fn, core_shared_config, data);
> +			found += 1;
> +		}
> +

What is the point of this "found++" when you will increment it for finding
the repository-local configuration anyway?

>  		ret += git_config_from_file(fn, repo_config, data);
>  		found += 1;
>  	}

I originally liked what the first two tried to do, but think about the use
case.  How would this whole thing work?

 - The user clones from the project to get a repository with a working
   tree;

 - The user somehow learns that s/he can run one command to get
   project-wide preference of the project:

     $ git config core.sharedconfig refs/remotes/origin/config:git.config

 - Everything hopefully should work the way project wishes in that blob,
   unless the end user later overrides them by adding different settings
   to .git/config.

How is that different from:

 - The user clones from the project to get a repository with a working
   tree;

 - The user somehow learns that s/he can run one command to get
   project-wide preference of the project:

     $ ./setup-project-preference.sh

   Typically, such a ./setup-project-preference.sh script would only
   consist of a series of "git config $foo $bar", so any user who can say
   "git config core.sharedconfig $foo" should be able to use it as well.

 - Everything should work the way project wishes with the settings made to
   .git/config by the script, unless the end user later overrides them by
   modifying settings in .git/config.

One minor difference is that some configuration variables are additive,
and you cannot subtract from them with your approach.

Re: [PATCH 3/3] config: add core.sharedconfig

[Jonathan Nieder]

Junio C Hamano wrote:

> I originally liked what the first two tried to do, but think about the use
> case.  How would this whole thing work?
> 
>  - The user clones from the project to get a repository with a working
>    tree;
> 
>  - The user somehow learns that s/he can run one command to get
>    project-wide preference of the project:
> 
>      $ git config core.sharedconfig refs/remotes/origin/config:git.config
> 
>  - Everything hopefully should work the way project wishes in that blob,
>    unless the end user later overrides them by adding different settings
>    to .git/config.
> 
> How is that different from:
> 
>  - The user clones from the project to get a repository with a working
>    tree;
> 
>  - The user somehow learns that s/he can run one command to get
>    project-wide preference of the project:
> 
>      $ ./setup-project-preference.sh
> 
>    Typically, such a ./setup-project-preference.sh script would only
>    consist of a series of "git config $foo $bar", so any user who can say
>    "git config core.sharedconfig $foo" should be able to use it as well.

Wouldn't this ./setup-project-preference.sh have to set up a post-fetch hook
to update the configuration when the project's preferences change?

Re: [PATCH 3/3] config: add core.sharedconfig

[Nguyen Thai Ngoc Duy]

2010/12/10 Junio C Hamano <gitster@pobox.com>:
>> @@ -910,6 +918,12 @@ int git_config(config_fn_t fn, void *data)
>>
>>       repo_config = git_pathdup("config");
>>       if (!access(repo_config, R_OK)) {
>> +             git_config_from_file(get_shared_config, repo_config, NULL);
>> +             if (core_shared_config) {
>> +                     ret += git_config_from_sha1(fn, core_shared_config, data);
>> +                     found += 1;
>> +             }
>> +
>
> What is the point of this "found++" when you will increment it for finding
> the repository-local configuration anyway?

It seems that every time a config source is used, found++. So I
increase it because there's is another source used, a blob.

> I originally liked what the first two tried to do, but think about the use
> case.  How would this whole thing work?
>
>  - The user clones from the project to get a repository with a working
>   tree;
>
>  - The user somehow learns that s/he can run one command to get
>   project-wide preference of the project:
>
>     $ git config core.sharedconfig refs/remotes/origin/config:git.config
>
>  - Everything hopefully should work the way project wishes in that blob,
>   unless the end user later overrides them by adding different settings
>   to .git/config.
>
> How is that different from:
>
>  - The user clones from the project to get a repository with a working
>   tree;
>
>  - The user somehow learns that s/he can run one command to get
>   project-wide preference of the project:
>
>     $ ./setup-project-preference.sh
>
>   Typically, such a ./setup-project-preference.sh script would only
>   consist of a series of "git config $foo $bar", so any user who can say
>   "git config core.sharedconfig $foo" should be able to use it as well.
>
>  - Everything should work the way project wishes with the settings made to
>   .git/config by the script, unless the end user later overrides them by
>   modifying settings in .git/config.

The shared config can be updated automatically (after fetching, of
course). Remote setup for a small team, for example, can benefit from
this. Every time a new member joins, somebody adds new remote to the
shared config and pushes out. Everybody else will have it.

> One minor difference is that some configuration variables are additive,
> and you cannot subtract from them with your approach.

You can't subtract some keys from $HOME/.gitconfig by modifying
$GIT_DIR/config either. I mean, that could be addressed in a general
way, not specific to shared config.
-- 
Duy