Re: [PATCH v3 5/5] remote: announce removal of "branches/" and "remotes/"

[Patrick Steinhardt <ps@pks.im>]

On Tue, Jan 21, 2025 at 01:25:56PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/builtin/remote.c b/builtin/remote.c
> > index 1ad3e70a6b..e565b2b3fe 100644
> > --- a/builtin/remote.c
> > +++ b/builtin/remote.c
> > @@ -640,10 +640,12 @@ static int migrate_file(struct remote *remote)
> >  	strbuf_addf(&buf, "remote.%s.fetch", remote->name);
> >  	for (i = 0; i < remote->fetch.nr; i++)
> >  		git_config_set_multivar(buf.buf, remote->fetch.items[i].raw, "^$", 0);
> > +#ifndef WITH_BREAKING_CHANGES
> >  	if (remote->origin == REMOTE_REMOTES)
> >  		unlink_or_warn(git_path("remotes/%s", remote->name));
> >  	else if (remote->origin == REMOTE_BRANCHES)
> >  		unlink_or_warn(git_path("branches/%s", remote->name));
> > +#endif /* WITH_BREAKING_CHANGES */
> >  	strbuf_release(&buf);
> 
> Interesting.  I wonder if our new warning should talk about whatever
> end-user facing interface that triggers this code path.  It would
> help them wean themselves away from the old interface, no?

Not quite sure that I understand what you're saying. Is it that we
should tell whether we were reading from "branches/" or "remotes/"? If
so we already do that.

> > diff --git a/remote.c b/remote.c
> > index 10104d11e3..5feb0ae886 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -293,6 +293,7 @@ static void add_instead_of(struct rewrite *rewrite, const char *instead_of)
> >  	rewrite->instead_of_nr++;
> >  }
> >  
> > +#ifndef WITH_BREAKING_CHANGES
> >  static const char *skip_spaces(const char *s)
> >  {
> >  	while (isspace(*s))
> > @@ -308,6 +309,13 @@ static void read_remotes_file(struct remote_state *remote_state,
> >  
> >  	if (!f)
> >  		return;
> > +
> > +	warning(_("Reading remote from \"remotes/%s\", which is nominated\n"
> > +		  "for removal. If you still use the \"remotes/\" directory\n"
> > +		  "it is recommended to migrate to config-based remotes. If\n"
> 
> Do we have a way to concisely say "how" to do this?  If I am reading
> the caller of migrate_file() in builtin/remote.c, it would be
> 
>     $ git remote mv foo foo
> 
> for any foo in .git/remotes/* or .git/branches/* hierarchy?
> 
> Of course they may be an ancient leftover file that the user even no
> longer is aware of having, in which case
> 
>     $ rm .git/remotes/foo
> 
> might be an OK answer, but even then
> 
>     $ git remote rm foo
> 
> would probably be more appropriate.

Very good idea indeed. We also have tests that exercise this behaviour
in t5505 (with the slight exception that it is `git remote rename`, not
`git remote mv`).

Patrick