On Tue, May 13, 2025 at 09:51:20AM -0700, Junio C Hamano wrote:
> shejialuo <shejialuo@xxxxxxxxx> writes:
>
> > During fsck, we use "strbuf_read" to read the content of "packed-refs"
> > without using mmap mechanism. This is a bad practice which would consume
> > more memory than using mmap mechanism. Besides, as all code paths in
> > "packed-backend.c" use this way, we should make "fsck" align with the
> > current codebase.
> >
> > As we have introduced the helper function "allocate_snapshot_buffer", we
> > could simple use this function to use mmap mechanism.
>
> "could simple" -> "can simply".
>
Nice catch.
> > Suggested-by: Jeff King <peff@xxxxxxxx>
> > Suggested-by: Patrick Steinhardt <ps@xxxxxx>
> > Signed-off-by: shejialuo <shejialuo@xxxxxxxxx>
> > ---
> > refs/packed-backend.c | 19 +++++++------------
> > 1 file changed, 7 insertions(+), 12 deletions(-)
>
> Nice loss of line count ;-)
>
> > - if (!st.st_size) {
> > + if (!allocate_snapshot_buffer(&snapshot, fd, &st)) {
>
> It is a bit funny to see that a helper function that works at a much
> higher conceptual level treat an empty file so specially (namely,
> should it be different from a header-only packed-refs file?). If I
> were doing this refactoring in 2 & 3, I would probalby have made the
> helper return "void", and have callers who do care about st.st_size
> check that themselves.
>
This is a good question. Actually, I intentionally design this. In the
very first implementation, I use above way. But later I think this is
inappropriate to let callers check `st.st_size`.
The reason is that we would do the same thing in "load_content" function
and fsck function. But after thinking, I think we should make the
runtime behavior and fsck consistent. That's our goal. And by using
this, let's say if one day we want to tighten the rule in the future, we
could just change `allocate_snapshot_buffer`.
I agree that introducing side effect into a function is not a good
design. But in this situation, we could make things consistent.
Thanks,
Jialuo
