Re: [PATCH v2 3/4] Add 'promisor-remote' capability to protocol v2

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 1, 2024, 10:14 UTC
Message-ID: <ZvvLbXskyA4V0gM3@pks.im>
In-Reply-To: <CAP8UFD3+YVE+pgGtSP6gDDN9=-0acuJCnDamWjeCX-g5aqTp3g@mail.gmail.com>

On Mon, Sep 30, 2024 at 03:28:20PM +0200, Christian Couder wrote:

Show 17 quoted lines

> On Mon, Sep 30, 2024 at 9:57 AM Patrick Steinhardt <ps@pks.im> wrote:
> > So I assume the intent here is to let the client add that promisor
> > remote with that exact, server-provided name? That makes me wonder about
> > two different scenarios:
> >
> >   - We must keep the remote from announcing "origin".
> 
> I agree that it might not be a good idea to have something else than
> the main remote named origin. I am not sure it's necessary to
> explicitly disallow it though.
> 
> >   - What if we eventually decide to allow users to provide their own
> >     names for remotes during git-clone(1)?
> 
> I think it could be confusing, so I would say that we should wait
> until a concrete case where it could be useful appear before allowing
> this.

I think we've been talking past another on this item. What I'm worried about is a potential future where the default remote isn't called "origin", but something else. I for example quite frequently rename the remote right after cloning because I add a handful of remotes, and "origin" would be too confusing. So there is a usecase that may at one point in the future cause us to make this configurable at clone-time.

Which brings me to the issue with the current design: if the remote dictates the names of additional remotes we basically cannot do the above change anymore because we have to assume that no matter which remote name is chosen, it could already be used by a promisor remote. Our hands are bound by potential implementations of this feature by a third party, which I think is not a good idea in general.

Now I'm not against advertising a name and storing it in our config when we create the additional remote, for example by storing it as a separate key "remote.<generated>.promisor-name". But the name of the remote itself should not be controlled by the server, but should instead be generated by the client.

Show 11 quoted lines

> > Overall, I don't think that it's a good idea to let the remote dictate
> > which name a client's remotes have.
> 
> Maybe a new mode like "KnownURL" but where only the URL and not the
> name should match could be interesting in some cases then? If that's
> the case it's very simple to add it. I just prefer not to do it for
> now as I am not yet convinced there is a very relevant use case. I
> think that if a client doesn't want to trust and cooperate with the
> server at all, it might just be better in most cases for it to just
> leave the server alone and not access it at all, independently of
> using promisor remote or not.

It's not only about trust, as explained above. It's more about not letting server operators dictate how Git can evolve in that context and not taking away the ability of a user to configure their repository how they want to.

Show 33 quoted lines

> > I wonder how that is supposed to interact
> > with clients that do not know about the "promisor-remote" capability at
> > all though.
> 
> When that happens, S can fetch from X the objects it doesn't have, and
> then proceed as usual to respond to the client. This has the drawback
> of duplicating these objects on S, but perhaps there could be some
> kind of garbage collection process that would regularly remove those
> duplicated objects from S.
> 
> Another possibility that could be added in the future would be for S
> to warn the client that it should be upgraded to have the
> "promisor-remote" capability. Or S could just refuse to serve the
> client in that case. I don't think we should implement these
> possibilities right now, but it could be useful to do it in the
> future.
> 
> > From my point of view the server should be able tot handle that just
> > fine and provide a full packfile to the client. That would of course
> > require the server to fetch missing objects from its own promisor
> > remotes.
> 
> It's what already happens.
> 
> > Do we want to state explicitly that this is a MUST for servers
> > so that we don't end up in a future where clients wouldn't be able to
> > fetch from some forges anymore?
> 
> I don't think we should enforce anything like this. For example in
> corporate setups, it might be easy to install the latest version of
> Git and it might be a good thing to make sure the server doesn't get
> overloaded with large files when they are supposed to only be stored
> on a promisor remote.

Partitioning the Git userbase depending on the Git version they can use doesn't feel sensible to me. We have been able to get by without breaking backwards compatibility on the transport layer until now. So it would be too bad if this new feature would break that.

Also, the argument with a corporate setup cuts both ways, I think. If the administrators tightly control the Git version anyway they can just upgrade it for all clients, and consequently all of the clients would know how to handle the new capability and thus the server wouldn't be overloaded.

Patrick