From 45ebdcc99a8d8e7c671eb1db1212d90f5f2db341 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 12:23:28 +0100 Subject: remote: die on config error when setting URL When invoking `git-remote --set-url` we do not check the return value when writing the actual new URL to the configuration file, pretending to the user that the configuration has been set while it was in fact not persisted. Fix this problem by dying early when setting the config fails. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/remote.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) (limited to 'builtin/remote.c') diff --git a/builtin/remote.c b/builtin/remote.c index 6694cf20e..0771e4251 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1579,11 +1579,12 @@ static int set_url(int argc, const char **argv) /* Special cases that add new entry. */ if ((!oldurl && !delete_mode) || add_mode) { if (add_mode) - git_config_set_multivar(name_buf.buf, newurl, - "^$", 0); + git_config_set_multivar_or_die(name_buf.buf, newurl, + "^$", 0); else - git_config_set(name_buf.buf, newurl); + git_config_set_or_die(name_buf.buf, newurl); strbuf_release(&name_buf); + return 0; } @@ -1604,9 +1605,9 @@ static int set_url(int argc, const char **argv) regfree(&old_regex); if (!delete_mode) - git_config_set_multivar(name_buf.buf, newurl, oldurl, 0); + git_config_set_multivar_or_die(name_buf.buf, newurl, oldurl, 0); else - git_config_set_multivar(name_buf.buf, NULL, oldurl, 1); + git_config_set_multivar_or_die(name_buf.buf, NULL, oldurl, 1); return 0; } -- cgit v1.2.1 From ab5e4b67e14cbd09e74d3b19c0553a3716205c45 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 12:23:29 +0100 Subject: remote: die on config error when setting/adding branches When we add or set new branches (e.g. by `git remote add -f` or `git remote set-branches`) we do not check for error codes when writing the branches to the configuration file. When persisting the configuration failed we are left with a remote that has none or not all of the branches that should have been set without notifying the user. Fix this issue by dying early on configuration error. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/remote.c | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) (limited to 'builtin/remote.c') diff --git a/builtin/remote.c b/builtin/remote.c index 0771e4251..6a7244e90 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -108,8 +108,8 @@ enum { #define MIRROR_PUSH 2 #define MIRROR_BOTH (MIRROR_FETCH|MIRROR_PUSH) -static int add_branch(const char *key, const char *branchname, - const char *remotename, int mirror, struct strbuf *tmp) +static void add_branch(const char *key, const char *branchname, + const char *remotename, int mirror, struct strbuf *tmp) { strbuf_reset(tmp); strbuf_addch(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_or_die(key, tmp->buf, "^$", 0); } static const char mirror_advice[] = @@ -206,9 +206,8 @@ 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); } } @@ -1412,21 +1411,17 @@ static int remove_all_fetch_refspecs(const char *remote, const char *key) return git_config_set_multivar(key, NULL, NULL, 1); } -static int add_branches(struct remote *remote, const char **branches, - const char *key) +static void add_branches(struct remote *remote, const char **branches, + const char *key) { const char *remotename = remote->name; int mirror = remote->mirror; 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,10 +1440,7 @@ static int set_remote_branches(const char *remotename, const char **branches, strbuf_release(&key); return 1; } - if (add_branches(remote, branches, key.buf)) { - strbuf_release(&key); - return 1; - } + add_branches(remote, branches, key.buf); strbuf_release(&key); return 0; -- cgit v1.2.1 From c397debf3d046713ffd040f2c7da5e6921121ce8 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 12:23:30 +0100 Subject: remote: die on config error when manipulating remotes When manipulating remotes we try to set various configuration values without checking if the values were persisted correctly, possibly leaving the remote in an inconsistent state. Fix this issue by dying early and notifying the user about the error. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/remote.c | 39 ++++++++++++--------------------------- 1 file changed, 12 insertions(+), 27 deletions(-) (limited to 'builtin/remote.c') diff --git a/builtin/remote.c b/builtin/remote.c index 6a7244e90..dee6551e3 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -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_or_die(buf.buf, url); if (!mirror || mirror & MIRROR_FETCH) { strbuf_reset(&buf); @@ -214,16 +213,14 @@ static int add(int argc, const char **argv) 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_or_die(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_or_die(buf.buf, + fetch_tags == TAGS_SET ? "--tags" : "--no-tags"); } if (fetch && fetch_remote(name)) @@ -589,25 +586,20 @@ 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_or_die(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_or_die(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_or_die(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) unlink_or_warn(git_path("branches/%s", remote->name)); + return 0; } @@ -654,8 +646,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_or_die(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; @@ -675,8 +666,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_or_die(buf.buf, buf2.buf, "^$", 0); } read_branches(); @@ -686,9 +676,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_or_die(buf.buf, rename.new); } } @@ -786,10 +774,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_or_die(buf.buf, NULL); } } } -- cgit v1.2.1 From 30598ad06f2adfef1f74d6348677358865cbf373 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 12:23:35 +0100 Subject: config: rename git_config_set to git_config_set_gently The desired default behavior for `git_config_set` is to die whenever an error occurs. Dying is the default for a lot of internal functions when failures occur and is in this case the right thing to do for most callers as otherwise we might run into inconsistent repositories without noticing. As some code may rely on the actual return values for `git_config_set` we still require the ability to invoke these functions without aborting. Rename the existing `git_config_set` functions to `git_config_set_gently` to keep them available for those callers. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/remote.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'builtin/remote.c') diff --git a/builtin/remote.c b/builtin/remote.c index dee6551e3..6cacbca02 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1393,7 +1393,7 @@ static int update(int argc, const char **argv) static int remove_all_fetch_refspecs(const char *remote, const char *key) { - return git_config_set_multivar(key, NULL, NULL, 1); + return git_config_set_multivar_gently(key, NULL, NULL, 1); } static void add_branches(struct remote *remote, const char **branches, -- cgit v1.2.1 From 3d1806487af395fb33d1de92633e96571b296305 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 12:23:36 +0100 Subject: config: rename git_config_set_or_die to git_config_set Rename git_config_set_or_die functions to git_config_set, leading to the new default behavior of dying whenever a configuration error occurs. By now all callers that shall die on error have been transitioned to the _or_die variants, thus making this patch a simple rename of the functions. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/remote.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) (limited to 'builtin/remote.c') diff --git a/builtin/remote.c b/builtin/remote.c index 6cacbca02..43136951b 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -119,7 +119,7 @@ static void add_branch(const char *key, const char *branchname, else strbuf_addf(tmp, "refs/heads/%s:refs/remotes/%s/%s", branchname, remotename, branchname); - git_config_set_multivar_or_die(key, tmp->buf, "^$", 0); + git_config_set_multivar(key, tmp->buf, "^$", 0); } static const char mirror_advice[] = @@ -197,7 +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); - git_config_set_or_die(buf.buf, url); + git_config_set(buf.buf, url); if (!mirror || mirror & MIRROR_FETCH) { strbuf_reset(&buf); @@ -213,14 +213,14 @@ static int add(int argc, const char **argv) if (mirror & MIRROR_PUSH) { strbuf_reset(&buf); strbuf_addf(&buf, "remote.%s.mirror", name); - git_config_set_or_die(buf.buf, "true"); + git_config_set(buf.buf, "true"); } if (fetch_tags != TAGS_DEFAULT) { strbuf_reset(&buf); strbuf_addf(&buf, "remote.%s.tagopt", name); - git_config_set_or_die(buf.buf, - fetch_tags == TAGS_SET ? "--tags" : "--no-tags"); + git_config_set(buf.buf, + fetch_tags == TAGS_SET ? "--tags" : "--no-tags"); } if (fetch && fetch_remote(name)) @@ -586,15 +586,15 @@ static int migrate_file(struct remote *remote) strbuf_addf(&buf, "remote.%s.url", remote->name); for (i = 0; i < remote->url_nr; i++) - git_config_set_multivar_or_die(buf.buf, remote->url[i], "^$", 0); + 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++) - git_config_set_multivar_or_die(buf.buf, remote->push_refspec[i], "^$", 0); + 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++) - git_config_set_multivar_or_die(buf.buf, remote->fetch_refspec[i], "^$", 0); + 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) @@ -646,7 +646,7 @@ static int mv(int argc, const char **argv) strbuf_reset(&buf); strbuf_addf(&buf, "remote.%s.fetch", rename.new); - git_config_set_multivar_or_die(buf.buf, NULL, NULL, 1); + 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; @@ -666,7 +666,7 @@ static int mv(int argc, const char **argv) "\tPlease update the configuration manually if necessary."), buf2.buf); - git_config_set_multivar_or_die(buf.buf, buf2.buf, "^$", 0); + git_config_set_multivar(buf.buf, buf2.buf, "^$", 0); } read_branches(); @@ -676,7 +676,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); - git_config_set_or_die(buf.buf, rename.new); + git_config_set(buf.buf, rename.new); } } @@ -774,7 +774,7 @@ static int rm(int argc, const char **argv) strbuf_reset(&buf); strbuf_addf(&buf, "branch.%s.%s", item->string, *k); - git_config_set_or_die(buf.buf, NULL); + git_config_set(buf.buf, NULL); } } } @@ -1556,10 +1556,10 @@ static int set_url(int argc, const char **argv) /* Special cases that add new entry. */ if ((!oldurl && !delete_mode) || add_mode) { if (add_mode) - git_config_set_multivar_or_die(name_buf.buf, newurl, + git_config_set_multivar(name_buf.buf, newurl, "^$", 0); else - git_config_set_or_die(name_buf.buf, newurl); + git_config_set(name_buf.buf, newurl); strbuf_release(&name_buf); return 0; @@ -1582,9 +1582,9 @@ static int set_url(int argc, const char **argv) regfree(&old_regex); if (!delete_mode) - git_config_set_multivar_or_die(name_buf.buf, newurl, oldurl, 0); + git_config_set_multivar(name_buf.buf, newurl, oldurl, 0); else - git_config_set_multivar_or_die(name_buf.buf, NULL, oldurl, 1); + git_config_set_multivar(name_buf.buf, NULL, oldurl, 1); return 0; } -- cgit v1.2.1