Re: [PATCH/RFC v2 1/2] git_default_config() rewritten using the config-set API
Matthieu Moy writes: > Tanay Abhra writes: > >> git_default_config() now uses config-set API functions to query for >> values. > > I believe you missed a few spots: > > $ git grep -n 'git_default_config[^(]' > Documentation/user-manual.txt:4287:git_config(git_default_config); > archive.c:416: git_config(git_default_config, NULL); > builtin/config.c:577: git_config(git_default_config, NULL); > color.h:73: * if you are just going to change to git_default_config, too. > fetch-pack.c:880: git_config(git_default_config, NULL); > http.c:393: config.cascade_fn = git_default_config; > rerere.c:580: git_config(git_default_config, NULL); > rerere.c:710: git_config(git_default_config, NULL); > > The following ones should probably be rewritten too: > > archive.c:416: git_config(git_default_config, NULL); > builtin/config.c:577: git_config(git_default_config, NULL); > fetch-pack.c:880: git_config(git_default_config, NULL); > rerere.c:580: git_config(git_default_config, NULL); > rerere.c:710: git_config(git_default_config, NULL); For a one-person toy project it is OK to repurpose the existing git_default_config() to do completely different thing and make it a flag day to switch the entire codebase, but in a collaborative environment where there may be multiple topics in flight, some of which may be happening where you are not even aware of, it is better to remove the existing git_default_config() and use a different name for the different function you are introducing, to force new places that expect the old git_default_config() to work as before to be noticed with a linkage error. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v2 1/2] git_default_config() rewritten using the config-set API
Tanay Abhra writes:
> git_default_config() now uses config-set API functions to query for
> values.
>
> Signed-off-by: Tanay Abhra
> ---
> Sorry, for the short log message, I will explain why.
> The git_default_config() rewrite is 100% complete, the only
> problem remains is the call sites; there are too many of them.
> Some are called from callback functions which pass the remaining
> variables to git_default_config() which they do not have any use for.
> Those call sites can remain as they are, because git_default_config()
> is a single call function now, and is guarded by a sentinel value.
They can remain as they are, but it would also be relatively easy to
turn them into non-callback style by doing something like this on each
call:
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -37,7 +37,7 @@ static int commit_tree_config(const char *var, const char
*value, void *cb)
sign_commit = git_config_bool(var, value) ? "" : NULL;
return 0;
}
- return git_default_config(var, value, cb);
+ return 0
}
int cmd_commit_tree(int argc, const char **argv, const char *prefix)
@@ -49,6 +49,7 @@ int cmd_commit_tree(int argc, const char **argv, const char
*prefix)
struct strbuf buffer = STRBUF_INIT;
git_config(commit_tree_config, NULL);
+ git_default_config();
if (argc < 2 || !strcmp(argv[1], "-h"))
usage(commit_tree_usage);
> -int git_default_config(const char *var, const char *value, void *dummy)
> +int git_default_config(const char *unused, const char *unused2, void *dummy)
By having these dummy arguments, you force callers to pass dummy actual
parameters.
Actually, you don't pass anything in PATCH 2, hence the result is not
compilable:
http-fetch.c:70:2: error: too few arguments to function ‘git_default_config’
git_default_config();
^
In file included from http-fetch.c:1:0:
cache.h:1299:12: note: declared here
extern int git_default_config(const char *, const char *, void *);
After your patch, there are two things git_default_config do:
1) normal callers want to call git_default_config();
2) callback-style callers want to write
return git_default_config(var, value, cb);
I think this deserves two functions, calling each others:
/* For 1) */
void git_load_default_config(void)
{
do the actual stuff
}
/* For 2) */
int git_default_config(const char *unused, const char *unused2, void *dummy)
{
if (default_config_loaded)
return;
git_load_default_config(NULL, NULL, NULL);
default_config_loaded = 1;
return 0;
}
In an ideal world, git_default_config would disappear after the rewrite
is completed. In practice, it may stay if needed, it doesn't harm
anyone.
PATCH 2 would turn git_config(git_default_config, NULL); into
git_load_default_config().
> @@ -2082,6 +1977,7 @@ int git_config_set_multivar_in_file(const char
> *config_filename,
>
> /* Invalidate the config cache */
> git_config_clear();
> + default_config_loaded = 0;
What about the other callsite in setup.c? We may have left the
configuration half-loaded, and if anyone calls git_load_default_config()
again after that, we do want to reload it, don't we?
Which leads to another question: why not put this default_config_loaded
= 0; inside git_config_clear(), to avoid forgetting?
> index 1d9b6e7..0db595f 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -392,29 +392,26 @@ int author_ident_sufficiently_given(void)
> return ident_is_sufficient(author_ident_explicitly_given);
> }
>
> -int git_ident_config(const char *var, const char *value, void *data)
> +void git_ident_config(void)
> {
> - if (!strcmp(var, "user.name")) {
> + const char *value = NULL;
> +
> + if (!git_config_get_value("user.name", &value)) {
> if (!value)
> - return config_error_nonbool(var);
> + git_die_config("user.name", "Missing value for
> 'user.name'");
I'd rather have git_config_get_string() and a free() afterwards to avoid
duplicating this "Missing value for 'user.name'" (which should be _()-ed
if it stays).
> -
> - if (!strcmp(var, "user.email")) {
> + if (!git_config_get_value("user.email", &value)) {
> if (!value)
> - return config_error_nonbool(var);
> + git_die_config("user.email", "Missing value for
> 'user.email'");
Likewise.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v2 1/2] git_default_config() rewritten using the config-set API
Tanay Abhra writes: > git_default_config() now uses config-set API functions to query for > values. I believe you missed a few spots: $ git grep -n 'git_default_config[^(]' Documentation/user-manual.txt:4287:git_config(git_default_config); archive.c:416: git_config(git_default_config, NULL); builtin/config.c:577: git_config(git_default_config, NULL); color.h:73: * if you are just going to change to git_default_config, too. fetch-pack.c:880: git_config(git_default_config, NULL); http.c:393: config.cascade_fn = git_default_config; rerere.c:580: git_config(git_default_config, NULL); rerere.c:710: git_config(git_default_config, NULL); The following ones should probably be rewritten too: archive.c:416: git_config(git_default_config, NULL); builtin/config.c:577: git_config(git_default_config, NULL); fetch-pack.c:880: git_config(git_default_config, NULL); rerere.c:580: git_config(git_default_config, NULL); rerere.c:710: git_config(git_default_config, NULL); -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC v2 1/2] git_default_config() rewritten using the config-set API
git_default_config() now uses config-set API functions to query for
values.
Signed-off-by: Tanay Abhra
---
Sorry, for the short log message, I will explain why.
The git_default_config() rewrite is 100% complete, the only
problem remains is the call sites; there are too many of them.
Some are called from callback functions which pass the remaining
variables to git_default_config() which they do not have any use for.
Those call sites can remain as they are, because git_default_config()
is a single call function now, and is guarded by a sentinel value.
So after one call, it would just return immediately instead of going on
checking.
For callers like git_config(git_default_config, NULL) (there are 38 of them),
we can leave them as they are or rewrite them as I have illustrated in the
next attached patch.
I will take this series out of RFC as soon as we have decided what to do with
the call sites.
Cheers,
Tanay Abhra.
advice.c | 17 ++--
advice.h | 2 +-
cache.h | 2 +-
config.c | 346 ++-
ident.c | 17 ++--
5 files changed, 136 insertions(+), 248 deletions(-)
diff --git a/advice.c b/advice.c
index 9b42033..34e1ccc 100644
--- a/advice.c
+++ b/advice.c
@@ -59,22 +59,17 @@ void advise(const char *advice, ...)
strbuf_release(&buf);
}
-int git_default_advice_config(const char *var, const char *value)
+void git_default_advice_config(void)
{
- const char *k;
+ struct strbuf var = STRBUF_INIT;
int i;
- if (!skip_prefix(var, "advice.", &k))
- return 0;
-
for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
- if (strcmp(k, advice_config[i].name))
- continue;
- *advice_config[i].preference = git_config_bool(var, value);
- return 0;
+ strbuf_addf(&var, "advice.%s", advice_config[i].name);
+ git_config_get_bool(var.buf, advice_config[i].preference);
+ strbuf_reset(&var);
}
-
- return 0;
+ strbuf_release(&var);
}
int error_resolve_conflict(const char *me)
diff --git a/advice.h b/advice.h
index 5ecc6c1..5bfe46c 100644
--- a/advice.h
+++ b/advice.h
@@ -19,7 +19,7 @@ extern int advice_set_upstream_failure;
extern int advice_object_name_warning;
extern int advice_rm_hints;
-int git_default_advice_config(const char *var, const char *value);
+void git_default_advice_config(void);
__attribute__((format (printf, 1, 2)))
void advise(const char *advice, ...);
int error_resolve_conflict(const char *me);
diff --git a/cache.h b/cache.h
index 2693a37..fa28b40 100644
--- a/cache.h
+++ b/cache.h
@@ -1065,7 +1065,7 @@ extern const char *fmt_name(const char *name, const char
*email);
extern const char *ident_default_email(void);
extern const char *git_editor(void);
extern const char *git_pager(int stdout_is_tty);
-extern int git_ident_config(const char *, const char *, void *);
+extern void git_ident_config(void);
struct ident_split {
const char *name_begin;
diff --git a/config.c b/config.c
index 427850a..36b9124 100644
--- a/config.c
+++ b/config.c
@@ -46,6 +46,8 @@ static int zlib_compression_seen;
*/
static struct config_set the_config_set;
+static int default_config_loaded;
+
static int config_file_fgetc(struct config_source *conf)
{
return fgetc(conf->u.file);
@@ -670,147 +672,91 @@ int git_config_pathname(const char **dest, const char
*var, const char *value)
return 0;
}
-static int git_default_core_config(const char *var, const char *value)
+static void git_default_core_config(void)
{
+ const char *value = NULL;
+ unsigned long v_l = 0;
+ int abbrev;
+ const char *comment;
+
/* This needs a better name */
- if (!strcmp(var, "core.filemode")) {
- trust_executable_bit = git_config_bool(var, value);
- return 0;
- }
- if (!strcmp(var, "core.trustctime")) {
- trust_ctime = git_config_bool(var, value);
- return 0;
- }
- if (!strcmp(var, "core.checkstat")) {
+ git_config_get_bool("core.filemode", &trust_executable_bit);
+ git_config_get_bool("core.trustctime", &trust_ctime);
+
+ if (!git_config_get_value("core.checkstat", &value)) {
if (!strcasecmp(value, "default"))
check_stat = 1;
else if (!strcasecmp(value, "minimal"))
check_stat = 0;
}
- if (!strcmp(var, "core.quotepath")) {
- quote_path_fully = git_config_bool(var, value);
- return 0;
- }
+ git_config_get_bool("core.quotepath", "e_path_fully);
+ git_config_get_bool("core.symlinks", &has_symlinks);
+ git_config_get_bool("core.ignorecase", &ignore_case);
+ git_config_get_pathname("core.attributesfile", &git_attributes_file);
+ git_config_get_bool("core.bare", &is_bare_repository_cfg);
+ git_
