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

From: Patrick Steinhardt <[email-masked]>
Date: Thu, 9 Jan 2025 11:20:29 +0100

On Tue, Jan 07, 2025 at 08:49:37AM -0800, Junio C Hamano wrote:
> Junio C Hamano <[email-masked]> writes:
> 
> > Patrick Steinhardt <[email-masked]> writes:
> >
> >> Makes sense indeed. We can easily add for something like below diff.
> >> I'll roll that into the next version, thanks!
> >
> > It is a good start, but is probably a bit too noisy.  Can we make
> > them appear ONLY when the definitions read from these older sources
> > are actually USED?
> >
> > Thanks.
> 
> Something along this line is what I had in mind.  Not even compile
> tested, and I am not claiming that all the uses of remote will go
> thourgh the code paths to use it with a transport, but you hopefully
> got the idea.

I had a look at your idea now, but I'm not convinced that it would be a
significantly improvement over my version. In almost all cases where we
execute `remote_get()` we'd also end up using the remote itself. So we'd
either have to add the explicit warning to all such locations, or we
risk not warning in cases where we really should.

The result of `remote_get()` (and `remotes_remote_get()`) is also being
cached, so if we retrieve the same remote multiple times from the same
remote state we'd only see the warning once.

So I'm inclined to leave this as-is, maybe adding an escape hatch to
silence the warnings via an environment variable to give users a bit of
time to migrate.

Patrick