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

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 9, 2025, 10:20 UTC
Message-ID: <Z3-i5mB4JVeONy4O@pks.im>
In-Reply-To: <xmqq4j2avaam.fsf@gitster.g>

On Tue, Jan 07, 2025 at 08:49:37AM -0800, Junio C Hamano wrote:

Show 17 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Patrick Steinhardt <ps@pks.im> 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