{"thread":{"id":"52731","subject":"[PATCH v4] remote rename/remove: gently handle remote.pushDefault config","startedAt":"2020-02-01T09:34:16Z","lastAt":"2020-02-04T20:11:31Z","messageCount":3,"participants":["Bert Wesarg","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"390982","messageId":"04a8673c3cb80802ee20fa4376872cb5ee464264.1580549512.git.bert.wesarg@googlemail.com","threadId":"52731","inReplyTo":null,"subject":"[PATCH v4] remote rename/remove: gently handle remote.pushDefault config","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2020-02-01T09:34:09Z","receivedAt":"2020-02-01T09:34:16Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"When renaming a remote with\n\n    git remote rename X Y\n    git remote remove X\n\nGit already renames or removes any branch.<name>.remote and\nbranch.<name>.pushRemote configurations if their value is X.\n\nHowever remote.pushDefault needs a more gentle approach, as this may be\nset in a non-repo configuration file. In such a case only a warning is\nprinted, such as:\n\nwarning: The global configuration remote.pushDefault in:\n\t$HOME/.gitconfig:35\nnow names the non-existent remote origin\n\nIt is changed to remote.pushDefault = Y or removed when set in a repo\nconfiguration though.\n\nSigned-off-by: Bert Wesarg <bert.wesarg@googlemail.com>\n---\n\nMatthew, you are in Cc because of your current work 'config: allow user to\nknow scope of config options'. I think I'm correct to assuming an ordering\nof the enum config_scope.\n\nChanges since v3:\n * do not use `test_config_global` in a subshell\n\nChanges since v1:\n * handle also 'git remote remove'\n\nCc: Junio C Hamano <gitster@pobox.com>\nCc: Johannes Schindelin <johannes.schindelin@gmx.de>\nCc: Matthew Rogers <mattr94@gmail.com>\n---\n builtin/remote.c  | 54 +++++++++++++++++++++++++++++++++\n t/t5505-remote.sh | 76 +++++++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 128 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex a2379a14bf..5af06b74a7 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -615,6 +615,55 @@ static int migrate_file(struct remote *remote)\n \treturn 0;\n }\n \n+struct push_default_info\n+{\n+\tconst char *old_name;\n+\tenum config_scope scope;\n+\tstruct strbuf origin;\n+\tint linenr;\n+};\n+\n+static int config_read_push_default(const char *key, const char *value,\n+\tvoid *cb)\n+{\n+\tstruct push_default_info* info = cb;\n+\tif (strcmp(key, \"remote.pushdefault\") || strcmp(value, info->old_name))\n+\t\treturn 0;\n+\n+\tinfo->scope = current_config_scope();\n+\tstrbuf_reset(&info->origin);\n+\tstrbuf_addstr(&info->origin, current_config_name());\n+\tinfo->linenr = current_config_line();\n+\n+\treturn 0;\n+}\n+\n+static void handle_push_default(const char* old_name, const char* new_name)\n+{\n+\tstruct push_default_info push_default = {\n+\t\told_name, CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 };\n+\tgit_config(config_read_push_default, &push_default);\n+\tif (push_default.scope >= CONFIG_SCOPE_COMMAND)\n+\t\t; /* pass */\n+\telse if (push_default.scope >= CONFIG_SCOPE_LOCAL) {\n+\t\tint result = git_config_set_gently(\"remote.pushDefault\",\n+\t\t\t\t\t\t   new_name);\n+\t\tif (new_name && result && result != CONFIG_NOTHING_SET)\n+\t\t\tdie(_(\"could not set '%s'\"), \"remote.pushDefault\");\n+\t\telse if (!new_name && result && result != CONFIG_NOTHING_SET)\n+\t\t\tdie(_(\"could not unset '%s'\"), \"remote.pushDefault\");\n+\t} else if (push_default.scope >= CONFIG_SCOPE_SYSTEM) {\n+\t\t/* warn */\n+\t\twarning(_(\"The %s configuration remote.pushDefault in:\\n\"\n+\t\t\t  \"\\t%s:%d\\n\"\n+\t\t\t  \"now names the non-existent remote '%s'\"),\n+\t\t\tconfig_scope_name(push_default.scope),\n+\t\t\tpush_default.origin.buf, push_default.linenr,\n+\t\t\told_name);\n+\t}\n+}\n+\n+\n static int mv(int argc, const char **argv)\n {\n \tstruct option options[] = {\n@@ -750,6 +799,9 @@ static int mv(int argc, const char **argv)\n \t\t\tdie(_(\"creating '%s' failed\"), buf.buf);\n \t}\n \tstring_list_clear(&remote_branches, 1);\n+\n+\thandle_push_default(rename.old_name, rename.new_name);\n+\n \treturn 0;\n }\n \n@@ -835,6 +887,8 @@ static int rm(int argc, const char **argv)\n \t\tstrbuf_addf(&buf, \"remote.%s\", remote->name);\n \t\tif (git_config_rename_section(buf.buf, NULL) < 1)\n \t\t\treturn error(_(\"Could not remove config section '%s'\"), buf.buf);\n+\n+\t\thandle_push_default(remote->name, NULL);\n \t}\n \n \treturn result;\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 082042b05a..dda81b7d07 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -734,6 +734,7 @@ test_expect_success 'reject adding remote with an invalid name' '\n # the last two ones check if the config is updated.\n \n test_expect_success 'rename a remote' '\n+\ttest_config_global remote.pushDefault origin &&\n \tgit clone one four &&\n \t(\n \t\tcd four &&\n@@ -744,7 +745,42 @@ test_expect_success 'rename a remote' '\n \t\ttest \"$(git rev-parse upstream/master)\" = \"$(git rev-parse master)\" &&\n \t\ttest \"$(git config remote.upstream.fetch)\" = \"+refs/heads/*:refs/remotes/upstream/*\" &&\n \t\ttest \"$(git config branch.master.remote)\" = \"upstream\" &&\n-\t\ttest \"$(git config branch.master.pushRemote)\" = \"upstream\"\n+\t\ttest \"$(git config branch.master.pushRemote)\" = \"upstream\" &&\n+\t\ttest \"$(git config --global remote.pushDefault)\" = \"origin\"\n+\t)\n+'\n+\n+test_expect_success 'rename a remote renames repo remote.pushDefault' '\n+\tgit clone one four.1 &&\n+\t(\n+\t\tcd four.1 &&\n+\t\tgit config remote.pushDefault origin &&\n+\t\tgit remote rename origin upstream &&\n+\t\ttest \"$(git config --local remote.pushDefault)\" = \"upstream\"\n+\t)\n+'\n+\n+test_expect_success 'rename a remote renames repo remote.pushDefault but ignores global' '\n+\ttest_config_global remote.pushDefault other &&\n+\tgit clone one four.2 &&\n+\t(\n+\t\tcd four.2 &&\n+\t\tgit config remote.pushDefault origin &&\n+\t\tgit remote rename origin upstream &&\n+\t\ttest \"$(git config --global remote.pushDefault)\" = \"other\" &&\n+\t\ttest \"$(git config --local remote.pushDefault)\" = \"upstream\"\n+\t)\n+'\n+\n+test_expect_success 'rename a remote renames repo remote.pushDefault but keeps global' '\n+\ttest_config_global remote.pushDefault origin &&\n+\tgit clone one four.3 &&\n+\t(\n+\t\tcd four.3 &&\n+\t\tgit config remote.pushDefault origin &&\n+\t\tgit remote rename origin upstream &&\n+\t\ttest \"$(git config --global remote.pushDefault)\" = \"origin\" &&\n+\t\ttest \"$(git config --local remote.pushDefault)\" = \"upstream\"\n \t)\n '\n \n@@ -787,6 +823,7 @@ test_expect_success 'rename succeeds with existing remote.<target>.prune' '\n '\n \n test_expect_success 'remove a remote' '\n+\ttest_config_global remote.pushDefault origin &&\n \tgit clone one four.five &&\n \t(\n \t\tcd four.five &&\n@@ -794,7 +831,42 @@ test_expect_success 'remove a remote' '\n \t\tgit remote remove origin &&\n \t\ttest -z \"$(git for-each-ref refs/remotes/origin)\" &&\n \t\ttest_must_fail git config branch.master.remote &&\n-\t\ttest_must_fail git config branch.master.pushRemote\n+\t\ttest_must_fail git config branch.master.pushRemote &&\n+\t\ttest \"$(git config --global remote.pushDefault)\" = \"origin\"\n+\t)\n+'\n+\n+test_expect_success 'remove a remote removes repo remote.pushDefault' '\n+\tgit clone one four.five.1 &&\n+\t(\n+\t\tcd four.five.1 &&\n+\t\tgit config remote.pushDefault origin &&\n+\t\tgit remote remove origin &&\n+\t\ttest_must_fail git config --local remote.pushDefault\n+\t)\n+'\n+\n+test_expect_success 'remove a remote removes repo remote.pushDefault but ignores global' '\n+\ttest_config_global remote.pushDefault other &&\n+\tgit clone one four.five.2 &&\n+\t(\n+\t\tcd four.five.2 &&\n+\t\tgit config remote.pushDefault origin &&\n+\t\tgit remote remove origin &&\n+\t\ttest \"$(git config --global remote.pushDefault)\" = \"other\" &&\n+\t\ttest_must_fail git config --local remote.pushDefault\n+\t)\n+'\n+\n+test_expect_success 'remove a remote removes repo remote.pushDefault but keeps global' '\n+\ttest_config_global remote.pushDefault origin &&\n+\tgit clone one four.five.3 &&\n+\t(\n+\t\tcd four.five.3 &&\n+\t\tgit config remote.pushDefault origin &&\n+\t\tgit remote remove origin &&\n+\t\ttest \"$(git config --global remote.pushDefault)\" = \"origin\" &&\n+\t\ttest_must_fail git config --local remote.pushDefault\n \t)\n '\n \n-- \n2.25.0.30.g00ce2e43d4\n\n"},{"id":"391011","messageId":"nycvar.QRO.7.76.6.2002021911260.46@tvgsbejvaqbjf.bet","threadId":"52731","inReplyTo":"04a8673c3cb80802ee20fa4376872cb5ee464264.1580549512.git.bert.wesarg@googlemail.com","subject":"Re: [PATCH v4] remote rename/remove: gently handle remote.pushDefault config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-02T20:54:31Z","receivedAt":"2020-02-02T20:54:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Bert,\n\nOn Sat, 1 Feb 2020, Bert Wesarg wrote:\n\n> When renaming a remote with\n>\n>     git remote rename X Y\n>     git remote remove X\n>\n> Git already renames or removes any branch.<name>.remote and\n> branch.<name>.pushRemote configurations if their value is X.\n>\n> However remote.pushDefault needs a more gentle approach, as this may be\n> set in a non-repo configuration file. In such a case only a warning is\n> printed, such as:\n>\n> warning: The global configuration remote.pushDefault in:\n> \t$HOME/.gitconfig:35\n> now names the non-existent remote origin\n>\n> It is changed to remote.pushDefault = Y or removed when set in a repo\n> configuration though.\n>\n> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>\n\nVery clear commit message. Thank you!\n\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index a2379a14bf..5af06b74a7 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -615,6 +615,55 @@ static int migrate_file(struct remote *remote)\n>  \treturn 0;\n>  }\n>\n> +struct push_default_info\n> +{\n> +\tconst char *old_name;\n> +\tenum config_scope scope;\n> +\tstruct strbuf origin;\n> +\tint linenr;\n> +};\n> +\n> +static int config_read_push_default(const char *key, const char *value,\n> +\tvoid *cb)\n> +{\n> +\tstruct push_default_info* info = cb;\n> +\tif (strcmp(key, \"remote.pushdefault\") || strcmp(value, info->old_name))\n> +\t\treturn 0;\n\nWe will have to be careful to not segfault if a user has this in their\nconfig:\n\n\t[remote]\n\t\tpushDefault\n\ni.e. we have to insert `!value || ` before the call to `strcmp()`.\n\nIt does not make much sense not to specify a value, of course, but we\nshould not segfault in such a case, either.\n\n> +\n> +\tinfo->scope = current_config_scope();\n\nDo we want to care about the case where above-mentioned invalid\n`remote.pushDefault` is configured _and_ overrides an otherwise-valid\nsetting in `~/.gitconfig`?\n\nOr for that matter, shouldn't we be careful to handle the case where `git\nconfig --global remote.pushDefault` returns `old_name` but that is\noverridden by a different `git config --local remote.pushDefault`?\n\nConcretely, I believe that the patched code will misbehave in this\nscenario:\n\n\tgit config --global remote.pushDefault january\n\tgit config remote.pushDefault february\n\tgit remote rename january march\n\nIf I read the patch right, this will incorrectly warn about the\n`pushDefault` setting in the user-wide config.\n\n> +\tstrbuf_reset(&info->origin);\n> +\tstrbuf_addstr(&info->origin, current_config_name());\n> +\tinfo->linenr = current_config_line();\n> +\n> +\treturn 0;\n> +}\n> +\n> +static void handle_push_default(const char* old_name, const char* new_name)\n\nThat name probably wants to convey better that the push default is handled\nin the `mv`/`rm` commands here, not in any other command. Maybe\n`handle_modified_push_default_remote()`?\n\n> +{\n> +\tstruct push_default_info push_default = {\n> +\t\told_name, CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 };\n\nPersonally, I would prefer the closing bracket to be on a new line,\nfollowed by an empty line to separate the variable declaration from the\nfollowing statements.\n\n> +\tgit_config(config_read_push_default, &push_default);\n> +\tif (push_default.scope >= CONFIG_SCOPE_COMMAND)\n> +\t\t; /* pass */\n> +\telse if (push_default.scope >= CONFIG_SCOPE_LOCAL) {\n> +\t\tint result = git_config_set_gently(\"remote.pushDefault\",\n> +\t\t\t\t\t\t   new_name);\n> +\t\tif (new_name && result && result != CONFIG_NOTHING_SET)\n> +\t\t\tdie(_(\"could not set '%s'\"), \"remote.pushDefault\");\n\nIsn't this more like a `BUG()`? Or do you see any valid scenario where\nthis could happen? If you do, it may make a lot of sense to call\n`die_errno()` here, to give the user _some_ sort of an actionable insight\nas to what went wrong.\n\n> +\t\telse if (!new_name && result && result != CONFIG_NOTHING_SET)\n> +\t\t\tdie(_(\"could not unset '%s'\"), \"remote.pushDefault\");\n\nSame here.\n\n> +\t} else if (push_default.scope >= CONFIG_SCOPE_SYSTEM) {\n> +\t\t/* warn */\n> +\t\twarning(_(\"The %s configuration remote.pushDefault in:\\n\"\n> +\t\t\t  \"\\t%s:%d\\n\"\n> +\t\t\t  \"now names the non-existent remote '%s'\"),\n> +\t\t\tconfig_scope_name(push_default.scope),\n> +\t\t\tpush_default.origin.buf, push_default.linenr,\n> +\t\t\told_name);\n> +\t}\n> +}\n> +\n> +\n>  static int mv(int argc, const char **argv)\n>  {\n>  \tstruct option options[] = {\n> @@ -750,6 +799,9 @@ static int mv(int argc, const char **argv)\n>  \t\t\tdie(_(\"creating '%s' failed\"), buf.buf);\n>  \t}\n>  \tstring_list_clear(&remote_branches, 1);\n> +\n> +\thandle_push_default(rename.old_name, rename.new_name);\n> +\n>  \treturn 0;\n>  }\n>\n> @@ -835,6 +887,8 @@ static int rm(int argc, const char **argv)\n>  \t\tstrbuf_addf(&buf, \"remote.%s\", remote->name);\n>  \t\tif (git_config_rename_section(buf.buf, NULL) < 1)\n>  \t\t\treturn error(_(\"Could not remove config section '%s'\"), buf.buf);\n> +\n> +\t\thandle_push_default(remote->name, NULL);\n>  \t}\n>\n>  \treturn result;\n> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\n> index 082042b05a..dda81b7d07 100755\n> --- a/t/t5505-remote.sh\n> +++ b/t/t5505-remote.sh\n> @@ -734,6 +734,7 @@ test_expect_success 'reject adding remote with an invalid name' '\n>  # the last two ones check if the config is updated.\n>\n>  test_expect_success 'rename a remote' '\n> +\ttest_config_global remote.pushDefault origin &&\n>  \tgit clone one four &&\n>  \t(\n>  \t\tcd four &&\n> @@ -744,7 +745,42 @@ test_expect_success 'rename a remote' '\n>  \t\ttest \"$(git rev-parse upstream/master)\" = \"$(git rev-parse master)\" &&\n>  \t\ttest \"$(git config remote.upstream.fetch)\" = \"+refs/heads/*:refs/remotes/upstream/*\" &&\n>  \t\ttest \"$(git config branch.master.remote)\" = \"upstream\" &&\n> -\t\ttest \"$(git config branch.master.pushRemote)\" = \"upstream\"\n> +\t\ttest \"$(git config branch.master.pushRemote)\" = \"upstream\" &&\n> +\t\ttest \"$(git config --global remote.pushDefault)\" = \"origin\"\n> +\t)\n> +'\n> +\n> +test_expect_success 'rename a remote renames repo remote.pushDefault' '\n> +\tgit clone one four.1 &&\n\nI am not sure that a full clone is warranted here. Maybe use\n`--no-checkout` here and in the subsequent test cases? Omitting that\noption makes the tests only slower for no gain.\n\n> +\t(\n> +\t\tcd four.1 &&\n> +\t\tgit config remote.pushDefault origin &&\n> +\t\tgit remote rename origin upstream &&\n> +\t\ttest \"$(git config --local remote.pushDefault)\" = \"upstream\"\n> +\t)\n> +'\n> +\n> +test_expect_success 'rename a remote renames repo remote.pushDefault but ignores global' '\n> +\ttest_config_global remote.pushDefault other &&\n> +\tgit clone one four.2 &&\n> +\t(\n> +\t\tcd four.2 &&\n> +\t\tgit config remote.pushDefault origin &&\n> +\t\tgit remote rename origin upstream &&\n> +\t\ttest \"$(git config --global remote.pushDefault)\" = \"other\" &&\n> +\t\ttest \"$(git config --local remote.pushDefault)\" = \"upstream\"\n> +\t)\n> +'\n> +\n> +test_expect_success 'rename a remote renames repo remote.pushDefault but keeps global' '\n> +\ttest_config_global remote.pushDefault origin &&\n> +\tgit clone one four.3 &&\n> +\t(\n> +\t\tcd four.3 &&\n> +\t\tgit config remote.pushDefault origin &&\n> +\t\tgit remote rename origin upstream &&\n> +\t\ttest \"$(git config --global remote.pushDefault)\" = \"origin\" &&\n> +\t\ttest \"$(git config --local remote.pushDefault)\" = \"upstream\"\n\nA lot of tests added. Personally, I would have probably only extended the\nexisting `rename a remote` to verify that a repository-local\n`remote.pushDefault` _is_ renamed, and then added one test case that\nverifies that not only is a user-wide `remote.pushDefault` left alone but\nalso warned about.\n\n>  \t)\n>  '\n>\n> @@ -787,6 +823,7 @@ test_expect_success 'rename succeeds with existing remote.<target>.prune' '\n>  '\n>\n>  test_expect_success 'remove a remote' '\n> +\ttest_config_global remote.pushDefault origin &&\n>  \tgit clone one four.five &&\n>  \t(\n>  \t\tcd four.five &&\n> @@ -794,7 +831,42 @@ test_expect_success 'remove a remote' '\n>  \t\tgit remote remove origin &&\n>  \t\ttest -z \"$(git for-each-ref refs/remotes/origin)\" &&\n>  \t\ttest_must_fail git config branch.master.remote &&\n> -\t\ttest_must_fail git config branch.master.pushRemote\n> +\t\ttest_must_fail git config branch.master.pushRemote &&\n> +\t\ttest \"$(git config --global remote.pushDefault)\" = \"origin\"\n> +\t)\n> +'\n> +\n> +test_expect_success 'remove a remote removes repo remote.pushDefault' '\n> +\tgit clone one four.five.1 &&\n> +\t(\n> +\t\tcd four.five.1 &&\n> +\t\tgit config remote.pushDefault origin &&\n> +\t\tgit remote remove origin &&\n> +\t\ttest_must_fail git config --local remote.pushDefault\n\nNow that I see this sort of \"in action\", I have to wonder whether I would\nbe okay with a `remote.pushDefault` simply vanishing. I think that would\npuzzle me (\"Didn't I set a push default before? I must be getting old and\ndelusional.\"). In the least, I would want to see a warning here (\"As the\nremote 'xyz' was deleted, so was the `remote.pushDefault = xyz` setting\"\nor some such).\n\nIt strikes me as a fundamentally different thing whether we simply\nre-target or delete a `pushDefault` setting. The former needs no further\nwarning, but the latter does.\n\n> +\t)\n> +'\n> +\n> +test_expect_success 'remove a remote removes repo remote.pushDefault but ignores global' '\n> +\ttest_config_global remote.pushDefault other &&\n> +\tgit clone one four.five.2 &&\n> +\t(\n> +\t\tcd four.five.2 &&\n> +\t\tgit config remote.pushDefault origin &&\n> +\t\tgit remote remove origin &&\n> +\t\ttest \"$(git config --global remote.pushDefault)\" = \"other\" &&\n> +\t\ttest_must_fail git config --local remote.pushDefault\n> +\t)\n> +'\n> +\n> +test_expect_success 'remove a remote removes repo remote.pushDefault but keeps global' '\n> +\ttest_config_global remote.pushDefault origin &&\n> +\tgit clone one four.five.3 &&\n> +\t(\n> +\t\tcd four.five.3 &&\n> +\t\tgit config remote.pushDefault origin &&\n> +\t\tgit remote remove origin &&\n> +\t\ttest \"$(git config --global remote.pushDefault)\" = \"origin\" &&\n> +\t\ttest_must_fail git config --local remote.pushDefault\n\nSince the `mv` case already covers those code paths, I would be a lot more\nparsimonious in adding test cases for `rm`.\n\nThere are voices claiming that adding regression tests is always a good\nthing, but I would counter that it has to strike a balance between\ncoverage and runtime. We see a more and more contributions -- even from\nlong-time contributors -- where the test suite obviously has not been run\n(because the CI builds are failing, and there is no doubt that seasoned\ncontributors in particular would fix those failures before contributing\ntheir patches, _if_ they were aware of those test failures), proving just\nhow costly our test suite has become.\n\nOther than that, the patch looks fine to me. Thank you,\nDscho\n\n>  \t)\n>  '\n>\n> --\n> 2.25.0.30.g00ce2e43d4\n>\n>\n"},{"id":"391099","messageId":"xmqqv9om6rkk.fsf@gitster-ct.c.googlers.com","threadId":"52731","inReplyTo":"nycvar.QRO.7.76.6.2002021911260.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4] remote rename/remove: gently handle remote.pushDefault config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-04T20:11:23Z","receivedAt":"2020-02-04T20:11:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> +\tstruct push_default_info* info = cb;\n>> +\tif (strcmp(key, \"remote.pushdefault\") || strcmp(value, info->old_name))\n>> +\t\treturn 0;\n>\n> We will have to be careful to not segfault if a user has this in their\n> config:\n>\n> \t[remote]\n> \t\tpushDefault\n>\n> i.e. we have to insert `!value || ` before the call to `strcmp()`.\n\nTrue.  The primary reader in remote.c::handle_config() uses\ngit_config_string() that complains that the variable is not bool,\nbut we should reat end-user input as something suspicious and\nprotect us against it.\n\n> Concretely, I believe that the patched code will misbehave in this\n> scenario:\n>\n> \tgit config --global remote.pushDefault january\n> \tgit config remote.pushDefault february\n> \tgit remote rename january march\n\nGood to see careful analysis.  Thanks.\n\n>> +static void handle_push_default(const char* old_name, const char* new_name)\n>\n> That name probably wants to convey better that the push default is handled\n> in the `mv`/`rm` commands here, not in any other command. Maybe\n> `handle_modified_push_default_remote()`?\n\nAlso, the asterisk sticks to the variable not the type ;-)\n\n>> +{\n>> +\tstruct push_default_info push_default = {\n>> +\t\told_name, CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 };\n>\n> Personally, I would prefer the closing bracket to be on a new line,\n> followed by an empty line to separate the variable declaration from the\n> following statements.\n\nYes, yes.\n"}]}