* [PATCH] remote: Ignore failure to remove missing branch.<name>.merge @ 2017-02-18 0:23 Ross Lagerwall 2017-02-21 19:32 ` Junio C Hamano 0 siblings, 1 reply; 4+ messages in thread From: Ross Lagerwall @ 2017-02-18 0:23 UTC (permalink / raw) To: git; +Cc: Ross Lagerwall If a branch is configured with a default remote but no branch.<name>.merge and then the remote is removed, git fails to remove the remote with: "fatal: could not unset 'branch.<name>.merge'" Instead, ignore this since it is not an error and shouldn't prevent the remote from being removed. Signed-off-by: Ross Lagerwall <rosslagerwall@gmail.com> --- builtin/remote.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/remote.c b/builtin/remote.c index e52cf3925..5dd22c2eb 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -769,7 +769,9 @@ static int rm(int argc, const char **argv) strbuf_reset(&buf); strbuf_addf(&buf, "branch.%s.%s", item->string, *k); - git_config_set(buf.buf, NULL); + result = git_config_set_gently(buf.buf, NULL); + if (result && result != CONFIG_NOTHING_SET) + die(_("COULd not unset '%s'"), buf.buf); } } } -- 2.11.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] remote: Ignore failure to remove missing branch.<name>.merge 2017-02-18 0:23 [PATCH] remote: Ignore failure to remove missing branch.<name>.merge Ross Lagerwall @ 2017-02-21 19:32 ` Junio C Hamano 2017-02-21 20:38 ` Ross Lagerwall 0 siblings, 1 reply; 4+ messages in thread From: Junio C Hamano @ 2017-02-21 19:32 UTC (permalink / raw) To: Ross Lagerwall; +Cc: git Ross Lagerwall <rosslagerwall@gmail.com> writes: > If a branch is configured with a default remote but no > branch.<name>.merge and then the remote is removed, git fails to remove > the remote with: > "fatal: could not unset 'branch.<name>.merge'" > > Instead, ignore this since it is not an error and shouldn't prevent the > remote from being removed. > > Signed-off-by: Ross Lagerwall <rosslagerwall@gmail.com> > --- I was waiting for others to comment on this patch but nobody seems to be interested. Which is a bit sad, because except for minor nits, this patch is very well done. The explanation of the motivation and solution in the proposed log message is excellent. It would have been perfect if you described HOW you get into a state where branch.<name>.remote is pointing at the remote being removed, without having branch.<name>.merge in the first place, but even if such a state is invalid or unplausible, removing the remote should be a usable way to recover from such a situation. And the proposed solution in the diff seems to correctly implement what the description of the solution in the log message (modulo a minor nit). > builtin/remote.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/builtin/remote.c b/builtin/remote.c > index e52cf3925..5dd22c2eb 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -769,7 +769,9 @@ static int rm(int argc, const char **argv) > strbuf_reset(&buf); > strbuf_addf(&buf, "branch.%s.%s", > item->string, *k); > - git_config_set(buf.buf, NULL); > + result = git_config_set_gently(buf.buf, NULL); > + if (result && result != CONFIG_NOTHING_SET) > + die(_("COULd not unset '%s'"), buf.buf); With s/COUL/coul/, the result would be more in line with our existing practice. > } > } > } We do want an additional test so that this fix will not be broken again in the future by mistake, perhaps in t5505. As it is unclear to me how you got into a state where branch.*.remote exists without branch.*.merge, the attached patch to the test manually removes it, which probably falls into a "deliberate sabotage" category. If there are a valid sequence of operations that leads to such a state without being a deliberate sabotage, we should use it instead in the real test. Thanks. builtin/remote.c | 4 +++- t/t5505-remote.sh | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/builtin/remote.c b/builtin/remote.c index e52cf3925b..01055b7272 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -769,7 +769,9 @@ static int rm(int argc, const char **argv) strbuf_reset(&buf); strbuf_addf(&buf, "branch.%s.%s", item->string, *k); - git_config_set(buf.buf, NULL); + result = git_config_set_gently(buf.buf, NULL); + if (result && result != CONFIG_NOTHING_SET) + die(_("could not unset '%s'"), buf.buf); } } } diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 8198d8eb05..af85a624fc 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -153,6 +153,25 @@ test_expect_success 'remove errors out early when deleting non-existent branch' ) ' +test_expect_success 'remove remote with a branch without configured merge' ' + test_when_finished "( + git -C test checkout master; + git -C test branch -D two; + git -C test config --remove-section remote.two; + git -C test config --remove-section branch.second; + true + )" && + ( + cd test && + git remote add two ../two && + git fetch two && + git checkout -t -b second two/master && + git checkout master && + git config --unset branch.second.merge && + git remote rm two + ) +' + test_expect_success 'rename errors out early when deleting non-existent branch' ' ( cd test && ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] remote: Ignore failure to remove missing branch.<name>.merge 2017-02-21 19:32 ` Junio C Hamano @ 2017-02-21 20:38 ` Ross Lagerwall 2017-02-21 22:03 ` Junio C Hamano 0 siblings, 1 reply; 4+ messages in thread From: Ross Lagerwall @ 2017-02-21 20:38 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Tue, Feb 21, 2017 at 11:32:54AM -0800, Junio C Hamano wrote: > Ross Lagerwall <rosslagerwall@gmail.com> writes: > > > If a branch is configured with a default remote but no > > branch.<name>.merge and then the remote is removed, git fails to remove > > the remote with: > > "fatal: could not unset 'branch.<name>.merge'" > > > > Instead, ignore this since it is not an error and shouldn't prevent the > > remote from being removed. > > > > Signed-off-by: Ross Lagerwall <rosslagerwall@gmail.com> > > --- > > I was waiting for others to comment on this patch but nobody seems > to be interested. Which is a bit sad, because except for minor > nits, this patch is very well done. > > The explanation of the motivation and solution in the proposed log > message is excellent. It would have been perfect if you described > HOW you get into a state where branch.<name>.remote is pointing at > the remote being removed, without having branch.<name>.merge in the > first place, but even if such a state is invalid or unplausible, > removing the remote should be a usable way to recover from such a > situation. I got into this situation by setting branch.<name>.remote directly. I was using push.default=current, and wanted a bare "git push" on the branch to push to a different remote from origin (which it defaults to). Configuring branch.<name>.remote made git do the right thing. Perhaps what I did was invalid, I'm not sure, but it achieved what I wanted. > > And the proposed solution in the diff seems to correctly implement > what the description of the solution in the log message (modulo a > minor nit). > > > builtin/remote.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/builtin/remote.c b/builtin/remote.c > > index e52cf3925..5dd22c2eb 100644 > > --- a/builtin/remote.c > > +++ b/builtin/remote.c > > @@ -769,7 +769,9 @@ static int rm(int argc, const char **argv) > > strbuf_reset(&buf); > > strbuf_addf(&buf, "branch.%s.%s", > > item->string, *k); > > - git_config_set(buf.buf, NULL); > > + result = git_config_set_gently(buf.buf, NULL); > > + if (result && result != CONFIG_NOTHING_SET) > > + die(_("COULd not unset '%s'"), buf.buf); > > With s/COUL/coul/, the result would be more in line with our > existing practice. Oops. That's what I get for coding when I should have been sleeping. > > > } > > } > > } > > We do want an additional test so that this fix will not be broken > again in the future by mistake, perhaps in t5505. > > As it is unclear to me how you got into a state where branch.*.remote > exists without branch.*.merge, the attached patch to the test manually > removes it, which probably falls into a "deliberate sabotage" category. > If there are a valid sequence of operations that leads to such a state > without being a deliberate sabotage, we should use it instead in the > real test. > See my explanation above. I wouldn't call it "deliberate sabotage", but rather using config knobs in unexpected ways. The test case looks reasonable. Do you want me to resend a patch with the test case included (and nit fixed), or will you fix it up? Thanks, -- Ross Lagerwall ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] remote: Ignore failure to remove missing branch.<name>.merge 2017-02-21 20:38 ` Ross Lagerwall @ 2017-02-21 22:03 ` Junio C Hamano 0 siblings, 0 replies; 4+ messages in thread From: Junio C Hamano @ 2017-02-21 22:03 UTC (permalink / raw) To: Ross Lagerwall; +Cc: git Ross Lagerwall <rosslagerwall@gmail.com> writes: > On Tue, Feb 21, 2017 at 11:32:54AM -0800, Junio C Hamano wrote: > >> I was waiting for others to comment on this patch but nobody seems >> to be interested. Which is a bit sad, because except for minor >> nits, this patch is very well done. >> >> The explanation of the motivation and solution in the proposed log >> message is excellent. It would have been perfect if you described >> HOW you get into a state where branch.<name>.remote is pointing at >> the remote being removed, without having branch.<name>.merge in the >> first place, but even if such a state is invalid or unplausible, >> removing the remote should be a usable way to recover from such a >> situation. > > I got into this situation by setting branch.<name>.remote directly. I > was using push.default=current, and wanted a bare "git push" on the > branch to push to a different remote from origin (which it defaults to). > Configuring branch.<name>.remote made git do the right thing. Ah, OK. As you may have seen from the test I sent, I thought the user started with git checkout -b <new> -t <remote>/<branch> in which case both are always set, and removed only one of them, and that is what I called "deliberate sabotage". What you did does sound like a very valid use case. Let's update the test to use that pattern and document the intended use case to help with this fix in the updated log message. Here is what I tentatively queued. Thanks. -- >8 -- From: Ross Lagerwall <rosslagerwall@gmail.com> Date: Sat, 18 Feb 2017 00:23:41 +0000 Subject: [PATCH] remote: ignore failure to remove missing branch.<name>.merge It is not all too unusual for a branch to use "branch.<name>.remote" without "branch.<name>.merge". You may be using the 'push.default' configuration set to 'current', for example, and do $ git checkout -b side colleague/side $ git config branch.side.remote colleague However, "git remote rm" to remove the remote used in such a manner fails with "fatal: could not unset 'branch.<name>.merge'" because it assumes that a branch that has .remote defined must also have .merge defined. Detect the "cannot unset because it is not set to begin with" case and ignore it. Signed-off-by: Ross Lagerwall <rosslagerwall@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/remote.c | 4 +++- t/t5505-remote.sh | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/builtin/remote.c b/builtin/remote.c index e52cf3925b..01055b7272 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -769,7 +769,9 @@ static int rm(int argc, const char **argv) strbuf_reset(&buf); strbuf_addf(&buf, "branch.%s.%s", item->string, *k); - git_config_set(buf.buf, NULL); + result = git_config_set_gently(buf.buf, NULL); + if (result && result != CONFIG_NOTHING_SET) + die(_("could not unset '%s'"), buf.buf); } } } diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 8198d8eb05..f558ad0b39 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -153,6 +153,25 @@ test_expect_success 'remove errors out early when deleting non-existent branch' ) ' +test_expect_success 'remove remote with a branch without configured merge' ' + test_when_finished "( + git -C test checkout master; + git -C test branch -D two; + git -C test config --remove-section remote.two; + git -C test config --remove-section branch.second; + true + )" && + ( + cd test && + git remote add two ../two && + git fetch two && + git checkout -b second two/master^0 && + git config branch.second.remote two && + git checkout master && + git remote rm two + ) +' + test_expect_success 'rename errors out early when deleting non-existent branch' ' ( cd test && -- 2.12.0-rc2-231-g83a1c8597c ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-02-21 22:03 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-02-18 0:23 [PATCH] remote: Ignore failure to remove missing branch.<name>.merge Ross Lagerwall 2017-02-21 19:32 ` Junio C Hamano 2017-02-21 20:38 ` Ross Lagerwall 2017-02-21 22:03 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).