* [PATCH v3] config: make git_config_set die on failure
@ 2016-02-01 10:59 Patrick Steinhardt
2016-02-01 19:10 ` Junio C Hamano
0 siblings, 1 reply; 2+ messages in thread
From: Patrick Steinhardt @ 2016-02-01 10:59 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, ps
Failure to write the configuration file may be caused by multiple
conditions, the most common one being the case where the
configuration is locked because of a leftover lock file or
because another process is currently writing to it. We used to
ignore those errors in many cases, possibly leading to
inconsistently configured repositories. More often than not git
even pretended that everything was fine and that the settings
have been applied when indeed they were not.
Handle these failures by dying by default whenever an error is
occured by the `git_config_set` family of functions. Introduce a
new set of functions `git_config_set_gently` that instead returns
an error code for the cases where we are required to explicitly
handle configuration errors.
---
In contrast to the old version ([1]) this version of the patch
introduces a set of functions `git_config_set_gently` instead of
`git_config_set_or_die`, thus going the other route of dying by
default.
It first seemed that going the default-die route instead of
or_die-wrappers might prove to be more contained and easier to
grasp. Seeing the result, though, I'm not convinced of this
anymore. This is a minimal compiling example without even caring
about test failures, which do occur now.
Previously in v2 we had a diffstat of +127/-55 lines where we now
have +88/-82, which amounts to roughly the same amount of lines
changed. The previous approach, though, was much easier to grasp
in my opinion as nearly all changes could be split up instead of
requiring one big change to make it compile. As said before, this
does not yet include the changes required for handling test
failures, thus the patch is expected to become bigger rather than
smaller.
Furthermore, I do think it's more explicit what the functions are
doing when there is a 'or_die' suffix. Without this suffix it may
be unexpected that the functions simply abort the program
whenever an error occurs.
I'd thus prefer to use the old style used in version 2. I could
try to make the second patch ([2]) a little bit smaller by
avoiding all the fuss about passing up the error code only to die
a little later.
Opinions?
Patrick
[1]: http://article.gmane.org/gmane.comp.version-control.git/285000
[2]: http://thread.gmane.org/gmane.comp.version-control.git/285002
builtin/branch.c | 2 +-
builtin/clone.c | 2 +-
builtin/config.c | 28 +++++++++++------------
builtin/remote.c | 70 ++++++++++++++++++--------------------------------------
cache.h | 12 ++++++----
config.c | 46 ++++++++++++++++++++++++++++++-------
submodule.c | 10 ++++----
7 files changed, 88 insertions(+), 82 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 3f6c825..461eebb 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -595,7 +595,7 @@ static int edit_branch_description(const char *branch_name)
strbuf_stripspace(&buf, 1);
strbuf_addf(&name, "branch.%s.description", branch_name);
- status = git_config_set(name.buf, buf.len ? buf.buf : NULL);
+ git_config_set(name.buf, buf.len ? buf.buf : NULL);
strbuf_release(&name);
strbuf_release(&buf);
diff --git a/builtin/clone.c b/builtin/clone.c
index a7c8def..8c01975 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -735,7 +735,7 @@ static int checkout(void)
static int write_one_config(const char *key, const char *value, void *data)
{
- return git_config_set_multivar(key, value ? value : "true", "^$", 0);
+ return git_config_set_multivar_gently(key, value ? value : "true", "^$", 0);
}
static void write_config(struct string_list *config)
diff --git a/builtin/config.c b/builtin/config.c
index adc7727..c26d6e7 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -582,7 +582,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
check_write();
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
- ret = git_config_set_in_file(given_config_source.file, argv[0], value);
+ ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);
if (ret == CONFIG_NOTHING_SET)
error("cannot overwrite multiple values with a single value\n"
" Use a regexp, --add or --replace-all to change %s.", argv[0]);
@@ -592,23 +592,23 @@ int cmd_config(int argc, const char **argv, const char *prefix)
check_write();
check_argc(argc, 2, 3);
value = normalize_value(argv[0], argv[1]);
- return git_config_set_multivar_in_file(given_config_source.file,
- argv[0], value, argv[2], 0);
+ return git_config_set_multivar_in_file_gently(given_config_source.file,
+ argv[0], value, argv[2], 0);
}
else if (actions == ACTION_ADD) {
check_write();
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
- return git_config_set_multivar_in_file(given_config_source.file,
- argv[0], value,
- CONFIG_REGEX_NONE, 0);
+ return git_config_set_multivar_in_file_gently(given_config_source.file,
+ argv[0], value,
+ CONFIG_REGEX_NONE, 0);
}
else if (actions == ACTION_REPLACE_ALL) {
check_write();
check_argc(argc, 2, 3);
value = normalize_value(argv[0], argv[1]);
- return git_config_set_multivar_in_file(given_config_source.file,
- argv[0], value, argv[2], 1);
+ return git_config_set_multivar_in_file_gently(given_config_source.file,
+ argv[0], value, argv[2], 1);
}
else if (actions == ACTION_GET) {
check_argc(argc, 1, 2);
@@ -634,17 +634,17 @@ int cmd_config(int argc, const char **argv, const char *prefix)
check_write();
check_argc(argc, 1, 2);
if (argc == 2)
- return git_config_set_multivar_in_file(given_config_source.file,
- argv[0], NULL, argv[1], 0);
+ return git_config_set_multivar_in_file_gently(given_config_source.file,
+ argv[0], NULL, argv[1], 0);
else
- return git_config_set_in_file(given_config_source.file,
- argv[0], NULL);
+ return git_config_set_in_file_gently(given_config_source.file,
+ argv[0], NULL);
}
else if (actions == ACTION_UNSET_ALL) {
check_write();
check_argc(argc, 1, 2);
- return git_config_set_multivar_in_file(given_config_source.file,
- argv[0], NULL, argv[1], 1);
+ return git_config_set_multivar_in_file_gently(given_config_source.file,
+ argv[0], NULL, argv[1], 1);
}
else if (actions == ACTION_RENAME_SECTION) {
int ret;
diff --git a/builtin/remote.c b/builtin/remote.c
index 2b2ff9b..05d665f 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -108,7 +108,7 @@ enum {
#define MIRROR_PUSH 2
#define MIRROR_BOTH (MIRROR_FETCH|MIRROR_PUSH)
-static int add_branch(const char *key, const char *branchname,
+static void add_branch(const char *key, const char *branchname,
const char *remotename, int mirror, struct strbuf *tmp)
{
strbuf_reset(tmp);
@@ -119,7 +119,7 @@ static int add_branch(const char *key, const char *branchname,
else
strbuf_addf(tmp, "refs/heads/%s:refs/remotes/%s/%s",
branchname, remotename, branchname);
- return git_config_set_multivar(key, tmp->buf, "^$", 0);
+ git_config_set_multivar(key, tmp->buf, "^$", 0);
}
static const char mirror_advice[] =
@@ -197,8 +197,7 @@ static int add(int argc, const char **argv)
die(_("'%s' is not a valid remote name"), name);
strbuf_addf(&buf, "remote.%s.url", name);
- if (git_config_set(buf.buf, url))
- return 1;
+ git_config_set(buf.buf, url);
if (!mirror || mirror & MIRROR_FETCH) {
strbuf_reset(&buf);
@@ -206,25 +205,22 @@ static int add(int argc, const char **argv)
if (track.nr == 0)
string_list_append(&track, "*");
for (i = 0; i < track.nr; i++) {
- if (add_branch(buf.buf, track.items[i].string,
- name, mirror, &buf2))
- return 1;
+ add_branch(buf.buf, track.items[i].string,
+ name, mirror, &buf2);
}
}
if (mirror & MIRROR_PUSH) {
strbuf_reset(&buf);
strbuf_addf(&buf, "remote.%s.mirror", name);
- if (git_config_set(buf.buf, "true"))
- return 1;
+ git_config_set(buf.buf, "true");
}
if (fetch_tags != TAGS_DEFAULT) {
strbuf_reset(&buf);
strbuf_addf(&buf, "remote.%s.tagopt", name);
- if (git_config_set(buf.buf,
- fetch_tags == TAGS_SET ? "--tags" : "--no-tags"))
- return 1;
+ git_config_set(buf.buf,
+ fetch_tags == TAGS_SET ? "--tags" : "--no-tags");
}
if (fetch && fetch_remote(name))
@@ -592,21 +588,15 @@ static int migrate_file(struct remote *remote)
strbuf_addf(&buf, "remote.%s.url", remote->name);
for (i = 0; i < remote->url_nr; i++)
- if (git_config_set_multivar(buf.buf, remote->url[i], "^$", 0))
- return error(_("Could not append '%s' to '%s'"),
- remote->url[i], buf.buf);
+ git_config_set_multivar(buf.buf, remote->url[i], "^$", 0);
strbuf_reset(&buf);
strbuf_addf(&buf, "remote.%s.push", remote->name);
for (i = 0; i < remote->push_refspec_nr; i++)
- if (git_config_set_multivar(buf.buf, remote->push_refspec[i], "^$", 0))
- return error(_("Could not append '%s' to '%s'"),
- remote->push_refspec[i], buf.buf);
+ git_config_set_multivar(buf.buf, remote->push_refspec[i], "^$", 0);
strbuf_reset(&buf);
strbuf_addf(&buf, "remote.%s.fetch", remote->name);
for (i = 0; i < remote->fetch_refspec_nr; i++)
- if (git_config_set_multivar(buf.buf, remote->fetch_refspec[i], "^$", 0))
- return error(_("Could not append '%s' to '%s'"),
- remote->fetch_refspec[i], buf.buf);
+ git_config_set_multivar(buf.buf, remote->fetch_refspec[i], "^$", 0);
if (remote->origin == REMOTE_REMOTES)
unlink_or_warn(git_path("remotes/%s", remote->name));
else if (remote->origin == REMOTE_BRANCHES)
@@ -657,8 +647,7 @@ static int mv(int argc, const char **argv)
strbuf_reset(&buf);
strbuf_addf(&buf, "remote.%s.fetch", rename.new);
- if (git_config_set_multivar(buf.buf, NULL, NULL, 1))
- return error(_("Could not remove config section '%s'"), buf.buf);
+ git_config_set_multivar(buf.buf, NULL, NULL, 1);
strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old);
for (i = 0; i < oldremote->fetch_refspec_nr; i++) {
char *ptr;
@@ -678,8 +667,7 @@ static int mv(int argc, const char **argv)
"\tPlease update the configuration manually if necessary."),
buf2.buf);
- if (git_config_set_multivar(buf.buf, buf2.buf, "^$", 0))
- return error(_("Could not append '%s'"), buf.buf);
+ git_config_set_multivar(buf.buf, buf2.buf, "^$", 0);
}
read_branches();
@@ -689,9 +677,7 @@ static int mv(int argc, const char **argv)
if (info->remote_name && !strcmp(info->remote_name, rename.old)) {
strbuf_reset(&buf);
strbuf_addf(&buf, "branch.%s.remote", item->string);
- if (git_config_set(buf.buf, rename.new)) {
- return error(_("Could not set '%s'"), buf.buf);
- }
+ git_config_set(buf.buf, rename.new);
}
}
@@ -789,10 +775,7 @@ static int rm(int argc, const char **argv)
strbuf_reset(&buf);
strbuf_addf(&buf, "branch.%s.%s",
item->string, *k);
- if (git_config_set(buf.buf, NULL)) {
- strbuf_release(&buf);
- return -1;
- }
+ git_config_set(buf.buf, NULL);
}
}
}
@@ -1411,12 +1394,12 @@ static int update(int argc, const char **argv)
return retval;
}
-static int remove_all_fetch_refspecs(const char *remote, const char *key)
+static void remove_all_fetch_refspecs(const char *remote, const char *key)
{
- return git_config_set_multivar(key, NULL, NULL, 1);
+ git_config_set_multivar(key, NULL, NULL, 1);
}
-static int add_branches(struct remote *remote, const char **branches,
+static void add_branches(struct remote *remote, const char **branches,
const char *key)
{
const char *remotename = remote->name;
@@ -1424,13 +1407,9 @@ static int add_branches(struct remote *remote, const char **branches,
struct strbuf refspec = STRBUF_INIT;
for (; *branches; branches++)
- if (add_branch(key, *branches, remotename, mirror, &refspec)) {
- strbuf_release(&refspec);
- return 1;
- }
+ add_branch(key, *branches, remotename, mirror, &refspec);
strbuf_release(&refspec);
- return 0;
}
static int set_remote_branches(const char *remotename, const char **branches,
@@ -1445,14 +1424,9 @@ static int set_remote_branches(const char *remotename, const char **branches,
die(_("No such remote '%s'"), remotename);
remote = remote_get(remotename);
- if (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) {
- strbuf_release(&key);
- return 1;
- }
- if (add_branches(remote, branches, key.buf)) {
- strbuf_release(&key);
- return 1;
- }
+ if (!add_mode)
+ remove_all_fetch_refspecs(remotename, key.buf);
+ add_branches(remote, branches, key.buf);
strbuf_release(&key);
return 0;
diff --git a/cache.h b/cache.h
index dfc459c..4841f2c 100644
--- a/cache.h
+++ b/cache.h
@@ -1507,12 +1507,16 @@ extern int git_config_bool(const char *, const char *);
extern int git_config_maybe_bool(const char *, const char *);
extern int git_config_string(const char **, const char *, const char *);
extern int git_config_pathname(const char **, const char *, const char *);
-extern int git_config_set_in_file(const char *, const char *, const char *);
-extern int git_config_set(const char *, const char *);
+extern void git_config_set_in_file(const char *, const char *, const char *);
+extern int git_config_set_in_file_gently(const char *, const char *, const char *);
+extern void git_config_set(const char *, const char *);
+extern int git_config_set_gently(const char *, const char *);
extern int git_config_parse_key(const char *, char **, int *);
extern int git_config_key_is_valid(const char *key);
-extern int git_config_set_multivar(const char *, const char *, const char *, int);
-extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);
+extern void git_config_set_multivar(const char *, const char *, const char *, int);
+extern int git_config_set_multivar_gently(const char *, const char *, const char *, int);
+extern void git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);
+extern int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, int);
extern int git_config_rename_section(const char *, const char *);
extern int git_config_rename_section_in_file(const char *, const char *, const char *);
extern const char *git_etc_gitconfig(void);
diff --git a/config.c b/config.c
index 86a5eb2..b7dbd2b 100644
--- a/config.c
+++ b/config.c
@@ -1825,15 +1825,27 @@ contline:
return offset;
}
-int git_config_set_in_file(const char *config_filename,
+void git_config_set_in_file(const char *config_filename,
const char *key, const char *value)
{
- return git_config_set_multivar_in_file(config_filename, key, value, NULL, 0);
+ git_config_set_multivar_in_file(config_filename, key, value, NULL, 0);
}
-int git_config_set(const char *key, const char *value)
+int git_config_set_in_file_gently(const char *config_filename,
+ const char *key, const char *value)
+{
+ return git_config_set_multivar_in_file_gently(config_filename,
+ key, value, NULL, 0);
+}
+
+void git_config_set(const char *key, const char *value)
+{
+ git_config_set_multivar(key, value, NULL, 0);
+}
+
+int git_config_set_gently(const char *key, const char *value)
{
- return git_config_set_multivar(key, value, NULL, 0);
+ return git_config_set_multivar_gently(key, value, NULL, 0);
}
/*
@@ -1925,6 +1937,17 @@ int git_config_key_is_valid(const char *key)
return !git_config_parse_key_1(key, NULL, NULL, 1);
}
+void git_config_set_multivar_in_file(const char *config_filename,
+ const char *key, const char *value,
+ const char *value_regex, int multi_replace)
+{
+ if (git_config_set_multivar_in_file_gently(config_filename,
+ key, value,
+ value_regex, multi_replace))
+ die(_("Could not set configuration key '%s' to '%s'"),
+ key, value);
+}
+
/*
* If value==NULL, unset in (remove from) config,
* if value_regex!=NULL, disregard key/value pairs where value does not match.
@@ -1950,7 +1973,7 @@ int git_config_key_is_valid(const char *key)
* - the config file is removed and the lock file rename()d to it.
*
*/
-int git_config_set_multivar_in_file(const char *config_filename,
+int git_config_set_multivar_in_file_gently(const char *config_filename,
const char *key, const char *value,
const char *value_regex, int multi_replace)
{
@@ -2179,11 +2202,18 @@ write_err_out:
}
-int git_config_set_multivar(const char *key, const char *value,
+void git_config_set_multivar(const char *key, const char *value,
const char *value_regex, int multi_replace)
{
- return git_config_set_multivar_in_file(NULL, key, value, value_regex,
- multi_replace);
+ git_config_set_multivar_in_file(NULL, key, value, value_regex,
+ multi_replace);
+}
+
+int git_config_set_multivar_gently(const char *key, const char *value,
+ const char *value_regex, int multi_replace)
+{
+ return git_config_set_multivar_in_file_gently(NULL, key, value,
+ value_regex, multi_replace);
}
static int section_name_match (const char *buf, const char *name)
diff --git a/submodule.c b/submodule.c
index b83939c..bc977d1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -69,7 +69,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
strbuf_addstr(&entry, "submodule.");
strbuf_addstr(&entry, submodule->name);
strbuf_addstr(&entry, ".path");
- if (git_config_set_in_file(".gitmodules", entry.buf, newpath) < 0) {
+ if (git_config_set_in_file_gently(".gitmodules", entry.buf, newpath) < 0) {
/* Maybe the user already did that, don't error out here */
warning(_("Could not update .gitmodules entry %s"), entry.buf);
strbuf_release(&entry);
@@ -1087,11 +1087,9 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
/* Update core.worktree setting */
strbuf_reset(&file_name);
strbuf_addf(&file_name, "%s/config", git_dir);
- if (git_config_set_in_file(file_name.buf, "core.worktree",
- relative_path(real_work_tree, git_dir,
- &rel_path)))
- die(_("Could not set core.worktree in %s"),
- file_name.buf);
+ git_config_set_in_file(file_name.buf, "core.worktree",
+ relative_path(real_work_tree, git_dir,
+ &rel_path));
strbuf_release(&file_name);
strbuf_release(&rel_path);
--
2.7.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v3] config: make git_config_set die on failure
2016-02-01 10:59 [PATCH v3] config: make git_config_set die on failure Patrick Steinhardt
@ 2016-02-01 19:10 ` Junio C Hamano
0 siblings, 0 replies; 2+ messages in thread
From: Junio C Hamano @ 2016-02-01 19:10 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King
Patrick Steinhardt <ps@pks.im> writes:
> Furthermore, I do think it's more explicit what the functions are
> doing when there is a 'or_die' suffix. Without this suffix it may
> be unexpected that the functions simply abort the program
> whenever an error occurs.
That largely depends on what you are used to see. Many internal API
functions do "if we cannot do this, die" without or_die suffix.
As the endgame state, I'd find it far more preferrable to see a
function that dies without _or_die(), while allowing outlier callers
to call the _gently() variant to do their own clean-up [*1*].
How we get to that endgame is a different matter. It is a viable
approach like you did in the original series to first temporarily
introduce _or_die() and convert the current callers in small chunks.
The second step would be to rename git_config_set() and friends that
do not die to hae _gently() suffix, and update the remaining callers
to call them. At that point, nobody calls git_config_set(), so we
can drop the _or_die() suffix, which would lead us to the endgame
state.
But because we are talking about only a very small number of
callers, I think either is OK.
Thanks.
[Footnote]
*1* Another consideration while talking about a transition like this
is what would happen in topics in flight (either in my tree
above 'next' or people privately are working on). New callsites
they added expecting git_config_set() to return error would have
to be adjusted to call _gently() variants, and we need to catch
such sites somehow. In this partcular transision, it can easily
be done and you've done so already by making the functions
return no value, so anybody who checked their return values
would be flagged by the compiler.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-02-01 19:10 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-01 10:59 [PATCH v3] config: make git_config_set die on failure Patrick Steinhardt
2016-02-01 19:10 ` Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.