Re: [PATCH v3 2/3] packed-backend: extract snapshot allocation in `load_contents`

From: Jeff King <peff@peff.net>
Date: May 12, 2025, 13:06 UTC
Message-ID: <20250512130619.GB1191360@coredump.intra.peff.net>
In-Reply-To: <aCCtzm2bDRSTgEO-@ArchLinux>

On Sun, May 11, 2025 at 10:01:50PM +0800, shejialuo wrote:

Show 8 quoted lines

> "load_contents" would choose which way to load the content of the
> "packed-refs". However, we cannot directly use this function when
> checking the consistency due to we don't want to open the file. And we
> also need to reuse the logic to avoid causing repetition.
> 
> Let's create a new helper function "allocate_snapshot_buffer" to extract
> the snapshot allocation logic in "load_contents" and update the
> "load_contents" to align with the behavior.

This looks good to me. One thing that did give me a slight pause while reviewing:

Show 15 quoted lines

>  static int load_contents(struct snapshot *snapshot)
>  {
> -	int fd;
>  	struct stat st;
> -	size_t size;
> -	ssize_t bytes_read;
> +	int ret = 1;
> [...]
> +	if (!allocate_snapshot_buffer(snapshot, fd, &st))
> +		ret = 0;
>  
> -	return 1;
> +	close(fd);
> +	return ret;
>  }

I wanted to see what the semantics of "ret" were, but there aren't any other assignments. So I think this is equivalent to:

  int ret;
  ...
  ret = allocate_snapshot_buffer(snapshot, fd, &st);

which makes it a little more clear we are just relaying the value from that function.

Probably not worth re-rolling for that, though.

-Peff