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);
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
