{"thread":{"id":"59043","subject":"[PATCH 0/5] cleaning up read_object() family of functions","startedAt":"2023-01-07T13:48:35Z","lastAt":"2023-01-13T17:49:36Z","messageCount":20,"participants":["Jeff King","Derrick Stolee","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"469875","messageId":"Y7l4LsEQcDT9HZ21@coredump.intra.peff.net","threadId":"59043","inReplyTo":null,"subject":"[PATCH 0/5] cleaning up read_object() family of functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-07T13:48:30Z","receivedAt":"2023-01-07T13:48:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I often get confused about the difference between:\n\n  - read_object()\n  - read_object_file();\n  - read_object_file_extended();\n  - repo_read_object_file();\n\nSince Jonathan's recent cleanups from 9e59b38c88 (object-file: emit\ncorruption errors when detected, 2022-12-14), these are mostly thin\nwrappers around each other and around oid_object_info_extended().\n\nThis series shuffles things around a little more so that we are down to\njust read_object_file() and repo_read_object_file(). And the\nrelationship there is pretty easy (and long-term we'd eventually merge\nthem once everyone has a repository object).\n\nIt is a net reduction in lines, even though some of the callers end up a\nlittle longer (because they have to stuff pointers into an object_info\nstruct). If that's too distasteful, the middle ground is to have a\nhelper like:\n\n  void *foo(struct repository *r, const struct object_id *oid,\n            enum object_type *type, unsigned long *size,\n\t    unsigned flags)\n  {\n\tstruct object_info oi = OBJECT_INFO_INIT;\n\tvoid *content;\n\n\toi.typep = type;\n\toi.sizep = size;\n\toi.contentp = ret;\n\n\tif (oid_object_info_extended(r, oid, &oi, flags) < 0)\n\t\treturn NULL;\n\treturn content;\n  }\n\nwhich is basically the same as read_object(), but makes it clear that\nyou can pass OBJECT_INFO flags. The trouble is that I could not come up\nwith a name for it that was not confusing. ;) So just having most places\ncall oid_object_info_extended() directly seemed better. It would be nice\nif that function had a shorter name, too, but I left that for another\nday.\n\n  [1/5]: object-file: inline calls to read_object()\n  [2/5]: streaming: inline call to read_object_file_extended()\n  [3/5]: read_object_file_extended(): drop lookup_replace option\n  [4/5]: repo_read_object_file(): stop wrapping read_object_file_extended()\n  [5/5]: packfile: inline custom read_object()\n\n object-file.c  | 52 ++++++++++++++++++--------------------------------\n object-store.h | 18 +++++------------\n packfile.c     | 26 +++++++++----------------\n streaming.c    | 11 ++++++++---\n 4 files changed, 41 insertions(+), 66 deletions(-)\n\n-Peff\n"},{"id":"469876","messageId":"Y7l4RznPhTr+O8ZP@coredump.intra.peff.net","threadId":"59043","inReplyTo":"Y7l4LsEQcDT9HZ21@coredump.intra.peff.net","subject":"[PATCH 1/5] object-file: inline calls to read_object()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-07T13:48:55Z","receivedAt":"2023-01-07T13:49:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since read_object() is these days just a thin wrapper around\noid_object_info_extended(), and since it only has two callers, let's\njust inline those calls. This has a few positive outcomes:\n\n  - it's a net reduction in source code lines\n\n  - even though the callers end up with a few extra lines, they're now\n    more flexible and can use object_info flags directly. So no more\n    need to convert die_if_corrupt between parameter/flag, and we can\n    ask for lookup replacement with a flag rather than doing it\n    ourselves.\n\n  - there's one fewer function in an already crowded namespace (e.g.,\n    the difference between read_object() and read_object_file() was not\n    immediately obvious; now we only have one of them).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n object-file.c  | 45 +++++++++++++++++----------------------------\n object-store.h |  2 +-\n 2 files changed, 18 insertions(+), 29 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 80a0cd3b35..ed1babbac2 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1671,23 +1671,6 @@ int oid_object_info(struct repository *r,\n \treturn type;\n }\n \n-static void *read_object(struct repository *r,\n-\t\t\t const struct object_id *oid, enum object_type *type,\n-\t\t\t unsigned long *size,\n-\t\t\t int die_if_corrupt)\n-{\n-\tstruct object_info oi = OBJECT_INFO_INIT;\n-\tvoid *content;\n-\toi.typep = type;\n-\toi.sizep = size;\n-\toi.contentp = &content;\n-\n-\tif (oid_object_info_extended(r, oid, &oi, die_if_corrupt\n-\t\t\t\t     ? OBJECT_INFO_DIE_IF_CORRUPT : 0) < 0)\n-\t\treturn NULL;\n-\treturn content;\n-}\n-\n int pretend_object_file(void *buf, unsigned long len, enum object_type type,\n \t\t\tstruct object_id *oid)\n {\n@@ -1709,25 +1692,28 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type,\n \n /*\n  * This function dies on corrupt objects; the callers who want to\n- * deal with them should arrange to call read_object() and give error\n- * messages themselves.\n+ * deal with them should arrange to call oid_object_info_extended() and give\n+ * error messages themselves.\n  */\n void *read_object_file_extended(struct repository *r,\n \t\t\t\tconst struct object_id *oid,\n \t\t\t\tenum object_type *type,\n \t\t\t\tunsigned long *size,\n \t\t\t\tint lookup_replace)\n {\n+\tstruct object_info oi = OBJECT_INFO_INIT;\n+\tunsigned flags = OBJECT_INFO_DIE_IF_CORRUPT;\n \tvoid *data;\n-\tconst struct object_id *repl = lookup_replace ?\n-\t\tlookup_replace_object(r, oid) : oid;\n \n-\terrno = 0;\n-\tdata = read_object(r, repl, type, size, 1);\n-\tif (data)\n-\t\treturn data;\n+\toi.typep = type;\n+\toi.sizep = size;\n+\toi.contentp = &data;\n+\tif (lookup_replace)\n+\t\tflags |= OBJECT_INFO_LOOKUP_REPLACE;\n+\tif (oid_object_info_extended(r, oid, &oi, flags))\n+\t    return NULL;\n \n-\treturn NULL;\n+\treturn data;\n }\n \n void *read_object_with_reference(struct repository *r,\n@@ -2255,15 +2241,18 @@ int force_object_loose(const struct object_id *oid, time_t mtime)\n {\n \tvoid *buf;\n \tunsigned long len;\n+\tstruct object_info oi = OBJECT_INFO_INIT;\n \tenum object_type type;\n \tchar hdr[MAX_HEADER_LEN];\n \tint hdrlen;\n \tint ret;\n \n \tif (has_loose_object(oid))\n \t\treturn 0;\n-\tbuf = read_object(the_repository, oid, &type, &len, 0);\n-\tif (!buf)\n+\toi.typep = &type;\n+\toi.sizep = &len;\n+\toi.contentp = &buf;\n+\tif (oid_object_info_extended(the_repository, oid, &oi, 0))\n \t\treturn error(_(\"cannot read object for %s\"), oid_to_hex(oid));\n \thdrlen = format_object_header(hdr, sizeof(hdr), type, len);\n \tret = write_loose_object(oid, hdr, hdrlen, buf, len, mtime, 0);\ndiff --git a/object-store.h b/object-store.h\nindex 98c1d67946..f0aa03bbb9 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -358,7 +358,7 @@ void assert_oid_type(const struct object_id *oid, enum object_type expect);\n /*\n  * Enabling the object read lock allows multiple threads to safely call the\n  * following functions in parallel: repo_read_object_file(), read_object_file(),\n- * read_object_file_extended(), read_object_with_reference(), read_object(),\n+ * read_object_file_extended(), read_object_with_reference(),\n  * oid_object_info() and oid_object_info_extended().\n  *\n  * obj_read_lock() and obj_read_unlock() may also be used to protect other\n-- \n2.39.0.469.g9000b9c396\n\n"},{"id":"469877","messageId":"Y7l4W/3kgT8/Ti2H@coredump.intra.peff.net","threadId":"59043","inReplyTo":"Y7l4LsEQcDT9HZ21@coredump.intra.peff.net","subject":"[PATCH 2/5] streaming: inline call to read_object_file_extended()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-07T13:49:15Z","receivedAt":"2023-01-07T13:49:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The open_istream_incore() function is the only direct user of\nread_object_file_extended(), and the only caller which unsets the\nlookup_replace flag. Since read_object_file_extended() is now just a\nthin wrapper around oid_object_info_extended(), let's inline the call.\nThat will let us simplify read_object_file_extended() in the next patch.\n\nThe inlined version here is a few more lines because of the query setup,\nbut it's much more flexible, since we can pass (or omit) any flags we\nwant.\n\nNote the updated comment in the istream struct definition. It was\nalready slightly wrong (we never called read_object(); it has been\nread_object_file_extended() since day one), but should now be accurate.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n streaming.c | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/streaming.c b/streaming.c\nindex 7b2f8b2b93..27841dc1d9 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -38,7 +38,7 @@ struct git_istream {\n \n \tunion {\n \t\tstruct {\n-\t\t\tchar *buf; /* from read_object() */\n+\t\t\tchar *buf; /* from oid_object_info_extended() */\n \t\t\tunsigned long read_ptr;\n \t\t} incore;\n \n@@ -388,12 +388,17 @@ static ssize_t read_istream_incore(struct git_istream *st, char *buf, size_t sz)\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+\tstruct object_info oi = OBJECT_INFO_INIT;\n+\n \tst->u.incore.read_ptr = 0;\n \tst->close = close_istream_incore;\n \tst->read = read_istream_incore;\n \n-\treturn st->u.incore.buf ? 0 : -1;\n+\toi.typep = type;\n+\toi.sizep = &st->size;\n+\toi.contentp = (void **)&st->u.incore.buf;\n+\treturn oid_object_info_extended(r, oid, &oi,\n+\t\t\t\t\tOBJECT_INFO_DIE_IF_CORRUPT);\n }\n \n /*****************************************************************************\n-- \n2.39.0.469.g9000b9c396\n\n"},{"id":"469878","messageId":"Y7l4m22z4ZT5SQkG@coredump.intra.peff.net","threadId":"59043","inReplyTo":"Y7l4LsEQcDT9HZ21@coredump.intra.peff.net","subject":"[PATCH 3/5] read_object_file_extended(): drop lookup_replace option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-07T13:50:19Z","receivedAt":"2023-01-07T13:50:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our sole caller always passes in \"1\", so we can just drop the parameter\nentirely. Anybody who doesn't want this behavior could easily call\noid_object_info_extended() themselves, as we're just a thin wrapper\naround it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n object-file.c  | 7 ++-----\n object-store.h | 4 ++--\n 2 files changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex ed1babbac2..f472f2d6a0 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1698,18 +1698,15 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type,\n void *read_object_file_extended(struct repository *r,\n \t\t\t\tconst struct object_id *oid,\n \t\t\t\tenum object_type *type,\n-\t\t\t\tunsigned long *size,\n-\t\t\t\tint lookup_replace)\n+\t\t\t\tunsigned long *size)\n {\n \tstruct object_info oi = OBJECT_INFO_INIT;\n-\tunsigned flags = OBJECT_INFO_DIE_IF_CORRUPT;\n+\tunsigned flags = OBJECT_INFO_DIE_IF_CORRUPT | OBJECT_INFO_LOOKUP_REPLACE;\n \tvoid *data;\n \n \toi.typep = type;\n \toi.sizep = size;\n \toi.contentp = &data;\n-\tif (lookup_replace)\n-\t\tflags |= OBJECT_INFO_LOOKUP_REPLACE;\n \tif (oid_object_info_extended(r, oid, &oi, flags))\n \t    return NULL;\n \ndiff --git a/object-store.h b/object-store.h\nindex f0aa03bbb9..6ccacc947b 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -244,13 +244,13 @@ void *map_loose_object(struct repository *r, const struct object_id *oid,\n void *read_object_file_extended(struct repository *r,\n \t\t\t\tconst struct object_id *oid,\n \t\t\t\tenum object_type *type,\n-\t\t\t\tunsigned long *size, int lookup_replace);\n+\t\t\t\tunsigned long *size);\n static inline void *repo_read_object_file(struct repository *r,\n \t\t\t\t\t  const struct object_id *oid,\n \t\t\t\t\t  enum object_type *type,\n \t\t\t\t\t  unsigned long *size)\n {\n-\treturn read_object_file_extended(r, oid, type, size, 1);\n+\treturn read_object_file_extended(r, oid, type, size);\n }\n #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n #define read_object_file(oid, type, size) repo_read_object_file(the_repository, oid, type, size)\n-- \n2.39.0.469.g9000b9c396\n\n"},{"id":"469879","messageId":"Y7l4qX4rcnal5R+N@coredump.intra.peff.net","threadId":"59043","inReplyTo":"Y7l4LsEQcDT9HZ21@coredump.intra.peff.net","subject":"[PATCH 4/5] repo_read_object_file(): stop wrapping read_object_file_extended()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-07T13:50:33Z","receivedAt":"2023-01-07T13:50:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The only caller of read_object_file_extended() is the thin wrapper of\nrepo_read_object_file(). Instead of wrapping, let's just rename the\ninner function and let people call it directly. This cleans up the\nnamespace and reduces confusion.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n object-file.c  |  8 ++++----\n object-store.h | 18 +++++-------------\n 2 files changed, 9 insertions(+), 17 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex f472f2d6a0..80b08fc389 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1695,10 +1695,10 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type,\n  * deal with them should arrange to call oid_object_info_extended() and give\n  * error messages themselves.\n  */\n-void *read_object_file_extended(struct repository *r,\n-\t\t\t\tconst struct object_id *oid,\n-\t\t\t\tenum object_type *type,\n-\t\t\t\tunsigned long *size)\n+void *repo_read_object_file(struct repository *r,\n+\t\t\t    const struct object_id *oid,\n+\t\t\t    enum object_type *type,\n+\t\t\t    unsigned long *size)\n {\n \tstruct object_info oi = OBJECT_INFO_INIT;\n \tunsigned flags = OBJECT_INFO_DIE_IF_CORRUPT | OBJECT_INFO_LOOKUP_REPLACE;\ndiff --git a/object-store.h b/object-store.h\nindex 6ccacc947b..1a713d89d7 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -241,17 +241,10 @@ const char *loose_object_path(struct repository *r, struct strbuf *buf,\n void *map_loose_object(struct repository *r, const struct object_id *oid,\n \t\t       unsigned long *size);\n \n-void *read_object_file_extended(struct repository *r,\n-\t\t\t\tconst struct object_id *oid,\n-\t\t\t\tenum object_type *type,\n-\t\t\t\tunsigned long *size);\n-static inline void *repo_read_object_file(struct repository *r,\n-\t\t\t\t\t  const struct object_id *oid,\n-\t\t\t\t\t  enum object_type *type,\n-\t\t\t\t\t  unsigned long *size)\n-{\n-\treturn read_object_file_extended(r, oid, type, size);\n-}\n+void *repo_read_object_file(struct repository *r,\n+\t\t\t    const struct object_id *oid,\n+\t\t\t    enum object_type *type,\n+\t\t\t    unsigned long *size);\n #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n #define read_object_file(oid, type, size) repo_read_object_file(the_repository, oid, type, size)\n #endif\n@@ -358,8 +351,7 @@ void assert_oid_type(const struct object_id *oid, enum object_type expect);\n /*\n  * Enabling the object read lock allows multiple threads to safely call the\n  * following functions in parallel: repo_read_object_file(), read_object_file(),\n- * read_object_file_extended(), read_object_with_reference(),\n- * oid_object_info() and oid_object_info_extended().\n+ * read_object_with_reference(), oid_object_info() and oid_object_info_extended().\n  *\n  * obj_read_lock() and obj_read_unlock() may also be used to protect other\n  * section which cannot execute in parallel with object reading. Since the used\n-- \n2.39.0.469.g9000b9c396\n\n"},{"id":"469880","messageId":"Y7l4vQwRZzGtxlBB@coredump.intra.peff.net","threadId":"59043","inReplyTo":"Y7l4LsEQcDT9HZ21@coredump.intra.peff.net","subject":"[PATCH 5/5] packfile: inline custom read_object()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-07T13:50:53Z","receivedAt":"2023-01-07T13:51:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the pack code was split into its own file[1], it got a copy of the\nstatic read_object() function. But there's only one caller here, so we\ncould just inline it. And it's worth doing so, as the name read_object()\ninvites comparisons to the public read_object_file(), but the two don't\nbehave quite the same.\n\n[1] The move happened over several commits, but the relevant one here is\n    f1d8130be0 (pack: move clear_delta_base_cache(), packed_object_info(),\n    unpack_entry(), 2017-08-18).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n packfile.c | 26 +++++++++-----------------\n 1 file changed, 9 insertions(+), 17 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex c0d7dd93f4..79e21ab18e 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1650,22 +1650,6 @@ struct unpack_entry_stack_ent {\n \tunsigned long size;\n };\n \n-static void *read_object(struct repository *r,\n-\t\t\t const struct object_id *oid,\n-\t\t\t enum object_type *type,\n-\t\t\t unsigned long *size)\n-{\n-\tstruct object_info oi = OBJECT_INFO_INIT;\n-\tvoid *content;\n-\toi.typep = type;\n-\toi.sizep = size;\n-\toi.contentp = &content;\n-\n-\tif (oid_object_info_extended(r, oid, &oi, 0) < 0)\n-\t\treturn NULL;\n-\treturn content;\n-}\n-\n void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,\n \t\t   enum object_type *final_type, unsigned long *final_size)\n {\n@@ -1798,14 +1782,22 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,\n \t\t\tuint32_t pos;\n \t\t\tstruct object_id base_oid;\n \t\t\tif (!(offset_to_pack_pos(p, obj_offset, &pos))) {\n+\t\t\t\tstruct object_info oi = OBJECT_INFO_INIT;\n+\n \t\t\t\tnth_packed_object_id(&base_oid, p,\n \t\t\t\t\t\t     pack_pos_to_index(p, pos));\n \t\t\t\terror(\"failed to read delta base object %s\"\n \t\t\t\t      \" at offset %\"PRIuMAX\" from %s\",\n \t\t\t\t      oid_to_hex(&base_oid), (uintmax_t)obj_offset,\n \t\t\t\t      p->pack_name);\n \t\t\t\tmark_bad_packed_object(p, &base_oid);\n-\t\t\t\tbase = read_object(r, &base_oid, &type, &base_size);\n+\n+\t\t\t\toi.typep = &type;\n+\t\t\t\toi.sizep = &base_size;\n+\t\t\t\toi.contentp = &base;\n+\t\t\t\tif (oid_object_info_extended(r, &base_oid, &oi, 0) < 0)\n+\t\t\t\t\tbase = NULL;\n+\n \t\t\t\texternal_base = base;\n \t\t\t}\n \t\t}\n-- \n2.39.0.469.g9000b9c396\n"},{"id":"469980","messageId":"f1028cba-5fc6-3584-3f21-545550012e9d@github.com","threadId":"59043","inReplyTo":"Y7l4LsEQcDT9HZ21@coredump.intra.peff.net","subject":"Re: [PATCH 0/5] cleaning up read_object() family of functions","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-01-09T15:09:32Z","receivedAt":"2023-01-09T15:09:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/7/2023 8:48 AM, Jeff King wrote:\n> I often get confused about the difference between:\n> \n>   - read_object()\n>   - read_object_file();\n>   - read_object_file_extended();\n>   - repo_read_object_file();\n> \n> Since Jonathan's recent cleanups from 9e59b38c88 (object-file: emit\n> corruption errors when detected, 2022-12-14), these are mostly thin\n> wrappers around each other and around oid_object_info_extended().\n> \n> This series shuffles things around a little more so that we are down to\n> just read_object_file() and repo_read_object_file(). And the\n> relationship there is pretty easy (and long-term we'd eventually merge\n> them once everyone has a repository object).\n\nI read the patches carefully and the translations look correct and\ndefinitely help with this confusing mess of method names.\n\n> It is a net reduction in lines, even though some of the callers end up a\n> little longer (because they have to stuff pointers into an object_info\n> struct). If that's too distasteful, the middle ground is to have a\n> helper like:\n> \n>   void *foo(struct repository *r, const struct object_id *oid,\n>             enum object_type *type, unsigned long *size,\n> \t    unsigned flags)\n>   {\n> \tstruct object_info oi = OBJECT_INFO_INIT;\n> \tvoid *content;\n> \n> \toi.typep = type;\n> \toi.sizep = size;\n> \toi.contentp = ret;\n> \n> \tif (oid_object_info_extended(r, oid, &oi, flags) < 0)\n> \t\treturn NULL;\n> \treturn content;\n>   }\n> \n> which is basically the same as read_object(), but makes it clear that\n> you can pass OBJECT_INFO flags. The trouble is that I could not come up\n> with a name for it that was not confusing. ;) So just having most places\n> call oid_object_info_extended() directly seemed better. It would be nice\n> if that function had a shorter name, too, but I left that for another\n> day.\n\nI did think that requiring callers to create their own object_info\nstructs (which takes at least four lines) would be too much, but\nthe number of new callers is so low that I think this is a fine place\nto stop.\n\nThanks,\n-Stolee\n"},{"id":"470096","messageId":"Y77/T8dktee3wOA5@coredump.intra.peff.net","threadId":"59043","inReplyTo":"f1028cba-5fc6-3584-3f21-545550012e9d@github.com","subject":"Re: [PATCH 0/5] cleaning up read_object() family of functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-11T18:26:23Z","receivedAt":"2023-01-11T18:26:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 09, 2023 at 10:09:32AM -0500, Derrick Stolee wrote:\n\n> I did think that requiring callers to create their own object_info\n> structs (which takes at least four lines) would be too much, but\n> the number of new callers is so low that I think this is a fine place\n> to stop.\n\nYeah, that was my feeling. I do wonder if there's a way to make it\neasier for callers of oid_object_info_extended(), but I couldn't come up\nwith anything that's nice enough to merit the complexity.\n\nFor example, here's an attempt to let the caller use designated\ninitializers to set up the query struct:\n\ndiff --git a/object-file.c b/object-file.c\nindex 80b08fc389..60ca75d755 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1700,13 +1700,12 @@ void *repo_read_object_file(struct repository *r,\n \t\t\t    enum object_type *type,\n \t\t\t    unsigned long *size)\n {\n-\tstruct object_info oi = OBJECT_INFO_INIT;\n \tunsigned flags = OBJECT_INFO_DIE_IF_CORRUPT | OBJECT_INFO_LOOKUP_REPLACE;\n \tvoid *data;\n+\tstruct object_info oi = OBJECT_INFO(.typep = type,\n+\t\t\t\t\t    .sizep = size,\n+\t\t\t\t\t    .contentp = &data);\n \n-\toi.typep = type;\n-\toi.sizep = size;\n-\toi.contentp = &data;\n \tif (oid_object_info_extended(r, oid, &oi, flags))\n \t    return NULL;\n \ndiff --git a/object-store.h b/object-store.h\nindex 1a713d89d7..e894cee61b 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -418,7 +418,8 @@ struct object_info {\n  * Initializer for a \"struct object_info\" that wants no items. You may\n  * also memset() the memory to all-zeroes.\n  */\n-#define OBJECT_INFO_INIT { 0 }\n+#define OBJECT_INFO(...) { 0, __VA_ARGS__ }\n+#define OBJECT_INFO_INIT OBJECT_INFO()\n \n /* Invoke lookup_replace_object() on the given hash */\n #define OBJECT_INFO_LOOKUP_REPLACE 1\n\nBut:\n\n  - it actually triggers a gcc warning, since OBJECT_INFO(.typep = foo)\n    sets typep twice (once for the default \"0\", and once by name). In\n    this case the \"0\" is superfluous, since that's the default, and we\n    could just do:\n\n      #define OBJECT_INFO(...) { __VA_ARGS__ }\n      #define OBJECT_INFO_INIT OBJECT_INFO(0)\n\n    but I was hoping to find a general technique for object\n    initializers.\n\n  - it's not really that much shorter than the existing code. The real\n    benefit of \"data = read_object(oid, type, size)\" is the implicit\n    number and names of the parameters. And the way to get that is to\n    provide an extra function.\n\nSo I think we are better off with the code that is longer but totally\nobvious, unless we really want to add a function wrapper for common\nqueries as syntactic sugar.\n\n-Peff\n"},{"id":"470102","messageId":"6c896cc8-2da9-a448-4ab6-2dc535fb0e2b@github.com","threadId":"59043","inReplyTo":"Y77/T8dktee3wOA5@coredump.intra.peff.net","subject":"Re: [PATCH 0/5] cleaning up read_object() family of functions","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-01-11T20:17:58Z","receivedAt":"2023-01-11T20:18:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/11/2023 1:26 PM, Jeff King wrote:\n> On Mon, Jan 09, 2023 at 10:09:32AM -0500, Derrick Stolee wrote:\n> \n>> I did think that requiring callers to create their own object_info\n>> structs (which takes at least four lines) would be too much, but\n>> the number of new callers is so low that I think this is a fine place\n>> to stop.\n> \n> Yeah, that was my feeling. I do wonder if there's a way to make it\n> easier for callers of oid_object_info_extended(), but I couldn't come up\n> with anything that's nice enough to merit the complexity.\n> \n> For example, here's an attempt to let the caller use designated\n> initializers to set up the query struct:\n\n> +\tstruct object_info oi = OBJECT_INFO(.typep = type,\n> +\t\t\t\t\t    .sizep = size,\n> +\t\t\t\t\t    .contentp = &data);\n\nYour macro expansion creates this format:\n\n\tstruct object_info oi = {\n\t\t.type = type,\n\t\t.sizep = size,\n\t\t.contentp = &data,\n\t};\n\nAnd even this expansion looks a bit better than the inline\nupdates:\n\n> -\toi.typep = type;\n> -\toi.sizep = size;\n> -\toi.contentp = &data;\n\nSo maybe that's a preferred pattern that we could establish\nby replacing the existing callers. It's also such a minor\npoint that I wouldn't say it's a high priority to do.\n\nThanks,\n-Stolee\n"},{"id":"470103","messageId":"Y78cWKKXz944pWBS@coredump.intra.peff.net","threadId":"59043","inReplyTo":"6c896cc8-2da9-a448-4ab6-2dc535fb0e2b@github.com","subject":"Re: [PATCH 0/5] cleaning up read_object() family of functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-11T20:30:16Z","receivedAt":"2023-01-11T20:30:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 11, 2023 at 03:17:58PM -0500, Derrick Stolee wrote:\n\n> > For example, here's an attempt to let the caller use designated\n> > initializers to set up the query struct:\n> \n> > +\tstruct object_info oi = OBJECT_INFO(.typep = type,\n> > +\t\t\t\t\t    .sizep = size,\n> > +\t\t\t\t\t    .contentp = &data);\n> \n> Your macro expansion creates this format:\n> \n> \tstruct object_info oi = {\n> \t\t.type = type,\n> \t\t.sizep = size,\n> \t\t.contentp = &data,\n> \t};\n> \n> And even this expansion looks a bit better than the inline\n> updates:\n\nThere's a subtle assumption in the expanded initializer, though, which\nis that everything not specified is OK to be zero-initialized. That\nworks for object_info, but not for arbitrary structs (which is why we\nhave these INIT macros in the first place).\n\n-Peff\n"},{"id":"470151","messageId":"230112.86o7r42k13.gmgdl@evledraar.gmail.com","threadId":"59043","inReplyTo":"Y7l4vQwRZzGtxlBB@coredump.intra.peff.net","subject":"Re: [PATCH 5/5] packfile: inline custom read_object()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-12T09:01:28Z","receivedAt":"2023-01-12T09:23:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jan 07 2023, Jeff King wrote:\n\n> When the pack code was split into its own file[1], it got a copy of the\n> static read_object() function. But there's only one caller here, so we\n> could just inline it. And it's worth doing so, as the name read_object()\n> invites comparisons to the public read_object_file(), but the two don't\n> behave quite the same.\n>\n> [1] The move happened over several commits, but the relevant one here is\n>     f1d8130be0 (pack: move clear_delta_base_cache(), packed_object_info(),\n>     unpack_entry(), 2017-08-18).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  packfile.c | 26 +++++++++-----------------\n>  1 file changed, 9 insertions(+), 17 deletions(-)\n>\n> diff --git a/packfile.c b/packfile.c\n> index c0d7dd93f4..79e21ab18e 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1650,22 +1650,6 @@ struct unpack_entry_stack_ent {\n>  \tunsigned long size;\n>  };\n>  \n> -static void *read_object(struct repository *r,\n> -\t\t\t const struct object_id *oid,\n> -\t\t\t enum object_type *type,\n> -\t\t\t unsigned long *size)\n> -{\n> -\tstruct object_info oi = OBJECT_INFO_INIT;\n> -\tvoid *content;\n> -\toi.typep = type;\n> -\toi.sizep = size;\n> -\toi.contentp = &content;\n> -\n> -\tif (oid_object_info_extended(r, oid, &oi, 0) < 0)\n> -\t\treturn NULL;\n> -\treturn content;\n> -}\n> -\n>  void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,\n>  \t\t   enum object_type *final_type, unsigned long *final_size)\n>  {\n> @@ -1798,14 +1782,22 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,\n>  \t\t\tuint32_t pos;\n>  \t\t\tstruct object_id base_oid;\n>  \t\t\tif (!(offset_to_pack_pos(p, obj_offset, &pos))) {\n> +\t\t\t\tstruct object_info oi = OBJECT_INFO_INIT;\n> +\n>  \t\t\t\tnth_packed_object_id(&base_oid, p,\n>  \t\t\t\t\t\t     pack_pos_to_index(p, pos));\n>  \t\t\t\terror(\"failed to read delta base object %s\"\n>  \t\t\t\t      \" at offset %\"PRIuMAX\" from %s\",\n>  \t\t\t\t      oid_to_hex(&base_oid), (uintmax_t)obj_offset,\n>  \t\t\t\t      p->pack_name);\n>  \t\t\t\tmark_bad_packed_object(p, &base_oid);\n> -\t\t\t\tbase = read_object(r, &base_oid, &type, &base_size);\n> +\n> +\t\t\t\toi.typep = &type;\n> +\t\t\t\toi.sizep = &base_size;\n> +\t\t\t\toi.contentp = &base;\n> +\t\t\t\tif (oid_object_info_extended(r, &base_oid, &oi, 0) < 0)\n> +\t\t\t\t\tbase = NULL;\n> +\n>  \t\t\t\texternal_base = base;\n>  \t\t\t}\n>  \t\t}\n\nThis isn't introducing a behavior difference, in fact it's diligently\nbending over backwards to preserve existing behavior, but I don't think\nwe need to do so, and shouldn't have this \"base = NULL\" line.\n\nHere we're within an \"if\" block where we tested that \"base == NULL\"\n(which is why we're trying to populate it)\n\nBefore when we had read_object() re-assigning to \"base\" here was the\nobvious thing to do, but now this seems like undue an incomplete\nparanoia.\n\nIf oid_object_info_extended() why can't we trust that it didn't touch\nour \"base\"? And if we can't trust that, why are we trusting that it left\n\"type\" and \"base_size\" untouched?\n\nI think squashing this in would be much better:\n\t\n\tdiff --git a/packfile.c b/packfile.c\n\tindex 79e21ab18e7..f45017422a1 100644\n\t--- a/packfile.c\n\t+++ b/packfile.c\n\t@@ -1795,10 +1795,8 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,\n\t \t\t\t\toi.typep = &type;\n\t \t\t\t\toi.sizep = &base_size;\n\t \t\t\t\toi.contentp = &base;\n\t-\t\t\t\tif (oid_object_info_extended(r, &base_oid, &oi, 0) < 0)\n\t-\t\t\t\t\tbase = NULL;\n\t-\n\t-\t\t\t\texternal_base = base;\n\t+\t\t\t\tif (!oid_object_info_extended(r, &base_oid, &oi, 0))\n\t+\t\t\t\t\texternal_base = base;\n\t \t\t\t}\n\t \t\t}\n\nNot only aren't we second-guessing that our \"base\" was left alone, we're\nusing the return value of oid_object_info_extended() to guard that\nassignment to \"external_base\" instead (it's NULL at this point too).\n\n\n\n\n"},{"id":"470152","messageId":"230112.86k01s2jzm.gmgdl@evledraar.gmail.com","threadId":"59043","inReplyTo":"Y7l4RznPhTr+O8ZP@coredump.intra.peff.net","subject":"Re: [PATCH 1/5] object-file: inline calls to read_object()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-12T09:13:20Z","receivedAt":"2023-01-12T09:25:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jan 07 2023, Jeff King wrote:\n\n> +\toi.typep = type;\n> +\toi.sizep = size;\n> +\toi.contentp = &data;\n> +\tif (lookup_replace)\n> +\t\tflags |= OBJECT_INFO_LOOKUP_REPLACE;\n> +\tif (oid_object_info_extended(r, oid, &oi, flags))\n> +\t    return NULL;\n\nStyle: This is \"\\t    \", but should be \"\\t\\t\".\n"},{"id":"470153","messageId":"230112.86fscg2jbm.gmgdl@evledraar.gmail.com","threadId":"59043","inReplyTo":"Y77/T8dktee3wOA5@coredump.intra.peff.net","subject":"Re: [PATCH 0/5] cleaning up read_object() family of functions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-12T09:21:46Z","receivedAt":"2023-01-12T09:34:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 11 2023, Jeff King wrote:\n\n> On Mon, Jan 09, 2023 at 10:09:32AM -0500, Derrick Stolee wrote:\n>\n>> I did think that requiring callers to create their own object_info\n>> structs (which takes at least four lines) would be too much, but\n>> the number of new callers is so low that I think this is a fine place\n>> to stop.\n>\n> Yeah, that was my feeling. I do wonder if there's a way to make it\n> easier for callers of oid_object_info_extended(), but I couldn't come up\n> with anything that's nice enough to merit the complexity.\n>\n> For example, here's an attempt to let the caller use designated\n> initializers to set up the query struct:\n>\n> diff --git a/object-file.c b/object-file.c\n> index 80b08fc389..60ca75d755 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1700,13 +1700,12 @@ void *repo_read_object_file(struct repository *r,\n>  \t\t\t    enum object_type *type,\n>  \t\t\t    unsigned long *size)\n>  {\n> -\tstruct object_info oi = OBJECT_INFO_INIT;\n>  \tunsigned flags = OBJECT_INFO_DIE_IF_CORRUPT | OBJECT_INFO_LOOKUP_REPLACE;\n>  \tvoid *data;\n> +\tstruct object_info oi = OBJECT_INFO(.typep = type,\n> +\t\t\t\t\t    .sizep = size,\n> +\t\t\t\t\t    .contentp = &data);\n>  \n> -\toi.typep = type;\n> -\toi.sizep = size;\n> -\toi.contentp = &data;\n>  \tif (oid_object_info_extended(r, oid, &oi, flags))\n>  \t    return NULL;\n>  \n> diff --git a/object-store.h b/object-store.h\n> index 1a713d89d7..e894cee61b 100644\n> --- a/object-store.h\n> +++ b/object-store.h\n> @@ -418,7 +418,8 @@ struct object_info {\n>   * Initializer for a \"struct object_info\" that wants no items. You may\n>   * also memset() the memory to all-zeroes.\n>   */\n> -#define OBJECT_INFO_INIT { 0 }\n> +#define OBJECT_INFO(...) { 0, __VA_ARGS__ }\n> +#define OBJECT_INFO_INIT OBJECT_INFO()\n>  \n>  /* Invoke lookup_replace_object() on the given hash */\n>  #define OBJECT_INFO_LOOKUP_REPLACE 1\n>\n> But:\n>\n>   - it actually triggers a gcc warning, since OBJECT_INFO(.typep = foo)\n>     sets typep twice (once for the default \"0\", and once by name). In\n>     this case the \"0\" is superfluous, since that's the default, and we\n>     could just do:\n>\n>       #define OBJECT_INFO(...) { __VA_ARGS__ }\n>       #define OBJECT_INFO_INIT OBJECT_INFO(0)\n>\n>     but I was hoping to find a general technique for object\n>     initializers.\n>\n>   - it's not really that much shorter than the existing code. The real\n>     benefit of \"data = read_object(oid, type, size)\" is the implicit\n>     number and names of the parameters. And the way to get that is to\n>     provide an extra function.\n>\n> So I think we are better off with the code that is longer but totally\n> obvious, unless we really want to add a function wrapper for common\n> queries as syntactic sugar.\n>\n> -Peff\n\nI agree that it's probably not worth it here, but I think you're just\ntying yourself in knots in trying to define these macros in terms of\neach other. This sort of thing will work if you just do:\n\t\n\tdiff --git a/object-store.h b/object-store.h\n\tindex e894cee61ba..bfcd2482dc5 100644\n\t--- a/object-store.h\n\t+++ b/object-store.h\n\t@@ -418,8 +418,8 @@ struct object_info {\n\t  * Initializer for a \"struct object_info\" that wants no items. You may\n\t  * also memset() the memory to all-zeroes.\n\t  */\n\t-#define OBJECT_INFO(...) { 0, __VA_ARGS__ }\n\t-#define OBJECT_INFO_INIT OBJECT_INFO()\n\t+#define OBJECT_INFO_INIT { 0 }\n\t+#define OBJECT_INFO(...) { __VA_ARGS__ }\n\t \n\t /* Invoke lookup_replace_object() on the given hash */\n\t #define OBJECT_INFO_LOOKUP_REPLACE 1\n\nWhich is just a twist on René's suggestion from [1], i.e.:\n\n\t#define CHILD_PROCESS_INIT_EX(...) { .args = STRVEC_INIT, __VA_ARGS__ }\n\nIn that case we always need to rely on the \"args\" being init'd, and the\nGCC warning you note is a feature, its initialization is \"private\", and\nyou should never override it.\n\nBut likewise you don't need the \"0\" there, if the user provides an empty\nlist that's their own fault, they should use OBJECT_INFO_INIT\ninstead.\n\nIf they do provide arguments it's an implementation detail how any\n\"default\" arguments get init'd, if they're not clobbering any \"private\"\narguments we're OK.\n\nSo using an explicit \"0\" is the same as providing nothing in the\n\"*_ARGS()\" case, in both cases we're just offloading that zero-init to\nthe language.\n\nThe only way I think you can dig yourself into a proper hole here is if\nyou're trying to support 0 or N args, as P99 shows that's possible, but\nquite complex (and not worth it, IMO).\n\n1. https://lore.kernel.org/git/749f6adc-928a-0978-e3a1-2ede9f07def0@web.de/\n"},{"id":"470205","messageId":"Y8AwGdtrsjnBdXN7@coredump.intra.peff.net","threadId":"59043","inReplyTo":"230112.86k01s2jzm.gmgdl@evledraar.gmail.com","subject":"[PATCH] object-file: fix indent-with-space","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-12T16:06:49Z","receivedAt":"2023-01-12T16:13:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 12, 2023 at 10:13:20AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> On Sat, Jan 07 2023, Jeff King wrote:\n> \n> > +\toi.typep = type;\n> > +\toi.sizep = size;\n> > +\toi.contentp = &data;\n> > +\tif (lookup_replace)\n> > +\t\tflags |= OBJECT_INFO_LOOKUP_REPLACE;\n> > +\tif (oid_object_info_extended(r, oid, &oi, flags))\n> > +\t    return NULL;\n> \n> Style: This is \"\\t    \", but should be \"\\t\\t\".\n\nHmph, I'm not sure how I managed that. Thanks for pointing it out. The\ncommit is in 'next', so I think we'd want this on top (of\njk/read-object-cleanup).\n\n-- >8 --\nSubject: [PATCH] object-file: fix indent-with-space\n\nCommit b25562e63f (object-file: inline calls to read_object(),\n2023-01-07) accidentally indented a conditional block with spaces\ninstead of a tab.\n\nReported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n object-file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 80b08fc389..ce9efae994 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1708,7 +1708,7 @@ void *repo_read_object_file(struct repository *r,\n \toi.sizep = size;\n \toi.contentp = &data;\n \tif (oid_object_info_extended(r, oid, &oi, flags))\n-\t    return NULL;\n+\t\treturn NULL;\n \n \treturn data;\n }\n-- \n2.39.0.508.g93b13bde48\n"},{"id":"470206","messageId":"230112.864jsv20r2.gmgdl@evledraar.gmail.com","threadId":"59043","inReplyTo":"Y8AwGdtrsjnBdXN7@coredump.intra.peff.net","subject":"Re: [PATCH] object-file: fix indent-with-space","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-12T16:08:35Z","receivedAt":"2023-01-12T16:14:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 12 2023, Jeff King wrote:\n\n> On Thu, Jan 12, 2023 at 10:13:20AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> \n>> On Sat, Jan 07 2023, Jeff King wrote:\n>> \n>> > +\toi.typep = type;\n>> > +\toi.sizep = size;\n>> > +\toi.contentp = &data;\n>> > +\tif (lookup_replace)\n>> > +\t\tflags |= OBJECT_INFO_LOOKUP_REPLACE;\n>> > +\tif (oid_object_info_extended(r, oid, &oi, flags))\n>> > +\t    return NULL;\n>> \n>> Style: This is \"\\t    \", but should be \"\\t\\t\".\n>\n> Hmph, I'm not sure how I managed that. Thanks for pointing it out. The\n> commit is in 'next', so I think we'd want this on top (of\n> jk/read-object-cleanup).\n\n> -- >8 --\n> Subject: [PATCH] object-file: fix indent-with-space\n>\n> Commit b25562e63f (object-file: inline calls to read_object(),\n> 2023-01-07) accidentally indented a conditional block with spaces\n> instead of a tab.\n>\n> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  object-file.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/object-file.c b/object-file.c\n> index 80b08fc389..ce9efae994 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1708,7 +1708,7 @@ void *repo_read_object_file(struct repository *r,\n>  \toi.sizep = size;\n>  \toi.contentp = &data;\n>  \tif (oid_object_info_extended(r, oid, &oi, flags))\n> -\t    return NULL;\n> +\t\treturn NULL;\n>  \n>  \treturn data;\n>  }\n\nThanks, I didn't notice (assuming it was too soon, it being less than a\nweek) that it was in \"next\" already. This change LGTM, thanks!\n"},{"id":"470207","messageId":"Y8AyTE3OS7HCAzKH@coredump.intra.peff.net","threadId":"59043","inReplyTo":"230112.86fscg2jbm.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 0/5] cleaning up read_object() family of functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-12T16:16:12Z","receivedAt":"2023-01-12T16:20:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 12, 2023 at 10:21:46AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> I agree that it's probably not worth it here, but I think you're just\n> tying yourself in knots in trying to define these macros in terms of\n> each other. This sort of thing will work if you just do:\n> \t\n> \tdiff --git a/object-store.h b/object-store.h\n> \tindex e894cee61ba..bfcd2482dc5 100644\n> \t--- a/object-store.h\n> \t+++ b/object-store.h\n> \t@@ -418,8 +418,8 @@ struct object_info {\n> \t  * Initializer for a \"struct object_info\" that wants no items. You may\n> \t  * also memset() the memory to all-zeroes.\n> \t  */\n> \t-#define OBJECT_INFO(...) { 0, __VA_ARGS__ }\n> \t-#define OBJECT_INFO_INIT OBJECT_INFO()\n> \t+#define OBJECT_INFO_INIT { 0 }\n> \t+#define OBJECT_INFO(...) { __VA_ARGS__ }\n\nRight, that works because the initializer is just \"0\", which the\ncompiler can do for us implicitly. I agree it works here to omit, but as\na general solution, it doesn't.\n\n> Which is just a twist on René's suggestion from [1], i.e.:\n> \n> \t#define CHILD_PROCESS_INIT_EX(...) { .args = STRVEC_INIT, __VA_ARGS__ }\n>\n> In that case we always need to rely on the \"args\" being init'd, and the\n> GCC warning you note is a feature, its initialization is \"private\", and\n> you should never override it.\n\nRight, and it works here because you'd never want to init .args to\nanything else (which I think is what you mean by \"private\"). But in the\ngeneral case the defaults can't set something that the caller might want\nto override, because the compiler's warning doesn't know the difference\nbetween \"override\" and \"oops, you specified this twice\".\n\nIt's mostly a non-issue because we tend to prefer 0-initialization when\npossible, but I think as a general technique this is probably opening a\ncan of worms for little benefit.\n\n-Peff\n"},{"id":"470211","messageId":"230112.86v8lbzpj1.gmgdl@evledraar.gmail.com","threadId":"59043","inReplyTo":"Y8AyTE3OS7HCAzKH@coredump.intra.peff.net","subject":"Re: [PATCH 0/5] cleaning up read_object() family of functions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-12T16:22:04Z","receivedAt":"2023-01-12T16:29:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 12 2023, Jeff King wrote:\n\n> On Thu, Jan 12, 2023 at 10:21:46AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> I agree that it's probably not worth it here, but I think you're just\n>> tying yourself in knots in trying to define these macros in terms of\n>> each other. This sort of thing will work if you just do:\n>> \t\n>> \tdiff --git a/object-store.h b/object-store.h\n>> \tindex e894cee61ba..bfcd2482dc5 100644\n>> \t--- a/object-store.h\n>> \t+++ b/object-store.h\n>> \t@@ -418,8 +418,8 @@ struct object_info {\n>> \t  * Initializer for a \"struct object_info\" that wants no items. You may\n>> \t  * also memset() the memory to all-zeroes.\n>> \t  */\n>> \t-#define OBJECT_INFO(...) { 0, __VA_ARGS__ }\n>> \t-#define OBJECT_INFO_INIT OBJECT_INFO()\n>> \t+#define OBJECT_INFO_INIT { 0 }\n>> \t+#define OBJECT_INFO(...) { __VA_ARGS__ }\n>\n> Right, that works because the initializer is just \"0\", which the\n> compiler can do for us implicitly. I agree it works here to omit, but as\n> a general solution, it doesn't.\n>\n>> Which is just a twist on René's suggestion from [1], i.e.:\n>> \n>> \t#define CHILD_PROCESS_INIT_EX(...) { .args = STRVEC_INIT, __VA_ARGS__ }\n>>\n>> In that case we always need to rely on the \"args\" being init'd, and the\n>> GCC warning you note is a feature, its initialization is \"private\", and\n>> you should never override it.\n>\n> Right, and it works here because you'd never want to init .args to\n> anything else (which I think is what you mean by \"private\"). But in the\n> general case the defaults can't set something that the caller might want\n> to override, because the compiler's warning doesn't know the difference\n> between \"override\" and \"oops, you specified this twice\".\n>\n> It's mostly a non-issue because we tend to prefer 0-initialization when\n> possible, but I think as a general technique this is probably opening a\n> can of worms for little benefit.\n\nYou're right in the general case, although I think that if we did\nencounter such a use-case a perfectly good solution would be to just\nsuppress the GCC-specific warning with the relevant GCC-specific macro\nmagic, this being perfectly valid C, just something it (rightly, as it's\nalmost always a mistake) complains about.\n\nBut I can't think of a case where this would matter for us in practice.\n\nWe have members like \"struct strbuf\"'s \"buf\", which always needs to be\ninit'd, but never \"maybe by the user\", so the pattern above would work\nthere.\n\nThen we have things like \"strdup_strings\" which we might imagine that\nthe user would override (with a hypothetical \"struct string_list\" that\ntook more arguments, but in those cases we could just add another init\nmacro, as \"STRING_LIST_INIT_{DUP,NODUP}\" does.\n\nFor any such member we could always just invert its boolean state, if it\ncame to that, couldn't we?\n\nAnyway, I agree that it's not worth pursuing this in this case.\n\nBut I think it's a neat pattern that we might find use for sooner than\nlater for something else.\n\nI don't think it's worth the churn to change it at this point (except\nmaybe with a sufficiently clever coccinelle rule), but I think it's\nalready \"worth it\" in the case of the run-command API, if we were adding\nthat code today under current constraints (i.e. being able to use C99\nmacro features).\n"},{"id":"470213","messageId":"Y8A1T5kzgqXV5vKr@coredump.intra.peff.net","threadId":"59043","inReplyTo":"230112.86o7r42k13.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 5/5] packfile: inline custom read_object()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-12T16:29:03Z","receivedAt":"2023-01-12T16:30:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 12, 2023 at 10:01:28AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > -\t\t\t\tbase = read_object(r, &base_oid, &type, &base_size);\n> > +\n> > +\t\t\t\toi.typep = &type;\n> > +\t\t\t\toi.sizep = &base_size;\n> > +\t\t\t\toi.contentp = &base;\n> > +\t\t\t\tif (oid_object_info_extended(r, &base_oid, &oi, 0) < 0)\n> > +\t\t\t\t\tbase = NULL;\n> > +\n> >  \t\t\t\texternal_base = base;\n> >  \t\t\t}\n> >  \t\t}\n> \n> This isn't introducing a behavior difference, in fact it's diligently\n> bending over backwards to preserve existing behavior, but I don't think\n> we need to do so, and shouldn't have this \"base = NULL\" line.\n> \n> Here we're within an \"if\" block where we tested that \"base == NULL\"\n> (which is why we're trying to populate it)\n> \n> Before when we had read_object() re-assigning to \"base\" here was the\n> obvious thing to do, but now this seems like undue an incomplete\n> paranoia.\n\nI think it's the same paranoia that was in read_object(). There it\ncatches the error and returns NULL, rather than the probably-NULL\n\"content\" (though to be fair, it simply did not initialize the pointer,\nso it would have had to do that to depend on it).\n\nI agree it's probably being overly defensive. But I don't think\noid_object_info_extended() makes any promises, and it's not completely\nclear to me if packed_object_info() could return a non-NULL entry here\non an error (e.g., if packed_to_object_type() fails even after we pulled\nout the content).\n\nSo probably yes, we could depend on that (and if not, arguably we should\nbe fixing oid_object_info_extended(), because we are probably leaking a\nbuffer in that case). But we definitely shouldn't be doing it in the\nmiddle of another patch.\n\n> If oid_object_info_extended() why can't we trust that it didn't touch\n> our \"base\"? And if we can't trust that, why are we trusting that it left\n> \"type\" and \"base_size\" untouched?\n\nMy assumption is that \"base\" gated access to \"type\" and \"base_size\". So\nas long as \"!base\", we do not look at the other two.\n\n> I think squashing this in would be much better:\n> \t\n> \tdiff --git a/packfile.c b/packfile.c\n> \tindex 79e21ab18e7..f45017422a1 100644\n> \t--- a/packfile.c\n> \t+++ b/packfile.c\n> \t@@ -1795,10 +1795,8 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,\n> \t \t\t\t\toi.typep = &type;\n> \t \t\t\t\toi.sizep = &base_size;\n> \t \t\t\t\toi.contentp = &base;\n> \t-\t\t\t\tif (oid_object_info_extended(r, &base_oid, &oi, 0) < 0)\n> \t-\t\t\t\t\tbase = NULL;\n> \t-\n> \t-\t\t\t\texternal_base = base;\n> \t+\t\t\t\tif (!oid_object_info_extended(r, &base_oid, &oi, 0))\n> \t+\t\t\t\t\texternal_base = base;\n> \t \t\t\t}\n> \t \t\t}\n> \n> Not only aren't we second-guessing that our \"base\" was left alone, we're\n> using the return value of oid_object_info_extended() to guard that\n> assignment to \"external_base\" instead (it's NULL at this point too).\n\nI don't think we need to guard the assignment (we know it will be NULL\nif we saw an error). But sure, I don't mind if you want to do that\nsimplification, but it should be on top if at all.\n\n-Peff\n"},{"id":"470220","messageId":"Y8A7Cb/w0VQk6ppV@coredump.intra.peff.net","threadId":"59043","inReplyTo":"230112.86v8lbzpj1.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 0/5] cleaning up read_object() family of functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-12T16:53:29Z","receivedAt":"2023-01-12T17:27:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 12, 2023 at 05:22:04PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> We have members like \"struct strbuf\"'s \"buf\", which always needs to be\n> init'd, but never \"maybe by the user\", so the pattern above would work\n> there.\n\nWe've discussed in the past having a strbuf that points to an existing\nbuffer, over which it takes ownership. Or a const string that we'd leave\nbehind (but not free) if we needed to grow.\n\nIn those cases you'd want to pass in a buffer to the allocator. Of\ncourse in the case of a strbuf those initializers would probably just be\ntotally separate from the regular slopbuf one, just because there's not\nmuch else in a strbuf to initialize. You don't gain much from trying to\navoid repetition.\n\n> Anyway, I agree that it's not worth pursuing this in this case.\n> \n> But I think it's a neat pattern that we might find use for sooner than\n> later for something else.\n\nI remain unconvinced. ;) Mostly just that the lines saved versus the\namount of magic and thought doesn't seem reasonable. But it's something\nwe can keep in mind as new opportunities show up.\n\n-Peff\n"},{"id":"470290","messageId":"xmqqsfge49kv.fsf@gitster.g","threadId":"59043","inReplyTo":"230112.864jsv20r2.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] object-file: fix indent-with-space","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-13T17:40:16Z","receivedAt":"2023-01-13T17:49:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> diff --git a/object-file.c b/object-file.c\n>> index 80b08fc389..ce9efae994 100644\n>> --- a/object-file.c\n>> +++ b/object-file.c\n>> @@ -1708,7 +1708,7 @@ void *repo_read_object_file(struct repository *r,\n>>  \toi.sizep = size;\n>>  \toi.contentp = &data;\n>>  \tif (oid_object_info_extended(r, oid, &oi, flags))\n>> -\t    return NULL;\n>> +\t\treturn NULL;\n>>  \n>>  \treturn data;\n>>  }\n\nThanks, both, for being extra careful.\n\n> Thanks, I didn't notice (assuming it was too soon, it being less than a\n> week) that it was in \"next\" already. This change LGTM, thanks!\n\nIt would be surprising if an ordinary topic goes to 'master' without\nspending a week in 'next', but it is something I aim to merge a\nreasonably well-done topic down to 'next' from 'seen' with minimum\namount of time.  Here minimum usually means 1 wallclock day, just to\ncatch silly typos, if the patches are reviewed adequately on list by\nfolks (or possibly by me).\n\nThanks.\n"}]}