git/object-file.c, branch v2.40.2

Merge branch 'jk/hash-object-fsck'

2023-01-30T22:24:22Z

"git hash-object" now checks that the resulting object is well formed with the same code as "git fsck". * jk/hash-object-fsck: fsck: do not assume NUL-termination of buffers hash-object: use fsck for object checks fsck: provide a function to fsck buffer without object struct t: use hash-object --literally when created malformed objects t7030: stop using invalid tag name t1006: stop using 0-padded timestamps t1007: modernize malformed object tests

hash-object: use fsck for object checks

2023-01-18T20:59:45Z

Since c879daa237 (Make hash-object more robust against malformed objects, 2011-02-05), we've done some rudimentary checks against objects we're about to write by running them through our usual parsers for trees, commits, and tags. These parsers catch some problems, but they are not nearly as careful as the fsck functions (which make sense; the parsers are designed to be fast and forgiving, bailing only when the input is unintelligible). We are better off doing the more thorough fsck checks when writing objects. Doing so at write time is much better than writing garbage only to find out later (after building more history atop it!) that fsck complains about it, or hosts with transfer.fsckObjects reject it. This is obviously going to be a user-visible behavior change, and the test changes earlier in this series show the scope of the impact. But I'd argue that this is OK: - the documentation for hash-object is already vague about which checks we might do, saying that --literally will allow "any garbage[...] which might not otherwise pass standard object parsing or git-fsck checks". So we are already covered under the documented behavior. - users don't generally run hash-object anyway. There are a lot of spots in the tests that needed to be updated because creating garbage objects is something that Git's tests disproportionately do. - it's hard to imagine anyone thinking the new behavior is worse. Any object we reject would be a potential problem down the road for the user. And if they really want to create garbage, --literally is already the escape hatch they need. Note that the change here is actually in index_mem(), which handles the HASH_FORMAT_CHECK flag passed by hash-object. That flag is also used by "git-replace --edit" to sanity-check the result. Covering that with more thorough checks likewise seems like a good thing. Besides being more thorough, there are a few other bonuses: - we get rid of some questionable stack allocations of object structs. These don't seem to currently cause any problems in practice, but they subtly violate some of the assumptions made by the rest of the code (e.g., the "struct commit" we put on the stack and zero-initialize will not have a proper index from alloc_comit_index(). - likewise, those parsed object structs are the source of some small memory leaks - the resulting messages are much better. For example: [before] $ echo 'tree 123' | git hash-object -t commit --stdin error: bogus commit object 0000000000000000000000000000000000000000 fatal: corrupt commit [after] $ echo 'tree 123' | git.compile hash-object -t commit --stdin error: object fails fsck: badTreeSha1: invalid 'tree' line format - bad sha1 fatal: refusing to create malformed object Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

object-file: fix indent-with-space

2023-01-13T17:36:15Z

Commit b25562e63f (object-file: inline calls to read_object(), 2023-01-07) accidentally indented a conditional block with spaces instead of a tab. Reported-by: Ævar Arnfjörð Bjarmason Signed-off-by: Jeff King Acked-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano

repo_read_object_file(): stop wrapping read_object_file_extended()

2023-01-08T01:52:55Z

The only caller of read_object_file_extended() is the thin wrapper of repo_read_object_file(). Instead of wrapping, let's just rename the inner function and let people call it directly. This cleans up the namespace and reduces confusion. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

read_object_file_extended(): drop lookup_replace option

2023-01-08T01:52:55Z

Our sole caller always passes in "1", so we can just drop the parameter entirely. Anybody who doesn't want this behavior could easily call oid_object_info_extended() themselves, as we're just a thin wrapper around it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

object-file: inline calls to read_object()

2023-01-08T01:52:54Z

Since read_object() is these days just a thin wrapper around oid_object_info_extended(), and since it only has two callers, let's just inline those calls. This has a few positive outcomes: - it's a net reduction in source code lines - even though the callers end up with a few extra lines, they're now more flexible and can use object_info flags directly. So no more need to convert die_if_corrupt between parameter/flag, and we can ask for lookup replacement with a flag rather than doing it ourselves. - there's one fewer function in an already crowded namespace (e.g., the difference between read_object() and read_object_file() was not immediately obvious; now we only have one of them). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

Merge branch 'jt/avoid-lazy-fetch-commits'

2023-01-05T06:07:17Z

Even in a repository with promisor remote, it is useless to attempt to lazily attempt fetching an object that is expected to be commit, because no "filter" mode omits commit objects. Take advantage of this assumption to fail fast on errors. * jt/avoid-lazy-fetch-commits: commit: don't lazy-fetch commits object-file: emit corruption errors when detected object-file: refactor map_loose_object_1() object-file: remove OBJECT_INFO_IGNORE_LOOSE

object-file: emit corruption errors when detected

2022-12-15T00:05:55Z

Instead of relying on errno being preserved across function calls, teach do_oid_object_info_extended() to itself report object corruption when it first detects it. There are 3 types of corruption being detected: - when a replacement object is missing - when a loose object is corrupt - when a packed object is corrupt and the object cannot be read in another way Note that in the RHS of this patch's diff, a check for ENOENT that was introduced in 3ba7a06552 (A loose object is not corrupt if it cannot be read due to EMFILE, 2010-10-28) is also removed. The purpose of this check is to avoid a false report of corruption if the errno contains something like EMFILE (or anything that is not ENOENT), in which case a more generic report is presented. Because, as of this patch, we no longer rely on such a heuristic to determine corruption, but surface the error message at the point when we read something that we did not expect, this check is no longer necessary. Besides being more resilient, this also prepares for a future patch in which an indirect caller of do_oid_object_info_extended() will need such functionality. Helped-by: Jeff King Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano

object-file: refactor map_loose_object_1()

2022-12-15T00:05:55Z

This function can do 3 things: 1. Gets an fd given a path 2. Simultaneously gets a path and fd given an OID 3. Memory maps an fd Keep 3 (renaming the function accordingly) and inline 1 and 2 into their respective callers. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano

object-file: remove OBJECT_INFO_IGNORE_LOOSE

2022-12-15T00:05:55Z

Its last user was removed in 97b2fa08b6 (fetch-pack: drop custom loose object cache, 2018-11-12), so we can remove it. Helped-by: Jeff King Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano