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

[shejialuo <shejialuo@gmail.com>]

On Mon, May 12, 2025 at 09:06:19AM -0400, Jeff King wrote:
> On Sun, May 11, 2025 at 10:01:50PM +0800, shejialuo wrote:
> 
> > "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:
> 
> >  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);
> 
> that function.
> 

That's right, it would be more clear. I will update in the next version.

> Probably not worth re-rolling for that, though.
> 
> -Peff