git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 06/13] fsck: stop using object_info->type_name strbuf

From
Patrick Steinhardt <ps@pks.im>
Date
May 16, 2025, 09:52 UTC
Message-ID
<aCcK2lfr8048Kh7E@pks.im>
In-Reply-To
<20250516044953.GF22242@coredump.intra.peff.net>
On Fri, May 16, 2025 at 12:49:53AM -0400, Jeff King wrote:
Show 21 quoted lines
> When fsck-ing a loose object, we use object_info's type_name strbuf to
> record the parsed object type as a string. For most objects this is
> redundant with the object_type enum, but it does let us report the
> string when we encounter an object with an unknown type (for which there
> is no matching enum value).
> 
> There are a few downsides, though:
> 
>   1. The code to report these cases is not actually robust. Since we did
>      not pass a strbuf to unpack_loose_header(), we only retrieved types
>      from headers up to 32 bytes. In longer cases, we'd simply say
>      "object corrupt or missing".
> 
>   2. This is the last caller that uses object_info's type_name strbuf
>      support. It would be nice to refactor it so that we can simplify
>      that code.
> 
>   3. Likewise, we'll check the hash of the object using its unknown type
>      (again, as long as that type is short enough). That depends on the
>      hash_object_file_literally() code, which we'd eventually like to
>      get rid of.
Oh, I'd very much welcome if this code path went away completely.
Show 21 quoted lines
> So we can simplify things by bailing immediately in read_loose_object()
> when we encounter an unknown type. This has a few user-visible effects:
> 
>   a. Instead of producing a single line of error output like this:
> 
>        error: 26ed13ce3564fbbb44e35bde42c7da717ea004a6: object is of unknown type 'bogus': .git/objects/26/ed13ce3564fbbb44e35bde42c7da717ea004a6
> 
>      we'll now issue two lines (the first from read_loose_object() when
>      we see the unparsable header, and the second from the fsck code,
>      since we couldn't read the object):
> 
>        error: unable to parse type from header 'bogus 4' of .git/objects/26/ed13ce3564fbbb44e35bde42c7da717ea004a6
>        error: 26ed13ce3564fbbb44e35bde42c7da717ea004a6: object corrupt or missing: .git/objects/26/ed13ce3564fbbb44e35bde42c7da717ea004a6
> 
>      This is a little more verbose, but this sort of error should be
>      rare (such objects are almost impossible to work with, and cannot
>      be transferred between repositories as they are not representable
>      in packfiles). And as a bonus, reporting the broken header in full
>      could help with debugging other cases (e.g., a header like "blob
>      xyzzy\0" would fail in parsing the size, but previously we'd not
>      have showed the offending bytes).
Yup, I would claim this is an improvement, as well.
>   b. An object with an unknown type will be reported as corrupt, without
>      actually doing a hash check. Again, I think this is unlikely to
>      matter in practice since such objects are totally unusable.
Agreed.
Show 48 quoted lines
> We'll update one fsck test to match the new error strings. And we can
> remove another test that covered the case of an object with an unknown
> type _and_ a hash corruption. Since we'll skip the hash check now in
> this case, the test is no longer interesting.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/fsck.c  | 13 ++-----------
>  object-file.c   | 12 +++++++++---
>  t/t1450-fsck.sh | 29 +++--------------------------
>  3 files changed, 14 insertions(+), 40 deletions(-)
> 
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 6cac28356c..e7d96a9c8e 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -614,12 +614,11 @@ static void get_default_heads(void)
>  struct for_each_loose_cb
>  {
>  	struct progress *progress;
> -	struct strbuf obj_type;
>  };
>  
> -static int fsck_loose(const struct object_id *oid, const char *path, void *data)
> +static int fsck_loose(const struct object_id *oid, const char *path,
> +		      void *data UNUSED)
>  {
> -	struct for_each_loose_cb *cb_data = data;
>  	struct object *obj;
>  	enum object_type type = OBJ_NONE;
>  	unsigned long size;
> @@ -629,8 +628,6 @@ static int fsck_loose(const struct object_id *oid, const char *path, void *data)
>  	struct object_id real_oid = *null_oid(the_hash_algo);
>  	int err = 0;
>  
> -	strbuf_reset(&cb_data->obj_type);
> -	oi.type_name = &cb_data->obj_type;
>  	oi.sizep = &size;
>  	oi.typep = &type;
>  
> @@ -642,10 +639,6 @@ static int fsck_loose(const struct object_id *oid, const char *path, void *data)
>  			err = error(_("%s: object corrupt or missing: %s"),
>  				    oid_to_hex(oid), path);
>  	}
> -	if (type != OBJ_NONE && type < 0)
> -		err = error(_("%s: object is of unknown type '%s': %s"),
> -			    oid_to_hex(&real_oid), cb_data->obj_type.buf,
> -			    path);

This one is a bit curious. But because we know that we have reported this error in `read_loose_object()` already we don't need to print the error over here anymore.

Show 17 quoted lines
> diff --git a/object-file.c b/object-file.c
> index 1127e154f6..7a35bde96e 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1662,6 +1662,12 @@ int read_loose_object(const char *path,
>  		goto out_inflate;
>  	}
>  
> +	if (*oi->typep < 0) {
> +		error(_("unable to parse type from header '%s' of %s"),
> +		      hdr, path);
> +		goto out_inflate;
> +	}
> +
>  	if (*oi->typep == OBJ_BLOB &&
>  	    *size > repo_settings_get_big_file_threshold(the_repository)) {
>  		if (check_stream_oid(&stream, hdr, *size, path, expected_oid) < 0)
So this is where we report the new error now. Makes sense.
Patrick
Previous: Jeff KingNext: Junio C Hamano
Message 13 of 35 in “dropping support for non-standard object types”
  1. 0/13 dropping support for non-standard object typesJeff King, May 16, 2025
  2. 01/13 object-file.h: fix typo in variable declarationJeff King, May 16, 2025
  3. 02/13 cat-file: make --allow-unknown-type a noopJeff King, May 16, 2025
  4. Patrick SteinhardtMay 16, 2025
  5. Jeff KingMay 19, 2025
  6. Patrick SteinhardtMay 19, 2025
  7. Junio C HamanoMay 16, 2025
  8. 03/13 object-file: drop OBJECT_INFO_ALLOW_UNKNOWN_TYPE flagJeff King, May 16, 2025
  9. 04/13 cat-file: use type enum instead of buffer for -t optionJeff King, May 16, 2025
  10. Junio C HamanoMay 16, 2025
  11. 05/13 oid_object_info_convert(): stop using string for object typeJeff King, May 16, 2025
  12. 06/13 fsck: stop using object_info->type_name strbufJeff King, May 16, 2025
  13. Patrick SteinhardtMay 16, 2025
  14. Junio C HamanoMay 19, 2025
  15. Jeff KingMay 19, 2025
  16. 07/13 oid_object_info(): drop type_name strbufJeff King, May 16, 2025
  17. Junio C HamanoMay 19, 2025
  18. 08/13 t/helper: add zlib test-toolJeff King, May 16, 2025
  19. Junio C HamanoMay 19, 2025
  20. Jeff KingMay 19, 2025
  21. Junio C HamanoMay 21, 2025
  22. 09/13 t: add lib-loose.shJeff King, May 16, 2025
  23. Patrick SteinhardtMay 16, 2025
  24. Jeff KingMay 19, 2025
  25. Junio C HamanoMay 19, 2025
  26. 10/13 hash-object: stop allowing unknown typesJeff King, May 16, 2025
  27. Junio C HamanoMay 19, 2025
  28. 11/13 hash-object: merge HASH_* and INDEX_* flagsJeff King, May 16, 2025
  29. Patrick SteinhardtMay 16, 2025
  30. 12/13 hash-object: handle --literally with OPT_NEGBITJeff King, May 16, 2025
  31. Junio C HamanoMay 19, 2025
  32. 13/13 object-file: drop support for writing objects with unknown typesJeff King, May 16, 2025
  33. Patrick SteinhardtMay 16, 2025
  34. Junio C HamanoMay 19, 2025
  35. Junio C HamanoMay 16, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.