Re: [PATCH 02/13] cat-file: make --allow-unknown-type a noop

From: Jeff King <peff@peff.net>
Date: May 19, 2025, 06:16 UTC
Message-ID: <20250519061617.GA102701@coredump.intra.peff.net>
In-Reply-To: <aCcK0p7wMURHppe7@pks.im>

On Fri, May 16, 2025 at 11:52:18AM +0200, Patrick Steinhardt wrote:

Show 9 quoted lines

> > The code change is fairly small (but we'll able to clean up more code in
> > follow-on patches). The test updates drop any use of the option. We
> > still retain tests that feed the broken objects to cat-file without
> > --allow-unknown-type, as we should continue to confirm that those
> > objects are rejected. Note that in one spot we can drop a layer of loop,
> > re-indenting the body; viewing the diff with "-w" helps there.
> 
> Shouldn't we have a test though that the option is still accepted, even
> though it doesn't do anything?

I dunno. It is obvious-ish from looking at the code that the option does nothing, so we know that it will behave the same whether it is provided or not. I guess it depends on how white/black-box we want our tests to be.

If we did this on top:

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 67a5ff2b9e..ff92b14201 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -1022,7 +1022,6 @@ int cmd_cat_file(int argc,
 	struct batch_options batch = {
 		.objects_filter = LIST_OBJECTS_FILTER_INIT,
 	};
-	int unknown_type = 0;
 	int input_nul_terminated = 0;
 	int nul_terminated = 0;
 	int ret;
@@ -1047,8 +1046,7 @@ int cmd_cat_file(int argc,
 		OPT_GROUP(N_("Emit [broken] object attributes")),
 		OPT_CMDMODE('t', NULL, &opt, N_("show object type (one of 'blob', 'tree', 'commit', 'tag', ...)"), 't'),
 		OPT_CMDMODE('s', NULL, &opt, N_("show object size"), 's'),
-		OPT_HIDDEN_BOOL(0, "allow-unknown-type", &unknown_type,
-			  N_("historical option -- no-op")),
+		OPT_NOOP_NOARG(0, "allow-unknown-type"),
 		OPT_BOOL(0, "use-mailmap", &use_mailmap, N_("use mail map file")),
 		OPT_ALIAS(0, "mailmap", "use-mailmap"),
 		/* Batch mode */

that would perhaps remove the "-ish" from "obvious-ish". I had left the
flag in place because I wondered if we might want to produce a
deprecation warning before dropping it completely.

> > @@ -202,9 +201,6 @@ flush::
> >  	only once, even if it is stored multiple times in the
> >  	repository.
> >  
> > ---allow-unknown-type::
> > -	Allow `-s` or `-t` to query broken/corrupt objects of unknown type.
> > -
> 
> Should we maybe introduce a "deprecated" section and spell out that this
> option is a no-op nowadays that will be removed for example in Git 3.0?

I don't have a strong opinion there. It mostly seems like clutter to me
in the manpage. In theory it could help somebody who had learned about
the option previously and wondered what happened. OTOH, the release
notes can help with that. With the patch above, "cat-file --help-all"
would also produce:

     --[no-]allow-unknown-type
                            no-op (backward compatibility)

though I don't really expect anybody to find that casually.

-Peff