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
