Options like hash-object --literally and cat-file --allow-unknown-type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[Breaking the In-Reply-To chain per recent discussions on list, would
have been sent out with [1] if not. See
https://lore.kernel.org/git/xmqqo8eem5hr.fsf@gitster.g/ for the
replied-to message in context].

Skip to "Get to the point" for the "tl;dr".

On Fri, Apr 16 2021, Junio C Hamano wrote:

> Han-Wen Nienhuys <hanwen@xxxxxxxxxx> writes:
>
>>> I would think that a better approach here would be to start with some
>>> (per-se unrelated) series to teach update-ref some mode like
>>> hash-object's --literally, i.e. "YOLO this ref update".
>>
>> I disagree.  I think this would be a job better suited to a
>> test-helper. Then we don't put tools into users' hands that
>> potentially corrupt the repository. I don't understand why hash-object
>> --literally is not a test helper either.
>
> As the person who invented the "--literally" option, I'd have to
> agree with this assessment.  It does make life a little bit easier
> for those who hack on Git codebase and reimplementations of it, but
> little practical value for those who use Git every day [*].
>
> If it were a tool to _dump_ the contents of a possibly corrupt
> object that the "--literally" option would have produced to make it
> easier to see by humans, I might be pursuaded to say that such a
> feature may be better in end-user accessible subcommands.  But the
> reason why we invented "--literally" was specifically to create
> corrupt objects in test repository to see end-user accessible tools
> behave, and in hindsight, such a use case shouldn't have been a good
> justification to add the option.
>
> I wasn't following the discussion of this particular patch closely,
> so I do not know what is being discussed is a tool on the diagnosis
> side (for which I may say it is OK to give to end-usres) or on the
> currupting side (for which I would regret to add to end-user tools),
> but hopefully the above would help guiding the decision between the
> two.
>
> Thanks.
>
>
> [Footnote]
>
> * ... for any purpose other than creating a corrupt repository,
>   asking somebody who is or claims to be a Git expert to figure out
>   what is wrong in his or her repository, and either waste expert's
>   time or have fun by watching the expert fail and gets embarrased,
>   that is ;-)

I totally get where you're coming from. We're talking about handing
users a loaded shotgun without the safety pin with --literally.

Having had some time to think about it though (and encountering various
code around it) since this E-Mail I think that on balance we're better
off with it than without it, and should extend it to update-ref et al.

I'm very much cognitively biased on the topic since 100% of my
interaction with these --literally objects is in hacking on git.git
itself, which if anything should serve as an argument not to have this
sort of thing in public tooling.

But I think one thing that makes git great is that we don't have opaque
magic at any level. Sure, we've got complexity, but you can always
devolve thing down to "simple" manipulation of the object store in cases
such as writing refs / loose objects.

So if for nothing else having a tool like hash-object take your raw
input is good for the educational value, even if it does (and should) do
sanity checking by default.

I actually think that like "mktag" it should do even more sanity
checking, I just think there should always be a way to turn that sort of
thing off. At the very lowest level our plumbing shouldn't be caring
about object validation at all. That also neatly allows us to
e.g. support creating an OBJ_NEW with a git version that doesn't
understand such a thing. We're unlikely to add new object types, but I
don't see why it shouldn't be supported.

In any case, to get to the point what prompted me to write this E-Mail
is that I'm annoyed that "cat-file --allow-unknown-type" exists at all,
i.e. I don't see the need for something like a "--literally" (originally
it was "cat-file --literally", see [2]) if we're talking about *reading*
objects, not writing them (hash-object without -w not withstanding).

In re-rolling my fsck fixes for what it does on unknown types[3] I've
ended up with sillyness like replacing the current bad error message
cat-file emits:

    $ git cat-file -t 7a52aafca353d2fd9f3faca575e9fe4fd48619d4
    fatal: invalid object type

With:

    $ ~/g/git/git cat-file -t 7a52aafca353d2fd9f3faca575e9fe4fd48619d4
    fatal: object 7a52aafca353d2fd9f3faca575e9fe4fd48619d4 is of unknown type 'bogus', refusing to emit it without --allow-unknown-type

I.e. now the guts of the object-file.c code don't have a way to ferry
that information up. Now it does via a "struct object_info", but (for
compatibility with documented behavior) it still refuses to "really"
print the type without --allow-unknown-type.

I think it should just print it unconditionally, perhaps with a
--no-allow-unknown-type "strict" mode.

We also currently don't support:

    $ git cat-file -p 7a52aafca353d2fd9f3faca575e9fe4fd48619d4
    fatal: invalid object type

But I've added support for that, we don't need to know about the type
itself to dump the raw content after the header, which is more useful,
and means you can roundtrip hash-object --literally and cat-file.

But most annoying is:

    $ echo 7a52aafca353d2fd9f3faca575e9fe4fd48619d4 | git cat-file --batch
    fatal: invalid object type

Now, looking at the implementation that's really a bug. We document:

    objecttype: The type of the object (the same as cat-file -t reports).

So you might expect it to accept an --allow-unknown-type, but it
doesn't. I could add support for that, but I think it would be stupid.

Why should you need to restart a "cat-file --batch" just because you
encounter a bad object? Just .. print it, we can do that safely. I
really don't see the point of having --allow-unknown-type at all. Ditto
for the --batch-check mode.

I mean, having read all the code I think I know why it's there. I think
It's because there was no way to ferry the information up from
object-file.c before my yet-to-be-submitted patches, so the solution was
to pass down a flag saying "please don't die()".

But is it something that anyone thinks is a good idea in the abstract? I
don't see why I shouldn't just document something like:

    Older versions of "cat-file" used to require an --allow-unknown-type
    flag to emit information about objects of unknown types. This is no
    longer required or the default. If you'd like to die on anything
    except known types (e.g. to die instead of bothering with parsing a
    "bad" type that possibly has spaces in it in the --batch output)
    supply --no-allow-unknown-type.

What do you think?

1.

    > In-reply-to: <xmqqo8eem5hr.fsf@gitster.g>

    > References:
    > <pull.847.v5.git.git.1615580397.gitgitgadget@xxxxxxxxx>
    > <pull.847.v6.git.git.1618255552.gitgitgadget@xxxxxxxxx>
    > <2dc73bf2ec96acae12a17c719083d11401775bc3.1618255553.git.gitgitgadget@xxxxxxxxx>
    > <87im4qejpk.fsf@xxxxxxxxxxxxxxxxxxx>
    > <CAFQ2z_OSbeciLQnoognG+Hh5S1tZTHO6WUviC9h5YMer766k6g@xxxxxxxxxxxxxx>
    > <xmqqo8eem5hr.fsf@gitster.g>

2. https://lore.kernel.org/git/CAPig+cTcAq_p3QXqcG+o1saWZyvDHCW=_JWYn6s7B1L4X5X1cQ@xxxxxxxxxxxxxx/

3. https://lore.kernel.org/git/87im493yos.fsf@xxxxxxxxxxxxxxxxxxx/



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux