On Tue, Feb 21, 2017 at 11:32:54AM -0800, Junio C Hamano wrote:
> Ross Lagerwall <rosslagerw...@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 <rosslagerw...@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
