threads / patch / 48781

v2, 6 partsconfig: move config_from_gitmodules to submodule-config.c

Subject: [PATCH v2 1/6] config: move config_from_gitmodules to submodule-config.c

## tl;dr

14 messages between Jun 26, 2018 and Jun 26, 2018. Diffs are folded; open one to read it.

replies: 13people: 3as markdown or json

Antonio Ospite· Jun 26, 2018, 10:47 UTC · lore

[PATCH v2 0/6] Restrict the usage of config_from_gitmodules to submodule-config

Hi,

this is version 2 of the series from https://public-inbox.org/git/20180622162656.19338-1-ao2@ao2.it/

The .gitmodules file is not meant for arbitrary configuration, it should be used only for submodules properties.

Plus, arbitrary git configuration should not be distributed with the repository, and .gitmodules might be a possible "vector" for that.

The series tries to alleviate both these issues by moving the 'config_from_gitmodules' function from config.[ch] to submodule-config.c and making it private.

This should discourage future code from using the function with arbitrary config callbacks which might turn .gitmodules into a mechanism to load arbitrary configuration stored in the repository.

Backward compatibility exceptions to the rules above are handled by ad-hoc helpers.

Finally (in patch 6) some duplication is removed by using 'config_from_gitmodules' to load the submodules configuration in 'repo_read_gitmodules'.

Changes since v1:
  * Remove an extra space before an arrow operator in patch 2
  * Fix a typo in the commit message of patch 3: s/fetchobjs/fetchjobs
  * Add a note in the commit message of patch 6 about checking the
    worktree before loading .gitmodules
  * Drop patch 7, it was meant as a cleanup but resulted in parsing the
    .gitmodules file twice

The series has been rebased on commit ed843436d ("First batch for 2.19 cycle", 2018-06-25) , and the test suite passes after each commit.

Thanks to Brandon Williams and Stefan Beller for the input.
Ciao,
   Antonio
Antonio Ospite (6):
  config: move config_from_gitmodules to submodule-config.c
  submodule-config: add helper function to get 'fetch' config from
    .gitmodules
  submodule-config: add helper to get 'update-clone' config from
    .gitmodules
  submodule-config: make 'config_from_gitmodules' private
  submodule-config: pass repository as argument to
    config_from_gitmodules
  submodule-config: reuse config_from_gitmodules in repo_read_gitmodules
 builtin/fetch.c             | 15 +-------
 builtin/submodule--helper.c |  8 ++--
 config.c                    | 17 ---------
 config.h                    | 10 -----
 submodule-config.c          | 75 +++++++++++++++++++++++++++++++------
 submodule-config.h          | 12 ++++++
 6 files changed, 80 insertions(+), 57 deletions(-)
-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Antonio Ospite· Jun 26, 2018, 10:47 UTC · re: Antonio Ospite · lore

The .gitmodules file is not meant as a place to store arbitrary configuration to distribute with the repository.

Move config_from_gitmodules() out of config.c and into submodule-config.c to make it even clearer that it is not a mechanism to retrieve arbitrary configuration from the .gitmodules file.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 config.c           | 17 -----------------
 config.h           | 10 ----------
 submodule-config.c | 17 +++++++++++++++++
 submodule-config.h | 11 +++++++++++
 4 files changed, 28 insertions(+), 27 deletions(-)
Show changes to 4 files +28 −27

config.c, config.h, submodule-config.c, submodule-config.h

diff --git a/config.c b/config.c
index a0a6ae198..fa78b1ff9 100644
--- a/config.c
+++ b/config.c
@@ -2172,23 +2172,6 @@ int git_config_get_pathname(const char *key, const char **dest)
 	return repo_config_get_pathname(the_repository, key, dest);
 }
 
-/*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
- *
- * Runs the provided config function on the '.gitmodules' file found in the
- * working directory.
- */
-void config_from_gitmodules(config_fn_t fn, void *data)
-{
-	if (the_repository->worktree) {
-		char *file = repo_worktree_path(the_repository, GITMODULES_FILE);
-		git_config_from_file(fn, file, data);
-		free(file);
-	}
-}
-
 int git_config_get_expiry(const char *key, const char **output)
 {
 	int ret = git_config_get_string_const(key, output);
diff --git a/config.h b/config.h
index 626d4654b..b95bb7649 100644
--- a/config.h
+++ b/config.h
@@ -215,16 +215,6 @@ extern int repo_config_get_maybe_bool(struct repository *repo,
 extern int repo_config_get_pathname(struct repository *repo,
 				    const char *key, const char **dest);
 
-/*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
- *
- * Runs the provided config function on the '.gitmodules' file found in the
- * working directory.
- */
-extern void config_from_gitmodules(config_fn_t fn, void *data);
-
 extern int git_config_get_value(const char *key, const char **value);
 extern const struct string_list *git_config_get_value_multi(const char *key);
 extern void git_config_clear(void);
diff --git a/submodule-config.c b/submodule-config.c
index 388ef1f89..b431555db 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -671,3 +671,20 @@ void submodule_free(struct repository *r)
 	if (r->submodule_cache)
 		submodule_cache_clear(r->submodule_cache);
 }
+
+/*
+ * Note: This function exists solely to maintain backward compatibility with
+ * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
+ * NOT be used anywhere else.
+ *
+ * Runs the provided config function on the '.gitmodules' file found in the
+ * working directory.
+ */
+void config_from_gitmodules(config_fn_t fn, void *data)
+{
+	if (the_repository->worktree) {
+		char *file = repo_worktree_path(the_repository, GITMODULES_FILE);
+		git_config_from_file(fn, file, data);
+		free(file);
+	}
+}
diff --git a/submodule-config.h b/submodule-config.h
index ca1f94e2d..5148801f4 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -2,6 +2,7 @@
 #define SUBMODULE_CONFIG_CACHE_H
 
 #include "cache.h"
+#include "config.h"
 #include "hashmap.h"
 #include "submodule.h"
 #include "strbuf.h"
@@ -55,4 +56,14 @@ void submodule_free(struct repository *r);
  */
 int check_submodule_name(const char *name);
 
+/*
+ * Note: This function exists solely to maintain backward compatibility with
+ * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
+ * NOT be used anywhere else.
+ *
+ * Runs the provided config function on the '.gitmodules' file found in the
+ * working directory.
+ */
+extern void config_from_gitmodules(config_fn_t fn, void *data);
+
 #endif /* SUBMODULE_CONFIG_H */

-- 
2.18.0
Antonio Ospite· Jun 26, 2018, 10:47 UTC · re: Antonio Ospite · lore

[PATCH v2 6/6] submodule-config: reuse config_from_gitmodules in repo_read_gitmodules

Reuse config_from_gitmodules in repo_read_gitmodules to remove some duplication and also have a single point where the .gitmodules file is read.

The change does not introduce any new behavior, the same gitmodules_cb config callback is still used, which only deals with configuration specific to submodules.

The check about the repo's worktree is removed from repo_read_gitmodules because it's already performed in config_from_gitmodules.

The config_from_gitmodules function is moved up in the file —unchanged— before its users to avoid a forward declaration.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 submodule-config.c | 50 +++++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 29 deletions(-)
Show changes to submodule-config.c +21 −29
diff --git a/submodule-config.c b/submodule-config.c
index 602c46af2..77421a497 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -591,6 +591,23 @@ static void submodule_cache_check_init(struct repository *repo)
 	submodule_cache_init(repo->submodule_cache);
 }
 
+/*
+ * Note: This function is private for a reason, the '.gitmodules' file should
+ * not be used as as a mechanism to retrieve arbitrary configuration stored in
+ * the repository.
+ *
+ * Runs the provided config function on the '.gitmodules' file found in the
+ * working directory.
+ */
+static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)
+{
+	if (repo->worktree) {
+		char *file = repo_worktree_path(repo, GITMODULES_FILE);
+		git_config_from_file(fn, file, data);
+		free(file);
+	}
+}
+
 static int gitmodules_cb(const char *var, const char *value, void *data)
 {
 	struct repository *repo = data;
@@ -608,19 +625,11 @@ void repo_read_gitmodules(struct repository *repo)
 {
 	submodule_cache_check_init(repo);
 
-	if (repo->worktree) {
-		char *gitmodules;
-
-		if (repo_read_index(repo) < 0)
-			return;
-
-		gitmodules = repo_worktree_path(repo, GITMODULES_FILE);
-
-		if (!is_gitmodules_unmerged(repo->index))
-			git_config_from_file(gitmodules_cb, gitmodules, repo);
+	if (repo_read_index(repo) < 0)
+		return;
 
-		free(gitmodules);
-	}
+	if (!is_gitmodules_unmerged(repo->index))
+		config_from_gitmodules(gitmodules_cb, repo, repo);
 
 	repo->submodule_cache->gitmodules_read = 1;
 }
@@ -672,23 +681,6 @@ void submodule_free(struct repository *r)
 		submodule_cache_clear(r->submodule_cache);
 }
 
-/*
- * Note: This function is private for a reason, the '.gitmodules' file should
- * not be used as as a mechanism to retrieve arbitrary configuration stored in
- * the repository.
- *
- * Runs the provided config function on the '.gitmodules' file found in the
- * working directory.
- */
-static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)
-{
-	if (repo->worktree) {
-		char *file = repo_worktree_path(repo, GITMODULES_FILE);
-		git_config_from_file(fn, file, data);
-		free(file);
-	}
-}
-
 struct fetch_config {
 	int *max_children;
 	int *recurse_submodules;

-- 
2.18.0
Antonio Ospite· Jun 26, 2018, 10:47 UTC · re: Antonio Ospite · lore

[PATCH v2 3/6] submodule-config: add helper to get 'update-clone' config from .gitmodules

Add a helper function to make it clearer that retrieving 'update-clone' configuration from the .gitmodules file is a special case supported solely for backward compatibility purposes.

This change removes one direct use of 'config_from_gitmodules' for options not strictly related to submodules: "submodule.fetchjobs" does not describe a property of a submodule, but a behavior of other commands when dealing with submodules, so it does not really belong to the .gitmodules file.

This is in the effort to communicate better that .gitmodules is not to be used as a mechanism to store arbitrary configuration in the repository that any command can retrieve.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 builtin/submodule--helper.c |  8 ++++----
 submodule-config.c          | 14 ++++++++++++++
 submodule-config.h          |  1 +
 3 files changed, 19 insertions(+), 4 deletions(-)
Show changes to 3 files +19 −4

builtin/submodule--helper.c, submodule-config.c, submodule-config.h

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 20ae9191c..110a47eca 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1706,8 +1706,8 @@ static int update_clone_task_finished(int result,
 	return 0;
 }
 
-static int gitmodules_update_clone_config(const char *var, const char *value,
-					  void *cb)
+static int git_update_clone_config(const char *var, const char *value,
+				   void *cb)
 {
 	int *max_jobs = cb;
 	if (!strcmp(var, "submodule.fetchjobs"))
@@ -1757,8 +1757,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	};
 	suc.prefix = prefix;
 
-	config_from_gitmodules(gitmodules_update_clone_config, &max_jobs);
-	git_config(gitmodules_update_clone_config, &max_jobs);
+	update_clone_config_from_gitmodules(&max_jobs);
+	git_config(git_update_clone_config, &max_jobs);
 
 	argc = parse_options(argc, argv, prefix, module_update_clone_options,
 			     git_submodule_helper_usage, 0);
diff --git a/submodule-config.c b/submodule-config.c
index f44d6a777..9a2b13d8b 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -716,3 +716,17 @@ void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)
 	};
 	config_from_gitmodules(gitmodules_fetch_config, &config);
 }
+
+static int gitmodules_update_clone_config(const char *var, const char *value,
+					  void *cb)
+{
+	int *max_jobs = cb;
+	if (!strcmp(var, "submodule.fetchjobs"))
+		*max_jobs = parse_submodule_fetchjobs(var, value);
+	return 0;
+}
+
+void update_clone_config_from_gitmodules(int *max_jobs)
+{
+	config_from_gitmodules(gitmodules_update_clone_config, &max_jobs);
+}
diff --git a/submodule-config.h b/submodule-config.h
index cff297a75..b6f19d0d4 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -67,5 +67,6 @@ int check_submodule_name(const char *name);
 extern void config_from_gitmodules(config_fn_t fn, void *data);
 
 extern void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules);
+extern void update_clone_config_from_gitmodules(int *max_jobs);
 
 #endif /* SUBMODULE_CONFIG_H */

-- 
2.18.0
Antonio Ospite· Jun 26, 2018, 10:47 UTC · re: Antonio Ospite · lore

[PATCH v2 5/6] submodule-config: pass repository as argument to config_from_gitmodules

Generlize config_from_gitmodules to accept a repository as an argument.

This is in preparation to reuse the function in repo_read_gitmodules in order to have a single point where the '.gitmodules' file is accessed.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 submodule-config.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to submodule-config.c +5 −5
diff --git a/submodule-config.c b/submodule-config.c
index cd1f1e06a..602c46af2 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -680,10 +680,10 @@ void submodule_free(struct repository *r)
  * Runs the provided config function on the '.gitmodules' file found in the
  * working directory.
  */
-static void config_from_gitmodules(config_fn_t fn, void *data)
+static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)
 {
-	if (the_repository->worktree) {
-		char *file = repo_worktree_path(the_repository, GITMODULES_FILE);
+	if (repo->worktree) {
+		char *file = repo_worktree_path(repo, GITMODULES_FILE);
 		git_config_from_file(fn, file, data);
 		free(file);
 	}
@@ -714,7 +714,7 @@ void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)
 		.max_children = max_children,
 		.recurse_submodules = recurse_submodules
 	};
-	config_from_gitmodules(gitmodules_fetch_config, &config);
+	config_from_gitmodules(gitmodules_fetch_config, the_repository, &config);
 }
 
 static int gitmodules_update_clone_config(const char *var, const char *value,
@@ -728,5 +728,5 @@ static int gitmodules_update_clone_config(const char *var, const char *value,
 
 void update_clone_config_from_gitmodules(int *max_jobs)
 {
-	config_from_gitmodules(gitmodules_update_clone_config, &max_jobs);
+	config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);
 }

-- 
2.18.0
Junio C Hamano· Jun 26, 2018, 20:15 UTC · re: Antonio Ospite · lore

Re: [PATCH v2 5/6] submodule-config: pass repository as argument to config_from_gitmodules

Antonio Ospite <ao2@ao2.it> writes:
> Generlize config_from_gitmodules to accept a repository as an argument.
generalize???
Show 43 quoted lines
>
> This is in preparation to reuse the function in repo_read_gitmodules in
> order to have a single point where the '.gitmodules' file is accessed.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>  submodule-config.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index cd1f1e06a..602c46af2 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -680,10 +680,10 @@ void submodule_free(struct repository *r)
>   * Runs the provided config function on the '.gitmodules' file found in the
>   * working directory.
>   */
> -static void config_from_gitmodules(config_fn_t fn, void *data)
> +static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)
>  {
> -	if (the_repository->worktree) {
> -		char *file = repo_worktree_path(the_repository, GITMODULES_FILE);
> +	if (repo->worktree) {
> +		char *file = repo_worktree_path(repo, GITMODULES_FILE);
>  		git_config_from_file(fn, file, data);
>  		free(file);
>  	}
> @@ -714,7 +714,7 @@ void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)
>  		.max_children = max_children,
>  		.recurse_submodules = recurse_submodules
>  	};
> -	config_from_gitmodules(gitmodules_fetch_config, &config);
> +	config_from_gitmodules(gitmodules_fetch_config, the_repository, &config);
>  }
>  
>  static int gitmodules_update_clone_config(const char *var, const char *value,
> @@ -728,5 +728,5 @@ static int gitmodules_update_clone_config(const char *var, const char *value,
>  
>  void update_clone_config_from_gitmodules(int *max_jobs)
>  {
> -	config_from_gitmodules(gitmodules_update_clone_config, &max_jobs);
> +	config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);
>  }
Antonio Ospite· Jun 26, 2018, 20:57 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 5/6] submodule-config: pass repository as argument to config_from_gitmodules

On Tue, 26 Jun 2018 13:15:33 -0700 Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines
> Antonio Ospite <ao2@ao2.it> writes:
> 
> > Generlize config_from_gitmodules to accept a repository as an argument.
> 
> generalize???
>

Of course I was going to miss a typo in the first word of the commit message :|

If this is the only change, I'd ask you to amend it when applying the patch, if it's not too much trouble.

If instead I have to add also the comments about the new public functions in submodule-config.c, as you asked for patch 2/6, I can send a v3 and fix the typo there.

Thanks,
   Antonio
-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Antonio Ospite· Jun 26, 2018, 10:47 UTC · re: Antonio Ospite · lore

[PATCH v2 2/6] submodule-config: add helper function to get 'fetch' config from .gitmodules

Add a helper function to make it clearer that retrieving 'fetch' configuration from the .gitmodules file is a special case supported solely for backward compatibility purposes.

This change removes one direct use of 'config_from_gitmodules' in code not strictly related to submodules, in the effort to communicate better that .gitmodules is not to be used as a mechanism to store arbitrary configuration in the repository that any command can retrieve.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 builtin/fetch.c    | 15 +--------------
 submodule-config.c | 28 ++++++++++++++++++++++++++++
 submodule-config.h |  2 ++
 3 files changed, 31 insertions(+), 14 deletions(-)
Show changes to 3 files +31 −14

builtin/fetch.c, submodule-config.c, submodule-config.h

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..92a5d235d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -93,19 +93,6 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 	return git_default_config(k, v, cb);
 }
 
-static int gitmodules_fetch_config(const char *var, const char *value, void *cb)
-{
-	if (!strcmp(var, "submodule.fetchjobs")) {
-		max_children = parse_submodule_fetchjobs(var, value);
-		return 0;
-	} else if (!strcmp(var, "fetch.recursesubmodules")) {
-		recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);
-		return 0;
-	}
-
-	return 0;
-}
-
 static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
 {
 	/*
@@ -1433,7 +1420,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	for (i = 1; i < argc; i++)
 		strbuf_addf(&default_rla, " %s", argv[i]);
 
-	config_from_gitmodules(gitmodules_fetch_config, NULL);
+	fetch_config_from_gitmodules(&max_children, &recurse_submodules);
 	git_config(git_fetch_config, NULL);
 
 	argc = parse_options(argc, argv, prefix,
diff --git a/submodule-config.c b/submodule-config.c
index b431555db..f44d6a777 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -688,3 +688,31 @@ void config_from_gitmodules(config_fn_t fn, void *data)
 		free(file);
 	}
 }
+
+struct fetch_config {
+	int *max_children;
+	int *recurse_submodules;
+};
+
+static int gitmodules_fetch_config(const char *var, const char *value, void *cb)
+{
+	struct fetch_config *config = cb;
+	if (!strcmp(var, "submodule.fetchjobs")) {
+		*(config->max_children) = parse_submodule_fetchjobs(var, value);
+		return 0;
+	} else if (!strcmp(var, "fetch.recursesubmodules")) {
+		*(config->recurse_submodules) = parse_fetch_recurse_submodules_arg(var, value);
+		return 0;
+	}
+
+	return 0;
+}
+
+void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)
+{
+	struct fetch_config config = {
+		.max_children = max_children,
+		.recurse_submodules = recurse_submodules
+	};
+	config_from_gitmodules(gitmodules_fetch_config, &config);
+}
diff --git a/submodule-config.h b/submodule-config.h
index 5148801f4..cff297a75 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -66,4 +66,6 @@ int check_submodule_name(const char *name);
  */
 extern void config_from_gitmodules(config_fn_t fn, void *data);
 
+extern void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules);
+
 #endif /* SUBMODULE_CONFIG_H */

-- 
2.18.0
Junio C Hamano· Jun 26, 2018, 20:11 UTC · re: Antonio Ospite · lore

Re: [PATCH v2 2/6] submodule-config: add helper function to get 'fetch' config from .gitmodules

Antonio Ospite <ao2@ao2.it> writes:
> Add a helper function to make it clearer that retrieving 'fetch'
> configuration from the .gitmodules file is a special case supported
> solely for backward compatibility purposes.
> ...

Then perhaps the new public function deserves a comment stating that?

Show 25 quoted lines
> +struct fetch_config {
> +	int *max_children;
> +	int *recurse_submodules;
> +};
> +
> +static int gitmodules_fetch_config(const char *var, const char *value, void *cb)
> +{
> +	struct fetch_config *config = cb;
> +	if (!strcmp(var, "submodule.fetchjobs")) {
> +		*(config->max_children) = parse_submodule_fetchjobs(var, value);
> +		return 0;
> +	} else if (!strcmp(var, "fetch.recursesubmodules")) {
> +		*(config->recurse_submodules) = parse_fetch_recurse_submodules_arg(var, value);
> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +
> +void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)
> +{
> +	struct fetch_config config = {
> +		.max_children = max_children,
> +		.recurse_submodules = recurse_submodules
> +	};

We started using designated initializers some time ago, and use of it improves readability of something like this ;-)

Show 13 quoted lines
> +	config_from_gitmodules(gitmodules_fetch_config, &config);
> +}
> diff --git a/submodule-config.h b/submodule-config.h
> index 5148801f4..cff297a75 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -66,4 +66,6 @@ int check_submodule_name(const char *name);
>   */
>  extern void config_from_gitmodules(config_fn_t fn, void *data);
>  
> +extern void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules);
> +
>  #endif /* SUBMODULE_CONFIG_H */
Antonio Ospite· Jun 26, 2018, 20:55 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/6] submodule-config: add helper function to get 'fetch' config from .gitmodules

On Tue, 26 Jun 2018 13:11:40 -0700 Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines
> Antonio Ospite <ao2@ao2.it> writes:
> 
> > Add a helper function to make it clearer that retrieving 'fetch'
> > configuration from the .gitmodules file is a special case supported
> > solely for backward compatibility purposes.
> > ...
> 
> Then perhaps the new public function deserves a comment stating
> that?
>
Hi Junio,

a comment about that is added to submodule-config.h in patch 4/6 in place of the comment about config_from_gitmodules.

I can add a note here as well if that one is not enough.
[...]
Show 10 quoted lines
> > +void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)
> > +{
> > +	struct fetch_config config = {
> > +		.max_children = max_children,
> > +		.recurse_submodules = recurse_submodules
> > +	};
> 
> We started using designated initializers some time ago, and use of
> it improves readability of something like this ;-)
>

Ah, TBH I didn't even consider whether it was allowed in git code, I just used the construct out of habit.

Ciao,
   Antonio
-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Antonio Ospite· Jun 26, 2018, 10:47 UTC · re: Antonio Ospite · lore

[PATCH v2 4/6] submodule-config: make 'config_from_gitmodules' private

Now that 'config_from_gitmodules' is not used in the open, it can be marked as private.

Hopefully this will prevent its usage for retrieving arbitrary configuration form the '.gitmodules' file.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 submodule-config.c |  8 ++++----
 submodule-config.h | 12 +++++-------
 2 files changed, 9 insertions(+), 11 deletions(-)
Show changes to 2 files +9 −11

submodule-config.c, submodule-config.h

diff --git a/submodule-config.c b/submodule-config.c
index 9a2b13d8b..cd1f1e06a 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -673,14 +673,14 @@ void submodule_free(struct repository *r)
 }
 
 /*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
+ * Note: This function is private for a reason, the '.gitmodules' file should
+ * not be used as as a mechanism to retrieve arbitrary configuration stored in
+ * the repository.
  *
  * Runs the provided config function on the '.gitmodules' file found in the
  * working directory.
  */
-void config_from_gitmodules(config_fn_t fn, void *data)
+static void config_from_gitmodules(config_fn_t fn, void *data)
 {
 	if (the_repository->worktree) {
 		char *file = repo_worktree_path(the_repository, GITMODULES_FILE);
diff --git a/submodule-config.h b/submodule-config.h
index b6f19d0d4..dc7278eea 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -57,15 +57,13 @@ void submodule_free(struct repository *r);
 int check_submodule_name(const char *name);
 
 /*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
+ * Note: these helper functions exist solely to maintain backward
+ * compatibility with 'fetch' and 'update_clone' storing configuration in
+ * '.gitmodules'.
  *
- * Runs the provided config function on the '.gitmodules' file found in the
- * working directory.
+ * New helpers to retrieve arbitrary configuration from the '.gitmodules' file
+ * should NOT be added.
  */
-extern void config_from_gitmodules(config_fn_t fn, void *data);
-
 extern void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules);
 extern void update_clone_config_from_gitmodules(int *max_jobs);
 

-- 
2.18.0
Junio C Hamano· Jun 26, 2018, 20:15 UTC · re: Antonio Ospite · lore

Re: [PATCH v2 4/6] submodule-config: make 'config_from_gitmodules' private

Antonio Ospite <ao2@ao2.it> writes:
> Now that 'config_from_gitmodules' is not used in the open, it can be
> marked as private.
Nice ;-)
Brandon Williams· Jun 26, 2018, 17:05 UTC · re: Antonio Ospite · lore

Re: [PATCH v2 0/6] Restrict the usage of config_from_gitmodules to submodule-config

On 06/26, Antonio Ospite wrote:
Show 33 quoted lines
> Hi,
> 
> this is version 2 of the series from
> https://public-inbox.org/git/20180622162656.19338-1-ao2@ao2.it/
> 
> The .gitmodules file is not meant for arbitrary configuration, it should
> be used only for submodules properties.
> 
> Plus, arbitrary git configuration should not be distributed with the
> repository, and .gitmodules might be a possible "vector" for that.
> 
> The series tries to alleviate both these issues by moving the
> 'config_from_gitmodules' function from config.[ch] to submodule-config.c
> and making it private.
> 
> This should discourage future code from using the function with
> arbitrary config callbacks which might turn .gitmodules into a mechanism
> to load arbitrary configuration stored in the repository.
> 
> Backward compatibility exceptions to the rules above are handled by
> ad-hoc helpers.
> 
> Finally (in patch 6) some duplication is removed by using
> 'config_from_gitmodules' to load the submodules configuration in
> 'repo_read_gitmodules'.
> 
> Changes since v1:
>   * Remove an extra space before an arrow operator in patch 2
>   * Fix a typo in the commit message of patch 3: s/fetchobjs/fetchjobs
>   * Add a note in the commit message of patch 6 about checking the
>     worktree before loading .gitmodules
>   * Drop patch 7, it was meant as a cleanup but resulted in parsing the
>     .gitmodules file twice
Thanks for making these changes, this version looks good to me!
-- 
Brandon Williams
Junio C Hamano· Jun 26, 2018, 20:24 UTC · re: Brandon Williams · lore

Re: [PATCH v2 0/6] Restrict the usage of config_from_gitmodules to submodule-config

Brandon Williams <bmwill@google.com> writes:
Show 9 quoted lines
>> Changes since v1:
>>   * Remove an extra space before an arrow operator in patch 2
>>   * Fix a typo in the commit message of patch 3: s/fetchobjs/fetchjobs
>>   * Add a note in the commit message of patch 6 about checking the
>>     worktree before loading .gitmodules
>>   * Drop patch 7, it was meant as a cleanup but resulted in parsing the
>>     .gitmodules file twice
>
> Thanks for making these changes, this version looks good to me!
Yup, thanks, both.

← back to recent threads