Re: [PATCH] remote: Ignore failure to remove missing branch.<name>.merge

[Ross Lagerwall <rosslagerwall@gmail.com>]

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