fetch: pass negotiation data around as a struct #5105
Conversation
We will need to pass some options to the negotiation step (like depth level for shallow clones)
| typedef struct { | ||
| const git_remote_head * const *refs; | ||
| size_t count; | ||
| } git_fetch_negotiation; |
There was a problem hiding this comment.
This is somewhat weird because, for the first time, we're the ones passing "sys-level" structs to "userspace", expecting that they might (or might not) be able to understand some of those options. I've refrained from adding a version field, because it's "feature-based", hence a monotonic number might not be sufficient.
|
Hmm. This would break the public API. I'm wondering if there's a way that we can do this without breaking the API. In #4747, instead of changing the Existing consumers that do not supply the |
|
I want a future-proof solution: adding a new callback might work once or twice. In that case, I'm the first with #4747, but the 2nd is whoever implements protocol-v2. My concern is that I have a hunch I need some kind of "feature" system to be able to say that a given custom transport cannot do To be clear, I'd prefer to not break the API, I could implement shallow given more brain-power, and have a PR. But I'd still be worried that this interface is a trap to us: I fail to see how we would not end up in a situation with a PR postponed to 2.0 because we cannot touch 1.0 APIs, but I'd happily be proven wrong. Hence, I'm ok breaking it for/on 1.0, as long as it frees us from future extensibility concerns: I don't plan on adding anything else on top of it, I see it as an interface problem that would be preferable to have fixed. |
|
I'm not sure that a struct is really the right way forward here. But either way: we made a guarantee that we'd stop breaking API, and we need to do that. So we need a way to support new stuff with the old stuff. We've been really successful in making a lot of API changes while staying backward compatible. I don't think this is any different. So the new API doesn't need to be I'm still not excited about dumping versioned structs at end-users and making them deal with that. We don't even have a compelling story for how we're going to deal with it, and we opted ourselves into it. Pushing this unknown onto users, too, seems unfair. |
|
Re-reading what I wrote: to be more constructive (or to try), I'm saying something like
|
|
I feel like we might want to introduce a new callback or transport field For old transports that use the pre-existing |
|
My thinking was going along :
I kinda like how it can be made pretty generic and be generally useful. I'm not sure how it would react to real usecases though, but let's discuss something. In our current case, there's many of those "featurelists" : what the transport is capable of, what we're asking it to do, what the remote is capable of. Ideally I'd bump the transport-level version to add a "featurelist" member, make all the necessary callbacks changes (since you'd have to touch the code anyway, so either the EDIT After re-reading @pks-t's comment above, I favored a stringly-based approach over a bitfield for namespacing reasons. That's debatable as well, but I feel that it would make backward-compatibility more complex to handle (eg. once you define that |
|
Mh. I'm not particular sure I like having strings in there, though. In effect, to the caller it's kind of the same whether he has to know a certain string or a certain define for a bit, but the latter compresses a lot better and is a lot easier to check for ( And I don't quite understand the deprecation/removal thing. We have to track that regardless of whether it is implemented as a string list or a bitfield, wouldn't we? |
It's easier to handle deprecation of a mostly-decentralized string list than the indices taken in a bitfield (case in point, BitTorrent's BEP10). I've just noticed that the (IMHO needed) decentralization is actually a consequence of trying to having generic featurelists for things like git's own repository extension system. This specific example of "fetch/transport" features could do with bitfields, if we're careful. |
|
Huh. I don't know, to be honest, I just feel like it puts additional burden on the user of such APIs. Let's get a tie-breaker in :) @ethomson, you've got any opinion on this? |
I admit that I've been a little checked out for the last week or two, but I'm not sure what I'm weighing in on. I'm having a little trouble following the conversation ☝️ since it feels a little theoretical. I'd love to see proposed code - even pseudocode - for what is being talked about, because I keep looking at the code in this PR and I think that I'm missing the bigger picture. |
|
Here's the "last" version of that struct, after some prodding onto the semantics of shallow clones, for a quick summary of what the "complete" feature would need. This PR is about making that struct explicit, so that further > 1.0 changes can be made without touching the function signature (my hunch is that backward compatibility issues with those are going to be painful, especially when packed into callback-structs, but this is a hunch). The rest is (unhealthy ? 😉) speculation/discussion about the similarities between the "repository extension system", and the concept of "what this transport is capable of understanding". This issues can be seen internally between the local and smart transport, as both will usually have support added "in sync", but user-provided ones would not at first and either behave unexpectedly, or have to use the I think I'd go with something like this (misnaming is likely to have occurred): /* sys/features.h */
#define GIT_FEATURE_BUILD(featurelist, …) /* static char wrangler used for registration */
static git_feature__check(const char *featurelist, const char *feature);
/* sys/transport.h */
#define GIT_TRANSPORT_FEATURES__SHALLOW "shallow"
typedef struct {
unsigned int version;
const char *featurelist;
/* ... */
} git_transport;
/* smart.c */
static int smart_negotiate_whatever(…)
{
git_transport transport = /* */;
if (git_feature__check(transport->featurelist, GIT_TRANSPORT_FEATURES__SHALLOW) < 0 &&
opts->shallow_depth > 0 /* version, potentially */) {
return GIT_ENOTSUP;
}
}
static git_transport smart_transport;
static int register_smart(…)
{
smart_transport.version = /* 1 */;
GIT_FEATURE_BUILD(&smart_transport.featurelist,
GIT_TRANSPORT_FEATURES__SHALLOW, /* more and more */, NULL);
git_register_transport(&smart_transport);
}It seems to me that something like this could be useful for the repository extension support as well (which, per the switch to SHA256 is likely to be made use of soon). Sorry for being too theorical sometimes 😉. |
|
I still like this idea, but don't want to break the API. 🙏 |
This is extracted from the shallow support transport level PR, and since it very clearly has an impact on API compatibility, we should arrange to be ready for anything. For example, shallow clones need to keep negotiated "things" (the list of server-received shallow OIDs, that should technically only be enforced if the fetch actually unpacks or something).
As the public interface changes to support some options to the negotiation step (like depth level for shallow clones), the underlying transport will need to be updated as well, so let's be prepared.