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

[Patrick Steinhardt <ps@pks.im>]

On Mon, May 19, 2025 at 02:16:17AM -0400, Jeff King wrote:
> On Fri, May 16, 2025 at 11:52:18AM +0200, Patrick Steinhardt wrote:
> 
> > > 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.

Fair. I don't have a strong opinion here.

> > > @@ -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.

Well, the use case I'm worried about is person X that inherits a script
that uses this option. They may wonder what the option does, but if we
don't mention it at all in the man page they won't have a chance to
learn that it is a no-op without digging into Git's history.

Patrick