{"thread":{"id":"55725","subject":"Options like hash-object --literally and cat-file --allow-unknown-type","startedAt":"2021-05-18T14:40:52Z","lastAt":"2021-05-19T00:26:01Z","messageCount":4,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"424880","messageId":"87r1i4qf4h.fsf@evledraar.gmail.com","threadId":"55725","inReplyTo":null,"subject":"Options like hash-object --literally and cat-file --allow-unknown-type","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-18T14:08:18Z","receivedAt":"2021-05-18T14:40:52Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\n[Breaking the In-Reply-To chain per recent discussions on list, would\nhave been sent out with [1] if not. See\nhttps://lore.kernel.org/git/xmqqo8eem5hr.fsf@gitster.g/ for the\nreplied-to message in context].\n\nSkip to \"Get to the point\" for the \"tl;dr\".\n\nOn Fri, Apr 16 2021, Junio C Hamano wrote:\n\n> Han-Wen Nienhuys writes:\n>\n>>> I would think that a better approach here would be to start with some\n>>> (per-se unrelated) series to teach update-ref some mode like\n>>> hash-object's --literally, i.e. \"YOLO this ref update\".\n>>\n>> I disagree. I think this would be a job better suited to a\n>> test-helper. Then we don't put tools into users' hands that\n>> potentially corrupt the repository. I don't understand why hash-object\n>> --literally is not a test helper either.\n>\n> As the person who invented the \"--literally\" option, I'd have to\n> agree with this assessment. It does make life a little bit easier\n> for those who hack on Git codebase and reimplementations of it, but\n> little practical value for those who use Git every day [*].\n>\n> If it were a tool to _dump_ the contents of a possibly corrupt\n> object that the \"--literally\" option would have produced to make it\n> easier to see by humans, I might be pursuaded to say that such a\n> feature may be better in end-user accessible subcommands. But the\n> reason why we invented \"--literally\" was specifically to create\n> corrupt objects in test repository to see end-user accessible tools\n> behave, and in hindsight, such a use case shouldn't have been a good\n> justification to add the option.\n>\n> I wasn't following the discussion of this particular patch closely,\n> so I do not know what is being discussed is a tool on the diagnosis\n> side (for which I may say it is OK to give to end-usres) or on the\n> currupting side (for which I would regret to add to end-user tools),\n> but hopefully the above would help guiding the decision between the\n> two.\n>\n> Thanks.\n>\n>\n> [Footnote]\n>\n> * ... for any purpose other than creating a corrupt repository,\n> asking somebody who is or claims to be a Git expert to figure out\n> what is wrong in his or her repository, and either waste expert's\n> time or have fun by watching the expert fail and gets embarrased,\n> that is ;-)\n\nI totally get where you're coming from. We're talking about handing\nusers a loaded shotgun without the safety pin with --literally.\n\nHaving had some time to think about it though (and encountering various\ncode around it) since this E-Mail I think that on balance we're better\noff with it than without it, and should extend it to update-ref et al.\n\nI'm very much cognitively biased on the topic since 100% of my\ninteraction with these --literally objects is in hacking on git.git\nitself, which if anything should serve as an argument not to have this\nsort of thing in public tooling.\n\nBut I think one thing that makes git great is that we don't have opaque\nmagic at any level. Sure, we've got complexity, but you can always\ndevolve thing down to \"simple\" manipulation of the object store in cases\nsuch as writing refs / loose objects.\n\nSo if for nothing else having a tool like hash-object take your raw\ninput is good for the educational value, even if it does (and should) do\nsanity checking by default.\n\nI actually think that like \"mktag\" it should do even more sanity\nchecking, I just think there should always be a way to turn that sort of\nthing off. At the very lowest level our plumbing shouldn't be caring\nabout object validation at all. That also neatly allows us to\ne.g. support creating an OBJ_NEW with a git version that doesn't\nunderstand such a thing. We're unlikely to add new object types, but I\ndon't see why it shouldn't be supported.\n\nIn any case, to get to the point what prompted me to write this E-Mail\nis that I'm annoyed that \"cat-file --allow-unknown-type\" exists at all,\ni.e. I don't see the need for something like a \"--literally\" (originally\nit was \"cat-file --literally\", see [2]) if we're talking about *reading*\nobjects, not writing them (hash-object without -w not withstanding).\n\nIn re-rolling my fsck fixes for what it does on unknown types[3] I've\nended up with sillyness like replacing the current bad error message\ncat-file emits:\n\n $ git cat-file -t 7a52aafca353d2fd9f3faca575e9fe4fd48619d4\n fatal: invalid object type\n\nWith:\n\n $ ~/g/git/git cat-file -t 7a52aafca353d2fd9f3faca575e9fe4fd48619d4\n fatal: object 7a52aafca353d2fd9f3faca575e9fe4fd48619d4 is of unknown type 'bogus', refusing to emit it without --allow-unknown-type\n\nI.e. now the guts of the object-file.c code don't have a way to ferry\nthat information up. Now it does via a \"struct object_info\", but (for\ncompatibility with documented behavior) it still refuses to \"really\"\nprint the type without --allow-unknown-type.\n\nI think it should just print it unconditionally, perhaps with a\n--no-allow-unknown-type \"strict\" mode.\n\nWe also currently don't support:\n\n $ git cat-file -p 7a52aafca353d2fd9f3faca575e9fe4fd48619d4\n fatal: invalid object type\n\nBut I've added support for that, we don't need to know about the type\nitself to dump the raw content after the header, which is more useful,\nand means you can roundtrip hash-object --literally and cat-file.\n\nBut most annoying is:\n\n $ echo 7a52aafca353d2fd9f3faca575e9fe4fd48619d4 | git cat-file --batch\n fatal: invalid object type\n\nNow, looking at the implementation that's really a bug. We document:\n\n objecttype: The type of the object (the same as cat-file -t reports).\n\nSo you might expect it to accept an --allow-unknown-type, but it\ndoesn't. I could add support for that, but I think it would be stupid.\n\nWhy should you need to restart a \"cat-file --batch\" just because you\nencounter a bad object? Just .. print it, we can do that safely. I\nreally don't see the point of having --allow-unknown-type at all. Ditto\nfor the --batch-check mode.\n\nI mean, having read all the code I think I know why it's there. I think\nIt's because there was no way to ferry the information up from\nobject-file.c before my yet-to-be-submitted patches, so the solution was\nto pass down a flag saying \"please don't die()\".\n\nBut is it something that anyone thinks is a good idea in the abstract? I\ndon't see why I shouldn't just document something like:\n\n Older versions of \"cat-file\" used to require an --allow-unknown-type\n flag to emit information about objects of unknown types. This is no\n longer required or the default. If you'd like to die on anything\n except known types (e.g. to die instead of bothering with parsing a\n \"bad\" type that possibly has spaces in it in the --batch output)\n supply --no-allow-unknown-type.\n\nWhat do you think?\n\n1.\n\n > In-reply-to: \n\n > References:\n > \n > \n > <2dc73bf2ec96acae12a17c719083d11401775bc3.1618255553.git.gitgitgadget@gmail.com>\n > <87im4qejpk.fsf@evledraar.gmail.com>\n > \n > \n\n2. https://lore.kernel.org/git/CAPig+cTcAq_p3QXqcG+o1saWZyvDHCW=_JWYn6s7B1L4X5X1cQ@mail.gmail.com/\n\n3. https://lore.kernel.org/git/87im493yos.fsf@evledraar.gmail.com/\n"},{"id":"424886","messageId":"xmqqtun0139q.fsf@gitster.g","threadId":"55725","inReplyTo":"87r1i4qf4h.fsf@evledraar.gmail.com","subject":"Re: Options like hash-object --literally and cat-file --allow-unknown-type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-18T15:16:01Z","receivedAt":"2021-05-18T15:16:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> So you might expect it to accept an --allow-unknown-type, but it\n> doesn't. I could add support for that, but I think it would be stupid.\n>\n> Why should you need to restart a \"cat-file --batch\" just because you\n> encounter a bad object? Just .. print it, we can do that safely. I\n> really don't see the point of having --allow-unknown-type at all. Ditto\n> for the --batch-check mode.\n>\n> I mean, having read all the code I think I know why it's there. I think\n> It's because there was no way to ferry the information up from\n> object-file.c before my yet-to-be-submitted patches, so the solution was\n> to pass down a flag saying \"please don't die()\".\n>\n> But is it something that anyone thinks is a good idea in the abstract? I\n> don't see why I shouldn't just document something like:\n>\n> Older versions of \"cat-file\" used to require an --allow-unknown-type\n> flag to emit information about objects of unknown types. This is no\n> longer required or the default. If you'd like to die on anything\n> except known types (e.g. to die instead of bothering with parsing a\n> \"bad\" type that possibly has spaces in it in the --batch output)\n> supply --no-allow-unknown-type.\n>\n> What do you think?\n\nThanks for thinking things through.\n\nMy knee-jerk reaction is\n\n - As long as \"cat-file -t $thing\" exits with non-zero status for an\n invalid thing, which was crafted using hash-object --literally,\n reporting the typename it read from the object header to its\n standard output would be fine without \"--allow-unknown-type\".\n But scripts would be upset if it suddenly started to return with\n zero status when asked to check what type of object the $thing\n is.\n\n - For \"cat-file --batch[-check]\", I am on the fence. A script may\n break because it is not prepared to see anything but four\n existing types (so it might even say \"do X if it is a blob, do Y\n if it is a tree, do Z if it is a tag, and do W for everything\n else\" and expect/assume that W will see only commits), so failing\n without --allow may still be a prudent thing to do. We could\n declare that such a script is already broken, even though it\n would not change the fact that the user has been successfully\n using it reliably.\n\nWith your new \"we can now bubble necessary information up from the\nobject-file.c layer without dying\" change, it might make it easier\nto arrange so that object-file.c layer would never die and I do not\nthink I have any objection against such a plan. The implementation\nof \"--[no-]allow-unknown-type\" would have to be migrated to the\ncaller at the higher level to decide what to do when it learns that\nthe object it asked the object_info() about is of an invalid type.\nAnd the choice of the default would become easier to change later\nwith such a change to the lower layer. But I am not sure if that\nwarrants switching of the default.\n"},{"id":"424898","messageId":"87o8d7rhwi.fsf@evledraar.gmail.com","threadId":"55725","inReplyTo":"xmqqtun0139q.fsf@gitster.g","subject":"Re: Options like hash-object --literally and cat-file --allow-unknown-type","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-18T18:42:26Z","receivedAt":"2021-05-18T18:55:30Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, May 19 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> So you might expect it to accept an --allow-unknown-type, but it\n>> doesn't. I could add support for that, but I think it would be stupid.\n>>\n>> Why should you need to restart a \"cat-file --batch\" just because you\n>> encounter a bad object? Just .. print it, we can do that safely. I\n>> really don't see the point of having --allow-unknown-type at all. Ditto\n>> for the --batch-check mode.\n>>\n>> I mean, having read all the code I think I know why it's there. I think\n>> It's because there was no way to ferry the information up from\n>> object-file.c before my yet-to-be-submitted patches, so the solution was\n>> to pass down a flag saying \"please don't die()\".\n>>\n>> But is it something that anyone thinks is a good idea in the abstract? I\n>> don't see why I shouldn't just document something like:\n>>\n>> Older versions of \"cat-file\" used to require an --allow-unknown-type\n>> flag to emit information about objects of unknown types. This is no\n>> longer required or the default. If you'd like to die on anything\n>> except known types (e.g. to die instead of bothering with parsing a\n>> \"bad\" type that possibly has spaces in it in the --batch output)\n>> supply --no-allow-unknown-type.\n>>\n>> What do you think?\n>\n> Thanks for thinking things through.\n>\n> My knee-jerk reaction is\n>\n> - As long as \"cat-file -t $thing\" exits with non-zero status for an\n> invalid thing, which was crafted using hash-object --literally,\n> reporting the typename it read from the object header to its\n> standard output would be fine without \"--allow-unknown-type\".\n> But scripts would be upset if it suddenly started to return with\n> zero status when asked to check what type of object the $thing\n> is.\n\nThat feels even weirder to me. Why conflate the \"this is naughty object\"\nwith \"we successfully inspected it\"? I don't think cat-file should be\nexiting non-zero unless it actually has an error inspecting the object.\n\nBut in terms of implementation that's trivial, just look if the object\nis on the whitelist of types before exiting. It's certainly less hassle\nthan what it's currently doing.\n\nI was thinking something like an advise() to stderr at most when we\nemitted one of these:\n\n> - For \"cat-file --batch[-check]\", I am on the fence. A script may\n> break because it is not prepared to see anything but four\n> existing types (so it might even say \"do X if it is a blob, do Y\n> if it is a tree, do Z if it is a tag, and do W for everything\n> else\" and expect/assume that W will see only commits), so failing\n> without --allow may still be a prudent thing to do. We could\n> declare that such a script is already broken, even though it\n> would not change the fact that the user has been successfully\n> using it reliably.\n\nThe main reason I even care about this was the fsck case, and it looking\nup and down the callstacks from OBJECT_INFO_ALLOW_UNKNOWN_TYPE I think\nwe're needlessly worrying about this in the cat-file case.\n\nI think even in the case you mentioned a script would be better off with\na default of --allow-unknown-type. Right now if you somehow fed it an\nobject with such a type it would just hard die, whereas after you could\nproceed to feed it other objects.\n\nBut realistically (and I may be wrong here) I don't think we need to\nworry about backwards compatibility with --allow-unknown-type at\nall. I'm fairly certain nobody's dealing with these objects in the wild\nexcept git's own test suite.\n\nNo server implementation will accept them, you can't even push/pull them\nlocally. It's just there for completeness with \"hash-object\n--literally\": https://lore.kernel.org/git/54EDACC9.5080204@gmail.com/\n\nSo I think it's fine just to say it was a mistake to require opt-in\noptions to *inspect* such naughty objects, as opposed to creating them\n(where we should have the opt-in --literally option).\n\n> With your new \"we can now bubble necessary information up from the\n> object-file.c layer without dying\" change, it might make it easier\n> to arrange so that object-file.c layer would never die and I do not\n> think I have any objection against such a plan. The implementation\n> of \"--[no-]allow-unknown-type\" would have to be migrated to the\n> caller at the higher level to decide what to do when it learns that\n> the object it asked the object_info() about is of an invalid type.\n> And the choice of the default would become easier to change later\n> with such a change to the lower layer.\n\nFor what it's worth the new API is just passing\nOBJECT_INFO_ALLOW_UNKNOWN_TYPE to oid_object_info_extended() along with\nthe (existing) \"typep\". You then see if the typep is < 0.\n\n> But I am not sure if that warrants switching of the default.\n\nHopefully either this thread or a re-roll of that series will convince\nyou, is this something you'd like to see on list in the next few days or\nis it better to hold off until after the RC period?\n\nAnother thing I didn't mention as a reason for getting rid of\n--allow-unknown-type is that part of the patches I was hacking up made\nvarious read-only commants behave sensibly in the face of these badly\ntyped objects, where now they'll just die.\n\nE.g. Jeff King's recent --disk-usage for rev-list will just die on it,\nsince we call \"what is the type of this\" without\nOBJECT_INFO_ALLOW_UNKNOWN_TYPE somewhere.\n\nI think it's a much better approach to just implicitly supply a\nOBJECT_INFO_ALLOW_UNKNOWN_TYPE at a high level (you just want the total\ndisk use, who cares if there's one bad object there), as opposed to\nteaching rev-list and others about --allow-unknown-type.\n\n"},{"id":"424906","messageId":"xmqqpmxn1sdo.fsf@gitster.g","threadId":"55725","inReplyTo":"87o8d7rhwi.fsf@evledraar.gmail.com","subject":"Re: Options like hash-object --literally and cat-file --allow-unknown-type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-19T00:25:55Z","receivedAt":"2021-05-19T00:26:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> But I am not sure if that warrants switching of the default.\n>\n> Hopefully either this thread or a re-roll of that series will convince\n> you, is this something you'd like to see on list in the next few days or\n> is it better to hold off until after the RC period?\n\nYou sent me a message, said that you wanted to change \"cat-file\" and\nthat you wanted my opinion on it, I gave a response, and in response\nto that, you just repeated that you still want to change it, and I\ndid not see any new justification with it in the message I am\nresponding to, so sorry, but I fail to see where that \"hopefully\"\ncomes from.\n\nThe way I see it, if there is no practical difference, leaving the\nexisting behaviour of \"cat-file\" the same would be the most prudent\nthing to do, especially when the primary goal of the topic is not\nabout improving \"cat-file\". This is merely a fallout from the work\nto improve lower layers so that \"fsck\" can work better with objects\nwith unexpected types, no?\n\nI suspect that this is one of the reasons your topics take way too\nmore time than they should to mature: biting too much at one time.\n\nSurely it will be about the same effort for you to change the\ndefault, while updating the lower layer that results in making it\neasier to have either default. However, having to argue for and\njustify the change of default of a command unrelated to the main\nfocus of the topic would make your series longer, and adds one more\nthing on the plate of reviewers and future readers of the series\nthat they have to reason about. If you keep the topic more focused\nto \"fsck\" and the improvement of the lower layer to support it, you\nmay still need to update the callers that share the same lower layer\n(\"cat-file\" in this case) but you would reduce the burden of\nreviewers if you can label the change as \"this adjust another caller\nto the updated lower layer, but no end-user visible change in its\nbehaviour\", as there is one fewer thing for them to have to think.\n\n> I think it's a much better approach to just implicitly supply a\n> OBJECT_INFO_ALLOW_UNKNOWN_TYPE at a high level (you just want the total\n> disk use, who cares if there's one bad object there), as opposed to\n> teaching rev-list and others about --allow-unknown-type.\n\nI actually was hoping that the update to the lower layer functions\nwould be to stop dying when they see an unknown type (instead they\ncan leave a bit of information in object_info to signal that the\nobject is bogus---perhaps the *typep having a value outside the four\nreal types may be a good enough implementation for that signal) and\nleave the policy decision to the callers. That is, there is no\noption bit OBJECT_INFO_ALLOW_UNKNOWN_TYPE that is passed from the\ncaller to the lower layer; the caller can decide what to do with a\nbogus object themselves, \"cat-file\" retains the current \"we'd die\nunless allow-unknown is passed\" default to reduce the impact from\nthe entire series to reduce cognitive load from reviewers to make it\neasier to review the series, and if you really feel like doing the\n\"while at it, cat-file requiring --allow does not make sense; if the\nuser wants the raw inflated content, just let them have it\", do so\noutside the series after the dust settles.\n\nI dunno. I may be agreeing with your \"it's a much better approach\nto just implicitly supply allow-unknown-type at a higher level\".\n\n"}]}