{"thread":{"id":"45173","subject":"[PATCH] remote: Ignore failure to remove missing branch.<name>.merge","startedAt":"2017-02-18T00:23:47Z","lastAt":"2017-02-21T22:03:25Z","messageCount":4,"participants":["Ross Lagerwall","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"312017","messageId":"20170218002341.23099-1-rosslagerwall@gmail.com","threadId":"45173","inReplyTo":null,"subject":"[PATCH] remote: Ignore failure to remove missing branch.<name>.merge","fromName":"Ross Lagerwall","fromEmail":"rosslagerwall@gmail.com","sentAt":"2017-02-18T00:23:41Z","receivedAt":"2017-02-18T00:23:47Z","isPatch":true,"sender":{"key":"rosslagerwall@gmail.com","avatar":"https://avatars.githubusercontent.com/u/878055?v=4"},"body":"If a branch is configured with a default remote but no\nbranch.<name>.merge and then the remote is removed, git fails to remove\nthe remote with:\n\"fatal: could not unset 'branch.<name>.merge'\"\n\nInstead, ignore this since it is not an error and shouldn't prevent the\nremote from being removed.\n\nSigned-off-by: Ross Lagerwall <rosslagerwall@gmail.com>\n---\n builtin/remote.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex e52cf3925..5dd22c2eb 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -769,7 +769,9 @@ static int rm(int argc, const char **argv)\n \t\t\t\tstrbuf_reset(&buf);\n \t\t\t\tstrbuf_addf(&buf, \"branch.%s.%s\",\n \t\t\t\t\t\titem->string, *k);\n-\t\t\t\tgit_config_set(buf.buf, NULL);\n+\t\t\t\tresult = git_config_set_gently(buf.buf, NULL);\n+\t\t\t\tif (result && result != CONFIG_NOTHING_SET)\n+\t\t\t\t\tdie(_(\"COULd not unset '%s'\"), buf.buf);\n \t\t\t}\n \t\t}\n \t}\n-- \n2.11.0\n\n"},{"id":"312216","messageId":"xmqqtw7nfift.fsf@gitster.mtv.corp.google.com","threadId":"45173","inReplyTo":"20170218002341.23099-1-rosslagerwall@gmail.com","subject":"Re: [PATCH] remote: Ignore failure to remove missing branch.<name>.merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-21T19:32:54Z","receivedAt":"2017-02-21T19:33:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ross Lagerwall <rosslagerwall@gmail.com> writes:\n\n> If a branch is configured with a default remote but no\n> branch.<name>.merge and then the remote is removed, git fails to remove\n> the remote with:\n> \"fatal: could not unset 'branch.<name>.merge'\"\n>\n> Instead, ignore this since it is not an error and shouldn't prevent the\n> remote from being removed.\n>\n> Signed-off-by: Ross Lagerwall <rosslagerwall@gmail.com>\n> ---\n\nI was waiting for others to comment on this patch but nobody seems\nto be interested.  Which is a bit sad, because except for minor\nnits, this patch is very well done.\n\nThe explanation of the motivation and solution in the proposed log\nmessage is excellent.  It would have been perfect if you described\nHOW you get into a state where branch.<name>.remote is pointing at\nthe remote being removed, without having branch.<name>.merge in the\nfirst place, but even if such a state is invalid or unplausible,\nremoving the remote should be a usable way to recover from such a\nsituation.\n\nAnd the proposed solution in the diff seems to correctly implement\nwhat the description of the solution in the log message (modulo a\nminor nit).\n\n>  builtin/remote.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index e52cf3925..5dd22c2eb 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -769,7 +769,9 @@ static int rm(int argc, const char **argv)\n>  \t\t\t\tstrbuf_reset(&buf);\n>  \t\t\t\tstrbuf_addf(&buf, \"branch.%s.%s\",\n>  \t\t\t\t\t\titem->string, *k);\n> -\t\t\t\tgit_config_set(buf.buf, NULL);\n> +\t\t\t\tresult = git_config_set_gently(buf.buf, NULL);\n> +\t\t\t\tif (result && result != CONFIG_NOTHING_SET)\n> +\t\t\t\t\tdie(_(\"COULd not unset '%s'\"), buf.buf);\n\nWith s/COUL/coul/, the result would be more in line with our\nexisting practice.\n\n>  \t\t\t}\n>  \t\t}\n>  \t}\n\nWe do want an additional test so that this fix will not be broken\nagain in the future by mistake, perhaps in t5505.\n\nAs it is unclear to me how you got into a state where branch.*.remote\nexists without branch.*.merge, the attached patch to the test manually\nremoves it, which probably falls into a \"deliberate sabotage\" category.\nIf there are a valid sequence of operations that leads to such a state\nwithout being a deliberate sabotage, we should use it instead in the\nreal test.\n\nThanks.\n\n builtin/remote.c  |  4 +++-\n t/t5505-remote.sh | 19 +++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex e52cf3925b..01055b7272 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -769,7 +769,9 @@ static int rm(int argc, const char **argv)\n \t\t\t\tstrbuf_reset(&buf);\n \t\t\t\tstrbuf_addf(&buf, \"branch.%s.%s\",\n \t\t\t\t\t\titem->string, *k);\n-\t\t\t\tgit_config_set(buf.buf, NULL);\n+\t\t\t\tresult = git_config_set_gently(buf.buf, NULL);\n+\t\t\t\tif (result && result != CONFIG_NOTHING_SET)\n+\t\t\t\t\tdie(_(\"could not unset '%s'\"), buf.buf);\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 8198d8eb05..af85a624fc 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -153,6 +153,25 @@ test_expect_success 'remove errors out early when deleting non-existent branch'\n \t)\n '\n \n+test_expect_success 'remove remote with a branch without configured merge' '\n+\ttest_when_finished \"(\n+\t\tgit -C test checkout master;\n+\t\tgit -C test branch -D two;\n+\t\tgit -C test config --remove-section remote.two;\n+\t\tgit -C test config --remove-section branch.second;\n+\t\ttrue\n+\t)\" &&\n+\t(\n+\t\tcd test &&\n+\t\tgit remote add two ../two &&\n+\t\tgit fetch two &&\n+\t\tgit checkout -t -b second two/master &&\n+\t\tgit checkout master &&\n+\t\tgit config --unset branch.second.merge &&\n+\t\tgit remote rm two\n+\t)\n+'\n+\n test_expect_success 'rename errors out early when deleting non-existent branch' '\n \t(\n \t\tcd test &&\n\n"},{"id":"312221","messageId":"20170221203836.GA11736@hobo.lan","threadId":"45173","inReplyTo":"xmqqtw7nfift.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] remote: Ignore failure to remove missing branch.<name>.merge","fromName":"Ross Lagerwall","fromEmail":"rosslagerwall@gmail.com","sentAt":"2017-02-21T20:38:36Z","receivedAt":"2017-02-21T20:38:50Z","isPatch":true,"sender":{"key":"rosslagerwall@gmail.com","avatar":"https://avatars.githubusercontent.com/u/878055?v=4"},"body":"On Tue, Feb 21, 2017 at 11:32:54AM -0800, Junio C Hamano wrote:\n> Ross Lagerwall <rosslagerwall@gmail.com> writes:\n> \n> > If a branch is configured with a default remote but no\n> > branch.<name>.merge and then the remote is removed, git fails to remove\n> > the remote with:\n> > \"fatal: could not unset 'branch.<name>.merge'\"\n> >\n> > Instead, ignore this since it is not an error and shouldn't prevent the\n> > remote from being removed.\n> >\n> > Signed-off-by: Ross Lagerwall <rosslagerwall@gmail.com>\n> > ---\n> \n> I was waiting for others to comment on this patch but nobody seems\n> to be interested.  Which is a bit sad, because except for minor\n> nits, this patch is very well done.\n> \n> The explanation of the motivation and solution in the proposed log\n> message is excellent.  It would have been perfect if you described\n> HOW you get into a state where branch.<name>.remote is pointing at\n> the remote being removed, without having branch.<name>.merge in the\n> first place, but even if such a state is invalid or unplausible,\n> removing the remote should be a usable way to recover from such a\n> situation.\n\nI got into this situation by setting branch.<name>.remote directly.  I\nwas using push.default=current, and wanted a bare \"git push\" on the\nbranch to push to a different remote from origin (which it defaults to).\nConfiguring branch.<name>.remote made git do the right thing.\n\nPerhaps what I did was invalid, I'm not sure, but it achieved what I\nwanted.\n\n> \n> And the proposed solution in the diff seems to correctly implement\n> what the description of the solution in the log message (modulo a\n> minor nit).\n> \n> >  builtin/remote.c | 4 +++-\n> >  1 file changed, 3 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/remote.c b/builtin/remote.c\n> > index e52cf3925..5dd22c2eb 100644\n> > --- a/builtin/remote.c\n> > +++ b/builtin/remote.c\n> > @@ -769,7 +769,9 @@ static int rm(int argc, const char **argv)\n> >  \t\t\t\tstrbuf_reset(&buf);\n> >  \t\t\t\tstrbuf_addf(&buf, \"branch.%s.%s\",\n> >  \t\t\t\t\t\titem->string, *k);\n> > -\t\t\t\tgit_config_set(buf.buf, NULL);\n> > +\t\t\t\tresult = git_config_set_gently(buf.buf, NULL);\n> > +\t\t\t\tif (result && result != CONFIG_NOTHING_SET)\n> > +\t\t\t\t\tdie(_(\"COULd not unset '%s'\"), buf.buf);\n> \n> With s/COUL/coul/, the result would be more in line with our\n> existing practice.\n\nOops. That's what I get for coding when I should have been sleeping.\n\n> \n> >  \t\t\t}\n> >  \t\t}\n> >  \t}\n> \n> We do want an additional test so that this fix will not be broken\n> again in the future by mistake, perhaps in t5505.\n> \n> As it is unclear to me how you got into a state where branch.*.remote\n> exists without branch.*.merge, the attached patch to the test manually\n> removes it, which probably falls into a \"deliberate sabotage\" category.\n> If there are a valid sequence of operations that leads to such a state\n> without being a deliberate sabotage, we should use it instead in the\n> real test.\n> \n\nSee my explanation above. I wouldn't call it \"deliberate sabotage\", but\nrather using config knobs in unexpected ways.\n\nThe test case looks reasonable. Do you want me to resend a patch with\nthe test case included (and nit fixed), or will you fix it up?\n\nThanks,\n-- \nRoss Lagerwall\n"},{"id":"312229","messageId":"xmqq37f7fbh6.fsf@gitster.mtv.corp.google.com","threadId":"45173","inReplyTo":"20170221203836.GA11736@hobo.lan","subject":"Re: [PATCH] remote: Ignore failure to remove missing branch.<name>.merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-21T22:03:17Z","receivedAt":"2017-02-21T22:03:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ross Lagerwall <rosslagerwall@gmail.com> writes:\n\n> On Tue, Feb 21, 2017 at 11:32:54AM -0800, Junio C Hamano wrote:\n>\n>> I was waiting for others to comment on this patch but nobody seems\n>> to be interested.  Which is a bit sad, because except for minor\n>> nits, this patch is very well done.\n>> \n>> The explanation of the motivation and solution in the proposed log\n>> message is excellent.  It would have been perfect if you described\n>> HOW you get into a state where branch.<name>.remote is pointing at\n>> the remote being removed, without having branch.<name>.merge in the\n>> first place, but even if such a state is invalid or unplausible,\n>> removing the remote should be a usable way to recover from such a\n>> situation.\n>\n> I got into this situation by setting branch.<name>.remote directly.  I\n> was using push.default=current, and wanted a bare \"git push\" on the\n> branch to push to a different remote from origin (which it defaults to).\n> Configuring branch.<name>.remote made git do the right thing.\n\nAh, OK.  As you may have seen from the test I sent, I thought the\nuser started with\n\n\tgit checkout -b <new> -t <remote>/<branch>\n\nin which case both are always set, and removed only one of them,\nand that is what I called \"deliberate sabotage\".\n\nWhat you did does sound like a very valid use case.  Let's update\nthe test to use that pattern and document the intended use case to\nhelp with this fix in the updated log message.\n\nHere is what I tentatively queued.\n\nThanks.\n\n-- >8 --\nFrom: Ross Lagerwall <rosslagerwall@gmail.com>\nDate: Sat, 18 Feb 2017 00:23:41 +0000\nSubject: [PATCH] remote: ignore failure to remove missing branch.<name>.merge\n\nIt is not all too unusual for a branch to use \"branch.<name>.remote\"\nwithout \"branch.<name>.merge\".  You may be using the 'push.default'\nconfiguration set to 'current', for example, and do\n\n    $ git checkout -b side colleague/side\n    $ git config branch.side.remote colleague\n\nHowever, \"git remote rm\" to remove the remote used in such a manner\nfails with\n\n    \"fatal: could not unset 'branch.<name>.merge'\"\n\nbecause it assumes that a branch that has .remote defined must also\nhave .merge defined.  Detect the \"cannot unset because it is not set\nto begin with\" case and ignore it.\n\nSigned-off-by: Ross Lagerwall <rosslagerwall@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/remote.c  |  4 +++-\n t/t5505-remote.sh | 19 +++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex e52cf3925b..01055b7272 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -769,7 +769,9 @@ static int rm(int argc, const char **argv)\n \t\t\t\tstrbuf_reset(&buf);\n \t\t\t\tstrbuf_addf(&buf, \"branch.%s.%s\",\n \t\t\t\t\t\titem->string, *k);\n-\t\t\t\tgit_config_set(buf.buf, NULL);\n+\t\t\t\tresult = git_config_set_gently(buf.buf, NULL);\n+\t\t\t\tif (result && result != CONFIG_NOTHING_SET)\n+\t\t\t\t\tdie(_(\"could not unset '%s'\"), buf.buf);\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 8198d8eb05..f558ad0b39 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -153,6 +153,25 @@ test_expect_success 'remove errors out early when deleting non-existent branch'\n \t)\n '\n \n+test_expect_success 'remove remote with a branch without configured merge' '\n+\ttest_when_finished \"(\n+\t\tgit -C test checkout master;\n+\t\tgit -C test branch -D two;\n+\t\tgit -C test config --remove-section remote.two;\n+\t\tgit -C test config --remove-section branch.second;\n+\t\ttrue\n+\t)\" &&\n+\t(\n+\t\tcd test &&\n+\t\tgit remote add two ../two &&\n+\t\tgit fetch two &&\n+\t\tgit checkout -b second two/master^0 &&\n+\t\tgit config branch.second.remote two &&\n+\t\tgit checkout master &&\n+\t\tgit remote rm two\n+\t)\n+'\n+\n test_expect_success 'rename errors out early when deleting non-existent branch' '\n \t(\n \t\tcd test &&\n-- \n2.12.0-rc2-231-g83a1c8597c\n"}]}