| Message ID | 04a8673c3cb80802ee20fa4376872cb5ee464264.1580549512.git.bert.wesarg@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] remote rename/remove: gently handle remote.pushDefault config | expand |
Hi Bert, On Sat, 1 Feb 2020, Bert Wesarg wrote: > When renaming a remote with > > git remote rename X Y > git remote remove X > > Git already renames or removes any branch.<name>.remote and > branch.<name>.pushRemote configurations if their value is X. > > However remote.pushDefault needs a more gentle approach, as this may be > set in a non-repo configuration file. In such a case only a warning is > printed, such as: > > warning: The global configuration remote.pushDefault in: > $HOME/.gitconfig:35 > now names the non-existent remote origin > > It is changed to remote.pushDefault = Y or removed when set in a repo > configuration though. > > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> Very clear commit message. Thank you! > diff --git a/builtin/remote.c b/builtin/remote.c > index a2379a14bf..5af06b74a7 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -615,6 +615,55 @@ static int migrate_file(struct remote *remote) > return 0; > } > > +struct push_default_info > +{ > + const char *old_name; > + enum config_scope scope; > + struct strbuf origin; > + int linenr; > +}; > + > +static int config_read_push_default(const char *key, const char *value, > + void *cb) > +{ > + struct push_default_info* info = cb; > + if (strcmp(key, "remote.pushdefault") || strcmp(value, info->old_name)) > + return 0; We will have to be careful to not segfault if a user has this in their config: [remote] pushDefault i.e. we have to insert `!value || ` before the call to `strcmp()`. It does not make much sense not to specify a value, of course, but we should not segfault in such a case, either. > + > + info->scope = current_config_scope(); Do we want to care about the case where above-mentioned invalid `remote.pushDefault` is configured _and_ overrides an otherwise-valid setting in `~/.gitconfig`? Or for that matter, shouldn't we be careful to handle the case where `git config --global remote.pushDefault` returns `old_name` but that is overridden by a different `git config --local remote.pushDefault`? Concretely, I believe that the patched code will misbehave in this scenario: git config --global remote.pushDefault january git config remote.pushDefault february git remote rename january march If I read the patch right, this will incorrectly warn about the `pushDefault` setting in the user-wide config. > + strbuf_reset(&info->origin); > + strbuf_addstr(&info->origin, current_config_name()); > + info->linenr = current_config_line(); > + > + return 0; > +} > + > +static void handle_push_default(const char* old_name, const char* new_name) That name probably wants to convey better that the push default is handled in the `mv`/`rm` commands here, not in any other command. Maybe `handle_modified_push_default_remote()`? > +{ > + struct push_default_info push_default = { > + old_name, CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 }; Personally, I would prefer the closing bracket to be on a new line, followed by an empty line to separate the variable declaration from the following statements. > + git_config(config_read_push_default, &push_default); > + if (push_default.scope >= CONFIG_SCOPE_COMMAND) > + ; /* pass */ > + else if (push_default.scope >= CONFIG_SCOPE_LOCAL) { > + int result = git_config_set_gently("remote.pushDefault", > + new_name); > + if (new_name && result && result != CONFIG_NOTHING_SET) > + die(_("could not set '%s'"), "remote.pushDefault"); Isn't this more like a `BUG()`? Or do you see any valid scenario where this could happen? If you do, it may make a lot of sense to call `die_errno()` here, to give the user _some_ sort of an actionable insight as to what went wrong. > + else if (!new_name && result && result != CONFIG_NOTHING_SET) > + die(_("could not unset '%s'"), "remote.pushDefault"); Same here. > + } else if (push_default.scope >= CONFIG_SCOPE_SYSTEM) { > + /* warn */ > + warning(_("The %s configuration remote.pushDefault in:\n" > + "\t%s:%d\n" > + "now names the non-existent remote '%s'"), > + config_scope_name(push_default.scope), > + push_default.origin.buf, push_default.linenr, > + old_name); > + } > +} > + > + > static int mv(int argc, const char **argv) > { > struct option options[] = { > @@ -750,6 +799,9 @@ static int mv(int argc, const char **argv) > die(_("creating '%s' failed"), buf.buf); > } > string_list_clear(&remote_branches, 1); > + > + handle_push_default(rename.old_name, rename.new_name); > + > return 0; > } > > @@ -835,6 +887,8 @@ static int rm(int argc, const char **argv) > strbuf_addf(&buf, "remote.%s", remote->name); > if (git_config_rename_section(buf.buf, NULL) < 1) > return error(_("Could not remove config section '%s'"), buf.buf); > + > + handle_push_default(remote->name, NULL); > } > > return result; > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh > index 082042b05a..dda81b7d07 100755 > --- a/t/t5505-remote.sh > +++ b/t/t5505-remote.sh > @@ -734,6 +734,7 @@ test_expect_success 'reject adding remote with an invalid name' ' > # the last two ones check if the config is updated. > > test_expect_success 'rename a remote' ' > + test_config_global remote.pushDefault origin && > git clone one four && > ( > cd four && > @@ -744,7 +745,42 @@ test_expect_success 'rename a remote' ' > test "$(git rev-parse upstream/master)" = "$(git rev-parse master)" && > test "$(git config remote.upstream.fetch)" = "+refs/heads/*:refs/remotes/upstream/*" && > test "$(git config branch.master.remote)" = "upstream" && > - test "$(git config branch.master.pushRemote)" = "upstream" > + test "$(git config branch.master.pushRemote)" = "upstream" && > + test "$(git config --global remote.pushDefault)" = "origin" > + ) > +' > + > +test_expect_success 'rename a remote renames repo remote.pushDefault' ' > + git clone one four.1 && I am not sure that a full clone is warranted here. Maybe use `--no-checkout` here and in the subsequent test cases? Omitting that option makes the tests only slower for no gain. > + ( > + cd four.1 && > + git config remote.pushDefault origin && > + git remote rename origin upstream && > + test "$(git config --local remote.pushDefault)" = "upstream" > + ) > +' > + > +test_expect_success 'rename a remote renames repo remote.pushDefault but ignores global' ' > + test_config_global remote.pushDefault other && > + git clone one four.2 && > + ( > + cd four.2 && > + git config remote.pushDefault origin && > + git remote rename origin upstream && > + test "$(git config --global remote.pushDefault)" = "other" && > + test "$(git config --local remote.pushDefault)" = "upstream" > + ) > +' > + > +test_expect_success 'rename a remote renames repo remote.pushDefault but keeps global' ' > + test_config_global remote.pushDefault origin && > + git clone one four.3 && > + ( > + cd four.3 && > + git config remote.pushDefault origin && > + git remote rename origin upstream && > + test "$(git config --global remote.pushDefault)" = "origin" && > + test "$(git config --local remote.pushDefault)" = "upstream" A lot of tests added. Personally, I would have probably only extended the existing `rename a remote` to verify that a repository-local `remote.pushDefault` _is_ renamed, and then added one test case that verifies that not only is a user-wide `remote.pushDefault` left alone but also warned about. > ) > ' > > @@ -787,6 +823,7 @@ test_expect_success 'rename succeeds with existing remote.<target>.prune' ' > ' > > test_expect_success 'remove a remote' ' > + test_config_global remote.pushDefault origin && > git clone one four.five && > ( > cd four.five && > @@ -794,7 +831,42 @@ test_expect_success 'remove a remote' ' > git remote remove origin && > test -z "$(git for-each-ref refs/remotes/origin)" && > test_must_fail git config branch.master.remote && > - test_must_fail git config branch.master.pushRemote > + test_must_fail git config branch.master.pushRemote && > + test "$(git config --global remote.pushDefault)" = "origin" > + ) > +' > + > +test_expect_success 'remove a remote removes repo remote.pushDefault' ' > + git clone one four.five.1 && > + ( > + cd four.five.1 && > + git config remote.pushDefault origin && > + git remote remove origin && > + test_must_fail git config --local remote.pushDefault Now that I see this sort of "in action", I have to wonder whether I would be okay with a `remote.pushDefault` simply vanishing. I think that would puzzle me ("Didn't I set a push default before? I must be getting old and delusional."). In the least, I would want to see a warning here ("As the remote 'xyz' was deleted, so was the `remote.pushDefault = xyz` setting" or some such). It strikes me as a fundamentally different thing whether we simply re-target or delete a `pushDefault` setting. The former needs no further warning, but the latter does. > + ) > +' > + > +test_expect_success 'remove a remote removes repo remote.pushDefault but ignores global' ' > + test_config_global remote.pushDefault other && > + git clone one four.five.2 && > + ( > + cd four.five.2 && > + git config remote.pushDefault origin && > + git remote remove origin && > + test "$(git config --global remote.pushDefault)" = "other" && > + test_must_fail git config --local remote.pushDefault > + ) > +' > + > +test_expect_success 'remove a remote removes repo remote.pushDefault but keeps global' ' > + test_config_global remote.pushDefault origin && > + git clone one four.five.3 && > + ( > + cd four.five.3 && > + git config remote.pushDefault origin && > + git remote remove origin && > + test "$(git config --global remote.pushDefault)" = "origin" && > + test_must_fail git config --local remote.pushDefault Since the `mv` case already covers those code paths, I would be a lot more parsimonious in adding test cases for `rm`. There are voices claiming that adding regression tests is always a good thing, but I would counter that it has to strike a balance between coverage and runtime. We see a more and more contributions -- even from long-time contributors -- where the test suite obviously has not been run (because the CI builds are failing, and there is no doubt that seasoned contributors in particular would fix those failures before contributing their patches, _if_ they were aware of those test failures), proving just how costly our test suite has become. Other than that, the patch looks fine to me. Thank you, Dscho > ) > ' > > -- > 2.25.0.30.g00ce2e43d4 > >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> + struct push_default_info* info = cb; >> + if (strcmp(key, "remote.pushdefault") || strcmp(value, info->old_name)) >> + return 0; > > We will have to be careful to not segfault if a user has this in their > config: > > [remote] > pushDefault > > i.e. we have to insert `!value || ` before the call to `strcmp()`. True. The primary reader in remote.c::handle_config() uses git_config_string() that complains that the variable is not bool, but we should reat end-user input as something suspicious and protect us against it. > Concretely, I believe that the patched code will misbehave in this > scenario: > > git config --global remote.pushDefault january > git config remote.pushDefault february > git remote rename january march Good to see careful analysis. Thanks. >> +static void handle_push_default(const char* old_name, const char* new_name) > > That name probably wants to convey better that the push default is handled > in the `mv`/`rm` commands here, not in any other command. Maybe > `handle_modified_push_default_remote()`? Also, the asterisk sticks to the variable not the type ;-) >> +{ >> + struct push_default_info push_default = { >> + old_name, CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 }; > > Personally, I would prefer the closing bracket to be on a new line, > followed by an empty line to separate the variable declaration from the > following statements. Yes, yes.
diff --git a/builtin/remote.c b/builtin/remote.c index a2379a14bf..5af06b74a7 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -615,6 +615,55 @@ static int migrate_file(struct remote *remote) return 0; } +struct push_default_info +{ + const char *old_name; + enum config_scope scope; + struct strbuf origin; + int linenr; +}; + +static int config_read_push_default(const char *key, const char *value, + void *cb) +{ + struct push_default_info* info = cb; + if (strcmp(key, "remote.pushdefault") || strcmp(value, info->old_name)) + return 0; + + info->scope = current_config_scope(); + strbuf_reset(&info->origin); + strbuf_addstr(&info->origin, current_config_name()); + info->linenr = current_config_line(); + + return 0; +} + +static void handle_push_default(const char* old_name, const char* new_name) +{ + struct push_default_info push_default = { + old_name, CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 }; + git_config(config_read_push_default, &push_default); + if (push_default.scope >= CONFIG_SCOPE_COMMAND) + ; /* pass */ + else if (push_default.scope >= CONFIG_SCOPE_LOCAL) { + int result = git_config_set_gently("remote.pushDefault", + new_name); + if (new_name && result && result != CONFIG_NOTHING_SET) + die(_("could not set '%s'"), "remote.pushDefault"); + else if (!new_name && result && result != CONFIG_NOTHING_SET) + die(_("could not unset '%s'"), "remote.pushDefault"); + } else if (push_default.scope >= CONFIG_SCOPE_SYSTEM) { + /* warn */ + warning(_("The %s configuration remote.pushDefault in:\n" + "\t%s:%d\n" + "now names the non-existent remote '%s'"), + config_scope_name(push_default.scope), + push_default.origin.buf, push_default.linenr, + old_name); + } +} + + static int mv(int argc, const char **argv) { struct option options[] = { @@ -750,6 +799,9 @@ static int mv(int argc, const char **argv) die(_("creating '%s' failed"), buf.buf); } string_list_clear(&remote_branches, 1); + + handle_push_default(rename.old_name, rename.new_name); + return 0; } @@ -835,6 +887,8 @@ static int rm(int argc, const char **argv) strbuf_addf(&buf, "remote.%s", remote->name); if (git_config_rename_section(buf.buf, NULL) < 1) return error(_("Could not remove config section '%s'"), buf.buf); + + handle_push_default(remote->name, NULL); } return result; diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 082042b05a..dda81b7d07 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -734,6 +734,7 @@ test_expect_success 'reject adding remote with an invalid name' ' # the last two ones check if the config is updated. test_expect_success 'rename a remote' ' + test_config_global remote.pushDefault origin && git clone one four && ( cd four && @@ -744,7 +745,42 @@ test_expect_success 'rename a remote' ' test "$(git rev-parse upstream/master)" = "$(git rev-parse master)" && test "$(git config remote.upstream.fetch)" = "+refs/heads/*:refs/remotes/upstream/*" && test "$(git config branch.master.remote)" = "upstream" && - test "$(git config branch.master.pushRemote)" = "upstream" + test "$(git config branch.master.pushRemote)" = "upstream" && + test "$(git config --global remote.pushDefault)" = "origin" + ) +' + +test_expect_success 'rename a remote renames repo remote.pushDefault' ' + git clone one four.1 && + ( + cd four.1 && + git config remote.pushDefault origin && + git remote rename origin upstream && + test "$(git config --local remote.pushDefault)" = "upstream" + ) +' + +test_expect_success 'rename a remote renames repo remote.pushDefault but ignores global' ' + test_config_global remote.pushDefault other && + git clone one four.2 && + ( + cd four.2 && + git config remote.pushDefault origin && + git remote rename origin upstream && + test "$(git config --global remote.pushDefault)" = "other" && + test "$(git config --local remote.pushDefault)" = "upstream" + ) +' + +test_expect_success 'rename a remote renames repo remote.pushDefault but keeps global' ' + test_config_global remote.pushDefault origin && + git clone one four.3 && + ( + cd four.3 && + git config remote.pushDefault origin && + git remote rename origin upstream && + test "$(git config --global remote.pushDefault)" = "origin" && + test "$(git config --local remote.pushDefault)" = "upstream" ) ' @@ -787,6 +823,7 @@ test_expect_success 'rename succeeds with existing remote.<target>.prune' ' ' test_expect_success 'remove a remote' ' + test_config_global remote.pushDefault origin && git clone one four.five && ( cd four.five && @@ -794,7 +831,42 @@ test_expect_success 'remove a remote' ' git remote remove origin && test -z "$(git for-each-ref refs/remotes/origin)" && test_must_fail git config branch.master.remote && - test_must_fail git config branch.master.pushRemote + test_must_fail git config branch.master.pushRemote && + test "$(git config --global remote.pushDefault)" = "origin" + ) +' + +test_expect_success 'remove a remote removes repo remote.pushDefault' ' + git clone one four.five.1 && + ( + cd four.five.1 && + git config remote.pushDefault origin && + git remote remove origin && + test_must_fail git config --local remote.pushDefault + ) +' + +test_expect_success 'remove a remote removes repo remote.pushDefault but ignores global' ' + test_config_global remote.pushDefault other && + git clone one four.five.2 && + ( + cd four.five.2 && + git config remote.pushDefault origin && + git remote remove origin && + test "$(git config --global remote.pushDefault)" = "other" && + test_must_fail git config --local remote.pushDefault + ) +' + +test_expect_success 'remove a remote removes repo remote.pushDefault but keeps global' ' + test_config_global remote.pushDefault origin && + git clone one four.five.3 && + ( + cd four.five.3 && + git config remote.pushDefault origin && + git remote remove origin && + test "$(git config --global remote.pushDefault)" = "origin" && + test_must_fail git config --local remote.pushDefault ) '
When renaming a remote with git remote rename X Y git remote remove X Git already renames or removes any branch.<name>.remote and branch.<name>.pushRemote configurations if their value is X. However remote.pushDefault needs a more gentle approach, as this may be set in a non-repo configuration file. In such a case only a warning is printed, such as: warning: The global configuration remote.pushDefault in: $HOME/.gitconfig:35 now names the non-existent remote origin It is changed to remote.pushDefault = Y or removed when set in a repo configuration though. Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> --- Matthew, you are in Cc because of your current work 'config: allow user to know scope of config options'. I think I'm correct to assuming an ordering of the enum config_scope. Changes since v3: * do not use `test_config_global` in a subshell Changes since v1: * handle also 'git remote remove' Cc: Junio C Hamano <gitster@pobox.com> Cc: Johannes Schindelin <johannes.schindelin@gmx.de> Cc: Matthew Rogers <mattr94@gmail.com> --- builtin/remote.c | 54 +++++++++++++++++++++++++++++++++ t/t5505-remote.sh | 76 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 128 insertions(+), 2 deletions(-)