Re: [PATCH 3/3] cat-file.c: add batch handling for submodules

From: Jeff King <peff@peff.net>
Date: Jun 4, 2025, 19:54 UTC
Message-ID: <20250604195455.GB1500045@coredump.intra.peff.net>
In-Reply-To: <980ab7f7ef56944df78530dcc9c79b54d1450806.1748890555.git.gitgitgadget@gmail.com>

On Mon, Jun 02, 2025 at 06:55:55PM +0000, Victoria Dye via GitGitGadget wrote:

Show 5 quoted lines

> To disambiguate without needing to invoke a separate Git process (e.g.
> 'ls-tree'), print the message "<oid> submodule" for such objects instead of
> "<object> missing". In addition to the change from "missing" to "submodule",
> the new message differs from the old in that it always prints the resolved
> tree entry's OID, rather than the input object specification.

OK. I read over the discussion from last year, which I think mostly centered around this patch. I do still think in the long run it would be nice for cat-file to produce what output it _can_ for a missing object (e.g., the oid and mode).

But I think it is OK to punt on that for now. Because "<oid> missing" lines already exist, we'd probably need to put such behavior behind a new command-line option. So while "<oid> submodule" lines would be unnecessary in that hypothetical future world, we are not digging the hole any deeper, from a backwards-compatibility standpoint.

Although speaking of backwards compatibility, I guess older readers may be surprised that the old "missing" message becomes a "submodule" one. They may need to be updated if they were written carefully to bail on unknown input (and were happy seeing "missing" messages for submodules). So there may be some fallout, but it's not like the existing messages were particularly useful in the first place.

> Note that this implementation maintains a distinction between submodules
> where the commit OID is not present in the repo, and submodules where the
> commit OID *is* present; the former will now print "<object> submodule", but
> the latter will still print the full object content.

Hmm, that is an interesting point. It feels kind of arbitrary, but I'm having trouble making a strong argument for one direction or the other. The way you've written it means that readers need to be prepared to parse _both_ the mode and "<oid> submodule" lines to find submodules. But maybe there's some value in finding out more information about submodule commits you do have in-repo.

The implementations are similar. Replacing this hunk:

Show 16 quoted lines

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index b11576756bcc..4b23fcecbd8e 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -496,7 +496,10 @@ static void batch_object_write(const char *obj_name,
>  						       &data->oid, &data->info,
>  						       OBJECT_INFO_LOOKUP_REPLACE);
>  		if (ret < 0) {
> -			report_object_status(opt, obj_name, &data->oid, "missing");
> +			if (data->mode == S_IFGITLINK)
> +				report_object_status(opt, oid_to_hex(&data->oid), &data->oid, "submodule");
> +			else
> +				report_object_status(opt, obj_name, &data->oid, "missing");
>  			return;
>  		}
>

with:

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 4b23fcecbd..1b200e1607 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -488,6 +488,11 @@ static void batch_object_write(const char *obj_name,
 		if (opt->objects_filter.choice == LOFC_BLOB_LIMIT)
 			data->info.sizep = &data->size;
 
+		if (data->mode == S_IFGITLINK) {
+			report_object_status(opt, oid_to_hex(&data->oid), &data->oid, "submodule");
+			return;
+		}
+
 		if (pack)
 			ret = packed_object_info(the_repository, pack, offset,
 						 &data->info);

so I think the decision is really about what people will find most
useful. So I dunno. It is mostly a coin-flip, leading me to say that
what you picked just came up "heads" and is good enough. ;)

-Peff