Calvin Wan <calvinwan@xxxxxxxxxx> writes:
> Currently on the server side of object-info, if the client does not send
> any object ids in the request or if the client requests an attribute the
> server does not support, the server will either not send any packets
> back or error out.
There is an early return when oid_str_list->nr (i.e. number of
objects we are queried) is zero, and we return without showing the
"size" token in the output (called "attribute"). The change in this
patch changes the behaviour and makes such a request responded with
"size" but without any size information for no objects.
It is unclear why that is a good change, though.
The loop that accepts requests in today's code sends an error when
it sees a line that it does not understand. The patch stops doing
so. Also, after ignoring such an unknown line, the requests that
were understood are responded to by send_info() function. The patch
instead refuses to give any output in such a case.
It is unclear why either of these two makes it a good change,
though.
> Consider the scenario where the client git version is
> ahead of the server git version, and the client can request additional
> object-info besides 'size'. There needs to be a way to tell whether the
> server can honor all of the client attribute requests before the client
> sends a request with all of the object ids.
Yes. If we want to learn about "size", "frotz", and "nitfol"
attributes about these objects, we can send "size", "frotz",
"nitfol", and then start feeding object names. Then we can observe
that "frotz" were not liked to learn that this old server does not
understand it, and the same for "nitfol". At least we would have
learned about size of these objects, without this patch.
Now, an updated server side with this patch would respond with
"size" and no other response. Does it mean it does not understand
"frotz", it does not understand "nitfol", or neither? Did we get no
response because we didn't feed objects, or because we asked for
something they don't know about?
How well does the current client work with today's server side and
the server side updated with this patch? I _think_ this change is
essentially a no-op when there is no version skew (i.e. we do not
ask for anything today's server does not know about and we do not
necessarily ask today's server about zero objects).
Am I correct to assume that those who are talking with today's
server side are all out-of-tree custom clients?
I am wondering how much damage we are causing them with this change
in behaviour, especially with the lack of "You asked for X, but I
don't understand X", which is replaced by no output, which would be
totally unexpected by them.
It totally is possible that this "object-info" request is treated as
an experimental curiosity that is not ready for production and has
no existing users. If this were in 2006, I would just _declare_
such is a case and move on, breaking the protocol for existing users
whose number is zero. But I am afraid that we no longer live in
such a simpler world, so...
> In a future patch, if the
> client were to make an initial command request with only attributes, the
> server would be able to confirm which attributes it could return.
>
> ---
> protocol-caps.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/protocol-caps.c b/protocol-caps.c
> index bbde91810a..bc7def0727 100644
> --- a/protocol-caps.c
> +++ b/protocol-caps.c
> @@ -11,6 +11,7 @@
>
> struct requested_info {
> unsigned size : 1;
> + unsigned unknown : 1;
> };
OK.
> /*
> @@ -40,12 +41,12 @@ static void send_info(struct repository *r, struct packet_writer *writer,
> struct string_list_item *item;
> struct strbuf send_buffer = STRBUF_INIT;
>
> - if (!oid_str_list->nr)
> - return;
> -
> if (info->size)
> packet_writer_write(writer, "size");
>
> + if (info->unknown || !oid_str_list->nr)
> + goto release;
> +
> for_each_string_list_item (item, oid_str_list) {
> const char *oid_str = item->string;
> struct object_id oid;
> @@ -72,12 +73,13 @@ static void send_info(struct repository *r, struct packet_writer *writer,
> packet_writer_write(writer, "%s", send_buffer.buf);
> strbuf_reset(&send_buffer);
> }
> +release:
> strbuf_release(&send_buffer);
> }
OK, except for the bypass for info->unknown case, which I am not
sure about.
> int cap_object_info(struct repository *r, struct packet_reader *request)
> {
> - struct requested_info info;
> + struct requested_info info = { 0 };
Wow. We have been using info uninitialized? Is this a silent
bugfix?
If info.size bit was on due to on-stack garbage, we would have given
our response with "size" attribute prefixed, even when the client
side never requested it.
> @@ -92,9 +94,7 @@ int cap_object_info(struct repository *r, struct packet_reader *request)
> if (parse_oid(request->line, &oid_str_list))
> continue;
>
> - packet_writer_error(&writer,
> - "object-info: unexpected line: '%s'",
> - request->line);
> + info.unknown = 1;
