Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions
Junio C Hamano writes: > Matthieu Moy writes: > >> In general, most strings one manipulates are "const char *", it's >> frequent to modify a pointer to a string, but rather rare to modify the >> string itself. > > We seem to have a disagreement. Unlike git_config_get_value() that > lets callers peek the only cached copy, git_config_get_string() > gives its caller a new copy that the caller needs to free. Such a > new string can and should be given as mutable, I would say. You're right (I guess you replied to this one before seeing my other message). imap_folder could be declared const char *, but git_config_get_string() shouldn't be the one to force this. -- 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] rewrite `git_default_config()` using config-set API functions
Matthieu Moy writes: > In general, most strings one manipulates are "const char *", it's > frequent to modify a pointer to a string, but rather rare to modify the > string itself. We seem to have a disagreement. Unlike git_config_get_value() that lets callers peek the only cached copy, git_config_get_string() gives its caller a new copy that the caller needs to free. Such a new string can and should be given as mutable, I would say. -- 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] rewrite `git_default_config()` using config-set API functions
Tanay Abhra writes:
>>
>>> + if > + git_config_get_string("core.notesref", (const
>>> char**)¬es_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.
>
> Same casts had to be used in imap-send.c patch, I will have to use an
> intermediate variable there to remove the cast thus destroying the one
> liners or will have to update the variable declarations.
Updating the declaration like this should just work:
--- a/imap-send.c
+++ b/imap-send.c
@@ -1324,7 +1324,7 @@ static int split_msg(struct strbuf *all_msgs, struct
strbuf *msg, int *ofs)
return 1;
}
-static char *imap_folder;
+static const char *imap_folder;
static void git_imap_config(void)
{
@@ -1332,7 +1332,7 @@ static void git_imap_config(void)
git_config_get_bool("imap.sslverify", &server.ssl_verify);
git_config_get_bool("imap.preformattedhtml", &server.use_html);
- git_config_get_string("imap.folder", (const char**)&imap_folder);
+ git_config_get_string("imap.folder", &imap_folder);
if (!git_config_get_value("imap.host", &val)) {
if(!val)
In general, most strings one manipulates are "const char *", it's
frequent to modify a pointer to a string, but rather rare to modify the
string itself.
--
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] rewrite `git_default_config()` using config-set API functions
>
>> +if > + git_config_get_string("core.notesref", (const
>> char**)¬es_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.
Same casts had to be used in imap-send.c patch, I will have to use an
intermediate variable there to remove the cast thus destroying the one
liners or will have to update the variable declarations.
--
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] rewrite `git_default_config()` using config-set API functions
On 21/07/14 12:44, Tanay Abhra wrote:
> Use `git_config_get_*()` family instead of `git_config()` to take advantage of
> the config-set API which provides a cleaner control flow.
>
> Signed-off-by: Tanay Abhra
> ---
> 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.
> After the first six patches in the series which you have already seen there
> are
> five or four left which can rewritten without touching git_default_config().
>
> Thus, this rewrite will serve as the base for rewriting other git_config()
> callers which pass control to git_default_config() at the end of the function.
> Also there are more than thirty direct callers to git_default_config()
> (i.e git_config(git_default_config, NULL)), so this rewrite solves them
> in one sweep.
>
> Slight behaviour change, config_error_nonbool() has been replaced with
> die("Missing value for '%s'", var);.
> The original code also alerted the file name and the line number which we
> lose here.
>
> Cheers,
> Tanay Abhra.
>
> advice.c | 18 ++--
> advice.h | 2 +-
> cache.h | 2 +-
> config.c | 287
> ---
> ident.c | 15 ++--
> 5 files changed, 104 insertions(+), 220 deletions(-)
>
[snip]
> diff --git a/config.c b/config.c
> index fe9f399..72196a9 100644
> --- a/config.c
> +++ b/config.c
> @@ -666,88 +666,47 @@ 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;
> /* 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;
> - }
> -
> - if (!strcmp(var, "core.symlinks")) {
> - has_symlinks = git_config_bool(var, value);
> - return 0;
> - }
> -
> - if (!strcmp(var, "core.ignorecase")) {
> - ignore_case = git_config_bool(var, value);
> - return 0;
> - }
> -
> - if (!strcmp(var, "core.attributesfile"))
> - return git_config_pathname(&git_attributes_file, var, value);
> -
> - if (!strcmp(var, "core.bare")) {
> - is_bare_repository_cfg = git_config_bool(var, value);
> - return 0;
> - }
> -
> - if (!strcmp(var, "core.ignorestat")) {
> - assume_unchanged = git_config_bool(var, value);
> - return 0;
> - }
> -
> - if (!strcmp(var, "core.prefersymlinkrefs")) {
> - prefer_symlink_refs = git_config_bool(var, value);
> - return 0;
> - }
> -
> - if (!strcmp(var, "core.logallrefupdates")) {
> - log_all_ref_updates = 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_config_get_bool("core.ignorestat", &assume_unchanged);
> + git_config_get_bool("core.prefersymlinkrefs", &prefer_symlink_refs);
> + git_config_get_bool("core.logallrefupdates", &log_all_ref_updates);
> + git_config_get_bool("core.warnambiguousrefs", &warn_ambiguous_refs);
>
> - if (!strcmp(var, "core.warnambiguousrefs")) {
> - warn_ambiguous_refs = git_config_bool(var, value);
> - return 0;
> + int abbrev;
declaration after statement.
> + if (!git_config_get_int("core.abbrev", &abbrev)) {
> + if (abbrev >= minimum_abbrev && abbrev <= 40)
> + default_abbrev = abbrev;
> }
>
> - if (!strcmp(var, "core.abbrev")) {
> - int abbrev = git_config_int(var, value);
> - if (abbrev < minimum_abbrev || abbrev > 40)
> - return -1;
> - default_abbrev = abbrev;
> - return 0;
> - }
> -
> - if (!strcm
Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions
Tanay Abhra 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**)¬es_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**)¬es_ref_name);
+ git_config_get_string("core.notesref", ¬es_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/envi
[PATCH/RFC] rewrite `git_default_config()` using config-set API functions
Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra
---
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.
After the first six patches in the series which you have already seen there are
five or four left which can rewritten without touching git_default_config().
Thus, this rewrite will serve as the base for rewriting other git_config()
callers which pass control to git_default_config() at the end of the function.
Also there are more than thirty direct callers to git_default_config()
(i.e git_config(git_default_config, NULL)), so this rewrite solves them
in one sweep.
Slight behaviour change, config_error_nonbool() has been replaced with
die("Missing value for '%s'", var);.
The original code also alerted the file name and the line number which we lose
here.
Cheers,
Tanay Abhra.
advice.c | 18 ++--
advice.h | 2 +-
cache.h | 2 +-
config.c | 287 ---
ident.c | 15 ++--
5 files changed, 104 insertions(+), 220 deletions(-)
diff --git a/advice.c b/advice.c
index 9b42033..92d89a9 100644
--- a/advice.c
+++ b/advice.c
@@ -59,22 +59,16 @@ 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 e53651c..e667d92 100644
--- a/cache.h
+++ b/cache.h
@@ -1061,7 +1061,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 fe9f399..72196a9 100644
--- a/config.c
+++ b/config.c
@@ -666,88 +666,47 @@ 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;
/* 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;
- }
-
- if (!strcmp(var, "core.symlinks")) {
- has_symlinks = git_config_bool(var, value);
- return 0;
- }
-
- if (!strcmp(var, "core.ignorecase")) {
- ignore_case = git_config_bool(var, value);
- return 0;
- }
-
- if (!strcmp(var, "core.attributesfile"))
- return git_config_pathname(&git_attributes_file, var, value);
-
- if (!strcmp(var, "core.bare")) {
- is_bare_repository_cfg = git_config_bool(var, value);
- return 0;
- }
-
- if (!strcmp
