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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Dec 07 2022, Jeff King wrote:

> On Wed, Dec 07, 2022 at 05:05:47AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> 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".
>
> I like this direction, though I'd give a few small suggestions. One is
> to make it unconditional to pass in a valid "fd". These kind of magic
> sentinel values sometimes lead to confusion or bugs, and it's easy
> enough for the caller to use git_open() itself.
>
> In fact, in the one caller who cares, it lets us produce a nicer
> error message:
>
> diff --git a/object-file.c b/object-file.c
> index 24793e1b47..7c2a85132b 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1219,9 +1219,6 @@ static void *map_loose_object_1(struct repository *r, const char *const path,
>  {
>  	void *map;
>  
> -	if (!fd)
> -		fd = git_open(path);
> -
>  	map = NULL;
>  	if (fd >= 0) {
>  		struct stat st;
> @@ -2790,13 +2787,18 @@ int read_loose_object(const char *path,
>  		      struct object_info *oi)
>  {
>  	int ret = -1;
> +	int fd;
>  	void *map = NULL;
>  	unsigned long mapsize;
>  	git_zstream stream;
>  	char hdr[MAX_HEADER_LEN];
>  	unsigned long *size = oi->sizep;
>  
> -	map = map_loose_object_1(the_repository, path, 0, &mapsize);
> +	fd = git_open(path);
> +	if (fd < 0)
> +		error_errno(_("unable to open %s"), path);
> +
> +	map = map_loose_object_1(the_repository, path, fd, &mapsize);
>  	if (!map) {
>  		error_errno(_("unable to mmap %s"), path);
>  		goto out;

Yeah, I think that's even better, although...

>> +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);
>
> The other weird thing here is ownership of "fd". Now some callers pass
> it in, but map_loose_object_1() always closes it. I think that's OK
> (since we want it closed even on success), but definitely surprising
> enough that we'd want to document that in a comment.
>
>> @@ -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);
>>  }
>
> It's also kind of weird that map_loose_object_1() is a noop on a
> negative descriptor. That technically makes this correct, but I think it
> would be much less surprising to always take a valid descriptor, and
> this code should do:
>
>   if (fd)
> 	return -1;
>   return map_loose_object_1(r, path, fd, size);
>
> If we are going to make map_loose_object_1() less confusing (and I think
> that is worth doing), let's go all the way.

...maybe we should go further in the other direction. I.e. with my
earlier suggestion we're left with the mess that the "fd" ownership
isn't clear.

But what I was trying to do was fix up the ownership around the
"mapped_path", but we don't need to xstrdup() it in the first place. We
already have the caller of open_loose_object() not doing that, we can
just say that you're not going to open two loose objects at a time.

Then this becomes easier, and we can just pass the maybe-NULL "const
char **oid_path" all the way to open_loose_object():


diff --git a/object-file.c b/object-file.c
index c7a513d123e..6e900737b76 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1176,7 +1176,7 @@ static int stat_loose_object(struct repository *r, const struct object_id *oid,
  * descriptor. See the caveats on the "path" parameter above.
  */
 static int open_loose_object(struct repository *r,
-			     const struct object_id *oid, const char **path)
+			     const struct object_id *oid, const char **oid_path)
 {
 	int fd;
 	struct object_directory *odb;
@@ -1185,8 +1185,12 @@ static int open_loose_object(struct repository *r,
 
 	prepare_alt_odb(r);
 	for (odb = r->objects->odb; odb; odb = odb->next) {
-		*path = odb_loose_path(odb, &buf, oid);
-		fd = git_open(*path);
+		const char *path;
+
+		path = odb_loose_path(odb, &buf, oid);
+		if (oid_path)
+			*oid_path = path;
+		fd = git_open(path);
 		if (fd >= 0)
 			return fd;
 
@@ -1214,19 +1218,22 @@ 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,
+static void *map_loose_object_1(struct repository *r, const char *const path,
 				const struct object_id *oid, unsigned long *size,
-				char **mapped_path)
+				const char **oid_path)
 {
 	void *map;
 	int fd;
 
+	if (path && oid_path)
+		BUG("don't tell me about the path, and ask me what it is!");
+	else if (!(path || oid))
+		BUG("must get an OID or a path!");
+
 	if (path)
 		fd = git_open(path);
 	else
-		fd = open_loose_object(r, oid, &path);
-	if (mapped_path)
-		*mapped_path = xstrdup(path);
+		fd = open_loose_object(r, oid, oid_path);
 
 	map = NULL;
 	if (fd >= 0) {
@@ -1236,7 +1243,8 @@ static void *map_loose_object_1(struct repository *r, const char *path,
 			*size = xsize_t(st.st_size);
 			if (!*size) {
 				/* mmap() is forbidden on empty files */
-				error(_("object file %s is empty"), path);
+				error(_("object file %s is empty"),
+				      path ? path : *oid_path);
 				close(fd);
 				return NULL;
 			}
@@ -1432,7 +1440,7 @@ static int loose_object_info(struct repository *r,
 {
 	int status = 0;
 	unsigned long mapsize;
-	char *mapped_path = NULL;
+	const char *oid_path;
 	void *map;
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
@@ -1464,11 +1472,9 @@ 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);
+	map = map_loose_object_1(r, NULL, oid, &mapsize, &oid_path);
+	if (!map)
 		return -1;
-	}
 
 	if (!oi->sizep)
 		oi->sizep = &size_scratch;
@@ -1506,11 +1512,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), oid_path);
 
 	git_inflate_end(&stream);
 cleanup:
-	free(mapped_path);
 	munmap(map, mapsize);
 	if (oi->sizep == &size_scratch)
 		oi->sizep = NULL;