git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,  Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH 3/3] cat-file: avoid verifying submodules' OIDs
Date: Tue, 12 Mar 2024 11:35:16 -0700	[thread overview]
Message-ID: <xmqqh6hbl2mz.fsf@gitster.g> (raw)
In-Reply-To: <951f73397c15f76da75bbd74a02f36da0116623f.1710183362.git.gitgitgadget@gmail.com> (Johannes Schindelin via GitGitGadget's message of "Mon, 11 Mar 2024 18:56:02 +0000")

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

> 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");

  parent reply	other threads:[~2024-03-12 18:35 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-11 18:55 [PATCH 0/3] cat-file: add %(objectmode) avoid verifying submodules' OIDs Johannes Schindelin via GitGitGadget
2024-03-11 18:56 ` [PATCH 1/3] t1006: update 'run_tests' to test generic object specifiers Victoria Dye via GitGitGadget
2024-03-11 21:54   ` Junio C Hamano
2024-03-11 18:56 ` [PATCH 2/3] cat-file: add %(objectmode) atom Victoria Dye via GitGitGadget
2024-03-11 22:15   ` Junio C Hamano
2024-03-13 21:23     ` Junio C Hamano
2024-03-11 18:56 ` [PATCH 3/3] cat-file: avoid verifying submodules' OIDs Johannes Schindelin via GitGitGadget
2024-03-12  8:58   ` Jeff King
2024-03-12 18:35   ` Junio C Hamano [this message]
2024-03-12 22:17     ` Jeff King
2024-03-13 15:22       ` Junio C Hamano
2024-03-11 21:43 ` [PATCH 0/3] cat-file: add %(objectmode) " Junio C Hamano
2024-03-12  8:59   ` Jeff King
2024-03-12 19:28     ` Junio C Hamano
2024-03-12 22:03       ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqh6hbl2mz.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=johannes.schindelin@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).