{"thread":{"id":"55623","subject":"[PATCH 0/5] streaming.c: refactor for smaller + easier to understand code","startedAt":"2021-05-05T12:33:45Z","lastAt":"2021-05-06T00:14:36Z","messageCount":12,"participants":["Ævar Arnfjörð Bjarmason","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"423667","messageId":"cover-0.5-00000000000-20210505T122816Z-avarab@gmail.com","threadId":"55623","inReplyTo":null,"subject":"[PATCH 0/5] streaming.c: refactor for smaller + easier to understand code","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-05T12:33:27Z","receivedAt":"2021-05-05T12:33:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This is a prep series for my yet-to-be-sent re-roll of [1],\naka. ab/fsck-unexpected-type.\n\nJeff King had a comment in [2] saying it was odd to have an \"oi2\"\nvariable, that was because there was on an \"oi\" name already defined\nvia the new-gone open_method_decl(loose) macro. As we'll see we could\ndo without the initial \"oi\" and a few other types of indirection in\nthis interface.\n\nJunio: Could you eject ab/fsck-unexpected-type while this is being\nconsidered/cooked? I didn't really see how to address Jeff's feedback\nabout that variable name in a way that wouldn't just make something\nlike this refactoring part of an even bigger series. I think ejecting\nthe functional changes & trying to get this in first is the least\nworst approach at this point. Eventually I'll submit a re-roll of\nab/fsck-unexpected-type either based on this, or master (if it's\nlanded already).\n\n1. https://lore.kernel.org/git/cover-0.6-00000000000-20210413T093734Z-avarab@gmail.com/#t\n2. https://lore.kernel.org/git/YILZHiuUyj0mt958@coredump.intra.peff.net/\n\nÆvar Arnfjörð Bjarmason (5):\n  streaming.c: avoid forward declarations\n  streaming.c: remove enum/function/vtbl indirection\n  streaming.c: remove {open,close,read}_method_decl() macros\n  streaming.c: stop passing around \"object_info *\" to open()\n  streaming.c: move {open,close,read} from vtable to \"struct\n    git_istream\"\n\n streaming.c | 268 ++++++++++++++++++++++------------------------------\n 1 file changed, 115 insertions(+), 153 deletions(-)\n\n-- \n2.31.1.838.g7ac6e98bb53\n\n"},{"id":"423668","messageId":"patch-1.5-7f8c4b8698b-20210505T122816Z-avarab@gmail.com","threadId":"55623","inReplyTo":"cover-0.5-00000000000-20210505T122816Z-avarab@gmail.com","subject":"[PATCH 1/5] streaming.c: avoid forward declarations","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-05T12:33:28Z","receivedAt":"2021-05-05T12:33:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change code added in 46bf043807c (streaming: a new API to read from\nthe object store, 2011-05-11) to avoid forward declarations of the\nfunctions it uses. We can instead move this code to the bottom of the\nfile, and thus avoid the open_method_decl() calls.\n\nAside from the addition of the \"static helpers[...]\" comment being\nadded here, and the removal of the forward declarations this is a\nmove-only change.\n\nThe style of the added \"static helpers[...]\"  comment isn't in line\nwith our usual coding style, but is consistent with several other\ncomments used in this file, so let's use that style consistently here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n streaming.c | 171 +++++++++++++++++++++++++---------------------------\n 1 file changed, 83 insertions(+), 88 deletions(-)\n\ndiff --git a/streaming.c b/streaming.c\nindex 800f07a52cc..b0d439e47a7 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -42,20 +42,6 @@ struct stream_vtbl {\n \tssize_t read_istream_ ##name \\\n \t(struct git_istream *st, char *buf, size_t sz)\n \n-/* forward declaration */\n-static open_method_decl(incore);\n-static open_method_decl(loose);\n-static open_method_decl(pack_non_delta);\n-static struct git_istream *attach_stream_filter(struct git_istream *st,\n-\t\t\t\t\t\tstruct stream_filter *filter);\n-\n-\n-static open_istream_fn open_istream_tbl[] = {\n-\topen_istream_incore,\n-\topen_istream_loose,\n-\topen_istream_pack_non_delta,\n-};\n-\n #define FILTER_BUFFER (1024*16)\n \n struct filtered_istream {\n@@ -97,80 +83,6 @@ struct git_istream {\n \t} u;\n };\n \n-int close_istream(struct git_istream *st)\n-{\n-\tint r = st->vtbl->close(st);\n-\tfree(st);\n-\treturn r;\n-}\n-\n-ssize_t read_istream(struct git_istream *st, void *buf, size_t sz)\n-{\n-\treturn st->vtbl->read(st, buf, sz);\n-}\n-\n-static enum input_source istream_source(struct repository *r,\n-\t\t\t\t\tconst struct object_id *oid,\n-\t\t\t\t\tenum object_type *type,\n-\t\t\t\t\tstruct object_info *oi)\n-{\n-\tunsigned long size;\n-\tint status;\n-\n-\toi->typep = type;\n-\toi->sizep = &size;\n-\tstatus = oid_object_info_extended(r, oid, oi, 0);\n-\tif (status < 0)\n-\t\treturn stream_error;\n-\n-\tswitch (oi->whence) {\n-\tcase OI_LOOSE:\n-\t\treturn loose;\n-\tcase OI_PACKED:\n-\t\tif (!oi->u.packed.is_delta && big_file_threshold < size)\n-\t\t\treturn pack_non_delta;\n-\t\t/* fallthru */\n-\tdefault:\n-\t\treturn incore;\n-\t}\n-}\n-\n-struct git_istream *open_istream(struct repository *r,\n-\t\t\t\t const struct object_id *oid,\n-\t\t\t\t enum object_type *type,\n-\t\t\t\t unsigned long *size,\n-\t\t\t\t struct stream_filter *filter)\n-{\n-\tstruct git_istream *st;\n-\tstruct object_info oi = OBJECT_INFO_INIT;\n-\tconst struct object_id *real = lookup_replace_object(r, oid);\n-\tenum input_source src = istream_source(r, real, type, &oi);\n-\n-\tif (src < 0)\n-\t\treturn NULL;\n-\n-\tst = xmalloc(sizeof(*st));\n-\tif (open_istream_tbl[src](st, r, &oi, real, type)) {\n-\t\tif (open_istream_incore(st, r, &oi, real, type)) {\n-\t\t\tfree(st);\n-\t\t\treturn NULL;\n-\t\t}\n-\t}\n-\tif (filter) {\n-\t\t/* Add \"&& !is_null_stream_filter(filter)\" for performance */\n-\t\tstruct git_istream *nst = attach_stream_filter(st, filter);\n-\t\tif (!nst) {\n-\t\t\tclose_istream(st);\n-\t\t\treturn NULL;\n-\t\t}\n-\t\tst = nst;\n-\t}\n-\n-\t*size = st->size;\n-\treturn st;\n-}\n-\n-\n /*****************************************************************\n  *\n  * Common helpers\n@@ -508,11 +420,94 @@ static open_method_decl(incore)\n \treturn st->u.incore.buf ? 0 : -1;\n }\n \n+/*****************************************************************************\n+ * static helpers variables and functions for users of streaming interface\n+ *****************************************************************************/\n+\n+static open_istream_fn open_istream_tbl[] = {\n+\topen_istream_incore,\n+\topen_istream_loose,\n+\topen_istream_pack_non_delta,\n+};\n+\n+static enum input_source istream_source(struct repository *r,\n+\t\t\t\t\tconst struct object_id *oid,\n+\t\t\t\t\tenum object_type *type,\n+\t\t\t\t\tstruct object_info *oi)\n+{\n+\tunsigned long size;\n+\tint status;\n+\n+\toi->typep = type;\n+\toi->sizep = &size;\n+\tstatus = oid_object_info_extended(r, oid, oi, 0);\n+\tif (status < 0)\n+\t\treturn stream_error;\n+\n+\tswitch (oi->whence) {\n+\tcase OI_LOOSE:\n+\t\treturn loose;\n+\tcase OI_PACKED:\n+\t\tif (!oi->u.packed.is_delta && big_file_threshold < size)\n+\t\t\treturn pack_non_delta;\n+\t\t/* fallthru */\n+\tdefault:\n+\t\treturn incore;\n+\t}\n+}\n+\n \n /****************************************************************\n  * Users of streaming interface\n  ****************************************************************/\n \n+int close_istream(struct git_istream *st)\n+{\n+\tint r = st->vtbl->close(st);\n+\tfree(st);\n+\treturn r;\n+}\n+\n+ssize_t read_istream(struct git_istream *st, void *buf, size_t sz)\n+{\n+\treturn st->vtbl->read(st, buf, sz);\n+}\n+\n+struct git_istream *open_istream(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t enum object_type *type,\n+\t\t\t\t unsigned long *size,\n+\t\t\t\t struct stream_filter *filter)\n+{\n+\tstruct git_istream *st;\n+\tstruct object_info oi = OBJECT_INFO_INIT;\n+\tconst struct object_id *real = lookup_replace_object(r, oid);\n+\tenum input_source src = istream_source(r, real, type, &oi);\n+\n+\tif (src < 0)\n+\t\treturn NULL;\n+\n+\tst = xmalloc(sizeof(*st));\n+\tif (open_istream_tbl[src](st, r, &oi, real, type)) {\n+\t\tif (open_istream_incore(st, r, &oi, real, type)) {\n+\t\t\tfree(st);\n+\t\t\treturn NULL;\n+\t\t}\n+\t}\n+\tif (filter) {\n+\t\t/* Add \"&& !is_null_stream_filter(filter)\" for performance */\n+\t\tstruct git_istream *nst = attach_stream_filter(st, filter);\n+\t\tif (!nst) {\n+\t\t\tclose_istream(st);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tst = nst;\n+\t}\n+\n+\t*size = st->size;\n+\treturn st;\n+}\n+\n int stream_blob_to_fd(int fd, const struct object_id *oid, struct stream_filter *filter,\n \t\t      int can_seek)\n {\n-- \n2.31.1.838.g7ac6e98bb53\n\n"},{"id":"423669","messageId":"patch-3.5-06961ee52bb-20210505T122816Z-avarab@gmail.com","threadId":"55623","inReplyTo":"cover-0.5-00000000000-20210505T122816Z-avarab@gmail.com","subject":"[PATCH 3/5] streaming.c: remove {open,close,read}_method_decl() macros","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-05T12:33:30Z","receivedAt":"2021-05-05T12:33:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the {open,close,read}_method_decl() macros added in\n46bf043807c (streaming: a new API to read from the object store,\n2011-05-11) in favor of inlining the definition of the arguments of\nthese functions.\n\nSince we'll end up using them via the \"{open,close,read}_istream_fn\"\ntypes we don't gain anything in the way of compiler checking by using\nthese macros, and as of preceding commits we no longer need to declare\nthese argument lists twice. So declaring them at a distance just\nserves to make the code less readable.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n streaming.c | 47 ++++++++++++++++++++++-------------------------\n 1 file changed, 22 insertions(+), 25 deletions(-)\n\ndiff --git a/streaming.c b/streaming.c\nindex 628369519b9..7e039df388b 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -21,20 +21,6 @@ struct stream_vtbl {\n \tread_istream_fn read;\n };\n \n-#define open_method_decl(name) \\\n-\tint open_istream_ ##name \\\n-\t(struct git_istream *st, struct repository *r, \\\n-\t struct object_info *oi, const struct object_id *oid, \\\n-\t enum object_type *type)\n-\n-#define close_method_decl(name) \\\n-\tint close_istream_ ##name \\\n-\t(struct git_istream *st)\n-\n-#define read_method_decl(name) \\\n-\tssize_t read_istream_ ##name \\\n-\t(struct git_istream *st, char *buf, size_t sz)\n-\n #define FILTER_BUFFER (1024*16)\n \n struct filtered_istream {\n@@ -95,13 +81,14 @@ static void close_deflated_stream(struct git_istream *st)\n  *\n  *****************************************************************/\n \n-static close_method_decl(filtered)\n+static int close_istream_filtered(struct git_istream *st)\n {\n \tfree_stream_filter(st->u.filtered.filter);\n \treturn close_istream(st->u.filtered.upstream);\n }\n \n-static read_method_decl(filtered)\n+static ssize_t read_istream_filtered(struct git_istream *st, char *buf,\n+\t\t\t\t     size_t sz)\n {\n \tstruct filtered_istream *fs = &(st->u.filtered);\n \tsize_t filled = 0;\n@@ -187,7 +174,7 @@ static struct git_istream *attach_stream_filter(struct git_istream *st,\n  *\n  *****************************************************************/\n \n-static read_method_decl(loose)\n+static ssize_t read_istream_loose(struct git_istream *st, char *buf, size_t sz)\n {\n \tsize_t total_read = 0;\n \n@@ -232,7 +219,7 @@ static read_method_decl(loose)\n \treturn total_read;\n }\n \n-static close_method_decl(loose)\n+static int close_istream_loose(struct git_istream *st)\n {\n \tclose_deflated_stream(st);\n \tmunmap(st->u.loose.mapped, st->u.loose.mapsize);\n@@ -244,7 +231,10 @@ static struct stream_vtbl loose_vtbl = {\n \tread_istream_loose,\n };\n \n-static open_method_decl(loose)\n+static int open_istream_loose(struct git_istream *st, struct repository *r,\n+\t\t\t      struct object_info *oi,\n+\t\t\t      const struct object_id *oid,\n+\t\t\t      enum object_type *type)\n {\n \tst->u.loose.mapped = map_loose_object(r, oid, &st->u.loose.mapsize);\n \tif (!st->u.loose.mapped)\n@@ -275,7 +265,8 @@ static open_method_decl(loose)\n  *\n  *****************************************************************/\n \n-static read_method_decl(pack_non_delta)\n+static ssize_t read_istream_pack_non_delta(struct git_istream *st, char *buf,\n+\t\t\t\t\t   size_t sz)\n {\n \tsize_t total_read = 0;\n \n@@ -333,7 +324,7 @@ static read_method_decl(pack_non_delta)\n \treturn total_read;\n }\n \n-static close_method_decl(pack_non_delta)\n+static int close_istream_pack_non_delta(struct git_istream *st)\n {\n \tclose_deflated_stream(st);\n \treturn 0;\n@@ -344,7 +335,11 @@ static struct stream_vtbl pack_non_delta_vtbl = {\n \tread_istream_pack_non_delta,\n };\n \n-static open_method_decl(pack_non_delta)\n+static int open_istream_pack_non_delta(struct git_istream *st,\n+\t\t\t\t       struct repository *r,\n+\t\t\t\t       struct object_info *oi,\n+\t\t\t\t       const struct object_id *oid,\n+\t\t\t\t       enum object_type *type)\n {\n \tstruct pack_window *window;\n \tenum object_type in_pack_type;\n@@ -379,13 +374,13 @@ static open_method_decl(pack_non_delta)\n  *\n  *****************************************************************/\n \n-static close_method_decl(incore)\n+static int close_istream_incore(struct git_istream *st)\n {\n \tfree(st->u.incore.buf);\n \treturn 0;\n }\n \n-static read_method_decl(incore)\n+static ssize_t read_istream_incore(struct git_istream *st, char *buf, size_t sz)\n {\n \tsize_t read_size = sz;\n \tsize_t remainder = st->size - st->u.incore.read_ptr;\n@@ -404,7 +399,9 @@ static struct stream_vtbl incore_vtbl = {\n \tread_istream_incore,\n };\n \n-static open_method_decl(incore)\n+static int open_istream_incore(struct git_istream *st, struct repository *r,\n+\t\t\t   struct object_info *oi, const struct object_id *oid,\n+\t\t\t   enum object_type *type)\n {\n \tst->u.incore.buf = read_object_file_extended(r, oid, type, &st->size, 0);\n \tst->u.incore.read_ptr = 0;\n-- \n2.31.1.838.g7ac6e98bb53\n\n"},{"id":"423670","messageId":"patch-2.5-13061f01212-20210505T122816Z-avarab@gmail.com","threadId":"55623","inReplyTo":"cover-0.5-00000000000-20210505T122816Z-avarab@gmail.com","subject":"[PATCH 2/5] streaming.c: remove enum/function/vtbl indirection","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-05T12:33:29Z","receivedAt":"2021-05-05T12:33:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the indirection of discovering a function pointer to use via an\nenum and virtual table. This refactors code added in\n46bf043807c (streaming: a new API to read from the object store,\n2011-05-11).\n\nWe can instead simply return an \"open_istream_fn\" for use from the\n\"istream_source()\" selector function directly. This allows us to get\nrid of the \"incore\", \"loose\" and \"pack_non_delta\" enum\nvariables. We'll return the functions instead.\n\nThe \"stream_error\" variable in that enum can likewise go in favor of\nreturning NULL, which is what the open_istream() was doing when it got\nthat value anyway.\n\nWe can thus remove the entire enum, and the \"open_istream_tbl\" virtual\ntable that (indirectly) referenced it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n streaming.c | 36 +++++++++++-------------------------\n 1 file changed, 11 insertions(+), 25 deletions(-)\n\ndiff --git a/streaming.c b/streaming.c\nindex b0d439e47a7..628369519b9 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -8,13 +8,6 @@\n #include \"replace-object.h\"\n #include \"packfile.h\"\n \n-enum input_source {\n-\tstream_error = -1,\n-\tincore = 0,\n-\tloose = 1,\n-\tpack_non_delta = 2\n-};\n-\n typedef int (*open_istream_fn)(struct git_istream *,\n \t\t\t       struct repository *,\n \t\t\t       struct object_info *,\n@@ -424,16 +417,10 @@ static open_method_decl(incore)\n  * static helpers variables and functions for users of streaming interface\n  *****************************************************************************/\n \n-static open_istream_fn open_istream_tbl[] = {\n-\topen_istream_incore,\n-\topen_istream_loose,\n-\topen_istream_pack_non_delta,\n-};\n-\n-static enum input_source istream_source(struct repository *r,\n-\t\t\t\t\tconst struct object_id *oid,\n-\t\t\t\t\tenum object_type *type,\n-\t\t\t\t\tstruct object_info *oi)\n+static open_istream_fn istream_source(struct repository *r,\n+\t\t\t\t      const struct object_id *oid,\n+\t\t\t\t      enum object_type *type,\n+\t\t\t\t      struct object_info *oi)\n {\n \tunsigned long size;\n \tint status;\n@@ -442,21 +429,20 @@ static enum input_source istream_source(struct repository *r,\n \toi->sizep = &size;\n \tstatus = oid_object_info_extended(r, oid, oi, 0);\n \tif (status < 0)\n-\t\treturn stream_error;\n+\t\treturn NULL;\n \n \tswitch (oi->whence) {\n \tcase OI_LOOSE:\n-\t\treturn loose;\n+\t\treturn open_istream_loose;\n \tcase OI_PACKED:\n \t\tif (!oi->u.packed.is_delta && big_file_threshold < size)\n-\t\t\treturn pack_non_delta;\n+\t\t\treturn open_istream_pack_non_delta;\n \t\t/* fallthru */\n \tdefault:\n-\t\treturn incore;\n+\t\treturn open_istream_incore;\n \t}\n }\n \n-\n /****************************************************************\n  * Users of streaming interface\n  ****************************************************************/\n@@ -482,13 +468,13 @@ struct git_istream *open_istream(struct repository *r,\n \tstruct git_istream *st;\n \tstruct object_info oi = OBJECT_INFO_INIT;\n \tconst struct object_id *real = lookup_replace_object(r, oid);\n-\tenum input_source src = istream_source(r, real, type, &oi);\n+\topen_istream_fn open_fn = istream_source(r, real, type, &oi);\n \n-\tif (src < 0)\n+\tif (!open_fn)\n \t\treturn NULL;\n \n \tst = xmalloc(sizeof(*st));\n-\tif (open_istream_tbl[src](st, r, &oi, real, type)) {\n+\tif (open_fn(st, r, &oi, real, type)) {\n \t\tif (open_istream_incore(st, r, &oi, real, type)) {\n \t\t\tfree(st);\n \t\t\treturn NULL;\n-- \n2.31.1.838.g7ac6e98bb53\n\n"},{"id":"423671","messageId":"patch-5.5-52606cd72ac-20210505T122816Z-avarab@gmail.com","threadId":"55623","inReplyTo":"cover-0.5-00000000000-20210505T122816Z-avarab@gmail.com","subject":"[PATCH 5/5] streaming.c: move {open,close,read} from vtable to \"struct git_istream\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-05T12:33:32Z","receivedAt":"2021-05-05T12:33:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the definition of the structure around the open/close/read\nfunctions introduced in 46bf043807c (streaming: a new API to read from\nthe object store, 2011-05-11) to instead populate \"close\" and \"read\"\nmembers in the \"struct git_istream\".\n\nThis gets us rid of an extra pointer deference, and I think makes more\nsense. The \"close\" and \"read\" functions are the primary interface to\nthe stream itself.\n\nLet's also populate a \"open\" callback in the same struct. That's now\nused by open_istream() after istream_source() decides what \"open\"\nfunction should be used. This isn't needed to get rid of the\n\"stream_vtbl\" variables, but makes sense for consistency.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n streaming.c | 72 +++++++++++++++++++++--------------------------------\n 1 file changed, 29 insertions(+), 43 deletions(-)\n\ndiff --git a/streaming.c b/streaming.c\nindex f6059ee828e..5f480ad50c4 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -15,11 +15,6 @@ typedef int (*open_istream_fn)(struct git_istream *,\n typedef int (*close_istream_fn)(struct git_istream *);\n typedef ssize_t (*read_istream_fn)(struct git_istream *, char *, size_t);\n \n-struct stream_vtbl {\n-\tclose_istream_fn close;\n-\tread_istream_fn read;\n-};\n-\n #define FILTER_BUFFER (1024*16)\n \n struct filtered_istream {\n@@ -33,7 +28,10 @@ struct filtered_istream {\n };\n \n struct git_istream {\n-\tconst struct stream_vtbl *vtbl;\n+\topen_istream_fn open;\n+\tclose_istream_fn close;\n+\tread_istream_fn read;\n+\n \tunsigned long size; /* inflated size of full object */\n \tgit_zstream z;\n \tenum { z_unused, z_used, z_done, z_error } z_state;\n@@ -146,18 +144,14 @@ static ssize_t read_istream_filtered(struct git_istream *st, char *buf,\n \treturn filled;\n }\n \n-static struct stream_vtbl filtered_vtbl = {\n-\tclose_istream_filtered,\n-\tread_istream_filtered,\n-};\n-\n static struct git_istream *attach_stream_filter(struct git_istream *st,\n \t\t\t\t\t\tstruct stream_filter *filter)\n {\n \tstruct git_istream *ifs = xmalloc(sizeof(*ifs));\n \tstruct filtered_istream *fs = &(ifs->u.filtered);\n \n-\tifs->vtbl = &filtered_vtbl;\n+\tifs->close = close_istream_filtered;\n+\tifs->read = read_istream_filtered;\n \tfs->upstream = st;\n \tfs->filter = filter;\n \tfs->i_end = fs->i_ptr = 0;\n@@ -225,11 +219,6 @@ static int close_istream_loose(struct git_istream *st)\n \treturn 0;\n }\n \n-static struct stream_vtbl loose_vtbl = {\n-\tclose_istream_loose,\n-\tread_istream_loose,\n-};\n-\n static int open_istream_loose(struct git_istream *st, struct repository *r,\n \t\t\t      const struct object_id *oid,\n \t\t\t      enum object_type *type)\n@@ -251,8 +240,9 @@ static int open_istream_loose(struct git_istream *st, struct repository *r,\n \tst->u.loose.hdr_used = strlen(st->u.loose.hdr) + 1;\n \tst->u.loose.hdr_avail = st->z.total_out;\n \tst->z_state = z_used;\n+\tst->close = close_istream_loose;\n+\tst->read = read_istream_loose;\n \n-\tst->vtbl = &loose_vtbl;\n \treturn 0;\n }\n \n@@ -328,11 +318,6 @@ static int close_istream_pack_non_delta(struct git_istream *st)\n \treturn 0;\n }\n \n-static struct stream_vtbl pack_non_delta_vtbl = {\n-\tclose_istream_pack_non_delta,\n-\tread_istream_pack_non_delta,\n-};\n-\n static int open_istream_pack_non_delta(struct git_istream *st,\n \t\t\t\t       struct repository *r,\n \t\t\t\t       const struct object_id *oid,\n@@ -358,7 +343,9 @@ static int open_istream_pack_non_delta(struct git_istream *st,\n \t\tbreak;\n \t}\n \tst->z_state = z_unused;\n-\tst->vtbl = &pack_non_delta_vtbl;\n+\tst->close = close_istream_pack_non_delta;\n+\tst->read = read_istream_pack_non_delta;\n+\n \treturn 0;\n }\n \n@@ -389,17 +376,13 @@ static ssize_t read_istream_incore(struct git_istream *st, char *buf, size_t sz)\n \treturn read_size;\n }\n \n-static struct stream_vtbl incore_vtbl = {\n-\tclose_istream_incore,\n-\tread_istream_incore,\n-};\n-\n static int open_istream_incore(struct git_istream *st, struct repository *r,\n \t\t\t       const struct object_id *oid, enum object_type *type)\n {\n \tst->u.incore.buf = read_object_file_extended(r, oid, type, &st->size, 0);\n \tst->u.incore.read_ptr = 0;\n-\tst->vtbl = &incore_vtbl;\n+\tst->close = close_istream_incore;\n+\tst->read = read_istream_incore;\n \n \treturn st->u.incore.buf ? 0 : -1;\n }\n@@ -408,10 +391,10 @@ static int open_istream_incore(struct git_istream *st, struct repository *r,\n  * static helpers variables and functions for users of streaming interface\n  *****************************************************************************/\n \n-static open_istream_fn istream_source(struct git_istream *st,\n-\t\t\t\t      struct repository *r,\n-\t\t\t\t      const struct object_id *oid,\n-\t\t\t\t      enum object_type *type)\n+static int istream_source(struct git_istream *st,\n+\t\t\t  struct repository *r,\n+\t\t\t  const struct object_id *oid,\n+\t\t\t  enum object_type *type)\n {\n \tunsigned long size;\n \tint status;\n@@ -421,20 +404,23 @@ static open_istream_fn istream_source(struct git_istream *st,\n \toi.sizep = &size;\n \tstatus = oid_object_info_extended(r, oid, &oi, 0);\n \tif (status < 0)\n-\t\treturn NULL;\n+\t\treturn status;\n \n \tswitch (oi.whence) {\n \tcase OI_LOOSE:\n-\t\treturn open_istream_loose;\n+\t\tst->open = open_istream_loose;\n+\t\treturn 0;\n \tcase OI_PACKED:\n \t\tif (!oi.u.packed.is_delta && big_file_threshold < size) {\n \t\t\tst->u.in_pack.pack = oi.u.packed.pack;\n \t\t\tst->u.in_pack.pos = oi.u.packed.offset;\n-\t\t\treturn open_istream_pack_non_delta;\n+\t\t\tst->open = open_istream_pack_non_delta;\n+\t\t\treturn 0;\n \t\t}\n \t\t/* fallthru */\n \tdefault:\n-\t\treturn open_istream_incore;\n+\t\tst->open = open_istream_incore;\n+\t\treturn 0;\n \t}\n }\n \n@@ -444,14 +430,14 @@ static open_istream_fn istream_source(struct git_istream *st,\n \n int close_istream(struct git_istream *st)\n {\n-\tint r = st->vtbl->close(st);\n+\tint r = st->close(st);\n \tfree(st);\n \treturn r;\n }\n \n ssize_t read_istream(struct git_istream *st, void *buf, size_t sz)\n {\n-\treturn st->vtbl->read(st, buf, sz);\n+\treturn st->read(st, buf, sz);\n }\n \n struct git_istream *open_istream(struct repository *r,\n@@ -462,14 +448,14 @@ struct git_istream *open_istream(struct repository *r,\n {\n \tstruct git_istream *st = xmalloc(sizeof(*st));\n \tconst struct object_id *real = lookup_replace_object(r, oid);\n-\topen_istream_fn open_fn = istream_source(st, r, real, type);\n+\tint ret = istream_source(st, r, real, type);\n \n-\tif (!open_fn) {\n+\tif (ret) {\n \t\tfree(st);\n \t\treturn NULL;\n \t}\n \n-\tif (open_fn(st, r, real, type)) {\n+\tif (st->open(st, r, real, type)) {\n \t\tif (open_istream_incore(st, r, real, type)) {\n \t\t\tfree(st);\n \t\t\treturn NULL;\n-- \n2.31.1.838.g7ac6e98bb53\n\n"},{"id":"423672","messageId":"patch-4.5-44e79d6cb66-20210505T122816Z-avarab@gmail.com","threadId":"55623","inReplyTo":"cover-0.5-00000000000-20210505T122816Z-avarab@gmail.com","subject":"[PATCH 4/5] streaming.c: stop passing around \"object_info *\" to open()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-05T12:33:31Z","receivedAt":"2021-05-05T12:33:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the streaming interface to stop passing around the \"struct\nobject_info\" the open() functions.\n\nAs seen in 7ef2d9a2604 (streaming: read non-delta incrementally from a\npack, 2011-05-13) which introduced the \"st->u.in_pack\" assignments\nbeing changed here only the open_istream_pack_non_delta() path need\nthese.\n\nSo let's instead do this when preparing the selected callback in the\nistream_source() function. This might also allow the compiler to\nreduce the lifetime of the \"oi\" variable, as we've moved it from\n\"git_istream()\" to \"istream_source()\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n streaming.c | 42 ++++++++++++++++++++----------------------\n 1 file changed, 20 insertions(+), 22 deletions(-)\n\ndiff --git a/streaming.c b/streaming.c\nindex 7e039df388b..f6059ee828e 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -10,7 +10,6 @@\n \n typedef int (*open_istream_fn)(struct git_istream *,\n \t\t\t       struct repository *,\n-\t\t\t       struct object_info *,\n \t\t\t       const struct object_id *,\n \t\t\t       enum object_type *);\n typedef int (*close_istream_fn)(struct git_istream *);\n@@ -232,7 +231,6 @@ static struct stream_vtbl loose_vtbl = {\n };\n \n static int open_istream_loose(struct git_istream *st, struct repository *r,\n-\t\t\t      struct object_info *oi,\n \t\t\t      const struct object_id *oid,\n \t\t\t      enum object_type *type)\n {\n@@ -337,15 +335,12 @@ static struct stream_vtbl pack_non_delta_vtbl = {\n \n static int open_istream_pack_non_delta(struct git_istream *st,\n \t\t\t\t       struct repository *r,\n-\t\t\t\t       struct object_info *oi,\n \t\t\t\t       const struct object_id *oid,\n \t\t\t\t       enum object_type *type)\n {\n \tstruct pack_window *window;\n \tenum object_type in_pack_type;\n \n-\tst->u.in_pack.pack = oi->u.packed.pack;\n-\tst->u.in_pack.pos = oi->u.packed.offset;\n \twindow = NULL;\n \n \tin_pack_type = unpack_object_header(st->u.in_pack.pack,\n@@ -400,8 +395,7 @@ static struct stream_vtbl incore_vtbl = {\n };\n \n static int open_istream_incore(struct git_istream *st, struct repository *r,\n-\t\t\t   struct object_info *oi, const struct object_id *oid,\n-\t\t\t   enum object_type *type)\n+\t\t\t       const struct object_id *oid, enum object_type *type)\n {\n \tst->u.incore.buf = read_object_file_extended(r, oid, type, &st->size, 0);\n \tst->u.incore.read_ptr = 0;\n@@ -414,26 +408,30 @@ static int open_istream_incore(struct git_istream *st, struct repository *r,\n  * static helpers variables and functions for users of streaming interface\n  *****************************************************************************/\n \n-static open_istream_fn istream_source(struct repository *r,\n+static open_istream_fn istream_source(struct git_istream *st,\n+\t\t\t\t      struct repository *r,\n \t\t\t\t      const struct object_id *oid,\n-\t\t\t\t      enum object_type *type,\n-\t\t\t\t      struct object_info *oi)\n+\t\t\t\t      enum object_type *type)\n {\n \tunsigned long size;\n \tint status;\n+\tstruct object_info oi = OBJECT_INFO_INIT;\n \n-\toi->typep = type;\n-\toi->sizep = &size;\n-\tstatus = oid_object_info_extended(r, oid, oi, 0);\n+\toi.typep = type;\n+\toi.sizep = &size;\n+\tstatus = oid_object_info_extended(r, oid, &oi, 0);\n \tif (status < 0)\n \t\treturn NULL;\n \n-\tswitch (oi->whence) {\n+\tswitch (oi.whence) {\n \tcase OI_LOOSE:\n \t\treturn open_istream_loose;\n \tcase OI_PACKED:\n-\t\tif (!oi->u.packed.is_delta && big_file_threshold < size)\n+\t\tif (!oi.u.packed.is_delta && big_file_threshold < size) {\n+\t\t\tst->u.in_pack.pack = oi.u.packed.pack;\n+\t\t\tst->u.in_pack.pos = oi.u.packed.offset;\n \t\t\treturn open_istream_pack_non_delta;\n+\t\t}\n \t\t/* fallthru */\n \tdefault:\n \t\treturn open_istream_incore;\n@@ -462,17 +460,17 @@ struct git_istream *open_istream(struct repository *r,\n \t\t\t\t unsigned long *size,\n \t\t\t\t struct stream_filter *filter)\n {\n-\tstruct git_istream *st;\n-\tstruct object_info oi = OBJECT_INFO_INIT;\n+\tstruct git_istream *st = xmalloc(sizeof(*st));\n \tconst struct object_id *real = lookup_replace_object(r, oid);\n-\topen_istream_fn open_fn = istream_source(r, real, type, &oi);\n+\topen_istream_fn open_fn = istream_source(st, r, real, type);\n \n-\tif (!open_fn)\n+\tif (!open_fn) {\n+\t\tfree(st);\n \t\treturn NULL;\n+\t}\n \n-\tst = xmalloc(sizeof(*st));\n-\tif (open_fn(st, r, &oi, real, type)) {\n-\t\tif (open_istream_incore(st, r, &oi, real, type)) {\n+\tif (open_fn(st, r, real, type)) {\n+\t\tif (open_istream_incore(st, r, real, type)) {\n \t\t\tfree(st);\n \t\t\treturn NULL;\n \t\t}\n-- \n2.31.1.838.g7ac6e98bb53\n\n"},{"id":"423674","messageId":"YJKgsfJXMsR47CkW@coredump.intra.peff.net","threadId":"55623","inReplyTo":"patch-2.5-13061f01212-20210505T122816Z-avarab@gmail.com","subject":"Re: [PATCH 2/5] streaming.c: remove enum/function/vtbl indirection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-05T13:42:09Z","receivedAt":"2021-05-05T13:42:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 05, 2021 at 02:33:29PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Remove the indirection of discovering a function pointer to use via an\n> enum and virtual table. This refactors code added in\n> 46bf043807c (streaming: a new API to read from the object store,\n> 2011-05-11).\n> \n> We can instead simply return an \"open_istream_fn\" for use from the\n> \"istream_source()\" selector function directly. This allows us to get\n> rid of the \"incore\", \"loose\" and \"pack_non_delta\" enum\n> variables. We'll return the functions instead.\n> \n> The \"stream_error\" variable in that enum can likewise go in favor of\n> returning NULL, which is what the open_istream() was doing when it got\n> that value anyway.\n> \n> We can thus remove the entire enum, and the \"open_istream_tbl\" virtual\n> table that (indirectly) referenced it.\n\nYeah, I think this is simpler. The value of the vtable was that we might\nhave added more functions to it, but we haven't done so over the course\nof the last 10 years. And I have trouble imagining for what purpose we\nwould. So it seems like unnecessary complexity.\n\n-Peff\n"},{"id":"423676","messageId":"YJKhKdWlzVjHTKjl@coredump.intra.peff.net","threadId":"55623","inReplyTo":"patch-3.5-06961ee52bb-20210505T122816Z-avarab@gmail.com","subject":"Re: [PATCH 3/5] streaming.c: remove {open,close,read}_method_decl() macros","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-05T13:44:09Z","receivedAt":"2021-05-05T13:44:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 05, 2021 at 02:33:30PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Remove the {open,close,read}_method_decl() macros added in\n> 46bf043807c (streaming: a new API to read from the object store,\n> 2011-05-11) in favor of inlining the definition of the arguments of\n> these functions.\n> \n> Since we'll end up using them via the \"{open,close,read}_istream_fn\"\n> types we don't gain anything in the way of compiler checking by using\n> these macros, and as of preceding commits we no longer need to declare\n> these argument lists twice. So declaring them at a distance just\n> serves to make the code less readable.\n\nHeh. I have a very similar patch pending. In addition to readability, my\nreasons there are:\n\n  - you can't find the functions with ctags, etc, when they're hidden\n    behind macros\n\n  - you can't annotate the function interfaces to avoid\n    -Wunused-parameter warnings. :)\n\nSo I am very much in favor of this (and patch 1 is nice here, too,\nbecause it skips an extra time you'd have to repeat the interface in the\nforward declaration).\n\n-Peff\n"},{"id":"423677","messageId":"YJKigIfSXWavM0QN@coredump.intra.peff.net","threadId":"55623","inReplyTo":"patch-4.5-44e79d6cb66-20210505T122816Z-avarab@gmail.com","subject":"Re: [PATCH 4/5] streaming.c: stop passing around \"object_info *\" to open()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-05T13:49:52Z","receivedAt":"2021-05-05T13:49:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 05, 2021 at 02:33:31PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Change the streaming interface to stop passing around the \"struct\n> object_info\" the open() functions.\n> \n> As seen in 7ef2d9a2604 (streaming: read non-delta incrementally from a\n> pack, 2011-05-13) which introduced the \"st->u.in_pack\" assignments\n> being changed here only the open_istream_pack_non_delta() path need\n> these.\n> \n> So let's instead do this when preparing the selected callback in the\n> istream_source() function. This might also allow the compiler to\n> reduce the lifetime of the \"oi\" variable, as we've moved it from\n> \"git_istream()\" to \"istream_source()\".\n\nOK. This blurs the lines a bit between what the generic istream_source()\nis responsible for, versus the type-specific helpers. I think that works\na bit against the original design, which was trying to be very abstract\nand polymorphic. But that doesn't seem to have bought us much. Just\nconsidering the whole set of streaming code as a module with helper\nfunctions is a bit simpler and more usual for our codebase.\n\nSo I'm OK with it unless Junio (as the original author) has strong\nobjections.\n\n-Peff\n"},{"id":"423680","messageId":"YJKj1bVEfWOWrvhs@coredump.intra.peff.net","threadId":"55623","inReplyTo":"patch-5.5-52606cd72ac-20210505T122816Z-avarab@gmail.com","subject":"Re: [PATCH 5/5] streaming.c: move {open,close,read} from vtable to \"struct git_istream\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-05T13:55:33Z","receivedAt":"2021-05-05T13:55:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 05, 2021 at 02:33:32PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Move the definition of the structure around the open/close/read\n> functions introduced in 46bf043807c (streaming: a new API to read from\n> the object store, 2011-05-11) to instead populate \"close\" and \"read\"\n> members in the \"struct git_istream\".\n> \n> This gets us rid of an extra pointer deference, and I think makes more\n> sense. The \"close\" and \"read\" functions are the primary interface to\n> the stream itself.\n> \n> Let's also populate a \"open\" callback in the same struct. That's now\n> used by open_istream() after istream_source() decides what \"open\"\n> function should be used. This isn't needed to get rid of the\n> \"stream_vtbl\" variables, but makes sense for consistency.\n\nYeah, I think if we are going to get rid of the open vtable in an earlier\npatch, it makes sense to go all the way here and make things consistent.\n\nIf we had a bunch of these objects, having the layer of indirection to\nthe vtable would save us some per-object memory. But we really only ever\nhave one of these open at a time.\n\n-Peff\n"},{"id":"423682","messageId":"YJKkOLvSCSgFRs7r@coredump.intra.peff.net","threadId":"55623","inReplyTo":"cover-0.5-00000000000-20210505T122816Z-avarab@gmail.com","subject":"Re: [PATCH 0/5] streaming.c: refactor for smaller + easier to understand code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-05T13:57:12Z","receivedAt":"2021-05-05T13:57:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 05, 2021 at 02:33:27PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> This is a prep series for my yet-to-be-sent re-roll of [1],\n> aka. ab/fsck-unexpected-type.\n> \n> Jeff King had a comment in [2] saying it was odd to have an \"oi2\"\n> variable, that was because there was on an \"oi\" name already defined\n> via the new-gone open_method_decl(loose) macro. As we'll see we could\n> do without the initial \"oi\" and a few other types of indirection in\n> this interface.\n> \n> Junio: Could you eject ab/fsck-unexpected-type while this is being\n> considered/cooked? I didn't really see how to address Jeff's feedback\n> about that variable name in a way that wouldn't just make something\n> like this refactoring part of an even bigger series. I think ejecting\n> the functional changes & trying to get this in first is the least\n> worst approach at this point. Eventually I'll submit a re-roll of\n> ab/fsck-unexpected-type either based on this, or master (if it's\n> landed already).\n\nI think my feedback was really just: if that patch didn't refactor\nparse_loose_header() to get rid of the \"simple\" and extended versions,\nthen you wouldn't have to touch streaming.c at all. It could just\ncontinue to use the simple version.\n\nThat said, I don't mind the cleanups here, especially getting rid of the\nmacros.\n\n-Peff\n"},{"id":"423721","messageId":"xmqqmtt83ehm.fsf@gitster.g","threadId":"55623","inReplyTo":"YJKgsfJXMsR47CkW@coredump.intra.peff.net","subject":"Re: [PATCH 2/5] streaming.c: remove enum/function/vtbl indirection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-06T00:14:29Z","receivedAt":"2021-05-06T00:14:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> We can thus remove the entire enum, and the \"open_istream_tbl\" virtual\n>> table that (indirectly) referenced it.\n>\n> Yeah, I think this is simpler. The value of the vtable was that we might\n> have added more functions to it, but we haven't done so over the course\n> of the last 10 years. And I have trouble imagining for what purpose we\n> would. So it seems like unnecessary complexity.\n\nThe code hasn't changed all that much since it was written, so\nanother equally plausible way forward is not to touch it at all ;-),\nbut I am fine with this simplification, too.\n\n"}]}