| Message ID | 20220728230210.2952731-5-calvinwan@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Calvin Wan <calvinwan@google.com> writes: > In order for a client to know what object-info components a server can > provide, advertise supported object-info features. This will allow a > client to decide whether to query the server for object-info or fetch as > a fallback. OK. Now we will no longer advertise a bare "object-info", but "object-info=size" (and possibly in the future things other than "size"). How would this change affect older clients who knows what to do with "object-info" but not "object-info=<values>" yet? > +static int object_info_advertise(struct repository *r, > + struct strbuf *value) > +{ > + if (value) > + strbuf_addstr(value, "size"); > + return 1; > +} > + > static void session_id_receive(struct repository *r, > const char *client_sid) > { > @@ -132,7 +140,7 @@ static struct protocol_capability capabilities[] = { > }, > { > .name = "object-info", > - .advertise = always_advertise, > + .advertise = object_info_advertise, > .command = cap_object_info, > }, > }; > diff --git a/t/t5555-http-smart-common.sh b/t/t5555-http-smart-common.sh > index b1cfe8b7db..5a16d4259a 100755 > --- a/t/t5555-http-smart-common.sh > +++ b/t/t5555-http-smart-common.sh > @@ -131,7 +131,7 @@ test_expect_success 'git upload-pack --advertise-refs: v2' ' > fetch=shallow wait-for-done > server-option > object-format=$(test_oid algo) > - object-info > + object-info=size > 0000 > EOF > > diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh > index 1896f671cb..ebb32644e3 100755 > --- a/t/t5701-git-serve.sh > +++ b/t/t5701-git-serve.sh > @@ -20,7 +20,7 @@ test_expect_success 'test capability advertisement' ' > fetch=shallow wait-for-done > server-option > object-format=$(test_oid algo) > - object-info > + object-info=size > 0000 > EOF
> OK. Now we will no longer advertise a bare "object-info", but > "object-info=size" (and possibly in the future things other than > "size"). How would this change affect older clients who knows what > to do with "object-info" but not "object-info=<values>" yet? This was a tricky tradeoff that I definitely think I should have discussed more in the commit message. The issue with how object info is currently implemented is that it is very inflexible for adding new parameters. This is how object-info currently parses a client request: while (packet_reader_read(request) == PACKET_READ_NORMAL) { if (!strcmp("size", request->line)) { info.size = 1; continue; } if (parse_oid(request->line, &oid_str_list)) continue; packet_writer_error(&writer, "object-info: unexpected line: '%s'", request->line); } Object-info supports "size" right now but, let's say I want to add "type" as a parameter. OK I add another if statement like: if (!strcmp("type", request->line)) { info.type = 1; continue; } And we update the docs to say "type" is now supported by object-info. If a user now attempts to request "size" and "type" from a server that has not been updated to support "type", then the user gets an error message "object-info: unexpected line: 'type'", which is another situation that is a bad experience for older clients. The client has no way of knowing that their failure is caused by a server version issue. Essentially I think at some point we have to bite the bullet and say we need to rework some part of the object-info advertisement (or if anyone has a better idea of achieving the same goal) so that we can make future incremental changes to object-info. If the supported parameters are posted in the advertisement, then the client doesn't have to first make a request to find out that their requested parameter isn't support by the server. While you noted that we can't make the assumption now that nobody is using the current object-info feature, I think the benefit of the change outweighs the cost of affecting the possibly small amount of users of this feature. (A quick search on stack overflow for "object-info" tagged under [git] returned no questions about it so that's what I used as a cursory estimate for how popular this feature is). Curious to hear what your thoughts on this are Junio, since as much as I'd like to create a seamless upgrade experience for older clients, I'm out of ideas as to how I would do so.
On Mon, Aug 01 2022, Calvin Wan wrote: >> OK. Now we will no longer advertise a bare "object-info", but >> "object-info=size" (and possibly in the future things other than >> "size"). How would this change affect older clients who knows what >> to do with "object-info" but not "object-info=<values>" yet? > > This was a tricky tradeoff that I definitely think I should have > discussed more in the commit message. The issue with how object > info is currently implemented is that it is very inflexible for adding > new parameters. > > This is how object-info currently parses a client request: > > while (packet_reader_read(request) == PACKET_READ_NORMAL) { > if (!strcmp("size", request->line)) { > info.size = 1; > continue; > } > > if (parse_oid(request->line, &oid_str_list)) > continue; > > packet_writer_error(&writer, > "object-info: unexpected line: '%s'", > request->line); > } > > Object-info supports "size" right now but, let's say I want to add > "type" as a parameter. OK I add another if statement like: > > if (!strcmp("type", request->line)) { > info.type = 1; > continue; > } > > And we update the docs to say "type" is now supported by > object-info. If a user now attempts to request "size" and "type" > from a server that has not been updated to support "type", > then the user gets an error message "object-info: unexpected > line: 'type'", which is another situation that is a bad experience > for older clients. The client has no way of knowing that their > failure is caused by a server version issue. > > Essentially I think at some point we have to bite the bullet and say > we need to rework some part of the object-info advertisement (or > if anyone has a better idea of achieving the same goal) so that we > can make future incremental changes to object-info. If the supported > parameters are posted in the advertisement, then the client doesn't > have to first make a request to find out that their requested > parameter isn't support by the server. While you noted that we can't > make the assumption now that nobody is using the current > object-info feature, I think the benefit of the change outweighs > the cost of affecting the possibly small amount of users of this > feature. (A quick search on stack overflow for "object-info" tagged > under [git] returned no questions about it so that's what I used as > a cursory estimate for how popular this feature is). > > Curious to hear what your thoughts on this are Junio, since as > much as I'd like to create a seamless upgrade experience for > older clients, I'm out of ideas as to how I would do so. I haven't looked deeply into this case, but in general for such protcol incompatibilities we could just create an object-info2, and have that use some extensible calling convention we wish we'd have used from day 1. Then have new clients understand both (and prefer the new verb), and older clients use object-info without breakage. Or we could call the new thing "cat-file", and have it accept any arbitrary options it does, and then limit it to some sensible subset for now :)
Calvin Wan <calvinwan@google.com> writes: > And we update the docs to say "type" is now supported by > object-info. If a user now attempts to request "size" and "type" > from a server that has not been updated to support "type", > then the user gets an error message "object-info: unexpected > line: 'type'", which is another situation that is a bad experience > for older clients. Is it? older clients by definition does not know about "type", no? I am perfectly happy with the capability advertisement to say not just "object-info", but also what attributes of objects you can be queried, by the way. If clients right now (i.e. without this series) expect that the other side who advertises "object-info" without "=<these>,<attributes>,<are>,<supported>" supports only "size", for example, perhaps treat the bare "object-info" capability just as a synonym of "object-info=size", or something? I do not deeply care either way, to be honest, as the deployed clients with bare "object-info" will not survive for a long time and will quickly be deprecated anyway. I just wanted to see the reasoning behind the decision to ignore them. Thanks.
> Is it? older clients by definition does not know about "type", no? Newer client, older remote server was the situation I was thinking about > I am perfectly happy with the capability advertisement to say not > just "object-info", but also what attributes of objects you can be > queried, by the way. If clients right now (i.e. without this > series) expect that the other side who advertises "object-info" > without "=<these>,<attributes>,<are>,<supported>" supports only > "size", for example, perhaps treat the bare "object-info" capability > just as a synonym of "object-info=size", or something? This is a good idea! Thanks
diff --git a/serve.c b/serve.c index 733347f602..1adf9df4a8 100644 --- a/serve.c +++ b/serve.c @@ -56,6 +56,14 @@ static int session_id_advertise(struct repository *r, struct strbuf *value) return 1; } +static int object_info_advertise(struct repository *r, + struct strbuf *value) +{ + if (value) + strbuf_addstr(value, "size"); + return 1; +} + static void session_id_receive(struct repository *r, const char *client_sid) { @@ -132,7 +140,7 @@ static struct protocol_capability capabilities[] = { }, { .name = "object-info", - .advertise = always_advertise, + .advertise = object_info_advertise, .command = cap_object_info, }, }; diff --git a/t/t5555-http-smart-common.sh b/t/t5555-http-smart-common.sh index b1cfe8b7db..5a16d4259a 100755 --- a/t/t5555-http-smart-common.sh +++ b/t/t5555-http-smart-common.sh @@ -131,7 +131,7 @@ test_expect_success 'git upload-pack --advertise-refs: v2' ' fetch=shallow wait-for-done server-option object-format=$(test_oid algo) - object-info + object-info=size 0000 EOF diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh index 1896f671cb..ebb32644e3 100755 --- a/t/t5701-git-serve.sh +++ b/t/t5701-git-serve.sh @@ -20,7 +20,7 @@ test_expect_success 'test capability advertisement' ' fetch=shallow wait-for-done server-option object-format=$(test_oid algo) - object-info + object-info=size 0000 EOF
In order for a client to know what object-info components a server can provide, advertise supported object-info features. This will allow a client to decide whether to query the server for object-info or fetch as a fallback. Signed-off-by: Calvin Wan <calvinwan@google.com> Helped-by: Jonathan Tan <jonathantanmy@google.com> --- serve.c | 10 +++++++++- t/t5555-http-smart-common.sh | 2 +- t/t5701-git-serve.sh | 2 +- 3 files changed, 11 insertions(+), 3 deletions(-)