Re: [PATCH 3/3] cat-file: avoid verifying submodules' OIDs

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 12, 2024, 18:35 UTC
Message-ID: <xmqqh6hbl2mz.fsf@gitster.g>
In-Reply-To: <951f73397c15f76da75bbd74a02f36da0116623f.1710183362.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +Submodules are handled specially in `git cat-file`, as the objects
> +corresponding to the recorded OIDs are not expected to be present in the
> +current repository. For that reason, submodules are reported as having
> +type `submodule` and mode 1600000 and all other fields are zeroed out.

While the above may not be technically wrong per-se, I am not sure if that is the more important part of what we want to tell our users. For example, "git ls-tree HEAD -- sha1collisiondetection" reports "160000 commit ...object.name.... sha1collisiondetection". Is it correct to say ...

    Submodules are handled specially in `git ls-tree`, as the
    objects corresponding to the recorded OIDs are not expected to
    be present in the current repository.

...?  I do not think so.

For the same reason, as an explanation for the reason why "git cat-file -t :sha1collisiondetection" just reports "submodule", the new text does not sit well.

I actually have to wonder if the new behaviour proposed by this patch is a solution that is in search of a problem, or trying to solve an unstated problem in a wrong way.

    O=$(git rev-parse --verify :sha1collisiondetection)
    git cat-file -t "$O"
    
should fail because the object whose name is $O is not available.
Why should then this succeed and give a different result?

    git cat-file -t :sha1collisiondetection

The "cat-file" command is about objects. While object's type may sometimes be inferrable (by being contained in a tree), if the user asks us to determine the type of the object, we should actually hit the object store, whether the commit object in question happens to be on our history or somebody else's history that our gitlink points at.

So, I am not yet convinced that I should take this patch. Previous two steps looked good, though.

Thanks.

Show 12 quoted lines

> index 73bd78c0b63..c59ad682d1f 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -128,7 +128,9 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
>  	switch (opt) {
>  	case 't':
>  		oi.type_name = &sb;
> -		if (oid_object_info_extended(the_repository, &oid, &oi, flags) < 0)
> +		if (obj_context.mode == S_IFGITLINK)
> +			strbuf_addstr(&sb, "submodule");
> +		else if (oid_object_info_extended(the_repository, &oid, &oi, flags) < 0)
>  			die("git cat-file: could not get object info");