{"thread":{"id":"55501","subject":"[PATCH] object-info: support for retrieving object info","startedAt":"2021-04-15T21:20:29Z","lastAt":"2021-04-20T23:43:48Z","messageCount":7,"participants":["Bruno Albuquerque","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"422061","messageId":"20210415212017.1407303-1-bga@google.com","threadId":"55501","inReplyTo":null,"subject":"[PATCH] object-info: support for retrieving object info","fromName":"Bruno Albuquerque","fromEmail":"bga@google.com","sentAt":"2021-04-15T21:20:17Z","receivedAt":"2021-04-15T21:20:29Z","isPatch":true,"sender":{"key":"bga@google.com","avatar":"https://avatars.githubusercontent.com/u/80971974?v=4"},"body":"Sometimes it is useful to get information of an object without having to\ndownload it completely.\n\nAdd the \"object-info\" capability that lets the client ask for\nobject-related information with their full hexadecimal object names.\n\nOnly sizes are returned for now.\n\nSigned-off-by: Bruno Albuquerque <bga@google.com>\n---\n Documentation/technical/protocol-v2.txt |  31 +++++++\n Makefile                                |   1 +\n protocol-caps.c                         | 115 ++++++++++++++++++++++++\n protocol-caps.h                         |  10 +++\n serve.c                                 |   2 +\n t/t5701-git-serve.sh                    |  26 ++++++\n 6 files changed, 185 insertions(+)\n create mode 100644 protocol-caps.c\n create mode 100644 protocol-caps.h\n\nHello.\n\nThis is my first git patch so I thought I would introduce myself. I am a\nsoftware engineer at Google and I have been involved with opensource for\na while (mostly with the Haiku OS project) and now I am working on some\nGit changes that hopefully will be generally usefull.\n\nFor this specific change, a clear usage scenario is implementing a VFS\non top of Git (something like https://github.com/microsoft/VFSForGit) in\na way that would not require someone to always fully download objects to\nget information about them. Object size is the obvious one and what is\nimplemented here.\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex a7c806a73e..f4ed141774 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -514,3 +514,34 @@ packet-line, and must not contain non-printable or whitespace characters. The\n current implementation uses trace2 session IDs (see\n link:api-trace2.html[api-trace2] for details), but this may change and users of\n the session ID should not rely on this fact.\n+\n+object-info\n+~~~~~~~~~~~\n+\n+`object-info` is the command to retrieve information about one or more objects.\n+Its main purpose is to allow a client to make decisions based on this\n+information without having to fully fetch objects. Object size is the only\n+information that is currently supported.\n+\n+An `object-info` request takes the following arguments:\n+\n+\tsize\n+\tRequests size information to be returned for each listed object id.\n+\n+\toid <oid>\n+\tIndicates to the server an object which the client wants to obtain\n+\tinformation for.\n+\n+The response of `object-info` is a list of the the requested object ids\n+and associated requested information, each separated by a single space.\n+\n+\toutput = info flush-pkt\n+\n+\tinfo = PKT-LINE(attrs) LF)\n+\t\t*PKT-LINE(obj-info LF)\n+\n+\tattrs = attr | attrs SP attrs\n+\n+\tattr = \"size\"\n+\n+\tobj-info = obj-id SP obj-size\ndiff --git a/Makefile b/Makefile\nindex 21c0bf1667..3225e37b63 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -961,6 +961,7 @@ LIB_OBJS += progress.o\n LIB_OBJS += promisor-remote.o\n LIB_OBJS += prompt.o\n LIB_OBJS += protocol.o\n+LIB_OBJS += protocol-caps.o\n LIB_OBJS += prune-packed.o\n LIB_OBJS += quote.o\n LIB_OBJS += range-diff.o\ndiff --git a/protocol-caps.c b/protocol-caps.c\nnew file mode 100644\nindex 0000000000..c15e397756\n--- /dev/null\n+++ b/protocol-caps.c\n@@ -0,0 +1,115 @@\n+#include \"git-compat-util.h\"\n+#include \"protocol-caps.h\"\n+#include \"gettext.h\"\n+#include \"pkt-line.h\"\n+#include \"strvec.h\"\n+#include \"hash.h\"\n+#include \"object.h\"\n+#include \"object-store.h\"\n+#include \"string-list.h\"\n+#include \"strbuf.h\"\n+\n+struct requested_info {\n+\tunsigned size : 1;\n+};\n+\n+/*\n+ * Parses oids from the given line and collects them in the given\n+ * oid_str_list. Returns 1 if parsing was successful and 0 otherwise.\n+ */\n+static int parse_oid(const char *line, struct string_list *oid_str_list)\n+{\n+\tconst char *arg;\n+\n+\tif (!skip_prefix(line, \"oid \", &arg))\n+\t\treturn 0;\n+\n+\tstring_list_append(oid_str_list, arg);\n+\n+\treturn 1;\n+}\n+\n+/*\n+ * Validates and send requested info back to the client. Any errors detected\n+ * are returned as they are detected.\n+ */\n+static void send_info(struct repository *r, struct packet_writer *writer,\n+\t\t      struct string_list *oid_str_list,\n+\t\t      struct requested_info *info)\n+{\n+\tstruct string_list_item *item;\n+\tstruct strbuf send_buffer = STRBUF_INIT;\n+\n+\tif (!oid_str_list->nr)\n+\t\treturn;\n+\n+\tif (info->size)\n+\t\tpacket_writer_write(writer, \"size\");\n+\n+\tfor_each_string_list_item (item, oid_str_list) {\n+\t\tconst char *oid_str = item->string;\n+\t\tstruct object_id oid;\n+\t\tunsigned long object_size;\n+\n+\t\tif (get_oid_hex(oid_str, &oid) < 0) {\n+\t\t\tpacket_writer_error(\n+\t\t\t\twriter,\n+\t\t\t\t\"object-info: protocol error, expected to get oid, not '%s'\",\n+\t\t\t\toid_str);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_addstr(&send_buffer, oid_str);\n+\n+\t\tif (info->size) {\n+\t\t\tif (oid_object_info(r, &oid, &object_size) < 0) {\n+\t\t\t\tstrbuf_addstr(&send_buffer, \" \");\n+\t\t\t} else {\n+\t\t\t\tstrbuf_addf(&send_buffer, \" %lu\", object_size);\n+\t\t\t}\n+\t\t}\n+\n+\t\tpacket_writer_write(writer, \"%s\",\n+\t\t\t\t    strbuf_detach(&send_buffer, NULL));\n+\t}\n+}\n+\n+int cap_object_info(struct repository *r, struct strvec *keys,\n+\t\t    struct packet_reader *request)\n+{\n+\tstruct packet_writer writer;\n+\tpacket_writer_init(&writer, 1);\n+\tint parsed_header;\n+\tstruct requested_info info;\n+\n+\tstruct string_list oid_str_list = STRING_LIST_INIT_DUP;\n+\n+\tparsed_header = 0;\n+\twhile (packet_reader_read(request) == PACKET_READ_NORMAL) {\n+\t\tif (!strcmp(\"size\", request->line)) {\n+\t\t\tinfo.size = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (parse_oid(request->line, &oid_str_list))\n+\t\t\tcontinue;\n+\n+\t\tpacket_writer_error(&writer,\n+\t\t\t\t    \"object-info: unexpected line: '%s'\",\n+\t\t\t\t    request->line);\n+\t}\n+\n+\tif (request->status != PACKET_READ_FLUSH) {\n+\t\tpacket_writer_error(\n+\t\t\t&writer, \"object-info: expected flush after arguments\");\n+\t\tdie(_(\"object-info: expected flush after arguments\"));\n+\t}\n+\n+\tsend_info(r, &writer, &oid_str_list, &info);\n+\n+\tstring_list_clear(&oid_str_list, 1);\n+\n+\tpacket_flush(1);\n+\n+\treturn 0;\n+}\ndiff --git a/protocol-caps.h b/protocol-caps.h\nnew file mode 100644\nindex 0000000000..6351648e37\n--- /dev/null\n+++ b/protocol-caps.h\n@@ -0,0 +1,10 @@\n+#ifndef PROTOCOL_CAPS_H\n+#define PROTOCOL_CAPS_H\n+\n+struct repository;\n+struct strvec;\n+struct packet_reader;\n+int cap_object_info(struct repository *r, struct strvec *keys,\n+\t\t    struct packet_reader *request);\n+\n+#endif /* PROTOCOL_CAPS_H */\n\\ No newline at end of file\ndiff --git a/serve.c b/serve.c\nindex ac20c72763..aa8209f147 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -5,6 +5,7 @@\n #include \"version.h\"\n #include \"strvec.h\"\n #include \"ls-refs.h\"\n+#include \"protocol-caps.h\"\n #include \"serve.h\"\n #include \"upload-pack.h\"\n \n@@ -78,6 +79,7 @@ static struct protocol_capability capabilities[] = {\n \t{ \"server-option\", always_advertise, NULL },\n \t{ \"object-format\", object_format_advertise, NULL },\n \t{ \"session-id\", session_id_advertise, NULL },\n+\t{ \"object-info\", always_advertise, cap_object_info },\n };\n \n static void advertise_capabilities(void)\ndiff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\nindex 509f379d49..73e74a9c54 100755\n--- a/t/t5701-git-serve.sh\n+++ b/t/t5701-git-serve.sh\n@@ -19,6 +19,7 @@ test_expect_success 'test capability advertisement' '\n \tfetch=shallow\n \tserver-option\n \tobject-format=$(test_oid algo)\n+\tobject-info\n \t0000\n \tEOF\n \n@@ -240,4 +241,29 @@ test_expect_success 'unexpected lines are not allowed in fetch request' '\n \tgrep \"unexpected line: .this-is-not-a-command.\" err\n '\n \n+# Test the basics of object-info\n+#\n+test_expect_success 'basics of object-info' '\n+\ttest-tool pkt-line pack >in <<-EOF &&\n+\tcommand=object-info\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\tsize\n+\toid $(git rev-parse two:two.t)\n+\toid $(git rev-parse two:two.t)\n+\t0000\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\tsize\n+\t$(git rev-parse two:two.t) $(wc -c <two.t | xargs)\n+\t$(git rev-parse two:two.t) $(wc -c <two.t | xargs)\n+\t0000\n+\tEOF\n+\n+\ttest-tool serve-v2 --stateless-rpc <in >out &&\n+\ttest-tool pkt-line unpack <out >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.31.1.368.gbe11c130af-goog\n\n"},{"id":"422065","messageId":"xmqq4kg7p63j.fsf@gitster.g","threadId":"55501","inReplyTo":"20210415212017.1407303-1-bga@google.com","subject":"Re: [PATCH] object-info: support for retrieving object info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-15T21:53:36Z","receivedAt":"2021-04-15T21:53:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bruno Albuquerque <bga@google.com> writes:\n\n> Sometimes it is useful to get information of an object without having to\n> download it completely.\n>\n> Add the \"object-info\" capability that lets the client ask for\n> object-related information with their full hexadecimal object names.\n>\n> Only sizes are returned for now.\n>\n> Signed-off-by: Bruno Albuquerque <bga@google.com>\n> ---\n\n>  Documentation/technical/protocol-v2.txt |  31 +++++++\n>  Makefile                                |   1 +\n>  protocol-caps.c                         | 115 ++++++++++++++++++++++++\n>  protocol-caps.h                         |  10 +++\n>  serve.c                                 |   2 +\n>  t/t5701-git-serve.sh                    |  26 ++++++\n>  6 files changed, 185 insertions(+)\n>  create mode 100644 protocol-caps.c\n>  create mode 100644 protocol-caps.h\n>\n> Hello.\n>\n> This is my first git patch so I thought I would introduce myself. I am a\n> software engineer at Google and I have been involved with opensource for\n> a while (mostly with the Haiku OS project) and now I am working on some\n> Git changes that hopefully will be generally usefull.\n>\n> For this specific change, a clear usage scenario is implementing a VFS\n> on top of Git (something like https://github.com/microsoft/VFSForGit) in\n> a way that would not require someone to always fully download objects to\n> get information about them. Object size is the obvious one and what is\n> implemented here.\n\nIs the assumption that such an implementation of VFS would fetch\nindividual tree object with the existing \"fetch this single object\nby the object name\" interface?\n\nWhat I am wondering is, as an ingredient for implementing VFS layer,\nif this is a bit too low level.  To respond to \"ls -l\" when you only\nhave a tree object name, you'd need two roundtrips, one to retrieve\nthe tree, and then after parsing the tree to find out what objects\nthe tree refers to with what pathname component, you'd issue the\nobject-info for all of them in a single request.\n\nIf a request takes a single (or multiple) tree object name, and lets\nyou retrieve _both_ the tree object itself _and_ object-info for the\nobjects the tree refers to, you can build \"ls -l\" with a single\nroundtrip instead.\n\nI do not know how much the latency matters (or more importantly, how\nmuch a naïve coutner-proposal like the above would help), but it is\nwhat immediately came to my mind.\n\nAssuming that we are good with an interface that needs two requests\nto obtain \"object contents\" and \"object info\" separately, I find\nwhat in this patch quite reasonable, though (admittedly, I've\nalready read this patch during internal review number of times).\n\nThanks.\n\n> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n> index a7c806a73e..f4ed141774 100644\n> --- a/Documentation/technical/protocol-v2.txt\n> +++ b/Documentation/technical/protocol-v2.txt\n> @@ -514,3 +514,34 @@ packet-line, and must not contain non-printable or whitespace characters. The\n>  current implementation uses trace2 session IDs (see\n>  link:api-trace2.html[api-trace2] for details), but this may change and users of\n>  the session ID should not rely on this fact.\n> +\n> +object-info\n> +~~~~~~~~~~~\n> +\n> +`object-info` is the command to retrieve information about one or more objects.\n> +Its main purpose is to allow a client to make decisions based on this\n> +information without having to fully fetch objects. Object size is the only\n> +information that is currently supported.\n> +\n> +An `object-info` request takes the following arguments:\n> +\n> +\tsize\n> +\tRequests size information to be returned for each listed object id.\n> +\n> +\toid <oid>\n> +\tIndicates to the server an object which the client wants to obtain\n> +\tinformation for.\n> +\n> +The response of `object-info` is a list of the the requested object ids\n> +and associated requested information, each separated by a single space.\n> +\n> +\toutput = info flush-pkt\n> +\n> +\tinfo = PKT-LINE(attrs) LF)\n> +\t\t*PKT-LINE(obj-info LF)\n> +\n> +\tattrs = attr | attrs SP attrs\n> +\n> +\tattr = \"size\"\n> +\n> +\tobj-info = obj-id SP obj-size\n> diff --git a/Makefile b/Makefile\n> index 21c0bf1667..3225e37b63 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -961,6 +961,7 @@ LIB_OBJS += progress.o\n>  LIB_OBJS += promisor-remote.o\n>  LIB_OBJS += prompt.o\n>  LIB_OBJS += protocol.o\n> +LIB_OBJS += protocol-caps.o\n>  LIB_OBJS += prune-packed.o\n>  LIB_OBJS += quote.o\n>  LIB_OBJS += range-diff.o\n> diff --git a/protocol-caps.c b/protocol-caps.c\n> new file mode 100644\n> index 0000000000..c15e397756\n> --- /dev/null\n> +++ b/protocol-caps.c\n> @@ -0,0 +1,115 @@\n> +#include \"git-compat-util.h\"\n> +#include \"protocol-caps.h\"\n> +#include \"gettext.h\"\n> +#include \"pkt-line.h\"\n> +#include \"strvec.h\"\n> +#include \"hash.h\"\n> +#include \"object.h\"\n> +#include \"object-store.h\"\n> +#include \"string-list.h\"\n> +#include \"strbuf.h\"\n> +\n> +struct requested_info {\n> +\tunsigned size : 1;\n> +};\n> +\n> +/*\n> + * Parses oids from the given line and collects them in the given\n> + * oid_str_list. Returns 1 if parsing was successful and 0 otherwise.\n> + */\n> +static int parse_oid(const char *line, struct string_list *oid_str_list)\n> +{\n> +\tconst char *arg;\n> +\n> +\tif (!skip_prefix(line, \"oid \", &arg))\n> +\t\treturn 0;\n> +\n> +\tstring_list_append(oid_str_list, arg);\n> +\n> +\treturn 1;\n> +}\n> +\n> +/*\n> + * Validates and send requested info back to the client. Any errors detected\n> + * are returned as they are detected.\n> + */\n> +static void send_info(struct repository *r, struct packet_writer *writer,\n> +\t\t      struct string_list *oid_str_list,\n> +\t\t      struct requested_info *info)\n> +{\n> +\tstruct string_list_item *item;\n> +\tstruct strbuf send_buffer = STRBUF_INIT;\n> +\n> +\tif (!oid_str_list->nr)\n> +\t\treturn;\n> +\n> +\tif (info->size)\n> +\t\tpacket_writer_write(writer, \"size\");\n> +\n> +\tfor_each_string_list_item (item, oid_str_list) {\n> +\t\tconst char *oid_str = item->string;\n> +\t\tstruct object_id oid;\n> +\t\tunsigned long object_size;\n> +\n> +\t\tif (get_oid_hex(oid_str, &oid) < 0) {\n> +\t\t\tpacket_writer_error(\n> +\t\t\t\twriter,\n> +\t\t\t\t\"object-info: protocol error, expected to get oid, not '%s'\",\n> +\t\t\t\toid_str);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tstrbuf_addstr(&send_buffer, oid_str);\n> +\n> +\t\tif (info->size) {\n> +\t\t\tif (oid_object_info(r, &oid, &object_size) < 0) {\n> +\t\t\t\tstrbuf_addstr(&send_buffer, \" \");\n> +\t\t\t} else {\n> +\t\t\t\tstrbuf_addf(&send_buffer, \" %lu\", object_size);\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tpacket_writer_write(writer, \"%s\",\n> +\t\t\t\t    strbuf_detach(&send_buffer, NULL));\n> +\t}\n> +}\n> +\n> +int cap_object_info(struct repository *r, struct strvec *keys,\n> +\t\t    struct packet_reader *request)\n> +{\n> +\tstruct packet_writer writer;\n> +\tpacket_writer_init(&writer, 1);\n> +\tint parsed_header;\n> +\tstruct requested_info info;\n> +\n> +\tstruct string_list oid_str_list = STRING_LIST_INIT_DUP;\n> +\n> +\tparsed_header = 0;\n> +\twhile (packet_reader_read(request) == PACKET_READ_NORMAL) {\n> +\t\tif (!strcmp(\"size\", request->line)) {\n> +\t\t\tinfo.size = 1;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tif (parse_oid(request->line, &oid_str_list))\n> +\t\t\tcontinue;\n> +\n> +\t\tpacket_writer_error(&writer,\n> +\t\t\t\t    \"object-info: unexpected line: '%s'\",\n> +\t\t\t\t    request->line);\n> +\t}\n> +\n> +\tif (request->status != PACKET_READ_FLUSH) {\n> +\t\tpacket_writer_error(\n> +\t\t\t&writer, \"object-info: expected flush after arguments\");\n> +\t\tdie(_(\"object-info: expected flush after arguments\"));\n> +\t}\n> +\n> +\tsend_info(r, &writer, &oid_str_list, &info);\n> +\n> +\tstring_list_clear(&oid_str_list, 1);\n> +\n> +\tpacket_flush(1);\n> +\n> +\treturn 0;\n> +}\n> diff --git a/protocol-caps.h b/protocol-caps.h\n> new file mode 100644\n> index 0000000000..6351648e37\n> --- /dev/null\n> +++ b/protocol-caps.h\n> @@ -0,0 +1,10 @@\n> +#ifndef PROTOCOL_CAPS_H\n> +#define PROTOCOL_CAPS_H\n> +\n> +struct repository;\n> +struct strvec;\n> +struct packet_reader;\n> +int cap_object_info(struct repository *r, struct strvec *keys,\n> +\t\t    struct packet_reader *request);\n> +\n> +#endif /* PROTOCOL_CAPS_H */\n> \\ No newline at end of file\n> diff --git a/serve.c b/serve.c\n> index ac20c72763..aa8209f147 100644\n> --- a/serve.c\n> +++ b/serve.c\n> @@ -5,6 +5,7 @@\n>  #include \"version.h\"\n>  #include \"strvec.h\"\n>  #include \"ls-refs.h\"\n> +#include \"protocol-caps.h\"\n>  #include \"serve.h\"\n>  #include \"upload-pack.h\"\n>  \n> @@ -78,6 +79,7 @@ static struct protocol_capability capabilities[] = {\n>  \t{ \"server-option\", always_advertise, NULL },\n>  \t{ \"object-format\", object_format_advertise, NULL },\n>  \t{ \"session-id\", session_id_advertise, NULL },\n> +\t{ \"object-info\", always_advertise, cap_object_info },\n>  };\n>  \n>  static void advertise_capabilities(void)\n> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh\n> index 509f379d49..73e74a9c54 100755\n> --- a/t/t5701-git-serve.sh\n> +++ b/t/t5701-git-serve.sh\n> @@ -19,6 +19,7 @@ test_expect_success 'test capability advertisement' '\n>  \tfetch=shallow\n>  \tserver-option\n>  \tobject-format=$(test_oid algo)\n> +\tobject-info\n>  \t0000\n>  \tEOF\n>  \n> @@ -240,4 +241,29 @@ test_expect_success 'unexpected lines are not allowed in fetch request' '\n>  \tgrep \"unexpected line: .this-is-not-a-command.\" err\n>  '\n>  \n> +# Test the basics of object-info\n> +#\n> +test_expect_success 'basics of object-info' '\n> +\ttest-tool pkt-line pack >in <<-EOF &&\n> +\tcommand=object-info\n> +\tobject-format=$(test_oid algo)\n> +\t0001\n> +\tsize\n> +\toid $(git rev-parse two:two.t)\n> +\toid $(git rev-parse two:two.t)\n> +\t0000\n> +\tEOF\n> +\n> +\tcat >expect <<-EOF &&\n> +\tsize\n> +\t$(git rev-parse two:two.t) $(wc -c <two.t | xargs)\n> +\t$(git rev-parse two:two.t) $(wc -c <two.t | xargs)\n> +\t0000\n> +\tEOF\n> +\n> +\ttest-tool serve-v2 --stateless-rpc <in >out &&\n> +\ttest-tool pkt-line unpack <out >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n"},{"id":"422069","messageId":"xmqqmttznqj6.fsf@gitster.g","threadId":"55501","inReplyTo":"20210415212017.1407303-1-bga@google.com","subject":"Re: [PATCH] object-info: support for retrieving object info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-15T22:15:09Z","receivedAt":"2021-04-15T22:15:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bruno Albuquerque <bga@google.com> writes:\n\n> +int cap_object_info(struct repository *r, struct strvec *keys,\n> +\t\t    struct packet_reader *request)\n> +{\n> +\tstruct packet_writer writer;\n> +\tpacket_writer_init(&writer, 1);\n\nThis triggers -Wdeclaration-after-statement below.  Move it down.\n\n> +\tint parsed_header;\n> +\tstruct requested_info info;\n> +\n\nPuzzling blank line here.  There does not seem to be a good reason\nwhy 'parsed_header' bit plus 'info' pair together closely than to\nthe other 'oid_str_list' variable, so it does not make much sense as\na grouping aid.\n\n> +\tstruct string_list oid_str_list = STRING_LIST_INIT_DUP;\n> +\n\nHere, just before \"parsed_header = 0;\" after the blank line that\nseparates the decls and the statements, is a good place to say\n\"packet_writer_init()\".  Also it may make more sense to give initial\nvalue to parsed_header where it is declared.\n\n> +\tparsed_header = 0;\n> +\twhile (packet_reader_read(request) == PACKET_READ_NORMAL) {\n> +\t\tif (!strcmp(\"size\", request->line)) {\n> +\t\t\tinfo.size = 1;\n> +\t\t\tcontinue;\n\nAnd upon further inspection, nobody seems to use parsed_header at\nall.  Let's lose it.\n\nPerhaps this minimum fix-up squashed in?\n\nNext time, perhaps try \"make DEVELOPER=YesPlease test\" to catch\npossible problems like these early?\n\nThanks.\n\n\n protocol-caps.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git c/protocol-caps.c w/protocol-caps.c\nindex c15e397756..922bfeb905 100644\n--- c/protocol-caps.c\n+++ w/protocol-caps.c\n@@ -78,13 +78,10 @@ int cap_object_info(struct repository *r, struct strvec *keys,\n \t\t    struct packet_reader *request)\n {\n \tstruct packet_writer writer;\n-\tpacket_writer_init(&writer, 1);\n-\tint parsed_header;\n \tstruct requested_info info;\n-\n \tstruct string_list oid_str_list = STRING_LIST_INIT_DUP;\n \n-\tparsed_header = 0;\n+\tpacket_writer_init(&writer, 1);\n \twhile (packet_reader_read(request) == PACKET_READ_NORMAL) {\n \t\tif (!strcmp(\"size\", request->line)) {\n \t\t\tinfo.size = 1;\n\n"},{"id":"422073","messageId":"CAPeR6H5wx6F9rAYoRC-1GwHDujBFyjMTUPdHkQAuZh-eYAoRsg@mail.gmail.com","threadId":"55501","inReplyTo":"xmqq4kg7p63j.fsf@gitster.g","subject":"Re: [PATCH] object-info: support for retrieving object info","fromName":"Bruno Albuquerque","fromEmail":"bga@google.com","sentAt":"2021-04-15T23:06:40Z","receivedAt":"2021-04-15T23:06:54Z","isPatch":true,"sender":{"key":"bga@google.com","avatar":"https://avatars.githubusercontent.com/u/80971974?v=4"},"body":"On Thu, Apr 15, 2021 at 2:53 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Is the assumption that such an implementation of VFS would fetch\n> individual tree object with the existing \"fetch this single object\n> by the object name\" interface?\n\nThat is the general idea, yes.\n\n> What I am wondering is, as an ingredient for implementing VFS layer,\n> if this is a bit too low level.  To respond to \"ls -l\" when you only\n> have a tree object name, you'd need two roundtrips, one to retrieve\n> the tree, and then after parsing the tree to find out what objects\n> the tree refers to with what pathname component, you'd issue the\n> object-info for all of them in a single request.\n\nYes, as it is designed you would need to do that. There are a couple\nfew reasons for this implementation to do this the way it does:\n\n1 - Although it is currently only returning object sizes, it was\ndesigned in a way that it can be extended if need to return other\nobject metadata.\n2 - Doing it like this is a fully backwards-compatible change and\nolder clients would still work without changes (just not make use of\nthis).\n3- In a real filesystem, it is common to have multiple directories\nunder a tree so in practice you can optimize requests (if needed) by\nretrieving several tree objects and doing a single object-info request\nfor all objects in those trees.\n\nNote I am not saying it could not be done in a different way, but it\ndoes look to me this change strikes a good balance between what it\nprovides and its cost,\n\n> If a request takes a single (or multiple) tree object name, and lets\n> you retrieve _both_ the tree object itself _and_ object-info for the\n> objects the tree refers to, you can build \"ls -l\" with a single\n> roundtrip instead.\n\nThat is true. But it seems to me that there is not a good place to fit\nsize  (and maybe other metadata eventually) information currently with\nthe existing protocol. But it is not impossible that I might simply be\nmissing something.\n\n> I do not know how much the latency matters (or more importantly, how\n> much a naïve coutner-proposal like the above would help), but it is\n> what immediately came to my mind.\n\nI do not expect the latency of the object-info request to be an issue\nespecially because fetching the information can be done in batches and\nalso prefetched in some cases (as we do not need to download the\nobjects, we do not need to worry about downloading possibly gigabytes\nof data while just iterating through directories). But, of course,\nassuming there is a clean way to make things even better, I am all for\nit.\n\n> Assuming that we are good with an interface that needs two requests\n> to obtain \"object contents\" and \"object info\" separately, I find\n> what in this patch quite reasonable, though (admittedly, I've\n> already read this patch during internal review number of times).\n\n\nFWIIW, I gave this a lot of thought and for the purposes of doing a\nremote file system (and one can even optimize git ls-tree to be a lot\nfaster for partial clones using this), I feel confident about the\nchange (module missing something, of course).\n\nThanks for your comments.\n"},{"id":"422128","messageId":"YHoJUrqeT26Nihua@camp.crustytoothpaste.net","threadId":"55501","inReplyTo":"20210415212017.1407303-1-bga@google.com","subject":"Re: [PATCH] object-info: support for retrieving object info","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-04-16T22:01:54Z","receivedAt":"2021-04-16T22:02:32Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-04-15 at 21:20:17, Bruno Albuquerque wrote:\n> Sometimes it is useful to get information of an object without having to\n> download it completely.\n> \n> Add the \"object-info\" capability that lets the client ask for\n> object-related information with their full hexadecimal object names.\n> \n> Only sizes are returned for now.\n> \n> Signed-off-by: Bruno Albuquerque <bga@google.com>\n> ---\n>  Documentation/technical/protocol-v2.txt |  31 +++++++\n>  Makefile                                |   1 +\n>  protocol-caps.c                         | 115 ++++++++++++++++++++++++\n>  protocol-caps.h                         |  10 +++\n>  serve.c                                 |   2 +\n>  t/t5701-git-serve.sh                    |  26 ++++++\n>  6 files changed, 185 insertions(+)\n>  create mode 100644 protocol-caps.c\n>  create mode 100644 protocol-caps.h\n> \n> Hello.\n> \n> This is my first git patch so I thought I would introduce myself. I am a\n> software engineer at Google and I have been involved with opensource for\n> a while (mostly with the Haiku OS project) and now I am working on some\n> Git changes that hopefully will be generally usefull.\n> \n> For this specific change, a clear usage scenario is implementing a VFS\n> on top of Git (something like https://github.com/microsoft/VFSForGit) in\n> a way that would not require someone to always fully download objects to\n> get information about them. Object size is the obvious one and what is\n> implemented here.\n\nI want to point out a few notes as someone who's worked on projects\nappurtenant to VFS for Git.  If your goal is to create a VFS, then\nlearning what Git thinks the object size is is not helpful.\n\nGit performs all sorts of operations on files when checking them out,\nsuch as smudge and clean filters (e.g., Git LFS), line ending\nconversion, and working tree encodings (e.g., to and from UTF-16 on\nWindows).  Thus, your VFS must necessarily exclude all of this\nfunctionality to use Git's concept of an object size as a VFS parameter.\n\nFor the average project, the worst functionality to lose is actually\nline ending conversion, since that means people will practically check\nin a mix of line endings unless you are very strict in CI.  VFS for Git\nwas originally designed to address the needs of a project that was not\ncross-platform and thus this was not a problem, but these limitations\nare substantial and I wouldn't recommend repeating them.\n\nSo I'm not opposed to seeing this functionality come in if you have\nother plans for it (which is fine and I'm certainly interested in\nhearing about them if you do), but I don't think this is helpful for VFS\nscenarios and shouldn't be used for that purpose.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"422341","messageId":"CAPeR6H71Hn3ei4CzZLnQH1JGTvTpy6Ru-Mpin5cVJ0_7GSkoag@mail.gmail.com","threadId":"55501","inReplyTo":"YHoJUrqeT26Nihua@camp.crustytoothpaste.net","subject":"Re: [PATCH] object-info: support for retrieving object info","fromName":"Bruno Albuquerque","fromEmail":"bga@google.com","sentAt":"2021-04-19T21:18:30Z","receivedAt":"2021-04-19T21:18:44Z","isPatch":true,"sender":{"key":"bga@google.com","avatar":"https://avatars.githubusercontent.com/u/80971974?v=4"},"body":"On Fri, Apr 16, 2021 at 3:02 PM brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n\n> I want to point out a few notes as someone who's worked on projects\n> appurtenant to VFS for Git.  If your goal is to create a VFS, then\n> learning what Git thinks the object size is is not helpful.\n>\n> Git performs all sorts of operations on files when checking them out,\n> such as smudge and clean filters (e.g., Git LFS), line ending\n> conversion, and working tree encodings (e.g., to and from UTF-16 on\n> Windows).  Thus, your VFS must necessarily exclude all of this\n> functionality to use Git's concept of an object size as a VFS parameter.\n\nThis is, of course, completely valid from a Git-specific point of view\nbut, because we are dealing with a hypothetical remote FS, it would\nnot be unheard of (and, actually, this is most likely the norm more\noften than not) that when exposing git objects in an actual FS, we\nwould not be doing any conversions whatsoever and would, instead, show\neverything as it is actually stored on the remote server.\n\n> For the average project, the worst functionality to lose is actually\n> line ending conversion, since that means people will practically check\n> in a mix of line endings unless you are very strict in CI.  VFS for Git\n> was originally designed to address the needs of a project that was not\n> cross-platform and thus this was not a problem, but these limitations\n> are substantial and I wouldn't recommend repeating them.\n\nAlthough I understand the issue here, this would be fine in the\ncontext of a remote filesystem as I see it. In fact this is something\nthat actually has  precedent in git anyway (\"git cat-file -s\" without\n--filters will emit the size without any conversions).\n\nAs long as the size we report match with the actual contents of the\nfile we expose (i.e. without conversions), this looks ok to me.\n\n> So I'm not opposed to seeing this functionality come in if you have\n> other plans for it (which is fine and I'm certainly interested in\n> hearing about them if you do), but I don't think this is helpful for VFS\n> scenarios and shouldn't be used for that purpose.\n\nThanks for your comments. This actually works for our purposes and,\nmore generally, it would be useful even if you do care about all the\nconversions (in this case, as a reasonable approximation of the actual\ndisk size I guess).\n"},{"id":"422505","messageId":"CAPeR6H4V2_bAHaU4SCC8uGorj1bQEKLX5LWtXFuoHNRpo=dYPg@mail.gmail.com","threadId":"55501","inReplyTo":"xmqqmttznqj6.fsf@gitster.g","subject":"Re: [PATCH] object-info: support for retrieving object info","fromName":"Bruno Albuquerque","fromEmail":"bga@google.com","sentAt":"2021-04-20T23:43:32Z","receivedAt":"2021-04-20T23:43:48Z","isPatch":true,"sender":{"key":"bga@google.com","avatar":"https://avatars.githubusercontent.com/u/80971974?v=4"},"body":"On Thu, Apr 15, 2021 at 3:15 PM Junio C Hamano <gitster@pobox.com> wrote:\n\n> > +int cap_object_info(struct repository *r, struct strvec *keys,\n> > +                 struct packet_reader *request)\n> > +{\n> > +     struct packet_writer writer;\n> > +     packet_writer_init(&writer, 1);\n>\n> This triggers -Wdeclaration-after-statement below.  Move it down.\n\nDone.\n\n> > +     int parsed_header;\n> > +     struct requested_info info;\n>\n> Puzzling blank line here.  There does not seem to be a good reason\n> why 'parsed_header' bit plus 'info' pair together closely than to\n> the other 'oid_str_list' variable, so it does not make much sense as\n> a grouping aid.\n\nDone.\n\n> > +     struct string_list oid_str_list = STRING_LIST_INIT_DUP;\n> > +\n>\n> Here, just before \"parsed_header = 0;\" after the blank line that\n> separates the decls and the statements, is a good place to say\n> \"packet_writer_init()\".  Also it may make more sense to give initial\n> value to parsed_header where it is declared.\n\nDone.\n\n> > +     parsed_header = 0;\n> > +     while (packet_reader_read(request) == PACKET_READ_NORMAL) {\n> > +             if (!strcmp(\"size\", request->line)) {\n> > +                     info.size = 1;\n> > +                     continue;\n>\n> And upon further inspection, nobody seems to use parsed_header at\n> all.  Let's lose it.\n\nDone.\n\n> Next time, perhaps try \"make DEVELOPER=YesPlease test\" to catch\n> possible problems like these early?\n\nI did. But I was coding on a MacOS machine (which defaults to clang)\nand it looks like the warnings are not triggered at all in clang even\nwith -Werror being explicitly set. I switched to a Linux machine and\ngot the warnings.\n"}]}