Re: [PATCH v4 1/9] submodule: add a print_config_from_gitmodules() helper
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2018-08-24 14:32:44
On Fri, Aug 24 2018, Antonio Ospite wrote:
quoted hunk ↗ jump to hunk
Add a new print_config_from_gitmodules() helper function to print values from .gitmodules just like "git config -f .gitmodules" would. This will be used by a new submodule--helper subcommand to be able to access the .gitmodules file in a more controlled way. Signed-off-by: Antonio Ospite <redacted> --- submodule-config.c | 25 +++++++++++++++++++++++++ submodule-config.h | 2 ++ 2 files changed, 27 insertions(+)diff --git a/submodule-config.c b/submodule-config.c index fc2c41b947..eef96c4198 100644 --- a/submodule-config.c +++ b/submodule-config.c@@ -682,6 +682,31 @@ void submodule_free(struct repository *r) submodule_cache_clear(r->submodule_cache); } +static int config_print_callback(const char *key_, const char *value_, void *cb_data) +{ + char *key = cb_data; + + if (!strcmp(key, key_)) + printf("%s\n", value_); + + return 0; +}
No problem with the code itself, but I'd find this a lot easier to read
in context if it was:
key_ -> var
value_ -> value
key -> wanted_key, perhaps?
I.e. the rest of the file uses the convention of the
config_from_gitmodules callbacks getting "var" and "value",
respectively.
We don't use this convention of suffixing variables with "_" if they're
arguments to the function anywhere else.
+int print_config_from_gitmodules(const char *key)
+{
+ int ret;
+ char *store_key;
+
+ ret = git_config_parse_key(key, &store_key, NULL);
+ if (ret < 0)
+ return CONFIG_INVALID_KEY;Isn't this a memory leak? I.e. we should free() and return here, no?
quoted hunk ↗ jump to hunk
+ config_from_gitmodules(config_print_callback, the_repository, store_key); + + free(store_key); + return 0; +} + struct fetch_config { int *max_children; int *recurse_submodules;diff --git a/submodule-config.h b/submodule-config.h index dc7278eea4..ed40e9a478 100644 --- a/submodule-config.h +++ b/submodule-config.h@@ -56,6 +56,8 @@ void submodule_free(struct repository *r); */ int check_submodule_name(const char *name); +int print_config_from_gitmodules(const char *key); + /* * Note: these helper functions exist solely to maintain backward * compatibility with 'fetch' and 'update_clone' storing configuration in
Another style nit: Makes more sense to put this new function right underneath "submodule_free" above, instead of under 1/2 of the functions that have a big comment describing how they work, since that comment doesn't apply to this new function.