{"thread":{"id":"64313","subject":"[PATCH] builtin/cat-file.c: simplify calling `report_object_status()`","startedAt":"2025-10-13T21:56:04Z","lastAt":"2025-10-14T00:11:35Z","messageCount":2,"participants":["Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"528671","messageId":"9236fa6e654a59337266430a2a8807da21210cd4.1760392502.git.me@ttaylorr.com","threadId":"64313","inReplyTo":null,"subject":"[PATCH] builtin/cat-file.c: simplify calling `report_object_status()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-10-13T21:56:01Z","receivedAt":"2025-10-13T21:56:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In b0b910e052 (cat-file.c: add batch handling for submodules,\n2025-06-02), we began handling submodule entries specially when batching\ncat-file like so:\n\n  $ echo :sha1collisiondetection | git.compile cat-file --batch-check\n  855827c583bc30645ba427885caa40c5b81764d2 submodule\n\nCommit b0b910e052 notes that submodules are handled differently than\nnon-existent objects, which print \"<given-name> <type>\", since there is\n(a) no object to resolve the OID of in the first place, and as commit\nb0b910e052 notes, (b) for submodules in particular, it is useful to know\nwhat commit it points at without having to spawn another Git process.\n\nThat commit does so by calling report_object_status() and passing in\n\"oid_to_hex(&data->oid)\" for the \"obj_name\" parameter. This is\nunnecessary, however, since report_object_status() will do the same\nautomatically if given a NULL \"obj_name\" argument.\n\nThat behavior dates back to 6a951937ae (cat-file: add\n--batch-all-objects option, 2015-06-22), so rely on that instead of\nhaving the caller open-code that part of report_object_status().\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nI noticed this while merging v2.50.1 into GitHub's private fork, and\nthought it was a good opportunity for some light clean-up.\n\n builtin/cat-file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex ee6715fa52..5ca2ca3852 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -495,7 +495,7 @@ static void batch_object_write(const char *obj_name,\n \t\t\t\t\t\t\t    OBJECT_INFO_LOOKUP_REPLACE);\n \t\tif (ret < 0) {\n \t\t\tif (data->mode == S_IFGITLINK)\n-\t\t\t\treport_object_status(opt, oid_to_hex(&data->oid), &data->oid, \"submodule\");\n+\t\t\t\treport_object_status(opt, NULL, &data->oid, \"submodule\");\n \t\t\telse\n \t\t\t\treport_object_status(opt, obj_name, &data->oid, \"missing\");\n \t\t\treturn;\n\nbase-commit: 4b71b294773cc4f7fe48ec3a70079aa8783f373d\n--\n2.51.0.491.g4b71b294773.dirty\n"},{"id":"528676","messageId":"20251014001128.GB1507@coredump.intra.peff.net","threadId":"64313","inReplyTo":"9236fa6e654a59337266430a2a8807da21210cd4.1760392502.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/cat-file.c: simplify calling `report_object_status()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-14T00:11:28Z","receivedAt":"2025-10-14T00:11:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 13, 2025 at 05:56:01PM -0400, Taylor Blau wrote:\n\n> That commit does so by calling report_object_status() and passing in\n> \"oid_to_hex(&data->oid)\" for the \"obj_name\" parameter. This is\n> unnecessary, however, since report_object_status() will do the same\n> automatically if given a NULL \"obj_name\" argument.\n\nYeah, looking at the code, this should obviously be a noop change, and I\nthink it simplifies things a little.\n\nIt is interesting that \"oid\" is not used in report_object_status()\nexcept for this fallback. Which kind of makes me wonder if we could\nditch it completely, and just pass int oid_to_hex() unconditionally\nhere. But it's hard to say if other code paths might end up with a NULL\nobj_name somehow (e.g., in an error path).\n\n...poking at it...\n\nAh, indeed. The patch below does fail one test in t5313 with a corrupted\npack. So not worth pursuing that further simplification.\n\nYour patch looks good to me. :)\n\n-Peff\n\n-- >8 --\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex ee6715fa52..19625b5a64 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -455,11 +455,9 @@ static void print_default_format(struct strbuf *scratch, struct expand_data *dat\n \n static void report_object_status(struct batch_options *opt,\n \t\t\t\t const char *obj_name,\n-\t\t\t\t const struct object_id *oid,\n \t\t\t\t const char *status)\n {\n-\tprintf(\"%s %s%c\", obj_name ? obj_name : oid_to_hex(oid),\n-\t       status, opt->output_delim);\n+\tprintf(\"%s %s%c\", obj_name, status, opt->output_delim);\n \tfflush(stdout);\n }\n \n@@ -495,9 +493,9 @@ static void batch_object_write(const char *obj_name,\n \t\t\t\t\t\t\t    OBJECT_INFO_LOOKUP_REPLACE);\n \t\tif (ret < 0) {\n \t\t\tif (data->mode == S_IFGITLINK)\n-\t\t\t\treport_object_status(opt, oid_to_hex(&data->oid), &data->oid, \"submodule\");\n+\t\t\t\treport_object_status(opt, oid_to_hex(&data->oid), \"submodule\");\n \t\t\telse\n-\t\t\t\treport_object_status(opt, obj_name, &data->oid, \"missing\");\n+\t\t\t\treport_object_status(opt, obj_name, \"missing\");\n \t\t\treturn;\n \t\t}\n \n@@ -507,25 +505,22 @@ static void batch_object_write(const char *obj_name,\n \t\tcase LOFC_BLOB_NONE:\n \t\t\tif (data->type == OBJ_BLOB) {\n \t\t\t\tif (!opt->all_objects)\n-\t\t\t\t\treport_object_status(opt, obj_name,\n-\t\t\t\t\t\t\t     &data->oid, \"excluded\");\n+\t\t\t\t\treport_object_status(opt, obj_name, \"excluded\");\n \t\t\t\treturn;\n \t\t\t}\n \t\t\tbreak;\n \t\tcase LOFC_BLOB_LIMIT:\n \t\t\tif (data->type == OBJ_BLOB &&\n \t\t\t    data->size >= opt->objects_filter.blob_limit_value) {\n \t\t\t\tif (!opt->all_objects)\n-\t\t\t\t\treport_object_status(opt, obj_name,\n-\t\t\t\t\t\t\t     &data->oid, \"excluded\");\n+\t\t\t\t\treport_object_status(opt, obj_name, \"excluded\");\n \t\t\t\treturn;\n \t\t\t}\n \t\t\tbreak;\n \t\tcase LOFC_OBJECT_TYPE:\n \t\t\tif (data->type != opt->objects_filter.object_type) {\n \t\t\t\tif (!opt->all_objects)\n-\t\t\t\t\treport_object_status(opt, obj_name,\n-\t\t\t\t\t\t\t     &data->oid, \"excluded\");\n+\t\t\t\t\treport_object_status(opt, obj_name, \"excluded\");\n \t\t\t\treturn;\n \t\t\t}\n \t\t\tbreak;\n@@ -581,10 +576,10 @@ static void batch_one_object(const char *obj_name,\n \tif (result != FOUND) {\n \t\tswitch (result) {\n \t\tcase MISSING_OBJECT:\n-\t\t\treport_object_status(opt, obj_name, &data->oid, \"missing\");\n+\t\t\treport_object_status(opt, obj_name, \"missing\");\n \t\t\tbreak;\n \t\tcase SHORT_NAME_AMBIGUOUS:\n-\t\t\treport_object_status(opt, obj_name, &data->oid, \"ambiguous\");\n+\t\t\treport_object_status(opt, obj_name, \"ambiguous\");\n \t\t\tbreak;\n \t\tcase DANGLING_SYMLINK:\n \t\t\tprintf(\"dangling %\"PRIuMAX\"%c%s%c\",\n"}]}