Re: [PATCH 4/6] odb/source-loose: deduplicate logic to mmap objects

[Kaartic Sivaraam <kaartic.sivaraam@gmail.com>]

On 11/2/26 16:50, Patrick Steinhardt wrote:
> The logic to mmap objects is duplicated across `map_fd()` and
> `odb_source_loose_map_object()`. The only difference is that the former
> takes an arbitrary path, whereas the latter derives the path from an
> object ID.
> 
> Deduplicate the logic.
> 

Makes sense.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>   odb/source-loose.c | 23 +++--------------------
>   1 file changed, 3 insertions(+), 20 deletions(-)
> 
> diff --git a/odb/source-loose.c b/odb/source-loose.c
> index a90268e200a..5cd679e802c 100644
> --- a/odb/source-loose.c
> +++ b/odb/source-loose.c
> @@ -544,28 +544,11 @@ static void *odb_source_loose_map_object(struct odb_source_loose *loose,
>   					 const struct object_id *oid,
>   					 unsigned long *size)
>   {
> -	const char *p;
> -	int fd = open_loose_object(loose, oid, &p);
> -	void *map = NULL;
> -	struct stat st;
> -
> +	const char *path;
> +	int fd = open_loose_object(loose, oid, &path);
>   	if (fd < 0)
>   		return NULL;
> -
> -	if (!fstat(fd, &st)) {
> -		*size = xsize_t(st.st_size);
> -		if (!*size) {
> -			/* mmap() is forbidden on empty files */
> -			error(_("object file %s is empty"), p);
> -			goto out;
> -		}
> -
> -		map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);
> -	}
> -
> -out:
> -	close(fd);
> -	return map;
> +	return map_fd(fd, path, size);

I just noticed that map_fd could follow the goto pattern followed above 
and avoid the redundant close() call. Something like:

diff --git a/odb/source-loose.c b/odb/source-loose.c
index 737d583f03..4ace94084f 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -532,11 +532,12 @@ static void *map_fd(int fd, const char *path, 
unsigned long *size)
  		if (!*size) {
  			/* mmap() is forbidden on empty files */
  			error(_("object file %s is empty"), path);
-			close(fd);
-			return NULL;
+			goto out;
  		}
  		map = xmmap(NULL, *size, PROT_READ, MAP_PRIVATE, fd, 0);
  	}
+
+out:
  	close(fd);
  	return map;
  }

Of course, this isn't really a show-stopper for this patch. Just another 
relevant cleanup I noticed.

-- 
Sivaraam