Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jul 21, 2014, 13:43 UTC
Message-ID: <vpqha2addyn.fsf@anie.imag.fr>
In-Reply-To: <53CCFD02.6010704@gmail.com>

Tanay Abhra <tanayabh@gmail.com> writes:

> Consider this as a proof of concept as the others callers have to be rewritten
> as well.
> I think that it is not so buggy as it passes all the tests.

Before and after your patch, git_default_config() is called once per config key. Before the patch, it made sense, but after the patch, it doesn't anymore, as it loads the whole config file in one call.

I think it's OK to have this as an intermediate patch, but it should be clear to reviewers.

At the end of your series, git_default_config should become an argumentless function.

Actually, I'm wondering whether it makes sense to keep git_default_config as-is, and introduce a new function like git_load_default_config(void), initially empty. Then, move and change code from git_default_config(...) to git_load_default_config(), and finally remove git_default_config(...) once it's empty.

You have several decl-after-statements in your patch. You should have something like this in config.mak to catch them:

CFLAGS += -Wdeclaration-after-statement -Wall -Werror

There are several (long unsigned int*) casts that seem useless to me. Useless casts distract the reader, and prevent usefull warnings from the compiler.

Actually, I'm wondering whether these casts are safe (they cast from size_t * to ulong *). On Windows 64 bits for example, sizeof(size_t) == 8 and sizeof(unsigned long) == 4 if google informed me correctly. On a big-endian machine, this would be totally broken.

It is probably safer to add a new git_config_get_size_t analog to git_config_get_ulong.

> +	if > +	git_config_get_string("core.notesref", (const char**)&notes_ref_name);

This cast is needed only because notes_ref_name is declared as non-const, but a better fix would be to make the variable const, and remove the cast.

The following patch solves these issues (modulo the question above on cast safety).

diff --git a/cache.h b/cache.h
index e667d92..1271904 100644
--- a/cache.h
+++ b/cache.h
@@ -674,7 +674,7 @@ enum object_creation_mode {
 
 extern enum object_creation_mode object_creation_mode;
 
-extern char *notes_ref_name;
+extern const char *notes_ref_name;
 
 extern int grafts_replace_parents;
 
diff --git a/config.c b/config.c
index 72196a9..c2664c3 100644
--- a/config.c
+++ b/config.c
@@ -669,6 +669,9 @@ int git_config_pathname(const char **dest, const char *var, const char *value)
 static void git_default_core_config(void)
 {
        const char *value = NULL;
+       int abbrev;
+       int level;
+       const char *comment;
        /* This needs a better name */
        git_config_get_bool("core.filemode", &trust_executable_bit);
        git_config_get_bool("core.trustctime", &trust_ctime);
@@ -690,13 +693,11 @@ static void git_default_core_config(void)
        git_config_get_bool("core.logallrefupdates", &log_all_ref_updates);
        git_config_get_bool("core.warnambiguousrefs", &warn_ambiguous_refs);
 
-       int abbrev;
        if (!git_config_get_int("core.abbrev", &abbrev)) {
                if (abbrev >= minimum_abbrev && abbrev <= 40)
                        default_abbrev = abbrev;
        }
 
-       int level;
        if (!git_config_get_int("core.loosecompression", &level)) {
                if (level == -1)
                        level = Z_DEFAULT_COMPRESSION;
@@ -717,7 +718,7 @@ static void git_default_core_config(void)
                        zlib_compression_level = level;
        }
 
-       if (!git_config_get_ulong("core.packedgitwindowsize", (long unsigned int*)&packed_git_window_size)) {
+       if (!git_config_get_ulong("core.packedgitwindowsize", &packed_git_window_size)) {
                int pgsz_x2 = getpagesize() * 2;
 
                /* This value must be multiple of (pagesize * 2) */
@@ -728,8 +729,8 @@ static void git_default_core_config(void)
        }
 
        git_config_get_ulong("core.bigfilethreshold", &big_file_threshold);
-       git_config_get_ulong("core.packedgitlimit", (long unsigned int*)&packed_git_limit);
-       git_config_get_ulong("core.deltabasecachelimit", (long unsigned int*)&delta_base_cache_limit);
+       git_config_get_ulong("core.packedgitlimit", &packed_git_limit);
+       git_config_get_ulong("core.deltabasecachelimit", &delta_base_cache_limit);
 
        if (!git_config_get_value("core.autocrlf", &value)) {
                if (value && !strcasecmp(value, "input")) {
@@ -760,11 +761,10 @@ static void git_default_core_config(void)
                        die("core.autocrlf=input conflicts with core.eol=crlf");
        }
 
-       git_config_get_string("core.notesref", (const char**)&notes_ref_name);
+       git_config_get_string("core.notesref", &notes_ref_name);
        git_config_get_string("core.pager", &pager_program);
        git_config_get_string("core.editor", &editor_program);
 
-       const char *comment;
        if (!git_config_get_string("core.commentchar", &comment)) {
                if (!strcasecmp(comment, "auto"))
                        auto_comment_line_char = 1;
diff --git a/environment.c b/environment.c
index 565f652..21d4dbb 100644
--- a/environment.c
+++ b/environment.c
@@ -56,7 +56,7 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
 #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
 #endif
 enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
-char *notes_ref_name;
+const char *notes_ref_name;
 int grafts_replace_parents = 1;
 int core_apply_sparse_checkout;
 int merge_log_config = -1;

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/