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

Re: [PATCH v2 2/3] object-file: emit corruption errors when detected

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Dec 7, 2022, 04:05 UTC
Message-ID
<221207.86359rc03e.gmgdl@evledraar.gmail.com>
In-Reply-To
<9ddfff3585c293c9801570e395b514505796a43f.1670373420.git.jonathantanmy@google.com>
On Tue, Dec 06 2022, Jonathan Tan wrote:
Show 52 quoted lines
> 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.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  object-file.c  | 63 ++++++++++++++++++++++++++------------------------
>  object-store.h |  3 +++
>  2 files changed, 36 insertions(+), 30 deletions(-)
>
> diff --git a/object-file.c b/object-file.c
> index 596dd049fd..c7a513d123 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1215,7 +1215,8 @@ static int quick_has_loose(struct repository *r,
>   * searching for a loose object named "oid".
>   */
>  static void *map_loose_object_1(struct repository *r, const char *path,
> -			     const struct object_id *oid, unsigned long *size)
> +				const struct object_id *oid, unsigned long *size,
> +				char **mapped_path)
>  {
>  	void *map;
>  	int fd;
> @@ -1224,6 +1225,9 @@ static void *map_loose_object_1(struct repository *r, const char *path,
>  		fd = git_open(path);
>  	else
>  		fd = open_loose_object(r, oid, &path);
> +	if (mapped_path)
> +		*mapped_path = xstrdup(path);
> +
>  	map = NULL;
>  	if (fd >= 0) {
>  		struct stat st;

I find this map_loose_object_1() function to be rather "busy". Part of it's the pre-image.

Callers at the end of this series are:
   1254:        return map_loose_object_1(r, NULL, oid, size, NULL);
   1467:        map = map_loose_object_1(r, NULL, oid, &mapsize, &mapped_path);
   2803:        map = map_loose_object_1(the_repository, path, NULL, &mapsize, NULL);

So, either we know the path already, and we pass it in, or we don't know the path, and may or may not be interested in what the path ends up being.

Which is why we pass in both a "path" and a "mapped_path".

Then, somewhat confusingly (maybe I'm the only one who finds this odd") the "path" variable itself does double-duty within the function. If we have a "path" already we leave it alone, but if we don't it's NULL, and then we write our new path to it.

We *might* then have a path already, *and* write to the "mapped_path", but in that case we'd be xstrdup() ing a string the user passed in. But this API use would make no sense.

So shouldn't we at least have a:
	if (path && mapped_path)
		BUG("either tell me the path, or ask me, not both!");

But I think it's better to just separate these concerns. Most of this refactoring is good, but I think this bit went a step too far, and as a result we now need to memory manage this "mapped_path". I.e. we'd get a "struct strbuf"'s "buf" before, but now loose_object_info() needs to have it xstrdup()'d, just to free() it again.

Isn't the below squashed in better? I.e. just always pass the "path", but maybe pass a "fd=0", in which case the function might need to git_open() it.

Then have map_loose_object() and loose_object_info() call open_loose_object(), and pass in the "path" and "fd".

diff --git a/object-file.c b/object-file.c
index c7a513d123e..24793e1b479 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1214,19 +1214,13 @@ static int quick_has_loose(struct repository *r,
  * Map the loose object at "path" if it is not NULL, or the path found by
  * searching for a loose object named "oid".
  */
-static void *map_loose_object_1(struct repository *r, const char *path,
-				const struct object_id *oid, unsigned long *size,
-				char **mapped_path)
+static void *map_loose_object_1(struct repository *r, const char *const path,
+				int fd, unsigned long *size)
 {
 	void *map;
-	int fd;
 
-	if (path)
+	if (!fd)
 		fd = git_open(path);
-	else
-		fd = open_loose_object(r, oid, &path);
-	if (mapped_path)
-		*mapped_path = xstrdup(path);
 
 	map = NULL;
 	if (fd >= 0) {
@@ -1251,7 +1245,10 @@ void *map_loose_object(struct repository *r,
 		       const struct object_id *oid,
 		       unsigned long *size)
 {
-	return map_loose_object_1(r, NULL, oid, size, NULL);
+	const char *path;
+	int fd = open_loose_object(r, oid, &path);
+
+	return map_loose_object_1(r, path,fd, size);
 }
 
 enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
@@ -1432,7 +1429,6 @@ static int loose_object_info(struct repository *r,
 {
 	int status = 0;
 	unsigned long mapsize;
-	char *mapped_path = NULL;
 	void *map;
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
@@ -1440,6 +1436,8 @@ static int loose_object_info(struct repository *r,
 	unsigned long size_scratch;
 	enum object_type type_scratch;
 	int allow_unknown = flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
+	int fd;
+	const char *path;
 
 	if (oi->delta_base_oid)
 		oidclr(oi->delta_base_oid);
@@ -1464,11 +1462,10 @@ static int loose_object_info(struct repository *r,
 		return 0;
 	}
 
-	map = map_loose_object_1(r, NULL, oid, &mapsize, &mapped_path);
-	if (!map) {
-		free(mapped_path);
+	fd = open_loose_object(r, oid, &path);
+	map = map_loose_object_1(r, path, fd, &mapsize);
+	if (!map)
 		return -1;
-	}
 
 	if (!oi->sizep)
 		oi->sizep = &size_scratch;
@@ -1506,11 +1503,10 @@ static int loose_object_info(struct repository *r,
 
 	if (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
 		die(_("loose object %s (stored in %s) is corrupt"),
-		    oid_to_hex(oid), mapped_path);
+		    oid_to_hex(oid), path);
 
 	git_inflate_end(&stream);
 cleanup:
-	free(mapped_path);
 	munmap(map, mapsize);
 	if (oi->sizep == &size_scratch)
 		oi->sizep = NULL;
@@ -2800,7 +2796,7 @@ int read_loose_object(const char *path,
 	char hdr[MAX_HEADER_LEN];
 	unsigned long *size = oi->sizep;
 
-	map = map_loose_object_1(the_repository, path, NULL, &mapsize, NULL);
+	map = map_loose_object_1(the_repository, path, 0, &mapsize);
 	if (!map) {
 		error_errno(_("unable to mmap %s"), path);
 		goto out;
Previous: Junio C HamanoNext: Jeff King
Message 34 of 85 in “Don't lazy-fetch commits when parsing them”
  1. 0/4 Don't lazy-fetch commits when parsing themJonathan Tan, Nov 30, 2022
  2. 1/4 object-file: reread object with exact same argsJonathan Tan, Nov 30, 2022
  3. 2/4 object-file: refactor corrupt object diagnosisJonathan Tan, Nov 30, 2022
  4. Jeff KingNov 30, 2022
  5. Junio C HamanoNov 30, 2022
  6. Jonathan TanDec 1, 2022
  7. 3/4 object-file: refactor replace object lookupJonathan Tan, Nov 30, 2022
  8. Jeff KingNov 30, 2022
  9. 4/4 commit: don't lazy-fetch commitsJonathan Tan, Nov 30, 2022
  10. Jeff KingNov 30, 2022
  11. Jonathan TanDec 1, 2022
  12. Jeff KingDec 1, 2022
  13. Junio C HamanoNov 30, 2022
  14. Jeff KingNov 30, 2022
  15. 0/4 Don't lazy-fetch commits when parsing themJonathan Tan, Dec 1, 2022
  16. 1/4 object-file: reread object with exact same argsJonathan Tan, Dec 1, 2022
  17. 3/4 object-file: refactor replace object lookupJonathan Tan, Dec 1, 2022
  18. 2/4 object-file: refactor corrupt object diagnosisJonathan Tan, Dec 1, 2022
  19. 4/4 commit: don't lazy-fetch commitsJonathan Tan, Dec 1, 2022
  20. Jeff KingDec 1, 2022
  21. Jonathan TanDec 1, 2022
  22. Jeff KingDec 2, 2022
  23. Jonathan TanDec 6, 2022
  24. Jeff KingDec 6, 2022
  25. Junio C HamanoDec 1, 2022
  26. 0/3 Don't lazy-fetch commits when parsing themJonathan Tan, Dec 7, 2022
  27. 1/3 object-file: don't exit early if skipping looseJonathan Tan, Dec 7, 2022
  28. Junio C HamanoDec 7, 2022
  29. Jeff KingDec 7, 2022
  30. Junio C HamanoDec 7, 2022
  31. Jonathan TanDec 7, 2022
  32. 2/3 object-file: emit corruption errors when detectedJonathan Tan, Dec 7, 2022
  33. Junio C HamanoDec 7, 2022
  34. Ævar Arnfjörð BjarmasonDec 7, 2022
  35. Jeff KingDec 7, 2022
  36. Ævar Arnfjörð BjarmasonDec 7, 2022
  37. Jonathan TanDec 7, 2022
  38. Ævar Arnfjörð BjarmasonDec 7, 2022
  39. Jeff KingDec 8, 2022
  40. Jeff KingDec 7, 2022
  41. 3/3 commit: don't lazy-fetch commitsJonathan Tan, Dec 7, 2022
  42. Junio C HamanoDec 7, 2022
  43. Jeff KingDec 7, 2022
  44. 0/4 Don't lazy-fetch commits when parsing themJonathan Tan, Dec 8, 2022
  45. 1/4 object-file: remove OBJECT_INFO_IGNORE_LOOSEJonathan Tan, Dec 8, 2022
  46. 2/4 object-file: refactor map_loose_object_1()Jonathan Tan, Dec 8, 2022
  47. Jeff KingDec 9, 2022
  48. Jonathan TanDec 9, 2022
  49. Jeff KingDec 9, 2022
  50. Jeff KingDec 9, 2022
  51. 3/4 object-file: emit corruption errors when detectedJonathan Tan, Dec 8, 2022
  52. Jeff KingDec 9, 2022
  53. Jonathan TanDec 9, 2022
  54. Ævar Arnfjörð BjarmasonDec 9, 2022
  55. Jonathan TanDec 9, 2022
  56. 4/4 commit: don't lazy-fetch commitsJonathan Tan, Dec 8, 2022
  57. Ævar Arnfjörð BjarmasonDec 9, 2022
  58. 0/4 Don't lazy-fetch commits when parsing themJonathan Tan, Dec 9, 2022
  59. 1/4 object-file: remove OBJECT_INFO_IGNORE_LOOSEJonathan Tan, Dec 9, 2022
  60. 2/4 object-file: refactor map_loose_object_1()Jonathan Tan, Dec 9, 2022
  61. 3/4 object-file: emit corruption errors when detectedJonathan Tan, Dec 9, 2022
  62. Junio C HamanoDec 10, 2022
  63. Jonathan TanDec 12, 2022
  64. Jeff KingDec 12, 2022
  65. Jonathan TanDec 12, 2022
  66. Jeff KingDec 12, 2022
  67. Jonathan TanDec 12, 2022
  68. Jeff KingDec 12, 2022
  69. Jonathan TanDec 12, 2022
  70. Jeff KingDec 13, 2022
  71. 4/4 commit: don't lazy-fetch commitsJonathan Tan, Dec 9, 2022
  72. 0/4 Don't lazy-fetch commits when parsing themJonathan Tan, Dec 12, 2022
  73. 1/4 object-file: remove OBJECT_INFO_IGNORE_LOOSEJonathan Tan, Dec 12, 2022
  74. 2/4 object-file: refactor map_loose_object_1()Jonathan Tan, Dec 12, 2022
  75. 4/4 commit: don't lazy-fetch commitsJonathan Tan, Dec 12, 2022
  76. 3/4 object-file: emit corruption errors when detectedJonathan Tan, Dec 12, 2022
  77. Junio C HamanoDec 13, 2022
  78. Jeff KingDec 13, 2022
  79. 0/4 Don't lazy-fetch commits when parsing themJonathan Tan, Dec 14, 2022
  80. 1/4 object-file: remove OBJECT_INFO_IGNORE_LOOSEJonathan Tan, Dec 14, 2022
  81. 2/4 object-file: refactor map_loose_object_1()Jonathan Tan, Dec 14, 2022
  82. 3/4 object-file: emit corruption errors when detectedJonathan Tan, Dec 14, 2022
  83. 4/4 commit: don't lazy-fetch commitsJonathan Tan, Dec 14, 2022
  84. Jeff KingDec 14, 2022
  85. Junio C HamanoDec 15, 2022

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.