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
