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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Nov 10, 2026 at 07:45:58PM +0530, Kaartic Sivaraam wrote:
> On 11/2/26 16:50, Patrick Steinhardt wrote:
> > 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.

True, but I'll skip this one as I don't otherwise touch `map_fd()`.

Patrick