From: Jeff King <peff@peff.net>
Date: Tue, 14 Oct 2025 00:11:28 GMT
Subject: Re: [PATCH] builtin/cat-file.c: simplify calling `report_object_status()`
Message-ID: <20251014001128.GB1507@coredump.intra.peff.net>
In-Reply-To: <9236fa6e654a59337266430a2a8807da21210cd4.1760392502.git.me@ttaylorr.com>

On Mon, Oct 13, 2025 at 05:56:01PM -0400, Taylor Blau wrote:

> That commit does so by calling report_object_status() and passing in
> "oid_to_hex(&data->oid)" for the "obj_name" parameter. This is
> unnecessary, however, since report_object_status() will do the same
> automatically if given a NULL "obj_name" argument.

Yeah, looking at the code, this should obviously be a noop change, and I
think it simplifies things a little.

It is interesting that "oid" is not used in report_object_status()
except for this fallback. Which kind of makes me wonder if we could
ditch it completely, and just pass int oid_to_hex() unconditionally
here. But it's hard to say if other code paths might end up with a NULL
obj_name somehow (e.g., in an error path).

...poking at it...

Ah, indeed. The patch below does fail one test in t5313 with a corrupted
pack. So not worth pursuing that further simplification.

Your patch looks good to me. :)

-Peff

-- >8 --
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index ee6715fa52..19625b5a64 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -455,11 +455,9 @@ static void print_default_format(struct strbuf *scratch, struct expand_data *dat
 
 static void report_object_status(struct batch_options *opt,
 				 const char *obj_name,
-				 const struct object_id *oid,
 				 const char *status)
 {
-	printf("%s %s%c", obj_name ? obj_name : oid_to_hex(oid),
-	       status, opt->output_delim);
+	printf("%s %s%c", obj_name, status, opt->output_delim);
 	fflush(stdout);
 }
 
@@ -495,9 +493,9 @@ static void batch_object_write(const char *obj_name,
 							    OBJECT_INFO_LOOKUP_REPLACE);
 		if (ret < 0) {
 			if (data->mode == S_IFGITLINK)
-				report_object_status(opt, oid_to_hex(&data->oid), &data->oid, "submodule");
+				report_object_status(opt, oid_to_hex(&data->oid), "submodule");
 			else
-				report_object_status(opt, obj_name, &data->oid, "missing");
+				report_object_status(opt, obj_name, "missing");
 			return;
 		}
 
@@ -507,25 +505,22 @@ static void batch_object_write(const char *obj_name,
 		case LOFC_BLOB_NONE:
 			if (data->type == OBJ_BLOB) {
 				if (!opt->all_objects)
-					report_object_status(opt, obj_name,
-							     &data->oid, "excluded");
+					report_object_status(opt, obj_name, "excluded");
 				return;
 			}
 			break;
 		case LOFC_BLOB_LIMIT:
 			if (data->type == OBJ_BLOB &&
 			    data->size >= opt->objects_filter.blob_limit_value) {
 				if (!opt->all_objects)
-					report_object_status(opt, obj_name,
-							     &data->oid, "excluded");
+					report_object_status(opt, obj_name, "excluded");
 				return;
 			}
 			break;
 		case LOFC_OBJECT_TYPE:
 			if (data->type != opt->objects_filter.object_type) {
 				if (!opt->all_objects)
-					report_object_status(opt, obj_name,
-							     &data->oid, "excluded");
+					report_object_status(opt, obj_name, "excluded");
 				return;
 			}
 			break;
@@ -581,10 +576,10 @@ static void batch_one_object(const char *obj_name,
 	if (result != FOUND) {
 		switch (result) {
 		case MISSING_OBJECT:
-			report_object_status(opt, obj_name, &data->oid, "missing");
+			report_object_status(opt, obj_name, "missing");
 			break;
 		case SHORT_NAME_AMBIGUOUS:
-			report_object_status(opt, obj_name, &data->oid, "ambiguous");
+			report_object_status(opt, obj_name, "ambiguous");
 			break;
 		case DANGLING_SYMLINK:
 			printf("dangling %"PRIuMAX"%c%s%c",